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

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

On 2024-01-29 20:43, Tyler Retzlaff wrote:
> 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];
> 
> 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 { ... };
> 

Sorry if I've missed some discussion on the list, but the current 
pattern of putting __rte_aligned(X) at the end doesn't work with MSVC, 
or why are we doing this? C11 purism doesn't seem like much of a driving 
force.

If one defined a macro as __declspec(align(X)) on MSVC and 
__attribute__(__aligned__(X)) on other compilers, could it do the work 
of both the above RTE_ALIGNAS() and RTE_ALIGN_TYPE()?

<a> struct <b> { int a; } <c>;

You would have to mandate the placement of such a __rte_aligned plug-in 
replacement being at <b> rather than (the more intuitive?) <a>, since 
clang doesn't like __attribute__s before the struct/union keyword, correct?

What about other <rte_common.h> __attribute__ wrappers like 
__rte_packed; would they also need to change placement to make DPDK work 
with MSVC?

> I'm not picky about what the names actualy are if you have better
> suggestions i'm happy to adopt them.
> 
> Thoughts? Comments?
> 
> Appreciate the discussion this has been helpful.
> 
> ty
>