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