RE: [PATCH] eal: introduce atomics abstraction

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

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 8 February 2023 02.21
> 
> On Tue, Feb 07, 2023 at 11:34:14PM +0000, Honnappa Nagarahalli wrote:
> > <snip>
> >
> > > > >
> > > > > Honnappa, please could you give your view on the future of
> atomics in
> > > DPDK?
> > > > Thanks Thomas, apologies it has taken me a while to get to this
> discussion.
> > > >
> > > > IMO, we do not need DPDK's own abstractions. APIs from
> stdatomic.h
> > > (stdatomics as is called here) already serve the purpose. These
> APIs are well
> > > understood and documented.
> > >
> > > i agree that whatever atomics APIs we advocate for should align
> with the
> > > standard C atomics for the reasons you state including implied
> semantics.
> > Another point I want to make is, we need 'xxx_explicit' APIs only, as
> we want memory ordering explicitly provided at each call site. (This
> can be discussed later).
> 
> i don't have any issue with removing the non-explicit versions. they're
> just just convenience for seq_cst anyway. if people don't want them we
> don't have to have them.

I agree with Honnappa on this point.

The non-explicit versions are for lazy (or not so experienced) developers, and might impact performance if used instead of the correct explicit versions.

I'm working on porting some of our application code from DPDK's rte_atomic32 operations to modern atomics, and I'm temporarily using acq_rel with a FIXME comment on each operation until I have the overview to determine if another memory order is better for each operation. And if I don't get around to fixing the memory order, it is still a step in the right direct direction to get rid of the old __sync based atomics; and the FIXME's remain to be fixed in a later release.

So here's an idea: Alternatively to omitting the non-explicit versions, we could include them for application developers, but document them as placeholders for "memory order to be determined later" and emit a warning when used. It might speed up the transition away from old atomic operations. Alternatively, we risk thoughtless use of seq_cst with the explicit versions, which might be difficult to detect in code reviews.
 
Either way, with or without non-explicit versions, is fine with me.

> 
> >
> > >
> > > >
> > > > For environments where stdatomics are not supported, we could
> have a
> > > stdatomic.h in DPDK implementing the same APIs (we have to support
> only
> > > _explicit APIs). This allows the code to use stdatomics APIs and
> when we move
> > > to minimum supported standard C11, we just need to get rid of the
> file in DPDK
> > > repo.
> > >
> > > my concern with this is that if we provide a stdatomic.h or
> introduce names
> > > from stdatomic.h it's a violation of the C standard.
> > >
> > > references:
> > >  * ISO/IEC 9899:2011 sections 7.1.2, 7.1.3.
> > >  * GNU libc manual
> > >    https://www.gnu.org/software/libc/manual/html_node/Reserved-
> > > Names.html
> > >
> > > in effect the header, the names and in some instances namespaces
> introduced
> > > are reserved by the implementation. there are several reasons in
> the GNU libc
> > Wouldn't this apply only after the particular APIs were introduced?
> i.e. it should not apply if the compiler does not support stdatomics.
> 
> yeah, i agree they're being a bit wishy washy in the wording, but i'm
> not convinced glibc folks are documenting this as permissive guidance
> against.
> 
> >
> > > manual that explain the justification for these reservations and if
> if we think
> > > about ODR and ABI compatibility we can conceive of others.
> > >
> > > i'll also remark that the inter-mingling of names from the POSIX
> standard
> > > implicitly exposed as a part of the EAL public API has been
> problematic for
> > > portability.
> > These should be exposed as EAL APIs only when compiled with a
> compiler that does not support stdatomics.
> 
> you don't necessarily compile dpdk, the application or its other
> dynamically linked dependencies with the same compiler at the same
> time.
> i.e. basically the model of any dpdk-dev package on any linux
> distribution.
> 
> if dpdk is built without real stdatomic types but the application has
> to
> interoperate with a different kit or library that does they would be
> forced
> to dance around dpdk with their own version of a shim to hide our
> faked up stdatomics.
> 

So basically, if we want a binary DPDK distribution to be compatible with a separate application build environment, they both have to implement atomics the same way, i.e. agree on the ABI for atomics.

Summing up, this leaves us with only two realistic options:

1. Go all in on C11 stdatomics, also requiring the application build environment to support C11 stdatomics.
2. Provide our own DPDK atomics library.

(As mentioned by Tyler, the third option - using C11 stdatomics inside DPDK, and requiring a build environment without C11 stdatomics to implement a shim - is not realistic!)

I strongly want atomics to be available for use across inline and compiled code; i.e. it must be possible for both compiled DPDK functions and inline functions to perform atomic transactions on the same atomic variable.

So either we upgrade the DPDK build requirements to support C11 (including the optional stdatomics), or we provide our own DPDK atomics.

> >
> > >
> > > let's discuss this from here. if there's still overwhelming desire
> to go this route
> > > then we'll just do our best.
> > >
> > > ty