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

[Tyler Retzlaff]

On Tue, Jan 30, 2024 at 09:09:20AM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla at 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 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.
> > 
> > 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!)

* naming suggestion noted.
* __declspec cannot be placed after struct/union definition.
  note: there are some structs in dpdk already placing it where
        accepted by msvc (not many but a handful).
> 
> 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()?

so this seems to be unresolved contention? for object alignment do we
want a macro or not which just trivially expands to alignas(). i comment
on this a bit below.

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

my best guess of __rte_macro vs RTE_MACRO as a convention interpretation
has typically been.

__rte_macro
expresses that the macro is private, while exposed by dpdk publicly as a
necessity is meant for dpdk use only. it is not a supported api and
may change at any time without notice.

RTE_MACRO
expresses that the macro is public and part of the api and probably
imposes the usual obligations.

interestingly this maybe lends some weight to the argument that we
should use alignas(a) directly to dance around having to provide
something that looks like an api either privately or publicly for the
cases where we can use alignas(a).

second, it probably means we continue to use __rte_aligned/__rte_cache_align
for the expansions of the non-standard attributes since i am not
propsing promotion of these to be any kind of a supported/formal api.

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

so given the above we would stay with __rte_align(a)

>