Re: [PATCH] eal: introduce atomics abstraction

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Feb 02, 2023 at 09:44:52PM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Thursday, 2 February 2023 20.00
> > 
> > On Thu, Feb 02, 2023 at 09:43:58AM +0100, Morten Brørup wrote:
> > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > Sent: Wednesday, 1 February 2023 22.41
> > > >
> > > > On Wed, Feb 01, 2023 at 01:07:59AM +0000, Honnappa Nagarahalli
> > wrote:
> > > > >
> > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > Sent: Tuesday, January 31, 2023 4:42 PM
> > > > > >
> > > > > > 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.
> > > >
> > > > >
> > > > > 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.
> > >
> > > Perhaps we can use something already existing, such as this:
> > > https://android.googlesource.com/platform/bionic/+/lollipop-
> > release/libc/include/stdatomic.h
> > >
> > > >
> > > > 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
> > > > manual that explain the justification for these reservations and if
> > > > if we think about ODR and ABI compatibility we can conceive of
> > others.
> > >
> > > I we are going to move to C11 soon, I consider the shim interim, and
> > am inclined to ignore these warning factors.
> > >
> > > If we are not moving to C11 soon, I would consider these
> > disadvantages more seriously.
> > 
> > I think it's reasonable to assume that we are talking years here.
> > 
> > We've had a few discussions about minimum C standard. I think my first
> > mailing list exchanges about C99 was almost 2 years ago. Given that we
> > still aren't on C99 now (though i know Bruce has a series up) indicates
> > that progression to C11 isn't going to happen any time soon and even if
> > it was the baseline we still can't just use it (reasons described
> > later).
> > 
> > Also, i'll point out that we seem to have accepted moving to C99 with
> > one of the holdback compilers technically being non-conformant but it
> > isn't blocking us because it provides the subset of C99 features
> > without
> > being conforming that we happen to be using.
> > 
> > >
> > > >
> > > > 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.
> > >
> > > This is a very important remark, which should be considered
> > carefully! Tyler has firsthand experience with DPDK portability. If he
> > thinks porting to Windows is going to be a headache if we expose the
> > stdatomic.h API, we must listen! So, what is your gut feeling here,
> > Tyler?
> > 
> > I think this is even more of a concern with language standard than it
> > is
> > with a platform standard. Because the language standard is used across
> > platforms.
> > 
> > On the surface it looks appealing to just go through all the dpdk code
> > one last time and #include <stdatomic.h> and directly depend on names
> > that "look" standard. In practice though we aren't depending on the
> > toolchain / libc surface we are binding ourselves to the shim and the
> > implementation it provides.
> > 
> > This is aside from the mechanics of making it work in the different
> > contexts we now have to care about. Here is a story of how things
> > become tricky.
> > 
> > When i #include <stdatomic.h> which one gets used if the implementation
> > provides one? Let's force our stdatomic.h
> > 
> > Now i need to force the build system to prefer my shim header? Keeping
> > in mind that the presence of a libc stdatomic.h does not mean that the
> > toolchain in fact supports standard atomics. Okay, that's under our
> > control by editing some meson.build files maybe it isn't so bad but...
> > 
> > It seems my application also has to do the same in their build system
> > now because...
> > 
> > The type definitions (size, alignment) and code generated from the
> > body of inline functions as seen by the application built translation
> > units may differ from those in the dpdk translation units if they don't
> > use our header. The potential for ABI compat problems is increasing but
> > maybe it is managable? it can be worse...
> > 
> > We can't limit our scope to thinking that there is just an
> > application (a single binary) and dpdk. Complex applications will
> > invariably depend on other libraries and if the application needs to
> > interface with those compatibily at the ABI level using standard
> > atomics
> > then we've made it very difficult since the application has to choose
> > to
> > use our conflicting named atomic types which may not be compatible or
> > the real standard atomics.  They can of course produce horrible shims
> > of their own to interoperate.
> > 
> > We need consistency across the entire binary at runtime and i don't
> > think it's practical to say that anyone who uses dpdk has to compile
> > their whole world with our shim. So dealing with all this complexity
> > for the sake of asthetics "looking" like the standard api seems kind
> > of not worth it. Sure it saves having to deprecate later and one last
> > session of shotgun surgery but that's kind of all we get.
> > 
> > Don't think i'm being biased in favor of windows/msvc here. From the
> > perspective of the windows/msvc combination i intend to use only the
> > standard C ABI provided by the implementation. I have no intention of
> > trying to introduce support for the current ABI that doesn't use the
> > standard atomic types. my discouraging of this approach is about
> > avoiding
> > subtle to detect but very painful problems on
> > {linux,unix}/compiler<version>
> > combinations that already have a shipped/stable ABI.
> > 
> > > >
> > > > let's discuss this from here. if there's still overwhelming desire
> > to
> > > > go
> > > > this route then we'll just do our best.
> > > >
> > > > ty
> > >
> > > I have a preference for exposing the stdatomic.h API. Tyler listed
> > the disadvantages above. (I also have a preference for moving to C11
> > soon.)
> > 
> > I am eager to see this happen, but as explained in my original proposal
> > it doesn't eliminate the need for an abstraction. Unless we are willing
> > to break our compatibility promises and potentially take a performance
> > hit on some platform/compiler combinations which as i understand is not
> > acceptable.
> > 
> > >
> > > Exposing a 1:1 similar API with RTE prefixes would also be acceptable
> > for me. The disadvantage is that the names are different than the C11
> > names, which might lead to some confusion. And from an ABI stability
> > perspective, such an important API should not be marked experimental.
> > This means that years will pass before we can get rid of it again, due
> > to ABI stability policies.
> > 
> > I think the key to success with rte_ prefixed names is making
> > absolutely
> > sure we mirror the semantics and types in the standard.
> > 
> > I will point out one bit of fine print here is that we will not support
> > atomic operations on struct/union types (something the standard
> > supports).
> > With the rte_ namespace i think this becomes less ambiguous, if we
> > present
> > standard C names though what's to avoid the confusion? Aside from it
> > fails
> > to compile with one compiler vs another.
> > 
> > I agree that this may be around for years. But how many years depends a
> > lot on how long we have to maintain compatibility for the existing
> > platform/compiler combinations that can't (and aren't enabled) to use
> > the standard.
> > 
> > Even if we introduced standard names we still have to undergo some kind
> > of mutant deprecation process to get the world to recompile everything
> > against the actual standard, so it doesn't give us forward
> > compatibility.
> > 
> > Let me know what folks would like to do, i guess i'm firmly leaned
> > toward no-shim and just rte_ explicit. But as a community i'll pursue
> > whatever you decide.
> > 
> > Thanks!
> 
> Tyler is making a very strong case here.
> 
> I have changed my mind, and now support Tyler's approach.
> 

Having read through the whole thread a second time, I am starting to
realise how complex a problem this could be. From what I read, I would tend
towards the opinion that we shouldn't provide any atomics in DPDK at all,
and just rely on the C standard ones. The main complication in any solution
I suspect is going to be the use of atomics in static inline functions we
have in our header files.

/Bruce