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

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> Sent: Tuesday, 30 January 2024 10.28
> 
> 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 consider such consistency a strong enough reason to introduce a macro (RTE_ALIGNAS()) for something that exists 1:1 in the C standard (alignas()). It doesn't make the code any cleaner. And since we require C11, alignas() works with all toolchains.

I guess it's a matter of taste. In this case I think it is superfluous, and prefer C11 purism. :-)

> 
> > 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).

That seems a very likely origin.
So the questions are:
1. Do Linux kernel coding conventions trump DPDK Coding Style guidelines?
2. We must change the __rte_aligned() macro, so do we keep using lower case for the new macro, or do we take the opportunity to fix it and make it upper case?

I think macros generally should be upper case, so we should make this one upper case too.
If we want to make some macros lower case, we should document when a macro can be lower case. E.g. we could allow inline function-like macros (which - unlike inline functions - can take typeless parameters) to be lower case, if they seen from the outside behave like inline functions, i.e. if they use each of their parameters exactly once.

<irony>
We should also rename likely()/unlikely() to RTE_LIKELY()/RTE_UNLIKELY()!
</irony>

> 
> > 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.

I prefer the shorter one too.

The meaning of CACHE_ALIGN (without _LINE) is unlikely to be misunderstood. But CACHE_SIZE (without _LINE) would mean something else than CACHE_LINE_SIZE.

No strong preference on this name, though.