From: Tyler Retzlaff <roretzla@linux.microsoft.com>
To: "Mattias Rönnblom" <hofors@lysator.liu.se>
Cc: "Morten Brørup" <mb@smartsharesystems.com>,
dev@dpdk.org, "Mattias Rönnblom" <mattias.ronnblom@ericsson.com>,
"Anatoly Burakov" <anatoly.burakov@intel.com>,
"Bruce Richardson" <bruce.richardson@intel.com>,
"David Christensen" <drc@linux.vnet.ibm.com>,
"Harry van Haaren" <harry.van.haaren@intel.com>,
"Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru>,
"Min Zhou" <zhoumin@loongson.cn>,
"Ruifeng Wang" <ruifeng.wang@arm.com>,
"Stanislaw Kardach" <kda@semihalf.com>,
thomas@monjalon.net
Subject: Re: [PATCH] RFC: use C11 alignas instead of GCC attribute aligned
Date: Tue, 30 Jan 2024 09:39:28 -0800 [thread overview]
Message-ID: <20240130173928.GA2943@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> (raw)
In-Reply-To: <cf8fec4c-2224-43c8-aee2-37cc89278191@lysator.liu.se>
On Tue, Jan 30, 2024 at 09:08:21AM +0100, Mattias Rönnblom wrote:
> 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:hofors@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:hofors@lysator.liu.se]
> >>>>>>Sent: Friday, 26 January 2024 11.05
> >>>>>>
> >>>>>>On 2024-01-25 23:53, Morten Brørup wrote:
> >>>>>>>>From: Tyler Retzlaff [mailto:roretzla@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.
__rte_aligned(X) at the end doesn't work with MSVC __declspec(align(n))
>
> 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>;
yes for struct/union. but only when placed at location you mark as <b>
when compiling both C and C++ for all toolchains.
maybe, for objects but ideally, we prefer alignas for consistent semantics
defined by standard rather than accomodating potential implementation
differences when conditionally expanding __aligned vs __declspec. as you
have noted __declspec has limitations/variations when compared to
__attribute__((__aligned__(n))).
>
> 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?
for struct/union there is a single placement accepted by all toolchains
for both C and C++ and it is <b>.
>
> What about other <rte_common.h> __attribute__ wrappers like
> __rte_packed; would they also need to change placement to make DPDK
> work with MSVC?
packing is a different problem that needs a separate RFC and discussion
of it's own.
>
> >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
> >
next prev parent reply other threads:[~2024-01-30 17:39 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-15 17:39 Tyler Retzlaff
2023-11-15 17:39 ` [PATCH] eal: " Tyler Retzlaff
2023-11-15 18:13 ` Bruce Richardson
2023-11-15 18:27 ` Tyler Retzlaff
2023-11-15 20:08 ` Morten Brørup
2023-11-15 21:03 ` Tyler Retzlaff
2023-11-15 22:43 ` Stanisław Kardach
2023-11-16 10:12 ` Mattias Rönnblom
2024-01-25 18:37 ` [PATCH] RFC: " Tyler Retzlaff
2024-01-25 22:53 ` Morten Brørup
2024-01-25 23:31 ` Tyler Retzlaff
2024-01-26 10:05 ` Mattias Rönnblom
2024-01-26 10:18 ` Morten Brørup
2024-01-27 19:15 ` Mattias Rönnblom
2024-01-28 8:57 ` Morten Brørup
2024-01-28 10:00 ` Mattias Rönnblom
2024-01-29 19:43 ` Tyler Retzlaff
2024-01-30 8:08 ` Mattias Rönnblom
2024-01-30 17:39 ` Tyler Retzlaff [this message]
2024-01-30 17:59 ` Bruce Richardson
2024-01-30 18:01 ` Bruce Richardson
2024-01-30 18:04 ` Tyler Retzlaff
2024-01-30 18:18 ` Mattias Rönnblom
2024-01-31 16:04 ` Mattias Rönnblom
2024-01-30 8:09 ` Morten Brørup
2024-01-30 9:28 ` Mattias Rönnblom
2024-01-30 10:17 ` Morten Brørup
2024-01-30 13:00 ` Morten Brørup
2024-01-30 17:54 ` Tyler Retzlaff
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240130173928.GA2943@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net \
--to=roretzla@linux.microsoft.com \
--cc=anatoly.burakov@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=drc@linux.vnet.ibm.com \
--cc=harry.van.haaren@intel.com \
--cc=hofors@lysator.liu.se \
--cc=kda@semihalf.com \
--cc=konstantin.v.ananyev@yandex.ru \
--cc=mattias.ronnblom@ericsson.com \
--cc=mb@smartsharesystems.com \
--cc=ruifeng.wang@arm.com \
--cc=thomas@monjalon.net \
--cc=zhoumin@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).