Received-SPF: pass (google.com: domain of linux-kernel+bounces-74743-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <5f372b1f-3f3e-4b44-bc21-95dd154bd61a@suse.com>
Date: Wed, 21 Feb 2024 13:58:28 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] kbuild: Use -fmin-function-alignment when available
Content-Language: en-US
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
References: <20240215151642.8970-1-petr.pavlu@suse.com>
 <CAK7LNAS=knGxd9ZRo37CaKTvjcc28bqNasx+XuqbV3S+XV=HtQ@mail.gmail.com>
 <9b067ec7-34e2-437b-a41b-319aaee4c7e6@suse.com>
 <CAK7LNAQ=iz8iY_VXmzGuU+7YPnaExm769k1BqCpSYvqSfRr=Fg@mail.gmail.com>
From: Petr Pavlu <petr.pavlu@suse.com>
In-Reply-To: <CAK7LNAQ=iz8iY_VXmzGuU+7YPnaExm769k1BqCpSYvqSfRr=Fg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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