From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Mattias Rönnblom" <hofors@lysator.liu.se>,
"Tyler Retzlaff" <roretzla@linux.microsoft.com>,
dev@dpdk.org
Cc: "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: Sun, 28 Jan 2024 09:57:10 +0100 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F1AA@smartserver.smartshare.dk> (raw)
In-Reply-To: <635f0d9f-6665-426b-b778-d61e5e732fbe@lysator.liu.se>
> 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.
Continuously cleaning up old cruft in DPDK is important, especially for reducing the learning curve for newcomers to the project.
For backwards compatibility, we can still keep the obsolete macros, but they should be removed from the project itself.
>
> >>
> >> __rte_cache_aligned is shorter, provides a tiny bit of abstraction,
> and
> >> is already an established DPDK standard. So just keep the macro. If
> it
> >> would change, I would argue for it to be changed to
> rte_cache_aligned
> >> (i.e., just moving it out of __ namespace, and maybe making it
> >> all-uppercase).
> >>
> >> Non-trivial C programs wrap things all the time, standard or not.
> It's
> >> not something to be overly concerned about, imo.
> >
> > Using the cache alignment macro was obviously a bad example for
> discussing the __rte_aligned() macro.
> >
> > FYI, Tyler later agreed to introducing the RTE_CACHE_ALIGNAS you had
> proposed in an earlier correspondence.
> >
> >>
> >>>
> >>> Note: I don't mind convenience macros for common use cases, so we
> >> could also introduce the macro suggested by Mattias [1]:
> >>>
> >>> #define RTE_CACHE_ALIGNAS alignas(RTE_CACHE_LINE_SIZE)
> >>>
> >>> [1]: https://inbox.dpdk.org/dev/dc3f3131-38e6-4219-861e-
> >> b31ec10c08bb@lysator.liu.se/
> >>>
> >>>>
> >>>> 2. where should we place alignas(n) or __rte_macro (if we use a
> >> macro)
> >>>>
> >>>> Should it be on the same line as the variable or field or on the
> >>>> preceeding line?
> >>>>
> >>>> /* same line example struct */
> >>>> struct T {
> >>>> /* alignas(64) applies to field0 *not* struct T type
> >> declaration
> >>>> */
> >>>> alignas(64) void *field0;
> >>>> void *field1;
> >>>>
> >>>> ... other fields ...
> >>>>
> >>>> alignas(64) uint64_t field5;
> >>>> uint32_t field6;
> >>>>
> >>>> ... more fields ...
> >>>>
> >>>> };
> >>>>
> >>>> /* same line example array */
> >>>> alignas(64) static const uint32_t array[4] = { ... };
> >>>>
> >>>> -- or --
> >>>>
> >>>> /* preceeding line example struct */
> >>>> struct T {
> >>>> /* alignas(64) applies to field0 *not* struct T type
> >> declaration
> >>>> */
> >>>> alignas(64)
> >>>> void *field0;
> >>>> void *field1;
> >>>>
> >>>> ... other fields ...
> >>>>
> >>>> alignas(64)
> >>>> uint64_t field5;
> >>>> uint32_t field6;
> >>>>
> >>>> ... more fields ...
> >>>>
> >>>> };
> >>>>
> >>>> /* preceeding line example array */
> >>>> alignas(64)
> >>>> static const uint32_t array[4] = { ... };
> >>>>
> >>>
> >>> Searching the net for what other projects do, I came across this
> >> required placement [2]:
> >>>
> >>> uint64_t alignas(64) field5;
> >>>
> >>> [2]:
> >>
> https://lore.kernel.org/buildroot/20230730000851.6faa3391@windsurf/T/
> >>>
> >>> So let's follow the standard's intention and put them on the same
> >> line.
> >>> On an case-by-case basis, we can wrap lines if it improves
> >> readability, like we do with function headers that have a lot of
> >> attributes.
> >>>
> >>>>
> >>>> I'll submit patches for lib/* once the discussion is concluded.
> >>>>
> >>>> thanks folks
> >>>
next prev parent reply other threads:[~2024-01-28 8:57 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 [this message]
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
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=98CBD80474FA8B44BF855DF32C47DC35E9F1AA@smartserver.smartshare.dk \
--to=mb@smartsharesystems.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=roretzla@linux.microsoft.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).