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