DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Tyler Retzlaff" <roretzla@linux.microsoft.com>
Cc: "Bruce Richardson" <bruce.richardson@intel.com>, <dev@dpdk.org>
Subject: RE: RFC abstracting atomics
Date: Wed, 11 Jan 2023 08:45:20 +0100	[thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8764B@smartserver.smartshare.dk> (raw)
In-Reply-To: <20230110203124.GD21476@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Tuesday, 10 January 2023 21.31
> 
> On Tue, Jan 10, 2023 at 12:45:05PM +0100, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Tuesday, 10 January 2023 10.17
> > >
> > > On Mon, Jan 09, 2023 at 02:56:04PM -0800, Tyler Retzlaff wrote:
> > > > hi folks,
> > > >
> > > > i would like to introduce a layer of abstraction that would allow
> > > > optional use of standard C11 atomics when the platform /
> toolchain
> > > > combination has them available.
> >
> > It would be great if that abstraction layer could expose the exact
> same (or very similar) interface as standard C11 atomics, so the API is
> given in advance, and developers don't have to learn the API of yet
> another shim. It would also make it easier getting rid of the
> abstraction layer at a later time.
> 
> the patch series i have prototype exactly mirrors the stdandard atomics
> from C11. it requires a few fixups to fill convenience builtins that
> the
> standard doesn't provide e.g. `__atomic_add_fetch(&o, v, order)`
> becomes
> `__atomic_fetch_add(&o, v, order) + v;` sort of stuff, mostly minor.

Excellent!

> 
> >
> > > >
> > > > making the option usable would be a phased approach intended to
> focus
> > > > review and minimize dealing with churn on such a broad change.
> > > >
> > > > 1. provide an initial series to add the abstraction and the
> ability
> > > >    control enablement with a meson option enable_stdatomics=false
> > > will
> > > >    be the default.
> > > >
> > > >    for all existing platform / toolchain combinations the default
> > > would
> > > >    remain false. i.e. i have no plans to enable it for existing
> > > platforms
> > > >    toolchain combinations but leaves a change of default open to
> the
> > > >    community as a future discussion if it is desired.
> > > >
> > > > 2. once the initial abstraction is integrated a series will be
> > > introduced to
> > > >    port the tree to the abstraction with enable_stdatomics=false.
> the
> > > goal
> > > >    being low or no change to the current use of gcc builtin C++11
> > > memory
> > > >    model atomics.
> > > >
> > > > 3. once the tree is ported a final series will be introduced to
> > > introduce
> > > >    the remaining change to allow the use of
> enable_stdatomics=true.
> > > >
> > > > would appreciate any assistance / suggestions you can provide to
> > > > introduce the abstraction smoothly.
> > > >
> > >
> > > Plan generally sounds ok. However, beyond point #3, would there
> then be
> > > plans to remove the option and always use stdatomics in future?
> >
> > We are already trying to migrate atomics in DPDK from using __sync
> built-ins (used in rte_atomic) to using __atomic built-ins; e.g. new
> contributions are recommended to use the latter (and not rte_atomic).
> When DPDK migrates to C11, I assume stdatomics will become the new
> standard for atomics in DPDK, and the process for migrating to
> stdatomics will resemble the currently ongoing process for migrating
> from rte_atomic to __atomic built-ins, which seems to be relatively
> uncomplicated (but slow).
> 
> the few __sync left in use are easily converted. there might be some
> argument against converting __sync -> __atomic because of sub-optimal
> codegen on older gcc for the semantically equivalent replacements.

It would be easy to simply replace __sync with similar __atomic functions in the rte_atomic functions, but that is not the challenge.
The challenge is that the use of rte_atomic assumes the very strict order imposed by __sync (a full barrier), but the performance of some use cases could be improved by using the specific order required for those use cases (e.g. acquire/release semantics).

> 
> the only comment i have on current/existing rte_atomic is they're
> annoyingly stealing a little bit of the rte_ namespace that would allow
> convenient alignment with the standard names. also some of them provide
> only signed or unsigned variations which forces kind of non-standardy
> things to be done in the code but are perfectly acceptable when using
> the gcc implementation.

Yes, their signedness is slightly annoying, and makes some code look weird.

> 
> with the set of macros i'm introducing migrating to direct use of C11
> atomics would likely be just a big search and replace (or that is my
> goal).

Again, excellent!

> 
> > Another thing: The proposed layer of abstraction should only live
> until DPDK supports C11, which is hopefully relatively soon. So is such
> a (hopefully) short-lived abstraction layer worth the effort?
> 
> i kept hoping we would just baseline on C11 but after prototyping the
> abstraction i realize there may be some valid arguments for not moving
> to stdatomics now or possibly ever for some platform/toolchain
> combinations (so long as they are declared supported). given the drawn
> out discussion about RHEL-7 has shown that the migration path may be
> long term. more possible headwinds were listed in my reply to Bruce.

No objections from me - especially with the stdatomics roadmap you described.

> 
> >
> > >
> > > To slightly expand the scope of the discussion - would it be
> worthwhile
> > > putting these abstractions in a new library in DPDK other than EAL,
> to
> > > start the process of splitting out some of the lower-level material
> > > from
> > > that library?
> >
> > While I applaud this idea, I think we first need to discuss in detail
> how to split up EAL, and especially what new structure we want to end
> up with instead. E.g. we already have architecture specific code in
> various libraries and drivers, which proves that architecture specific
> stuff does not necessarily need to go into the EAL. We also need to
> discuss challenges/barriers to separating stuff from the EAL, and how
> we can solve those challenges; e.g. some parts are inside the EAL only
> due to startup sequence requirements, which might be solvable by
> offering better startup sequencing support for libraries in the EAL.
> Perhaps the EAL has even outlived itself, and could be replaced by set
> of DPDK Core libraries instead - some could be optional (at build
> time). But let's make this a separate discussion.
> 
> i gave some detail about this to my reply with Bruce. i think when i
> post the series we can discuss it more. but for atomics i expect it
> isn't worth separating out.

I think we're all in agreement at this point.
But as Bruce just did, we should think twice every time something is being proposed to be added to the EAL.


  reply	other threads:[~2023-01-11  7:45 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-09 22:56 Tyler Retzlaff
2023-01-10  9:16 ` Bruce Richardson
2023-01-10 11:45   ` Morten Brørup
2023-01-10 20:31     ` Tyler Retzlaff
2023-01-11  7:45       ` Morten Brørup [this message]
2023-01-10 20:10   ` Tyler Retzlaff
2023-01-11 10:10     ` Bruce Richardson
2023-01-11 10:23       ` Morten Brørup
2023-01-11 11:56         ` Bruce Richardson
2023-01-11 12:46           ` Morten Brørup
2023-01-11 14:18             ` Bruce Richardson
2023-01-11 15:07               ` Morten Brørup
2023-01-13 14:18                 ` Ben Magistro
2023-01-13 16:10                   ` Jerin Jacob
2023-01-13 17:17                     ` 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=98CBD80474FA8B44BF855DF32C47DC35D8764B@smartserver.smartshare.dk \
    --to=mb@smartsharesystems.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=roretzla@linux.microsoft.com \
    /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).