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:hof...@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:hof...@lysator.liu.se]
> >>>>Sent: Friday, 26 January 2024 11.05
> >>>>
> >>>>On 2024-01-25 23:53, Morten Brørup wrote:
> >>>>>>From: Tyler Retzlaff [mailto:roret...@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 { ... };
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
