> From: Mattias Rönnblom [mailto:hof...@lysator.liu.se] > Sent: Tuesday, 30 January 2024 10.28 > > On 2024-01-30 09:09, Morten Brørup wrote: > >> From: Tyler Retzlaff [mailto:roret...@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: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]; > > > > 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.
