RE: [PATCH] eal: introduce atomics abstraction

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

> From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]
> Sent: Thursday, 9 February 2023 01.17
> 
> <snip>
> 
> > > > > >
> > > > > > >
> > > > > > > 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.
> I think I agree here.
> 
> >
> > >
> > > 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.
> I am not sure why you are calling it as ABI break. Referring to [1], I
> just see wrappers around intrinsics (though [2] does not use the
> intrinsics).
> 
> [1] https://github.com/gcc-
> mirror/gcc/blob/master/gcc/ginclude/stdatomic.h
> [2] https://github.com/llvm-
> mirror/clang/blob/master/lib/Headers/stdatomic.h

Good input, Honnappa.

This means that the ABI break is purely academic, and there is no ABI breakage in reality.

Since the underlying implementation is the same, it is perfectly OK to mix C11 and intrinsic atomics, even when the DPDK and the application are built in different environments (with and without C11 atomics, or vice versa).

This eliminates my only remaining practical concern about this approach.

> 
> >
> > the abstraction (whatever namespace it resides) allows the existing
> > toolchain/platform combinations to maintain compatibility by
> defaulting to
> > current non-standard intrinsics.
> How about using the intrinsics (__atomic_xxx) name space for
> abstraction? This covers the GCC and Clang compilers.
> If there is another platform that uses the same name space for
> something else, I think DPDK should not be supporting that platform.
> What problems do you see?
> 
> >
> > 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.
> +1
> 
> > * we will make no attempt to support operate on struct/union atomics
> >   with our apis.
> +1
> 
> > * 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