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

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

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Tuesday, 30 January 2024 11.17
> 
> > 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.

I just realized that the macros in rte_common.h related to attributes are all lower case and "__" prefixed.
I guess it's an undocumented convention, so we should probably stick with it.

That would make the new macro's name "__rte_align()", which is really close to the "__rte_aligned()" it replaces. It doesn't bother me, but let's see if anyone complains about it.

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

The convenience macro should probably follow the attribute macro naming convention too:
#define __rte_cache_align __rte_align(RTE_CACHE_LINE_SIZE)