RE: RFC abstracting atomics

["Morten Brørup" <mb@smartsharesystems.com>]

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