From: Yinan Liu <[email protected]>
When the kernel starts, the initialization of ftrace takes
up a portion of the time (approximately 6~8ms) to sort mcount
addresses. We can save this time by moving mcount-sorting to
compile time.
Link: https://lkml.kernel.org/r/[email protected]
Signed-off-by: Yinan Liu <[email protected]>
Reported-by: kernel test robot <[email protected]>
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
kernel/trace/ftrace.c | 11 +++-
scripts/Makefile | 6 +-
scripts/link-vmlinux.sh | 6 +-
scripts/sorttable.c | 2 +
scripts/sorttable.h | 120 +++++++++++++++++++++++++++++++++++++++-
5 files changed, 137 insertions(+), 8 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 30bc880c3849..9ca63df6553a 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6406,8 +6406,15 @@ static int ftrace_process_locs(struct module *mod,
if (!count)
return 0;
- sort(start, count, sizeof(*start),
- ftrace_cmp_ips, NULL);
+ /*
+ * Sorting mcount in vmlinux at build time depend on
+ * CONFIG_BUILDTIME_TABLE_SORT, while mcount loc in
+ * modules can not be sorted at build time.
+ */
+ if (!IS_ENABLED(CONFIG_BUILDTIME_TABLE_SORT) || mod) {
+ sort(start, count, sizeof(*start),
+ ftrace_cmp_ips, NULL);
+ }
start_pg = ftrace_allocate_pages(count);
if (!start_pg)
diff --git a/scripts/Makefile b/scripts/Makefile
index 9adb6d247818..b082d2f93357 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -17,6 +17,7 @@ hostprogs-always-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE) += insert-sys-cert
hostprogs-always-$(CONFIG_SYSTEM_REVOCATION_LIST) += extract-cert
HOSTCFLAGS_sorttable.o = -I$(srctree)/tools/include
+HOSTLDLIBS_sorttable = -lpthread
HOSTCFLAGS_asn1_compiler.o = -I$(srctree)/include
HOSTCFLAGS_sign-file.o = $(CRYPTO_CFLAGS)
HOSTLDLIBS_sign-file = $(CRYPTO_LIBS)
@@ -29,7 +30,10 @@ ARCH := x86
endif
HOSTCFLAGS_sorttable.o += -I$(srctree)/tools/arch/x86/include
HOSTCFLAGS_sorttable.o += -DUNWINDER_ORC_ENABLED
-HOSTLDLIBS_sorttable = -lpthread
+endif
+
+ifdef CONFIG_DYNAMIC_FTRACE
+HOSTCFLAGS_sorttable.o += -DMCOUNT_SORT_ENABLED
endif
# The following programs are only built on demand
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 5cdd9bc5c385..dd9955f45774 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -400,6 +400,9 @@ if [ -n "${CONFIG_DEBUG_INFO_BTF}" -a -n "${CONFIG_BPF}" ]; then
${RESOLVE_BTFIDS} vmlinux
fi
+info SYSMAP System.map
+mksysmap vmlinux System.map
+
if [ -n "${CONFIG_BUILDTIME_TABLE_SORT}" ]; then
info SORTTAB vmlinux
if ! sorttable vmlinux; then
@@ -408,9 +411,6 @@ if [ -n "${CONFIG_BUILDTIME_TABLE_SORT}" ]; then
fi
fi
-info SYSMAP System.map
-mksysmap vmlinux System.map
-
# step a (see comment above)
if [ -n "${CONFIG_KALLSYMS}" ]; then
mksysmap ${kallsyms_vmlinux} .tmp_System.map
diff --git a/scripts/sorttable.c b/scripts/sorttable.c
index b7c2ad71f9cf..70bdc787ddfb 100644
--- a/scripts/sorttable.c
+++ b/scripts/sorttable.c
@@ -30,6 +30,8 @@
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
+#include <errno.h>
+#include <pthread.h>
#include <tools/be_byteshift.h>
#include <tools/le_byteshift.h>
diff --git a/scripts/sorttable.h b/scripts/sorttable.h
index 7b9745cf8c70..1e8b77928fa4 100644
--- a/scripts/sorttable.h
+++ b/scripts/sorttable.h
@@ -19,6 +19,9 @@
#undef extable_ent_size
#undef compare_extable
+#undef get_mcount_loc
+#undef sort_mcount_loc
+#undef elf_mcount_loc
#undef do_sort
#undef Elf_Addr
#undef Elf_Ehdr
@@ -41,6 +44,9 @@
#ifdef SORTTABLE_64
# define extable_ent_size 16
# define compare_extable compare_extable_64
+# define get_mcount_loc get_mcount_loc_64
+# define sort_mcount_loc sort_mcount_loc_64
+# define elf_mcount_loc elf_mcount_loc_64
# define do_sort do_sort_64
# define Elf_Addr Elf64_Addr
# define Elf_Ehdr Elf64_Ehdr
@@ -62,6 +68,9 @@
#else
# define extable_ent_size 8
# define compare_extable compare_extable_32
+# define get_mcount_loc get_mcount_loc_32
+# define sort_mcount_loc sort_mcount_loc_32
+# define elf_mcount_loc elf_mcount_loc_32
# define do_sort do_sort_32
# define Elf_Addr Elf32_Addr
# define Elf_Ehdr Elf32_Ehdr
@@ -84,8 +93,6 @@
#if defined(SORTTABLE_64) && defined(UNWINDER_ORC_ENABLED)
/* ORC unwinder only support X86_64 */
-#include <errno.h>
-#include <pthread.h>
#include <asm/orc_types.h>
#define ERRSTR_MAXSZ 256
@@ -191,7 +198,64 @@ static int compare_extable(const void *a, const void *b)
return 1;
return 0;
}
+#ifdef MCOUNT_SORT_ENABLED
+struct elf_mcount_loc {
+ Elf_Ehdr *ehdr;
+ Elf_Shdr *init_data_sec;
+ uint_t start_mcount_loc;
+ uint_t stop_mcount_loc;
+};
+
+/* Sort the addresses stored between __start_mcount_loc to __stop_mcount_loc in vmlinux */
+static void *sort_mcount_loc(void *arg)
+{
+ struct elf_mcount_loc *emloc = (struct elf_mcount_loc *)arg;
+ uint_t offset = emloc->start_mcount_loc - _r(&(emloc->init_data_sec)->sh_addr)
+ + _r(&(emloc->init_data_sec)->sh_offset);
+ uint_t count = emloc->stop_mcount_loc - emloc->start_mcount_loc;
+ unsigned char *start_loc = (void *)emloc->ehdr + offset;
+
+ qsort(start_loc, count/sizeof(uint_t), sizeof(uint_t), compare_extable);
+ return NULL;
+}
+
+/* Get the address of __start_mcount_loc and __stop_mcount_loc in System.map */
+static void get_mcount_loc(uint_t *_start, uint_t *_stop)
+{
+ FILE *file_start, *file_stop;
+ char start_buff[20];
+ char stop_buff[20];
+ int len = 0;
+
+ file_start = popen(" grep start_mcount System.map | awk '{print $1}' ", "r");
+ if (!file_start) {
+ fprintf(stderr, "get start_mcount_loc error!");
+ return;
+ }
+
+ file_stop = popen(" grep stop_mcount System.map | awk '{print $1}' ", "r");
+ if (!file_stop) {
+ fprintf(stderr, "get stop_mcount_loc error!");
+ pclose(file_start);
+ return;
+ }
+
+ while (fgets(start_buff, sizeof(start_buff), file_start) != NULL) {
+ len = strlen(start_buff);
+ start_buff[len - 1] = '\0';
+ }
+ *_start = strtoul(start_buff, NULL, 16);
+
+ while (fgets(stop_buff, sizeof(stop_buff), file_stop) != NULL) {
+ len = strlen(stop_buff);
+ stop_buff[len - 1] = '\0';
+ }
+ *_stop = strtoul(stop_buff, NULL, 16);
+ pclose(file_start);
+ pclose(file_stop);
+}
+#endif
static int do_sort(Elf_Ehdr *ehdr,
char const *const fname,
table_sort_t custom_sort)
@@ -217,6 +281,12 @@ static int do_sort(Elf_Ehdr *ehdr,
int idx;
unsigned int shnum;
unsigned int shstrndx;
+#ifdef MCOUNT_SORT_ENABLED
+ struct elf_mcount_loc mstruct;
+ uint_t _start_mcount_loc = 0;
+ uint_t _stop_mcount_loc = 0;
+ pthread_t mcount_sort_thread;
+#endif
#if defined(SORTTABLE_64) && defined(UNWINDER_ORC_ENABLED)
unsigned int orc_ip_size = 0;
unsigned int orc_size = 0;
@@ -253,6 +323,17 @@ static int do_sort(Elf_Ehdr *ehdr,
symtab_shndx = (Elf32_Word *)((const char *)ehdr +
_r(&s->sh_offset));
+#ifdef MCOUNT_SORT_ENABLED
+ /* locate the .init.data section in vmlinux */
+ if (!strcmp(secstrings + idx, ".init.data")) {
+ get_mcount_loc(&_start_mcount_loc, &_stop_mcount_loc);
+ mstruct.ehdr = ehdr;
+ mstruct.init_data_sec = s;
+ mstruct.start_mcount_loc = _start_mcount_loc;
+ mstruct.stop_mcount_loc = _stop_mcount_loc;
+ }
+#endif
+
#if defined(SORTTABLE_64) && defined(UNWINDER_ORC_ENABLED)
/* locate the ORC unwind tables */
if (!strcmp(secstrings + idx, ".orc_unwind_ip")) {
@@ -294,6 +375,23 @@ static int do_sort(Elf_Ehdr *ehdr,
goto out;
}
#endif
+
+#ifdef MCOUNT_SORT_ENABLED
+ if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
+ fprintf(stderr,
+ "incomplete mcount's sort in file: %s\n",
+ fname);
+ goto out;
+ }
+
+ /* create thread to sort mcount_loc concurrently */
+ if (pthread_create(&mcount_sort_thread, NULL, &sort_mcount_loc, &mstruct)) {
+ fprintf(stderr,
+ "pthread_create mcount_sort_thread failed '%s': %s\n",
+ strerror(errno), fname);
+ goto out;
+ }
+#endif
if (!extab_sec) {
fprintf(stderr, "no __ex_table in file: %s\n", fname);
goto out;
@@ -376,5 +474,23 @@ static int do_sort(Elf_Ehdr *ehdr,
}
}
#endif
+
+#ifdef MCOUNT_SORT_ENABLED
+ if (mcount_sort_thread) {
+ void *retval = NULL;
+ /* wait for mcount sort done */
+ rc = pthread_join(mcount_sort_thread, &retval);
+ if (rc) {
+ fprintf(stderr,
+ "pthread_join failed '%s': %s\n",
+ strerror(errno), fname);
+ } else if (retval) {
+ rc = -1;
+ fprintf(stderr,
+ "failed to sort mcount '%s': %s\n",
+ (char *)retval, fname);
+ }
+ }
+#endif
return rc;
}
--
2.33.0
Hi Steven and Yinan,
On Tue, Jan 11, 2022 at 12:30:41PM -0500, Steven Rostedt wrote:
> From: Yinan Liu <[email protected]>
>
> When the kernel starts, the initialization of ftrace takes
> up a portion of the time (approximately 6~8ms) to sort mcount
> addresses. We can save this time by moving mcount-sorting to
> compile time.
>
> Link: https://lkml.kernel.org/r/[email protected]
>
> Signed-off-by: Yinan Liu <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
This change as commit 72b3942a173c ("scripts: ftrace - move the
sort-processing in ftrace_init") in -next causes a bunch of warnings at
the beginning of the build when using clang as the host compiler:
$ make -skj"$(nproc)" LLVM=1 distclean allmodconfig init/main.o
In file included from scripts/sorttable.c:195:
scripts/sorttable.h:380:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:380:2: note: remove the 'if' if its condition is always false
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:380:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever '||' condition is true [-Wsometimes-uninitialized]
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:380:6: note: remove the '||' if its condition is always false
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:380:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever '||' condition is true [-Wsometimes-uninitialized]
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:380:6: note: remove the '||' if its condition is always false
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:288:30: note: initialize the variable 'mcount_sort_thread' to silence this warning
pthread_t mcount_sort_thread;
^
= 0
In file included from scripts/sorttable.c:197:
scripts/sorttable.h:380:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:380:2: note: remove the 'if' if its condition is always false
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:380:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever '||' condition is true [-Wsometimes-uninitialized]
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:380:6: note: remove the '||' if its condition is always false
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:380:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever '||' condition is true [-Wsometimes-uninitialized]
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:380:6: note: remove the '||' if its condition is always false
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
^~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:370:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (pthread_create(&orc_sort_thread, NULL,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:370:2: note: remove the 'if' if its condition is always false
if (pthread_create(&orc_sort_thread, NULL,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:360:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (orc_ip_size % sizeof(int) != 0 ||
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:360:2: note: remove the 'if' if its condition is always false
if (orc_ip_size % sizeof(int) != 0 ||
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:360:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever '||' condition is true [-Wsometimes-uninitialized]
if (orc_ip_size % sizeof(int) != 0 ||
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:360:6: note: remove the '||' if its condition is always false
if (orc_ip_size % sizeof(int) != 0 ||
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:360:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever '||' condition is true [-Wsometimes-uninitialized]
if (orc_ip_size % sizeof(int) != 0 ||
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:360:6: note: remove the '||' if its condition is always false
if (orc_ip_size % sizeof(int) != 0 ||
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:353:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (!g_orc_ip_table || !g_orc_table) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:353:2: note: remove the 'if' if its condition is always false
if (!g_orc_ip_table || !g_orc_table) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
scripts/sorttable.h:353:6: warning: variable 'mcount_sort_thread' is used uninitialized whenever '||' condition is true [-Wsometimes-uninitialized]
if (!g_orc_ip_table || !g_orc_table) {
^~~~~~~~~~~~~~~
scripts/sorttable.h:479:6: note: uninitialized use occurs here
if (mcount_sort_thread) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:353:6: note: remove the '||' if its condition is always false
if (!g_orc_ip_table || !g_orc_table) {
^~~~~~~~~~~~~~~~~~
scripts/sorttable.h:288:30: note: initialize the variable 'mcount_sort_thread' to silence this warning
pthread_t mcount_sort_thread;
^
= 0
12 warnings generated.
Should mcount_sort_thread be zero initialized or is there something else
going on here? I am currently hunting down a bunch of other regressions
so apologies for just the report rather than a patch to fix it.
Cheers,
Nathan
On Sat, 15 Jan 2022 13:36:04 -0700
Nathan Chancellor <[email protected]> wrote:
> Hi Steven and Yinan,
>
> On Tue, Jan 11, 2022 at 12:30:41PM -0500, Steven Rostedt wrote:
> > From: Yinan Liu <[email protected]>
> >
> > When the kernel starts, the initialization of ftrace takes
> > up a portion of the time (approximately 6~8ms) to sort mcount
> > addresses. We can save this time by moving mcount-sorting to
> > compile time.
> >
> > Link: https://lkml.kernel.org/r/[email protected]
> >
> > Signed-off-by: Yinan Liu <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > Signed-off-by: Steven Rostedt <[email protected]>
>
> This change as commit 72b3942a173c ("scripts: ftrace - move the
> sort-processing in ftrace_init") in -next causes a bunch of warnings at
> the beginning of the build when using clang as the host compiler:
>
>
> Should mcount_sort_thread be zero initialized or is there something else
> going on here? I am currently hunting down a bunch of other regressions
> so apologies for just the report rather than a patch to fix it.
Can this really happen? We have:
if (pthread_create(&mcount_sort_thread, NULL, &sort_mcount_loc, &mstruct)) {
fprintf(stderr,
"pthread_create mcount_sort_thread failed '%s': %s\n",
strerror(errno), fname);
goto out;
}
[..]
if (mcount_sort_thread) {
void *retval = NULL;
/* wait for mcount sort done */
rc = pthread_join(mcount_sort_thread, &retval);
if (rc) {
fprintf(stderr,
"pthread_join failed '%s': %s\n",
strerror(errno), fname);
} else if (retval) {
rc = -1;
fprintf(stderr,
"failed to sort mcount '%s': %s\n",
(char *)retval, fname);
}
}
Shouldn't the pthread_create() initialize it? And I'm not even sure if we
need that if statement?
Or is there a path to get there without pthread_create() initializing it?
-- Steve
On Sat, Jan 15, 2022 at 10:59:20PM -0500, Steven Rostedt wrote:
> On Sat, 15 Jan 2022 13:36:04 -0700
> Nathan Chancellor <[email protected]> wrote:
>
> > Hi Steven and Yinan,
> >
> > On Tue, Jan 11, 2022 at 12:30:41PM -0500, Steven Rostedt wrote:
> > > From: Yinan Liu <[email protected]>
> > >
> > > When the kernel starts, the initialization of ftrace takes
> > > up a portion of the time (approximately 6~8ms) to sort mcount
> > > addresses. We can save this time by moving mcount-sorting to
> > > compile time.
> > >
> > > Link: https://lkml.kernel.org/r/[email protected]
> > >
> > > Signed-off-by: Yinan Liu <[email protected]>
> > > Reported-by: kernel test robot <[email protected]>
> > > Reported-by: kernel test robot <[email protected]>
> > > Signed-off-by: Steven Rostedt <[email protected]>
> >
> > This change as commit 72b3942a173c ("scripts: ftrace - move the
> > sort-processing in ftrace_init") in -next causes a bunch of warnings at
> > the beginning of the build when using clang as the host compiler:
> >
>
>
> >
> > Should mcount_sort_thread be zero initialized or is there something else
> > going on here? I am currently hunting down a bunch of other regressions
> > so apologies for just the report rather than a patch to fix it.
>
> Can this really happen? We have:
The way the code is written now, yes.
> if (pthread_create(&mcount_sort_thread, NULL, &sort_mcount_loc, &mstruct)) {
> fprintf(stderr,
> "pthread_create mcount_sort_thread failed '%s': %s\n",
> strerror(errno), fname);
> goto out;
> }
> [..]
>
> if (mcount_sort_thread) {
> void *retval = NULL;
> /* wait for mcount sort done */
> rc = pthread_join(mcount_sort_thread, &retval);
> if (rc) {
> fprintf(stderr,
> "pthread_join failed '%s': %s\n",
> strerror(errno), fname);
> } else if (retval) {
> rc = -1;
> fprintf(stderr,
> "failed to sort mcount '%s': %s\n",
> (char *)retval, fname);
> }
> }
>
> Shouldn't the pthread_create() initialize it? And I'm not even sure if we
> need that if statement?
>
> Or is there a path to get there without pthread_create() initializing it?
Yes. If the if statment right above the pthread_create() call triggers,
we jump to the out label, which hits the if (mcount_sort_thread), and
mcount_sort_thread won't be initialized.
if (!mstruct.init_data_sec || !_start_mcount_loc || !_stop_mcount_loc) {
fprintf(stderr,
"incomplete mcount's sort in file: %s\n",
fname);
goto out;
}
if (pthread_create(&mcount_sort_thread, ...)) {
...
out:
...
if (mcount_sort_thread) {
If I am misunderstanding something, please let me know.
Cheers,
Nathan
Hi,steven and Nathan
Thanks to Nathan, I realize that there are some problems with initialization.
Initialization of pthreads and mstructs may fail or be skipped, in which case
it is possible to use uninitialized values in "out" logic. I'm not sure if
such an error would happen, but it's certainly safe to initialize it.
I use mcount_sort_pthread as a global variable, which is consistent with
orc_sort_pthread. And initialize the mstruct in function do_sort.
Yinan Liu (1):
script/sorttable: fix some initialization problems
scripts/sorttable.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
--
2.19.1.6.gb485710b
elf_mcount_loc and mcount_sort_thread definitions are not
initialized immediately within the function, which can cause
the judgment logic to use uninitialized values when the
initialization logic of subsequent code fails.
Link: https://lkml.kernel.org/r/[email protected]
Fixes:72b3942a173c (scripts: ftrace - move the sort-processing in ftrace_init)
Signed-off-by: Yinan Liu <[email protected]>
---
scripts/sorttable.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/scripts/sorttable.h b/scripts/sorttable.h
index 1e8b77928fa4..13ae3262ec96 100644
--- a/scripts/sorttable.h
+++ b/scripts/sorttable.h
@@ -199,6 +199,8 @@ static int compare_extable(const void *a, const void *b)
return 0;
}
#ifdef MCOUNT_SORT_ENABLED
+pthread_t mcount_sort_thread;
+
struct elf_mcount_loc {
Elf_Ehdr *ehdr;
Elf_Shdr *init_data_sec;
@@ -282,10 +284,9 @@ static int do_sort(Elf_Ehdr *ehdr,
unsigned int shnum;
unsigned int shstrndx;
#ifdef MCOUNT_SORT_ENABLED
- struct elf_mcount_loc mstruct;
+ struct elf_mcount_loc mstruct = {NULL, NULL, 0, 0};
uint_t _start_mcount_loc = 0;
uint_t _stop_mcount_loc = 0;
- pthread_t mcount_sort_thread;
#endif
#if defined(SORTTABLE_64) && defined(UNWINDER_ORC_ENABLED)
unsigned int orc_ip_size = 0;
--
2.19.1.6.gb485710b
On Mon, Jan 17, 2022 at 02:23:44PM +0800, Yinan Liu wrote:
> elf_mcount_loc and mcount_sort_thread definitions are not
> initialized immediately within the function, which can cause
> the judgment logic to use uninitialized values when the
> initialization logic of subsequent code fails.
>
> Link: https://lkml.kernel.org/r/[email protected]
>
> Fixes:72b3942a173c (scripts: ftrace - move the sort-processing in ftrace_init)
This should be:
Fixes: 72b3942a173c ("scripts: ftrace - move the sort-processing in ftrace_init")
You can add an alias to always get the format right, like:
$ git config --global alias.fixes 'show -s --format="Fixes: %h (\"%s\")"'
$ git fixes 72b3942a173c387b27860ba1069636726e208777
Fixes: 72b3942a173c ("scripts: ftrace - move the sort-processing in ftrace_init")
> Signed-off-by: Yinan Liu <[email protected]>
This resolves the warnings I reported and does not introduce any new
ones. It seems reasonable to me.
Reviewed-by: Nathan Chancellor <[email protected]>
Tested-by: Nathan Chancellor <[email protected]>
> ---
> scripts/sorttable.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/sorttable.h b/scripts/sorttable.h
> index 1e8b77928fa4..13ae3262ec96 100644
> --- a/scripts/sorttable.h
> +++ b/scripts/sorttable.h
> @@ -199,6 +199,8 @@ static int compare_extable(const void *a, const void *b)
> return 0;
> }
> #ifdef MCOUNT_SORT_ENABLED
> +pthread_t mcount_sort_thread;
> +
> struct elf_mcount_loc {
> Elf_Ehdr *ehdr;
> Elf_Shdr *init_data_sec;
> @@ -282,10 +284,9 @@ static int do_sort(Elf_Ehdr *ehdr,
> unsigned int shnum;
> unsigned int shstrndx;
> #ifdef MCOUNT_SORT_ENABLED
> - struct elf_mcount_loc mstruct;
> + struct elf_mcount_loc mstruct = {NULL, NULL, 0, 0};
Wonder if this would be better using either '= {}' or '= {0}'?
> uint_t _start_mcount_loc = 0;
> uint_t _stop_mcount_loc = 0;
> - pthread_t mcount_sort_thread;
> #endif
> #if defined(SORTTABLE_64) && defined(UNWINDER_ORC_ENABLED)
> unsigned int orc_ip_size = 0;
> --
> 2.19.1.6.gb485710b
>
>
elf_mcount_loc and mcount_sort_thread definitions are not
initialized immediately within the function, which can cause
the judgment logic to use uninitialized values when the
initialization logic of subsequent code fails.
Link: https://lkml.kernel.org/r/[email protected]
Fixes: 72b3942a173c ("scripts: ftrace - move the sort-processing in ftrace_init")
Tested-by: Nathan Chancellor <[email protected]>
Signed-off-by: Yinan Liu <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
---
scripts/sorttable.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/scripts/sorttable.h b/scripts/sorttable.h
index 1e8b77928fa4..deb7c1d3e979 100644
--- a/scripts/sorttable.h
+++ b/scripts/sorttable.h
@@ -199,6 +199,8 @@ static int compare_extable(const void *a, const void *b)
return 0;
}
#ifdef MCOUNT_SORT_ENABLED
+pthread_t mcount_sort_thread;
+
struct elf_mcount_loc {
Elf_Ehdr *ehdr;
Elf_Shdr *init_data_sec;
@@ -282,10 +284,9 @@ static int do_sort(Elf_Ehdr *ehdr,
unsigned int shnum;
unsigned int shstrndx;
#ifdef MCOUNT_SORT_ENABLED
- struct elf_mcount_loc mstruct;
+ struct elf_mcount_loc mstruct = {0};
uint_t _start_mcount_loc = 0;
uint_t _stop_mcount_loc = 0;
- pthread_t mcount_sort_thread;
#endif
#if defined(SORTTABLE_64) && defined(UNWINDER_ORC_ENABLED)
unsigned int orc_ip_size = 0;
--
2.19.1.6.gb485710b