On 2024-01-29 20:43, Tyler Retzlaff wrote:
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 { ... };
Sorry if I've missed some discussion on the list, but the current
pattern of putting __rte_aligned(X) at the end doesn't work with MSVC,
or why are we doing this? C11 purism doesn't seem like much of a driving
force.
If one defined a macro as __declspec(align(X)) on MSVC and
__attribute__(__aligned__(X)) on other compilers, could it do the work
of both the above RTE_ALIGNAS() and RTE_ALIGN_TYPE()?
<a> struct <b> { int a; } <c>;
You would have to mandate the placement of such a __rte_aligned plug-in
replacement being at <b> rather than (the more intuitive?) <a>, since
clang doesn't like __attribute__s before the struct/union keyword, correct?
What about other <rte_common.h> __attribute__ wrappers like
__rte_packed; would they also need to change placement to make DPDK work
with MSVC?
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
