[v6,3/7] Adjust symbol ordering in text output section

Commit Message

Rong Xu Oct. 26, 2024, 5:14 a.m. UTC

When the -ffunction-sections compiler option is enabled, each function
is placed in a separate section named .text.function_name rather than
putting all functions in a single .text section.

However, using -function-sections can cause problems with the
linker script. The comments included in include/asm-generic/vmlinux.lds.h
note these issues.:
  “TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
   code elimination is enabled, so these sections should be converted
   to use ".." first.”

It is unclear whether there is a straightforward method for converting
a suffix to "..".

This patch modifies the order of subsections within the text output
section. Specifically, it repositions sections with certain fixed patterns
(for example .text.unlikely) before TEXT_MAIN, ensuring that they are
grouped and matched together. It also places .text.hot section at the
beginning of a page to help the TLB performance.

Note that the limitation arises because the linker script employs glob
patterns instead of regular expressions for string matching. While there
is a method to maintain the current order using complex patterns, this
significantly complicates the pattern and increases the likelihood of
errors.

This patch also changes vmlinux.lds.S for the sparc64 architecture to
accommodate specific symbol placement requirements.

Co-developed-by: Han Shen <[e-mail]>
Signed-off-by: Han Shen <[e-mail]>
Signed-off-by: Rong Xu <[e-mail]>
Suggested-by: Sriraman Tallam <[e-mail]>
Suggested-by: Krzysztof Pszeniczny <[e-mail]>
Tested-by: Yonghong Song <[e-mail]>
Tested-by: Yabin Cui <[e-mail]>
Change-Id: I5202d40bc7e24f93c2bfb2f0d987e9dc57dec1b1
---
 arch/sparc/kernel/vmlinux.lds.S   |  5 +++++
 include/asm-generic/vmlinux.lds.h | 19 ++++++++++++-------
 2 files changed, 17 insertions(+), 7 deletions(-)

Comments

Kees Cook Oct. 29, 2024, 12:05 a.m. UTC | #1

On Fri, Oct 25, 2024 at 10:14:05PM -0700, Rong Xu wrote:
> When the -ffunction-sections compiler option is enabled, each function
> is placed in a separate section named .text.function_name rather than
> putting all functions in a single .text section.
> 
> However, using -function-sections can cause problems with the
> linker script. The comments included in include/asm-generic/vmlinux.lds.h
> note these issues.:
>   “TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
>    code elimination is enabled, so these sections should be converted
>    to use ".." first.”
> 
> It is unclear whether there is a straightforward method for converting
> a suffix to "..".

While I'm not a fan of the potential loss of a page, I'm not aware of
a way to do this renaming. If we want to keep these as "cold", then this
is probably the best way to do it.

> This patch modifies the order of subsections within the text output
> section. Specifically, it repositions sections with certain fixed patterns
> (for example .text.unlikely) before TEXT_MAIN, ensuring that they are
> grouped and matched together. It also places .text.hot section at the
> beginning of a page to help the TLB performance.
> 
> Note that the limitation arises because the linker script employs glob
> patterns instead of regular expressions for string matching. While there
> is a method to maintain the current order using complex patterns, this
> significantly complicates the pattern and increases the likelihood of
> errors.
> 
> This patch also changes vmlinux.lds.S for the sparc64 architecture to
> accommodate specific symbol placement requirements.
> 
> Co-developed-by: Han Shen <[e-mail]>
> Signed-off-by: Han Shen <[e-mail]>
> Signed-off-by: Rong Xu <[e-mail]>
> Suggested-by: Sriraman Tallam <[e-mail]>
> Suggested-by: Krzysztof Pszeniczny <[e-mail]>
> Tested-by: Yonghong Song <[e-mail]>
> Tested-by: Yabin Cui <[e-mail]>
> Change-Id: I5202d40bc7e24f93c2bfb2f0d987e9dc57dec1b1

Reviewed-by: Kees Cook <[e-mail]>

-Kees

> ---
>  arch/sparc/kernel/vmlinux.lds.S   |  5 +++++
>  include/asm-generic/vmlinux.lds.h | 19 ++++++++++++-------
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/sparc/kernel/vmlinux.lds.S b/arch/sparc/kernel/vmlinux.lds.S
> index d317a843f7ea9..f1b86eb303404 100644
> --- a/arch/sparc/kernel/vmlinux.lds.S
> +++ b/arch/sparc/kernel/vmlinux.lds.S
> @@ -48,6 +48,11 @@ SECTIONS
>  	{
>  		_text = .;
>  		HEAD_TEXT
> +	        ALIGN_FUNCTION();
> +#ifdef CONFIG_SPARC64
> +	        /* Match text section symbols in head_64.S first */
> +	        *head_64.o(.text)
> +#endif
>  		TEXT_TEXT
>  		SCHED_TEXT
>  		LOCK_TEXT
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index eeadbaeccf88b..fd901951549c0 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -553,19 +553,24 @@
>   * .text section. Map to function alignment to avoid address changes
>   * during second ld run in second ld pass when generating System.map
>   *
> - * TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
> - * code elimination is enabled, so these sections should be converted
> - * to use ".." first.
> + * TEXT_MAIN here will match symbols with a fixed pattern (for example,
> + * .text.hot or .text.unlikely) if dead code elimination or
> + * function-section is enabled. Match these symbols first before
> + * TEXT_MAIN to ensure they are grouped together.
> + *
> + * Also placing .text.hot section at the beginning of a page, this
> + * would help the TLB performance.
>   */
>  #define TEXT_TEXT							\
>  		ALIGN_FUNCTION();					\
> +		*(.text.asan.* .text.tsan.*)				\
> +		*(.text.unknown .text.unknown.*)			\
> +		*(.text.unlikely .text.unlikely.*)			\
> +		. = ALIGN(PAGE_SIZE);					\
>  		*(.text.hot .text.hot.*)				\
>  		*(TEXT_MAIN .text.fixup)				\
> -		*(.text.unlikely .text.unlikely.*)			\
> -		*(.text.unknown .text.unknown.*)			\
>  		NOINSTR_TEXT						\
> -		*(.ref.text)						\
> -		*(.text.asan.* .text.tsan.*)
> +		*(.ref.text)
>  
>  
>  /* sched.text is aling to function alignment to secure we have same
> -- 
> 2.47.0.163.g1226f6d8fa-goog
>

Masahiro Yamada Nov. 1, 2024, 6:05 p.m. UTC | #2

On Sat, Oct 26, 2024 at 7:14 AM Rong Xu <[e-mail]> wrote:
>
> When the -ffunction-sections compiler option is enabled, each function
> is placed in a separate section named .text.function_name rather than
> putting all functions in a single .text section.
>
> However, using -function-sections can cause problems with the
> linker script. The comments included in include/asm-generic/vmlinux.lds.h
> note these issues.:
>   “TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
>    code elimination is enabled, so these sections should be converted
>    to use ".." first.”
>
> It is unclear whether there is a straightforward method for converting
> a suffix to "..".
>
> This patch modifies the order of subsections within the text output
> section. Specifically, it repositions sections with certain fixed patterns
> (for example .text.unlikely) before TEXT_MAIN, ensuring that they are
> grouped and matched together. It also places .text.hot section at the
> beginning of a page to help the TLB performance.


The fixed patterns are currently listed in this order:

  .text.hot, .text_unlikely, .text.unknown, .text.asan.

You reorder them to:

  .text.asan, .text.unknown, .text.unlikely, .text.hot


I believe it is better to describe your thoughts
about the reshuffling among the fixed pattern sections.

Otherwise, It is unclear to me.




--
Best Regards
Masahiro Yamada

Rong Xu Nov. 1, 2024, 6:37 p.m. UTC | #3

Current order is:
.text.hot, .text, .text_unlikely, .text.unknown, .text.asan

The patch reorders them to:
.text.asan, .text.unknown, .text_unlikely, .text.hot, .text

The majority of the code resides in three sections: .text.hot, .text, and
 .text.unlikely, with .text.unknown containing a negligible amount.
.text.asan is only generated in ASAN builds.

Our primary goal is to group code segments based on their execution
frequency (hotness).

First, we want to place .text.hot adjacent to .text. Since we cannot put
.text.hot after .text (Due to constraints with -ffunction-sections,
placing .text.hot after .text is problematic), we need to put
.text.hot before .text.

Then it comes to .text.unlikely, we cannot put it after .text
(same -ffunction-sections issue) . Therefore, we'll position .text.unlikely
before .text.hot.

.text.unknown and .tex.asan follow the same logic.

This revised ordering effectively reverses the original arrangement (for
.text.unlikely, .text.unknown, and .tex.asan), maintaining a similar level of
affinity between sections.

I hope this explains the reason for the new ordering.

-Rong

On Fri, Nov 1, 2024 at 11:06 AM Masahiro Yamada <[e-mail]> wrote:
>
> On Sat, Oct 26, 2024 at 7:14 AM Rong Xu <[e-mail]> wrote:
> >
> > When the -ffunction-sections compiler option is enabled, each function
> > is placed in a separate section named .text.function_name rather than
> > putting all functions in a single .text section.
> >
> > However, using -function-sections can cause problems with the
> > linker script. The comments included in include/asm-generic/vmlinux.lds.h
> > note these issues.:
> >   “TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
> >    code elimination is enabled, so these sections should be converted
> >    to use ".." first.”
> >
> > It is unclear whether there is a straightforward method for converting
> > a suffix to "..".
> >
> > This patch modifies the order of subsections within the text output
> > section. Specifically, it repositions sections with certain fixed patterns
> > (for example .text.unlikely) before TEXT_MAIN, ensuring that they are
> > grouped and matched together. It also places .text.hot section at the
> > beginning of a page to help the TLB performance.
>
>
> The fixed patterns are currently listed in this order:
>
>   .text.hot, .text_unlikely, .text.unknown, .text.asan.
>
> You reorder them to:
>
>   .text.asan, .text.unknown, .text.unlikely, .text.hot
>
>
> I believe it is better to describe your thoughts
> about the reshuffling among the fixed pattern sections.
>
> Otherwise, It is unclear to me.
>
>
>
>
> --
> Best Regards
> Masahiro Yamada

Masahiro Yamada Nov. 1, 2024, 10:03 p.m. UTC | #4

On Sat, Nov 2, 2024 at 3:37 AM Rong Xu <[e-mail]> wrote:
>
> Current order is:
> .text.hot, .text, .text_unlikely, .text.unknown, .text.asan
>
> The patch reorders them to:
> .text.asan, .text.unknown, .text_unlikely, .text.hot, .text
>
> The majority of the code resides in three sections: .text.hot, .text, and
>  .text.unlikely, with .text.unknown containing a negligible amount.
> .text.asan is only generated in ASAN builds.
>
> Our primary goal is to group code segments based on their execution
> frequency (hotness).
>
> First, we want to place .text.hot adjacent to .text. Since we cannot put
> .text.hot after .text (Due to constraints with -ffunction-sections,
> placing .text.hot after .text is problematic), we need to put
> .text.hot before .text.
>
> Then it comes to .text.unlikely, we cannot put it after .text
> (same -ffunction-sections issue) . Therefore, we'll position .text.unlikely
> before .text.hot.
>
> .text.unknown and .tex.asan follow the same logic.
>
> This revised ordering effectively reverses the original arrangement (for
> .text.unlikely, .text.unknown, and .tex.asan), maintaining a similar level of
> affinity between sections.
>
> I hope this explains the reason for the new ordering.


Make sense.

Please describe the above in the commit description.

Then, it will be clearer why you not only moved up fixed patterns
but also reversed the order.

Klara Modin Nov. 9, 2024, 3:38 p.m. UTC | #5

Hi,

On 2024-10-26 07:14, Rong Xu wrote:
> When the -ffunction-sections compiler option is enabled, each function
> is placed in a separate section named .text.function_name rather than
> putting all functions in a single .text section.
> 
> However, using -function-sections can cause problems with the
> linker script. The comments included in include/asm-generic/vmlinux.lds.h
> note these issues.:
>    “TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
>     code elimination is enabled, so these sections should be converted
>     to use ".." first.”
> 
> It is unclear whether there is a straightforward method for converting
> a suffix to "..".
> 
> This patch modifies the order of subsections within the text output
> section. Specifically, it repositions sections with certain fixed patterns
> (for example .text.unlikely) before TEXT_MAIN, ensuring that they are
> grouped and matched together. It also places .text.hot section at the
> beginning of a page to help the TLB performance.
> 
> Note that the limitation arises because the linker script employs glob
> patterns instead of regular expressions for string matching. While there
> is a method to maintain the current order using complex patterns, this
> significantly complicates the pattern and increases the likelihood of
> errors.
> 
> This patch also changes vmlinux.lds.S for the sparc64 architecture to
> accommodate specific symbol placement requirements.

With this patch (622240ea8d71a75055399fd4b3cc2b190e44d2e2 in 
next-20241108) my Edgerouter 6P hangs on boot (Cavium Octeon III, 
mips64, running in big endian). It's using device tree passed from the 
vendored u-boot (attached in case it's relevant).

Disabling dead code elimination does not fix the issue.

Please let me know if there's anything else you need.

Regards,
Klara Modin

> 
> Co-developed-by: Han Shen <[e-mail]>
> Signed-off-by: Han Shen <[e-mail]>
> Signed-off-by: Rong Xu <[e-mail]>
> Suggested-by: Sriraman Tallam <[e-mail]>
> Suggested-by: Krzysztof Pszeniczny <[e-mail]>
> Tested-by: Yonghong Song <[e-mail]>
> Tested-by: Yabin Cui <[e-mail]>
> Change-Id: I5202d40bc7e24f93c2bfb2f0d987e9dc57dec1b1
> ---
>   arch/sparc/kernel/vmlinux.lds.S   |  5 +++++
>   include/asm-generic/vmlinux.lds.h | 19 ++++++++++++-------
>   2 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/sparc/kernel/vmlinux.lds.S b/arch/sparc/kernel/vmlinux.lds.S
> index d317a843f7ea9..f1b86eb303404 100644
> --- a/arch/sparc/kernel/vmlinux.lds.S
> +++ b/arch/sparc/kernel/vmlinux.lds.S
> @@ -48,6 +48,11 @@ SECTIONS
>   	{
>   		_text = .;
>   		HEAD_TEXT
> +	        ALIGN_FUNCTION();
> +#ifdef CONFIG_SPARC64
> +	        /* Match text section symbols in head_64.S first */
> +	        *head_64.o(.text)
> +#endif
>   		TEXT_TEXT
>   		SCHED_TEXT
>   		LOCK_TEXT
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index eeadbaeccf88b..fd901951549c0 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -553,19 +553,24 @@
>    * .text section. Map to function alignment to avoid address changes
>    * during second ld run in second ld pass when generating System.map
>    *
> - * TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
> - * code elimination is enabled, so these sections should be converted
> - * to use ".." first.
> + * TEXT_MAIN here will match symbols with a fixed pattern (for example,
> + * .text.hot or .text.unlikely) if dead code elimination or
> + * function-section is enabled. Match these symbols first before
> + * TEXT_MAIN to ensure they are grouped together.
> + *
> + * Also placing .text.hot section at the beginning of a page, this
> + * would help the TLB performance.
>    */
>   #define TEXT_TEXT							\
>   		ALIGN_FUNCTION();					\
> +		*(.text.asan.* .text.tsan.*)				\
> +		*(.text.unknown .text.unknown.*)			\
> +		*(.text.unlikely .text.unlikely.*)			\
> +		. = ALIGN(PAGE_SIZE);					\
>   		*(.text.hot .text.hot.*)				\
>   		*(TEXT_MAIN .text.fixup)				\
> -		*(.text.unlikely .text.unlikely.*)			\
> -		*(.text.unknown .text.unknown.*)			\
>   		NOINSTR_TEXT						\
> -		*(.ref.text)						\
> -		*(.text.asan.* .text.tsan.*)
> +		*(.ref.text)
>   
>   
>   /* sched.text is aling to function alignment to secure we have same
# bad: [291b13f025250e2fa3b5e2ff714aaaa227dff02c] of: WARN on deprecated #address-cells/#size-cells handling
git bisect start 'HEAD'
# status: waiting for good commit(s), bad commit known
# good: [bfc64d9b7e8cac82be6b8629865e137d962578f8] Merge tag 'net-6.12-rc7' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
git bisect good bfc64d9b7e8cac82be6b8629865e137d962578f8
# bad: [5090ed9c92a1ba84f8563486550c6bf0b39954f2] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
git bisect bad 5090ed9c92a1ba84f8563486550c6bf0b39954f2
# bad: [dbaafa9b8d4a351fb925efb44f4177cabc95d26d] Merge branch 'fs-next' of linux-next
git bisect bad dbaafa9b8d4a351fb925efb44f4177cabc95d26d
# bad: [d81b3235857fe7abd24ab2928a36626a0b4734b1] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git
git bisect bad d81b3235857fe7abd24ab2928a36626a0b4734b1
# bad: [cc030679964c0cf8e2217fa57935c19d851aa9ea] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/rmk/linux.git
git bisect bad cc030679964c0cf8e2217fa57935c19d851aa9ea
# good: [5d2ae3246a2d15e2d384637ee55fc0320546be63] foo
git bisect good 5d2ae3246a2d15e2d384637ee55fc0320546be63
# bad: [10540b9bb270cbffbc56ca2f828439675abbdccc] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
git bisect bad 10540b9bb270cbffbc56ca2f828439675abbdccc
# good: [e038e395a05e3a1cf2f5450bb2d08adcffca80b4] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git
git bisect good e038e395a05e3a1cf2f5450bb2d08adcffca80b4
# good: [aded6a2e0817d76cdd8010ea7c0b217b55cbe78b] Merge branch 'for-linux-next-fixes' of https://gitlab.freedesktop.org/drm/misc/kernel.git
git bisect good aded6a2e0817d76cdd8010ea7c0b217b55cbe78b
# good: [375a4f4ea719ad68e305373cfe0f77ecd1378531] kconfig: qconf: remove unnecessary lastWindowClosed() signal connection
git bisect good 375a4f4ea719ad68e305373cfe0f77ecd1378531
# good: [397a479b511df4e6e7c665d7d8991943645b4cab] kbuild: simplify rustfmt target
git bisect good 397a479b511df4e6e7c665d7d8991943645b4cab
# bad: [84712ea5d9b003d298b0df6f957d06bbcac99ef5] AutoFDO: Enable -ffunction-sections for the AutoFDO build
git bisect bad 84712ea5d9b003d298b0df6f957d06bbcac99ef5
# good: [18e885099f1c52755f054202525cb60d3edcda44] objtool: Fix unreachable instruction warnings for weak functions
git bisect good 18e885099f1c52755f054202525cb60d3edcda44
# bad: [9a92584c8ef545cf92299e4cadbe2148b93dafa1] vmlinux.lds.h: Add markers for text_unlikely and text_hot sections
git bisect bad 9a92584c8ef545cf92299e4cadbe2148b93dafa1
# bad: [622240ea8d71a75055399fd4b3cc2b190e44d2e2] vmlinux.lds.h: Adjust symbol ordering in text output section
git bisect bad 622240ea8d71a75055399fd4b3cc2b190e44d2e2
# first bad commit: [622240ea8d71a75055399fd4b3cc2b190e44d2e2] vmlinux.lds.h: Adjust symbol ordering in text output section

Rong Xu Nov. 11, 2024, 8:43 p.m. UTC | #6

Thanks for reporting this issue!

I'm assuming your kernel build enables dead code elimination and
uses the --ffunction-sections compiler flag. Without this patch, all
the functions
-- I think there are only .text.unlikely.* and .text.* are grouped
together in the
final vmlinux. This patch modifies the linker script to place
.text.unlikely.* functions
 before .text.* functions. I've examined arch/mips/kernel/vmlinux.lds.S, and
haven't found any obvious issue.

Can you send me the following?
(1) the kernel build command
(2) System.map without the patch
(3) System.map with the patch

Best regards,

-Rong

On Sat, Nov 9, 2024 at 7:39 AM Klara Modin <[e-mail]> wrote:
>
> Hi,
>
> On 2024-10-26 07:14, Rong Xu wrote:
> > When the -ffunction-sections compiler option is enabled, each function
> > is placed in a separate section named .text.function_name rather than
> > putting all functions in a single .text section.
> >
> > However, using -function-sections can cause problems with the
> > linker script. The comments included in include/asm-generic/vmlinux.lds.h
> > note these issues.:
> >    “TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
> >     code elimination is enabled, so these sections should be converted
> >     to use ".." first.”
> >
> > It is unclear whether there is a straightforward method for converting
> > a suffix to "..".
> >
> > This patch modifies the order of subsections within the text output
> > section. Specifically, it repositions sections with certain fixed patterns
> > (for example .text.unlikely) before TEXT_MAIN, ensuring that they are
> > grouped and matched together. It also places .text.hot section at the
> > beginning of a page to help the TLB performance.
> >
> > Note that the limitation arises because the linker script employs glob
> > patterns instead of regular expressions for string matching. While there
> > is a method to maintain the current order using complex patterns, this
> > significantly complicates the pattern and increases the likelihood of
> > errors.
> >
> > This patch also changes vmlinux.lds.S for the sparc64 architecture to
> > accommodate specific symbol placement requirements.
>
> With this patch (622240ea8d71a75055399fd4b3cc2b190e44d2e2 in
> next-20241108) my Edgerouter 6P hangs on boot (Cavium Octeon III,
> mips64, running in big endian). It's using device tree passed from the
> vendored u-boot (attached in case it's relevant).
>
> Disabling dead code elimination does not fix the issue.
>
> Please let me know if there's anything else you need.
>
> Regards,
> Klara Modin
>
> >
> > Co-developed-by: Han Shen <[e-mail]>
> > Signed-off-by: Han Shen <[e-mail]>
> > Signed-off-by: Rong Xu <[e-mail]>
> > Suggested-by: Sriraman Tallam <[e-mail]>
> > Suggested-by: Krzysztof Pszeniczny <[e-mail]>
> > Tested-by: Yonghong Song <[e-mail]>
> > Tested-by: Yabin Cui <[e-mail]>
> > Change-Id: I5202d40bc7e24f93c2bfb2f0d987e9dc57dec1b1
> > ---
> >   arch/sparc/kernel/vmlinux.lds.S   |  5 +++++
> >   include/asm-generic/vmlinux.lds.h | 19 ++++++++++++-------
> >   2 files changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/sparc/kernel/vmlinux.lds.S b/arch/sparc/kernel/vmlinux.lds.S
> > index d317a843f7ea9..f1b86eb303404 100644
> > --- a/arch/sparc/kernel/vmlinux.lds.S
> > +++ b/arch/sparc/kernel/vmlinux.lds.S
> > @@ -48,6 +48,11 @@ SECTIONS
> >       {
> >               _text = .;
> >               HEAD_TEXT
> > +             ALIGN_FUNCTION();
> > +#ifdef CONFIG_SPARC64
> > +             /* Match text section symbols in head_64.S first */
> > +             *head_64.o(.text)
> > +#endif
> >               TEXT_TEXT
> >               SCHED_TEXT
> >               LOCK_TEXT
> > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > index eeadbaeccf88b..fd901951549c0 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -553,19 +553,24 @@
> >    * .text section. Map to function alignment to avoid address changes
> >    * during second ld run in second ld pass when generating System.map
> >    *
> > - * TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
> > - * code elimination is enabled, so these sections should be converted
> > - * to use ".." first.
> > + * TEXT_MAIN here will match symbols with a fixed pattern (for example,
> > + * .text.hot or .text.unlikely) if dead code elimination or
> > + * function-section is enabled. Match these symbols first before
> > + * TEXT_MAIN to ensure they are grouped together.
> > + *
> > + * Also placing .text.hot section at the beginning of a page, this
> > + * would help the TLB performance.
> >    */
> >   #define TEXT_TEXT                                                   \
> >               ALIGN_FUNCTION();                                       \
> > +             *(.text.asan.* .text.tsan.*)                            \
> > +             *(.text.unknown .text.unknown.*)                        \
> > +             *(.text.unlikely .text.unlikely.*)                      \
> > +             . = ALIGN(PAGE_SIZE);                                   \
> >               *(.text.hot .text.hot.*)                                \
> >               *(TEXT_MAIN .text.fixup)                                \
> > -             *(.text.unlikely .text.unlikely.*)                      \
> > -             *(.text.unknown .text.unknown.*)                        \
> >               NOINSTR_TEXT                                            \
> > -             *(.ref.text)                                            \
> > -             *(.text.asan.* .text.tsan.*)
> > +             *(.ref.text)
> >
> >
> >   /* sched.text is aling to function alignment to secure we have same

Klara Modin Nov. 11, 2024, 9:32 p.m. UTC | #7

On 2024-11-11 21:43, Rong Xu wrote:
> Thanks for reporting this issue!
> 
> I'm assuming your kernel build enables dead code elimination and
> uses the --ffunction-sections compiler flag. Without this patch, all
> the functions
> -- I think there are only .text.unlikely.* and .text.* are grouped
> together in the
> final vmlinux. This patch modifies the linker script to place
> .text.unlikely.* functions
>   before .text.* functions. I've examined arch/mips/kernel/vmlinux.lds.S, and
> haven't found any obvious issue.
> 
> Can you send me the following?
> (1) the kernel build command
> (2) System.map without the patch
> (3) System.map with the patch
> 
> Best regards,
> 
> -Rong
> 
I don't set -ffunction-sections explicitly but it seems to be used when 
I look at the .cmd files. The build command is nothing fancy, I just set 
ARCH=mips CROSS_COMPILE=mips64-unknown-linux-gnuabin32- and build with 
make -j24.

I've attached the System.map, built on next-20241111 as well as it with 
this series reverted.

Regards,
Klara Modin

Rong Xu Nov. 11, 2024, 10:39 p.m. UTC | #8

In the new System.map, we have:
ffffffff81112400 T _stext

This looks wrong. It should point to the beginning of the text, like
ffffffff81100400 T _stext

I'll do some debugging on this.

-Rong

On Mon, Nov 11, 2024 at 1:32 PM Klara Modin <[e-mail]> wrote:
>
> On 2024-11-11 21:43, Rong Xu wrote:
> > Thanks for reporting this issue!
> >
> > I'm assuming your kernel build enables dead code elimination and
> > uses the --ffunction-sections compiler flag. Without this patch, all
> > the functions
> > -- I think there are only .text.unlikely.* and .text.* are grouped
> > together in the
> > final vmlinux. This patch modifies the linker script to place
> > .text.unlikely.* functions
> >   before .text.* functions. I've examined arch/mips/kernel/vmlinux.lds.S, and
> > haven't found any obvious issue.
> >
> > Can you send me the following?
> > (1) the kernel build command
> > (2) System.map without the patch
> > (3) System.map with the patch
> >
> > Best regards,
> >
> > -Rong
> >
> I don't set -ffunction-sections explicitly but it seems to be used when
> I look at the .cmd files. The build command is nothing fancy, I just set
> ARCH=mips CROSS_COMPILE=mips64-unknown-linux-gnuabin32- and build with
> make -j24.
>
> I've attached the System.map, built on next-20241111 as well as it with
> this series reverted.
>
> Regards,
> Klara Modin

Rong Xu Nov. 12, 2024, 5:38 a.m. UTC | #9

I compared the System.map files from Klara Modin. The linker script is
doing what I expected: relocating the unlikely executed functions to the
beginning of the .text section.

However, the problem is with the _stext symbol. It belongs to the
.text section, so
it is positioned after the unlikely (or hot) functions. But it really
needs to be
the start of the text section.

I checked all vmlinux.lds.S in arch/, I found that most archs
explicitly assign _stext to the same address as _text, with the
following 3 exceptions:
  arch/sh/kernel/vmlinux.lds.S
  arch/mips/kernel/vmlinux.lds.S
  arch/sparc/kernel/vmlinux.lds.S

Note that we already partially handled arch/sparc/kernel/vmlinux.lds.S
for sparc64.
But we need to handle sparc32 also.

Additionally, the boot/compressed/vmlinux.lds.S also the TEXT_TEXT
template. However,
I presume these files do not generate the .text.unlikely. or
.text.hot.* sections.

I sent the following patch to Klara because I don't have an
environment to build and test.
====================
diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
index 9ff55cb80a64..5f130af44247 100644
--- a/arch/mips/kernel/vmlinux.lds.S
+++ b/arch/mips/kernel/vmlinux.lds.S
@@ -61,6 +61,7 @@ SECTIONS
        /* read-only */
        _text = .;      /* Text and read-only data */
        .text : {
+               _stext = .;
                TEXT_TEXT
                SCHED_TEXT
                LOCK_TEXT
======================

If Klara confirms the fix, I will send the patch for review.

Thanks,

-Rong


On Mon, Nov 11, 2024 at 2:39 PM Rong Xu <[e-mail]> wrote:
>
> In the new System.map, we have:
> ffffffff81112400 T _stext
>
> This looks wrong. It should point to the beginning of the text, like
> ffffffff81100400 T _stext
>
> I'll do some debugging on this.
>
> -Rong
>
> On Mon, Nov 11, 2024 at 1:32 PM Klara Modin <[e-mail]> wrote:
> >
> > On 2024-11-11 21:43, Rong Xu wrote:
> > > Thanks for reporting this issue!
> > >
> > > I'm assuming your kernel build enables dead code elimination and
> > > uses the --ffunction-sections compiler flag. Without this patch, all
> > > the functions
> > > -- I think there are only .text.unlikely.* and .text.* are grouped
> > > together in the
> > > final vmlinux. This patch modifies the linker script to place
> > > .text.unlikely.* functions
> > >   before .text.* functions. I've examined arch/mips/kernel/vmlinux.lds.S, and
> > > haven't found any obvious issue.
> > >
> > > Can you send me the following?
> > > (1) the kernel build command
> > > (2) System.map without the patch
> > > (3) System.map with the patch
> > >
> > > Best regards,
> > >
> > > -Rong
> > >
> > I don't set -ffunction-sections explicitly but it seems to be used when
> > I look at the .cmd files. The build command is nothing fancy, I just set
> > ARCH=mips CROSS_COMPILE=mips64-unknown-linux-gnuabin32- and build with
> > make -j24.
> >
> > I've attached the System.map, built on next-20241111 as well as it with
> > this series reverted.
> >
> > Regards,
> > Klara Modin

Klara Modin Nov. 12, 2024, 7:45 a.m. UTC | #10

On 2024-11-12 06:38, Rong Xu wrote:
> I compared the System.map files from Klara Modin. The linker script is
> doing what I expected: relocating the unlikely executed functions to the
> beginning of the .text section.
> 
> However, the problem is with the _stext symbol. It belongs to the
> .text section, so
> it is positioned after the unlikely (or hot) functions. But it really
> needs to be
> the start of the text section.
> 
> I checked all vmlinux.lds.S in arch/, I found that most archs
> explicitly assign _stext to the same address as _text, with the
> following 3 exceptions:
>    arch/sh/kernel/vmlinux.lds.S
>    arch/mips/kernel/vmlinux.lds.S
>    arch/sparc/kernel/vmlinux.lds.S
> 
> Note that we already partially handled arch/sparc/kernel/vmlinux.lds.S
> for sparc64.
> But we need to handle sparc32 also.
> 
> Additionally, the boot/compressed/vmlinux.lds.S also the TEXT_TEXT
> template. However,
> I presume these files do not generate the .text.unlikely. or
> .text.hot.* sections.
> 
> I sent the following patch to Klara because I don't have an
> environment to build and test.
> ====================
> diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
> index 9ff55cb80a64..5f130af44247 100644
> --- a/arch/mips/kernel/vmlinux.lds.S
> +++ b/arch/mips/kernel/vmlinux.lds.S
> @@ -61,6 +61,7 @@ SECTIONS
>          /* read-only */
>          _text = .;      /* Text and read-only data */
>          .text : {
> +               _stext = .;
>                  TEXT_TEXT
>                  SCHED_TEXT
>                  LOCK_TEXT
> ======================
> 
> If Klara confirms the fix, I will send the patch for review.
> 
> Thanks,
> 
> -Rong
> 

This does indeed fix the issue for me.

Thanks,
Tested-by: Klara Modin <[e-mail]>

Rong Xu Nov. 12, 2024, 8:13 p.m. UTC | #11

I sent the following patch for review:
https://lkml.org/lkml/2024/11/12/1565

Thanks!

-Rong

On Mon, Nov 11, 2024 at 11:45 PM Klara Modin <[e-mail]> wrote:
>
> On 2024-11-12 06:38, Rong Xu wrote:
> > I compared the System.map files from Klara Modin. The linker script is
> > doing what I expected: relocating the unlikely executed functions to the
> > beginning of the .text section.
> >
> > However, the problem is with the _stext symbol. It belongs to the
> > .text section, so
> > it is positioned after the unlikely (or hot) functions. But it really
> > needs to be
> > the start of the text section.
> >
> > I checked all vmlinux.lds.S in arch/, I found that most archs
> > explicitly assign _stext to the same address as _text, with the
> > following 3 exceptions:
> >    arch/sh/kernel/vmlinux.lds.S
> >    arch/mips/kernel/vmlinux.lds.S
> >    arch/sparc/kernel/vmlinux.lds.S
> >
> > Note that we already partially handled arch/sparc/kernel/vmlinux.lds.S
> > for sparc64.
> > But we need to handle sparc32 also.
> >
> > Additionally, the boot/compressed/vmlinux.lds.S also the TEXT_TEXT
> > template. However,
> > I presume these files do not generate the .text.unlikely. or
> > .text.hot.* sections.
> >
> > I sent the following patch to Klara because I don't have an
> > environment to build and test.
> > ====================
> > diff --git a/arch/mips/kernel/vmlinux.lds.S b/arch/mips/kernel/vmlinux.lds.S
> > index 9ff55cb80a64..5f130af44247 100644
> > --- a/arch/mips/kernel/vmlinux.lds.S
> > +++ b/arch/mips/kernel/vmlinux.lds.S
> > @@ -61,6 +61,7 @@ SECTIONS
> >          /* read-only */
> >          _text = .;      /* Text and read-only data */
> >          .text : {
> > +               _stext = .;
> >                  TEXT_TEXT
> >                  SCHED_TEXT
> >                  LOCK_TEXT
> > ======================
> >
> > If Klara confirms the fix, I will send the patch for review.
> >
> > Thanks,
> >
> > -Rong
> >
>
> This does indeed fix the issue for me.
>
> Thanks,
> Tested-by: Klara Modin <[e-mail]>

Patch

diff --git a/arch/sparc/kernel/vmlinux.lds.S b/arch/sparc/kernel/vmlinux.lds.S
index d317a843f7ea9..f1b86eb303404 100644
--- a/arch/sparc/kernel/vmlinux.lds.S
+++ b/arch/sparc/kernel/vmlinux.lds.S
@@ -48,6 +48,11 @@  SECTIONS
 	{
 		_text = .;
 		HEAD_TEXT
+	        ALIGN_FUNCTION();
+#ifdef CONFIG_SPARC64
+	        /* Match text section symbols in head_64.S first */
+	        *head_64.o(.text)
+#endif
 		TEXT_TEXT
 		SCHED_TEXT
 		LOCK_TEXT
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index eeadbaeccf88b..fd901951549c0 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -553,19 +553,24 @@ 
  * .text section. Map to function alignment to avoid address changes
  * during second ld run in second ld pass when generating System.map
  *
- * TEXT_MAIN here will match .text.fixup and .text.unlikely if dead
- * code elimination is enabled, so these sections should be converted
- * to use ".." first.
+ * TEXT_MAIN here will match symbols with a fixed pattern (for example,
+ * .text.hot or .text.unlikely) if dead code elimination or
+ * function-section is enabled. Match these symbols first before
+ * TEXT_MAIN to ensure they are grouped together.
+ *
+ * Also placing .text.hot section at the beginning of a page, this
+ * would help the TLB performance.
  */
 #define TEXT_TEXT							\
 		ALIGN_FUNCTION();					\
+		*(.text.asan.* .text.tsan.*)				\
+		*(.text.unknown .text.unknown.*)			\
+		*(.text.unlikely .text.unlikely.*)			\
+		. = ALIGN(PAGE_SIZE);					\
 		*(.text.hot .text.hot.*)				\
 		*(TEXT_MAIN .text.fixup)				\
-		*(.text.unlikely .text.unlikely.*)			\
-		*(.text.unknown .text.unknown.*)			\
 		NOINSTR_TEXT						\
-		*(.ref.text)						\
-		*(.text.asan.* .text.tsan.*)
+		*(.ref.text)
 
 
 /* sched.text is aling to function alignment to secure we have same