[PATCH] RFC: use C11 alignas instead of GCC attribute aligned

[Mattias Rönnblom]

On 2024-01-28 09:57, Morten Brørup wrote:
>> From: Mattias Rönnblom [mailto:hofors at lysator.liu.se]
>> Sent: Saturday, 27 January 2024 20.15
>>
>> On 2024-01-26 11:18, Morten Brørup wrote:
>>>> From: Mattias Rönnblom [mailto:hofors at lysator.liu.se]
>>>> Sent: Friday, 26 January 2024 11.05
>>>>
>>>> On 2024-01-25 23:53, Morten Brørup wrote:
>>>>>> From: Tyler Retzlaff [mailto:roretzla at linux.microsoft.com]
>>>>>> Sent: Thursday, 25 January 2024 19.37
>>>>>>
>>>>>> ping.
>>>>>>
>>>>>> Please review this thread if you have time, the main point of
>>>>>> discussion
>>>>>> I would like to receive consensus on the following questions.
>>>>>>
>>>>>> 1. Should we continue to expand common alignments behind an
>>>> __rte_macro
>>>>>>
>>>>>>      i.e. what do we prefer to appear in code
>>>>>>
>>>>>>      alignas(RTE_CACHE_LINE_MIN_SIZE)
>>>>>>
>>>>>>      -- or --
>>>>>>
>>>>>>      __rte_cache_aligned
>>>>>>
>>>>>> One of the benefits of dropping the macro is it provides a clear
>>>> visual
>>>>>> indicator that it is not placed in the same location or get
>> applied
>>>>>> to types as is done with __attribute__((__aligned__(n))).
>>>>>
>>>>> We don't want our own proprietary variant of something that already
>>>> exists in the C standard. Now that we have moved to C11, the __rte
>>>> alignment macros should be considered obsolete.
>>>>
>>>> Making so something cache-line aligned is not in C11.
>>>
>>> We are talking about the __rte_aligned() macro, not the cache
>> alignment macro.
>>>
>>
>> OK, in that case, what is the relevance of question 1 above?
> 
> With this in mind, try re-reading Tyler's clarifications in this tread.
> 
> Briefly: alignas() can be attached to variables and structure fields, but not to types (like __rte_aligned()), so to align a structure:
> 
> struct foo {
> 	int alignas(64) bar; /* alignas(64) must be here */
> 	int             baz;
> }; /* __rte_aligned(64) was here, but alignas(64) cannot be here. */
> 
> So the question is: Do we want to eliminate the __rte_aligned() macro - which relies on compiler attributes - and migrate to using the C11 standard alignas()?
> 
> I think yes; after updating to C11, the workaround for pre-C11 not offering alignment is obsolete, and its removal should be on the roadmap.
> 

OK, thanks for the explanation. Interesting limitation in the standard.

If the construct the standard is offering is less effective (in this 
case, less readable) and the non-standard-based option is possible to 
implement on all compilers (i.e., on MSVC too), then we should keep the 
custom option. Especially if it's already there, but also in cases where 
it isn't.

In fact, one could argue *everything* related to alignment should go 
through something rte_, __rte_ or RTE_-prefixed. So, "int 
RTE_ALIGNAS(64) bar;". Maybe that would be silly, but it would be 
consistent with RTE_CACHE_ALIGNAS.

I would worry more about allowing DPDK developers writing clean and 
readable code, than very slightly lowering the bar for the fraction of 
newcomers experienced with the latest and greatest from the C standard, 
and *not* familiar with age-old GCC extensions.

> Continuously cleaning up old cruft in DPDK is important, especially for reducing the learning curve for newcomers to the project.
> 
> For backwards compatibility, we can still keep the obsolete macros, but they should be removed from the project itself.
> 
>>
>>>>
>>>> __rte_cache_aligned is shorter, provides a tiny bit of abstraction,
>> and
>>>> is already an established DPDK standard. So just keep the macro. If
>> it
>>>> would change, I would argue for it to be changed to
>> rte_cache_aligned
>>>> (i.e., just moving it out of __ namespace, and maybe making it
>>>> all-uppercase).
>>>>
>>>> Non-trivial C programs wrap things all the time, standard or not.
>> It's
>>>> not something to be overly concerned about, imo.
>>>
>>> Using the cache alignment macro was obviously a bad example for
>> discussing the __rte_aligned() macro.
>>>
>>> FYI, Tyler later agreed to introducing the RTE_CACHE_ALIGNAS you had
>> proposed in an earlier correspondence.
>>>
>>>>
>>>>>
>>>>> Note: I don't mind convenience macros for common use cases, so we
>>>> could also introduce the macro suggested by Mattias [1]:
>>>>>
>>>>> #define RTE_CACHE_ALIGNAS alignas(RTE_CACHE_LINE_SIZE)
>>>>>
>>>>> [1]: https://inbox.dpdk.org/dev/dc3f3131-38e6-4219-861e-
>>>> b31ec10c08bb at lysator.liu.se/
>>>>>
>>>>>>
>>>>>> 2. where should we place alignas(n) or __rte_macro (if we use a
>>>> macro)
>>>>>>
>>>>>> Should it be on the same line as the variable or field or on the
>>>>>> preceeding line?
>>>>>>
>>>>>>      /* same line example struct */
>>>>>>      struct T {
>>>>>>          /* alignas(64) applies to field0 *not* struct T type
>>>> declaration
>>>>>> */
>>>>>>          alignas(64) void *field0;
>>>>>>          void *field1;
>>>>>>
>>>>>>          ... other fields ...
>>>>>>
>>>>>>          alignas(64) uint64_t field5;
>>>>>>          uint32_t field6;
>>>>>>
>>>>>>          ... more fields ...
>>>>>>
>>>>>>      };
>>>>>>
>>>>>>      /* same line example array */
>>>>>>      alignas(64) static const uint32_t array[4] = { ... };
>>>>>>
>>>>>>      -- or --
>>>>>>
>>>>>>      /* preceeding line example struct */
>>>>>>      struct T {
>>>>>>          /* alignas(64) applies to field0 *not* struct T type
>>>> declaration
>>>>>> */
>>>>>>          alignas(64)
>>>>>>          void *field0;
>>>>>>          void *field1;
>>>>>>
>>>>>>          ... other fields ...
>>>>>>
>>>>>>          alignas(64)
>>>>>>          uint64_t field5;
>>>>>>          uint32_t field6;
>>>>>>
>>>>>>          ... more fields ...
>>>>>>
>>>>>>      };
>>>>>>
>>>>>>      /* preceeding line example array */
>>>>>>      alignas(64)
>>>>>>      static const uint32_t array[4] = { ... };
>>>>>>
>>>>>
>>>>> Searching the net for what other projects do, I came across this
>>>> required placement [2]:
>>>>>
>>>>> uint64_t alignas(64) field5;
>>>>>
>>>>> [2]:
>>>>
>> https://lore.kernel.org/buildroot/20230730000851.6faa3391@windsurf/T/
>>>>>
>>>>> So let's follow the standard's intention and put them on the same
>>>> line.
>>>>> On an case-by-case basis, we can wrap lines if it improves
>>>> readability, like we do with function headers that have a lot of
>>>> attributes.
>>>>>
>>>>>>
>>>>>> I'll submit patches for lib/* once the discussion is concluded.
>>>>>>
>>>>>> thanks folks
>>>>>