Re: [PATCH] eal: introduce atomics abstraction

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

On Wed, Feb 08, 2023 at 09:31:32AM +0100, Morten Brørup wrote:
> > 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.

i think it may be cleaner to ust remove the non-explicit versions. if we
are publishing api in the rte_xxx namespace then there are no
pre-existing expectations that they are present.

it also reduces the api surface that eventually gets retired ~years from
now when all ports and compilers in the matrix are std=C11.

i'll update the patch accordingly just so we have a visual.

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

i consider it a mandatory requirement. i don't see practically how we
could withdraw existing use and even if we had clean way i don't see why
we would want to. so this item is defintely settled if you were
concerned.

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

i think the issue of requiring a toolchain conformant to a specific
standard is a separate matter because any adoption of C11 standard
atomics is a potential abi break from the current use of intrinsics.

the abstraction (whatever namespace it resides) allows the existing
toolchain/platform combinations to maintain compatibility by defaulting
to current non-standard intrinsics.

once in place it provides an opportunity to introduce new toolchain/platform
combinations and enables an opt-in capability to use stdatomics on
existing toolchain/platform combinations subject to community discussion
on how/if/when.

it would be good to get more participants into the discussion so i'll cc
techboard for some attention. i feel like the only area that isn't
decided is to do or not do this in rte_ namespace.

i'm strongly in favor of rte_ namespace after discussion, mainly due to
to disadvantages of trying to overlap with the standard namespace while not
providing a compatible api/abi and because it provides clear
disambiguation of that difference in semantics and compatibility with
the standard api.

so far i've noted the following

* we will not provide the non-explicit apis.
* we will make no attempt to support operate on struct/union atomics
  with our apis.
* we will mirror the standard api potentially in the rte_ namespace to
  - reference the standard api documentation.
  - assume compatible semantics (sans exceptions from first 2 points).

my vote is to remove 'potentially' from the last point above for reasons
previously discussed in postings to the mail thread.

thanks all for the discussion, i'll send up a patch removing
non-explicit apis for viewing.

ty