Re: [PATCH v8] scripts: ftrace - move the sort-processing in ftrace_init

Re: [PATCH v8] scripts: ftrace - move the sort-processing in ftrace_init

[Sven Schnelle]

Hi Yinan,

Yinan Liu <yinan@linux.alibaba.com> writes:

> 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.
>
> Signed-off-by: Yinan Liu <yinan@linux.alibaba.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: kernel test robot <oliver.sang@intel.com>
> ---
>  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(-)

while i like the idea, this unfortunately breaks ftrace on s390. The
reason for that is that the compiler generates relocation entries for
all the addresses in __mcount_loc. During boot, the s390 decompressor
iterates through all the relocations and overwrites the nicely
sorted list between __start_mcount_loc and __stop_mcount_loc with
the unsorted list because the relocations entries are not adjusted.

Of course we could just disable that option, but that would make us
different compared to x86 which i don't like. Adding code to sort the
relocation would of course also fix that, but i don't think it is a good
idea to rely on the order of relocations.

Any thoughts how a fix could look like, and whether that could also be a
problem on other architectures?

Thanks
Sven

Re: [PATCH v8] scripts: ftrace - move the sort-processing in ftrace_init

[Heiko Carstens]

On Fri, Jan 21, 2022 at 10:46:36AM +0100, Sven Schnelle wrote:
> Hi Yinan,
> 
> Yinan Liu <yinan@linux.alibaba.com> writes:
> 
> > 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.
> >
> > Signed-off-by: Yinan Liu <yinan@linux.alibaba.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > ---
> >  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(-)
> 
> while i like the idea, this unfortunately breaks ftrace on s390. The
> reason for that is that the compiler generates relocation entries for
> all the addresses in __mcount_loc. During boot, the s390 decompressor
> iterates through all the relocations and overwrites the nicely
> sorted list between __start_mcount_loc and __stop_mcount_loc with
> the unsorted list because the relocations entries are not adjusted.
> 
> Of course we could just disable that option, but that would make us
> different compared to x86 which i don't like. Adding code to sort the
> relocation would of course also fix that, but i don't think it is a good
> idea to rely on the order of relocations.
> 
> Any thoughts how a fix could look like, and whether that could also be a
> problem on other architectures?

Sven, thanks for figuring this out. Can you confirm that reverting
commit 72b3942a173c ("scripts: ftrace - move the sort-processing in
ftrace_init") fixes the problem?

This really should be addressed before rc1 is out, otherwise s390 is
broken if somebody enables ftrace.
Where "broken" translates to random crashes as soon as ftrace is
enabled, which again is nowadays quite common.

Re: [PATCH v8] scripts: ftrace - move the sort-processing in ftrace_init

[Sven Schnelle]

Heiko Carstens <hca@linux.ibm.com> writes:

> On Fri, Jan 21, 2022 at 10:46:36AM +0100, Sven Schnelle wrote:
>> Hi Yinan,
>> 
>> Yinan Liu <yinan@linux.alibaba.com> writes:
>> 
>> > 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.
>> >
>> > Signed-off-by: Yinan Liu <yinan@linux.alibaba.com>
>> > Reported-by: kernel test robot <lkp@intel.com>
>> > Reported-by: kernel test robot <oliver.sang@intel.com>
>> > ---
>> >  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(-)
>> 
>> while i like the idea, this unfortunately breaks ftrace on s390. The
>> reason for that is that the compiler generates relocation entries for
>> all the addresses in __mcount_loc. During boot, the s390 decompressor
>> iterates through all the relocations and overwrites the nicely
>> sorted list between __start_mcount_loc and __stop_mcount_loc with
>> the unsorted list because the relocations entries are not adjusted.
>> 
>> Of course we could just disable that option, but that would make us
>> different compared to x86 which i don't like. Adding code to sort the
>> relocation would of course also fix that, but i don't think it is a good
>> idea to rely on the order of relocations.
>> 
>> Any thoughts how a fix could look like, and whether that could also be a
>> problem on other architectures?
>
> Sven, thanks for figuring this out. Can you confirm that reverting
> commit 72b3942a173c ("scripts: ftrace - move the sort-processing in
> ftrace_init") fixes the problem?

Yes, reverting this commit fixes it.

> This really should be addressed before rc1 is out, otherwise s390 is
> broken if somebody enables ftrace.
> Where "broken" translates to random crashes as soon as ftrace is
> enabled, which again is nowadays quite common.

I wasn't able to reproduce these crashes on my systems so far. For the
readers here, we're seeing about 10-15 systems crashing every night,
usually in the 00basic/ ftrace testcases.

In most of the case it looks like register corruption, where some random
register is or'd or parts are overwritten with 0x0004000000000000,
sometimes 0x00f4000000000000. I haven't found yts found a commit that
might cause this.

/Sven

Re: [PATCH v8] scripts: ftrace - move the sort-processing in ftrace_init

[Sven Schnelle]

Sven Schnelle <svens@linux.ibm.com> writes:

> Heiko Carstens <hca@linux.ibm.com> writes:
>
>> On Fri, Jan 21, 2022 at 10:46:36AM +0100, Sven Schnelle wrote:
>>> Hi Yinan,
>>> 
>>> Yinan Liu <yinan@linux.alibaba.com> writes:
>>> 
>>> > 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.
>>> >
>>> > Signed-off-by: Yinan Liu <yinan@linux.alibaba.com>
>>> > Reported-by: kernel test robot <lkp@intel.com>
>>> > Reported-by: kernel test robot <oliver.sang@intel.com>
>>> > ---
>>> >  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(-)
>>> 
>>> while i like the idea, this unfortunately breaks ftrace on s390. The
>>> reason for that is that the compiler generates relocation entries for
>>> all the addresses in __mcount_loc. During boot, the s390 decompressor
>>> iterates through all the relocations and overwrites the nicely
>>> sorted list between __start_mcount_loc and __stop_mcount_loc with
>>> the unsorted list because the relocations entries are not adjusted.
>>> 
>>> Of course we could just disable that option, but that would make us
>>> different compared to x86 which i don't like. Adding code to sort the
>>> relocation would of course also fix that, but i don't think it is a good
>>> idea to rely on the order of relocations.
>>> 
>>> Any thoughts how a fix could look like, and whether that could also be a
>>> problem on other architectures?
>>
>> Sven, thanks for figuring this out. Can you confirm that reverting
>> commit 72b3942a173c ("scripts: ftrace - move the sort-processing in
>> ftrace_init") fixes the problem?
>
> Yes, reverting this commit fixes it.
>
>> This really should be addressed before rc1 is out, otherwise s390 is
>> broken if somebody enables ftrace.
>> Where "broken" translates to random crashes as soon as ftrace is
>> enabled, which again is nowadays quite common.
>
> I wasn't able to reproduce these crashes on my systems so far. For the
> readers here, we're seeing about 10-15 systems crashing every night,
> usually in the 00basic/ ftrace testcases.
>
> In most of the case it looks like register corruption, where some random
> register is or'd or parts are overwritten with 0x0004000000000000,
> sometimes 0x00f4000000000000. I haven't found yts found a commit that
> might cause this.

Thinking of it, 04 and f4 are exactly the bytes we're patching in our brcl
instructions right at the beginning of the function. So i guess that
because of this bug the ftrace code now writes those bytes to the wrong
location, sometimes hitting the register save area. I haven't verified
that, but i think there's a high likelyhood.

/Sven

Re: [PATCH v8] scripts: ftrace - move the sort-processing in ftrace_init

[Steven Rostedt]

On Fri, 21 Jan 2022 11:42:28 +0100
Heiko Carstens <hca@linux.ibm.com> wrote:

> This really should be addressed before rc1 is out, otherwise s390 is
> broken if somebody enables ftrace.
> Where "broken" translates to random crashes as soon as ftrace is
> enabled, which again is nowadays quite common.

Instead of reverting, should we just add this patch?

-- Steve

diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index f468767bc287..752ed89a293b 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -70,6 +70,13 @@ config HAVE_C_RECORDMCOUNT
 	help
 	  C version of recordmcount available?
 
+config BUILDTIME_MCOUNT_SORT
+       bool
+       default y
+       depends on BUILDTIME_TABLE_SORT && !S390
+       help
+         Sort the mcount_loc section at build time.
+
 config TRACER_MAX_TRACE
 	bool
 
@@ -918,7 +925,7 @@ config EVENT_TRACE_TEST_SYSCALLS
 config FTRACE_SORT_STARTUP_TEST
        bool "Verify compile time sorting of ftrace functions"
        depends on DYNAMIC_FTRACE
-       depends on BUILDTIME_TABLE_SORT
+       depends on BUILDTIME_MCOUNT_SORT
        help
 	 Sorting of the mcount_loc sections that is used to find the
 	 where the ftrace knows where to patch functions for tracing
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 403e485bf091..b01e1fa62193 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6429,10 +6429,10 @@ static int ftrace_process_locs(struct module *mod,
 
 	/*
 	 * Sorting mcount in vmlinux at build time depend on
-	 * CONFIG_BUILDTIME_TABLE_SORT, while mcount loc in
+	 * CONFIG_BUILDTIME_MCOUNT_SORT, while mcount loc in
 	 * modules can not be sorted at build time.
 	 */
-	if (!IS_ENABLED(CONFIG_BUILDTIME_TABLE_SORT) || mod) {
+	if (!IS_ENABLED(CONFIG_BUILDTIME_MCOUNT_SORT) || mod) {
 		sort(start, count, sizeof(*start),
 		     ftrace_cmp_ips, NULL);
 	} else {

Re: [PATCH v8] scripts: ftrace - move the sort-processing in ftrace_init

[Heiko Carstens]

On Fri, Jan 21, 2022 at 01:11:48PM -0500, Steven Rostedt wrote:
> On Fri, 21 Jan 2022 11:42:28 +0100
> Heiko Carstens <hca@linux.ibm.com> wrote:
> 
> > This really should be addressed before rc1 is out, otherwise s390 is
> > broken if somebody enables ftrace.
> > Where "broken" translates to random crashes as soon as ftrace is
> > enabled, which again is nowadays quite common.
> 
> Instead of reverting, should we just add this patch?

That would work as well.

Tested-by: Heiko Carstens <hca@linux.ibm.com>