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

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2024-01-30 09:09, Morten Brørup wrote:
>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>> Sent: Monday, 29 January 2024 20.44
>>
>> On Sun, Jan 28, 2024 at 11:00:31AM +0100, Mattias Rönnblom wrote:
>>> On 2024-01-28 09:57, Morten Brørup wrote:
>>>>> From: Mattias Rönnblom [mailto:hofors@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@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@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.
>>
>> I’d just like to summarize where my understanding is at after reviewing
>> this discussion and my downstream branch. But I also want to make it
>> clear that we probably need to use both standard C and non-standard
>> attribute/declspec for object and struct/union type alignment
>> respectively.
>>
>> I've assumed we prefer avoiding per-compiler conditional expansion when
>> possible through the use of standard C mechanisms. But there are
>> instances when alignas is awkward.
>>
>> So I think the following is consistent with what Mattias is advocating
>> sans any discussions related to actual naming of macros.
>>
>> We should have 2 macros, upon which others may be built to expand to
>> well-known values for e.g. cache line size.
>>
>> RTE_ALIGNAS(n) object;
>>
>> * This macro is used to align C objects i.e. variable, array,
>> struct/union
>>    fields etc.
>> * Trivially expands to alignas(n) for all toolchains.
>> * Placed in a location that both C and C++ translation units accept
>> that
>>    is on the same line preceeding the object type.
>>    example:
>>    // RTE_ALIGNAS(n) object;
>>    RTE_ALIGNAS(16) char somearray[16];
> 
> Shouldn't the location be:
> 
> [static] [const] char RTE_ALIGNAS(16) somearray[16];
> 
>>
>> RTE_ALIGN_TYPE(n)
>>
>> * This macro is used to align struct/union types.
>> * Conditionally expands to __declspec(align(n)) (msvc) and
>>    __attribute__((__aligned__(n))) (for all other toolchains)
>> * Placed in a location that for all gcc,clang,msvc and both C and C++
>>    translation units accept.
>>    example:
>>    // {struct,union} RTE_ALIGN_TYPE(n) tag { ... };
>>    struct RTE_ALIGN_TYPE(64) sometype { ... };
>>
>> I'm not picky about what the names actualy are if you have better
>> suggestions i'm happy to adopt them.
> 
> Being able to align types is very convenient, and since it works on all toolchains, replacing __rte_aligned() with RTE_ALIGN() (in present tense, like "inline" not past tense like "inlined") is perfectly acceptable with me. (I suppose MSVC requires this other location when using it, so we simply have to accept that. It's a minor change only, it could have been much worse!)
> 
> Now, if we have RTE_ALIGN[_TYPE](), what do we need RTE_ALIGNAS() for?
> 
> And what is the point of introducing RTE_ALIGNAS() when the C standard already has alignas()?
> 

The argument I made, which may not be a very strong one, is if you 
needed two constructs for alignment-related purposes, they should both 
have the RTE_ prefix, for consistency reasons.

> I don't know why the existing alignment macros are lower case and prefixed with double underscore (__rte_macro), instead of upper case like other macros (RTE_MACRO). If someone can explain why that code convention is still relevant, the new macros should follow it; otherwise follow the code convention for macros, i.e. RTE_MACRO.
> 

A lot the low-level DPDK stuff looks like it's borrowed from either 
Linux or *BSD kernels. __aligned(16) (Linux, FreeBSD) -> __rte_aligned(16).

> PS: #define RTE_CACHE_ALIGN RTE_ALIGN(RTE_CACHE_LINE_SIZE) for brevity still seems like a good idea to me.
> 

RTE_CACHE_ALIGN or RTE_CACHE_LINE_ALIGN?

The former is shorter, the latter consistent with RTE_CACHE_LINE_SIZE. I 
think I prefer the former.