Linux-KBuild Archive mirror
 help / color / mirror / Atom feed
From: Petr Pavlu <petr.pavlu@suse.com>
To: Masahiro Yamada <masahiroy@kernel.org>
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 13:58:28 +0100	[thread overview]
Message-ID: <5f372b1f-3f3e-4b44-bc21-95dd154bd61a@suse.com> (raw)
In-Reply-To: <CAK7LNAQ=iz8iY_VXmzGuU+7YPnaExm769k1BqCpSYvqSfRr=Fg@mail.gmail.com>

On 2/21/24 12:38, Masahiro Yamada wrote:
> 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

Ah, that looks as the simplest option, thanks.

-- Petr

      reply	other threads:[~2024-02-21 12:58 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-15 15:16 [PATCH v2] kbuild: Use -fmin-function-alignment when available Petr Pavlu
2024-02-17  0:07 ` Nathan Chancellor
2024-02-19 17:20 ` Mark Rutland
2024-02-20 15:28   ` Petr Pavlu
2024-02-21 10:50     ` Mark Rutland
2024-02-20 13:39 ` Masahiro Yamada
2024-02-21 10:38   ` Petr Pavlu
2024-02-21 10:49     ` Mark Rutland
2024-02-21 11:38     ` Masahiro Yamada
2024-02-21 12:58       ` Petr Pavlu [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5f372b1f-3f3e-4b44-bc21-95dd154bd61a@suse.com \
    --to=petr.pavlu@suse.com \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=masahiroy@kernel.org \
    --cc=nathan@kernel.org \
    --cc=nicolas@fjasle.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).