On 2/21/24 12:38, Masahiro Yamada wrote: > On Wed, Feb 21, 2024 at 7:38 PM Petr Pavlu <petr.pavlu@xxxxxxxx> wrote: >> >> On 2/20/24 14:39, Masahiro Yamada wrote: >>> On Fri, Feb 16, 2024 at 12:16 AM Petr Pavlu <petr.pavlu@xxxxxxxx> 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@xxxxxxxx> >>>> --- >>> >>> [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 Ah, that looks as the simplest option, thanks. -- Petr
