Re: [PATCH v2] kbuild: Use -fmin-function-alignment when available

From: Masahiro Yamada <masahiroy@kernel.org>
To: Petr Pavlu <petr.pavlu@suse.com>
Cc: nathan@kernel.org, nicolas@fjasle.eu, mark.rutland@arm.com,
	 linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] kbuild: Use -fmin-function-alignment when available
Date: Wed, 21 Feb 2024 20:38:08 +0900	[thread overview]
Message-ID: <CAK7LNAQ=iz8iY_VXmzGuU+7YPnaExm769k1BqCpSYvqSfRr=Fg@mail.gmail.com> (raw)
In-Reply-To: <9b067ec7-34e2-437b-a41b-319aaee4c7e6@suse.com>

On Wed, Feb 21, 2024 at 7:38 PM Petr Pavlu <petr.pavlu@suse.com> wrote:
>
> On 2/20/24 14:39, Masahiro Yamada wrote:
> > On Fri, Feb 16, 2024 at 12:16 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
> >>
> >> GCC recently added option -fmin-function-alignment, which should appear
> >> in GCC 14. Unlike -falign-functions, this option causes all functions to
> >> be aligned at the specified value, including the cold ones.
> >>
> >> Detect availability of -fmin-function-alignment and use it instead of
> >> -falign-functions when present. Introduce CC_HAS_SANE_FUNCTION_ALIGNMENT
> >> and make the workarounds for the broken function alignment conditional
> >> on this setting.
> >>
> >> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> >> ---
> >
> > [snip]
> >
> >> index dfb963d2f862..5a6fed4ad3df 100644
> >> --- a/kernel/exit.c
> >> +++ b/kernel/exit.c
> >> @@ -1920,7 +1920,10 @@ EXPORT_SYMBOL(thread_group_exited);
> >>   *
> >>   * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c11
> >>   */
> >> -__weak __function_aligned void abort(void)
> >> +#ifndef CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
> >> +__function_aligned
> >> +#endif
> >> +__weak void abort(void)
> >>  {
> >>         BUG();
> >
> >
> >
> >
> >
> > __function_aligned is conditionally defined in
> > include/linux/compiler_types.h, and then it is
> > conditionally used in kernel/exit.c
> >
> > This is unreadable.
> >
> >
> >
> >
> > You may want to move CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT
> > to include/linux/compiler_types.h, as this is more
> > aligned with what you did for __cold.
> >
> >
> >
> > if !defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) && \
> >                CONFIG_FUNCTION_ALIGNMENT > 0
> > #define __function_aligned       __aligned(CONFIG_FUNCTION_ALIGNMENT)
> > #else
> > #define __function_aligned
> > #endif
> >
> >
> >
> >
> >
> > However, an even more elegant approach is to unify
> > the two #ifdef blocks because __cold and __function_aligned
> > are related to each other.
> >
> >
> >
> > #if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || \
> >                  (CONFIG_FUNCTION_ALIGNMENT == 0)
> > #define __cold                 __attribute__((__cold__))
> > #define __function_aligned
> > #else
> > #define __cold
> > #define __function_aligned     __aligned(CONFIG_FUNCTION_ALIGNMENT)
> > #endif
>
> I didn't want to make __function_aligned conditional on
> CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT because the macro has a fairly
> general name. One could decide to mark a variable as __function_aligned
> and with the above code, it would no longer produce an expected result
> when -fmin-function-alignment is available.
>
> __function_aligned was introduced c27cd083cfb9 ("Compiler attributes:
> GCC cold function alignment workarounds") only for aligning the abort()
> function and has not been so far used anywhere else.
>
> If the above unification is preferred, I think it would be good to
> additionally rename the macro in order to prevent the mentioned misuse,
> perhaps to __force_function_alignment.
>
> #if defined(CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT) || \
>                 (CONFIG_FUNCTION_ALIGNMENT == 0)
> #define __cold                          __attribute__((__cold__))
> #define __force_function_alignment
> #else
> #define __cold
> #define __force_function_alignment      __aligned(CONFIG_FUNCTION_ALIGNMENT)
> #endif
>
> Would this be ok?

Or, you can always add __function_aligned to abort()
whether CONFIG_CC_HAS_SANE_FUNCTION_ALIGNMENT is y or n.

I think you did not need to modify kernel/exit.c

--
Best Regards
Masahiro Yamada