RE: C11 atomics adoption blocked

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

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Tuesday, 8 August 2023 21.20
> 
> On Tue, Aug 08, 2023 at 07:23:41PM +0100, Bruce Richardson wrote:
> > On Tue, Aug 08, 2023 at 10:53:03AM -0700, Tyler Retzlaff wrote:
> > > Hi folks,
> > >
> > > Moving this discussion to the dev mailing list for broader comment.
> > >
> > > Unfortunately, we've hit a roadblock with integrating C11 atomics
> > > for DPDK.  The main issue is that GNU C++ prior to -std=c++23
> explicitly
> > > cannot be integrated with C11 stdatomic.h.  Basically, you can't
> include
> > > the header and you can't use `_Atomic' type specifier to declare
> atomic
> > > types. This is not a problem with LLVM or MSVC as they both allow
> > > integration with C11 stdatomic.h, but going forward with C11 atomics
> > > would break using DPDK in C++ programs when building with GNU g++.
> > >
> > > Essentially you cannot compile the following with g++.
> > >
> > >   #include <stdatomic.h>
> > >
> > >   int main(int argc, char *argv[]) { return 0; }
> > >
> > >   In file included from atomic.cpp:1:
> > >   /usr/lib/gcc/x86_64-pc-cygwin/11/include/stdatomic.h:40:9: error:
> > >   ‘_Atomic’ does not name a type
> > >      40 | typedef _Atomic _Bool atomic_bool;
> > >
> > >   ... more errors of same ...
> > >
> > > It's also acknowledged as something known and won't fix by GNU g++
> > > maintainers.
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60932
> > >
> > > Given the timeframe I would like to propose the minimally invasive,
> > > lowest risk solution as follows.
> > >
> > > 1.  Adopt stdatomic.h for all Windows targets, leave all Linux/BSD
> targets
> > >     using GCC builtin C++11 memory model atomics.
> > > 2.  Introduce a macro that allows _Atomic type specifier to be
> applied to
> > >     function parameter, structure field types and variable
> declarations.
> > >
> > >     * The macro would expand empty for Linux/BSD targets.
> > >     * The macro would expand to C11 _Atomic keyword for Windows
> targets.
> > >
> > > 3.  Introduce basic macro that allows __atomic_xxx  for normalized
> use
> > >     internal to DPDK.
> > >
> > >     * The macro would not be defined for Linux/BSD targets.
> > >     * The macro would expand __atomic_xxx to corresponding
> stdatomic.h
> > >       atomic_xxx operations for Windows targets.
> > >

Regarding naming of these macros (suggested in 2. and 3.), they should probably bear the rte_ prefix instead of overlapping existing names, so applications can also use them directly.

E.g.:
#define rte_atomic for _Atomic or nothing,
#define rte_atomic_fetch_add() for atomic_fetch_add() or __atomic_fetch_add(), and
#define RTE_MEMORY_ORDER_SEQ_CST for memory_order_seq_cst or __ATOMIC_SEQ_CST.

Maybe that is what you meant already. I'm not sure of the scope and details of your suggestion here.

> > > 4.  We re-evaluate adoption of C11 atomics and corresponding
> requirement of
> > >     -std=c++23 compliant compiler at the next long term ABI promise
> release.
> > >
> > > Q: Why not define macros that look like the standard and expand
> those
> > >    names to builtins?
> > > A: Because introducing the names is a violation of the C standard,
> we
> > >    can't / shouldn't define atomic_xxx names in the applications
> namespace
> > >    as we are not ``the implementation''.
> > > A: Because the builtins offer a subset of stdatomic.h capability
> they
> > >    can only operate on pointer and integer types. If we presented
> the
> > >    stdatomic.h names there might be some confusion attempting to
> perform
> > >    atomic operations on e.g. _Atomic specified struct would fail but
> only
> > >    on BSD/Linux builds (with the proposed solution).
> > >
> >
> > Out of interest, rather than splitting on Windows vs *nix OS for the
> > atomics, what would it look like if we split behaviour based on C vs
> C++
> > use? Would such a thing work?
> 
> Unfortunately no. The reason is binary packages and we don't know which
> toolchain consumes them.
> 
> For example.
> 
> Assume we build libeal-dev package with gcc. We'll end up with headers
> that contain the _Atomic specifier.
> 
> Now we write an application and build it with
>   * gcc, sure works fine it knows about _Atomic
>   * clang, same as gcc
>   * clang++, works but is implementation detail that it works (it isn't
> standard)
>   * g++, does not work
> 
> So the LCD is build package without _Atomic i.e. what we already have
> today
> on BSD/Linux.
> 

I agree with Tyler's conceptual solution as proposed in the first email in this thread, but with a twist:

Instead of splitting Windows vs. Linux/BSD, the split should be a build time configuration parameter, e.g. USE_STDATOMIC_H. This would be default true for Windows, and default false for Linux/BSD distros - i.e. behave exactly as Tyler described.

Having a build time configuration parameter would also allow the use of stdatomic.h for applications that build DPDK from scratch, instead of using the DPDK included with the distro. This could be C applications built with the distro's C compiler or some other C compiler, or C++ applications built with a newer GNU C++ compiler or CLANG++.

It might also allow building C++ applications using an old GNU C++ compiler on Windows (where the application is built with DPDK from scratch). Not really an argument, just mentioning it.

> > Also, just wondering about the scope of the changes here. How many
> header
> > files are affected where we publicly expose atomics?
> 
> So what is impacted is roughly what is in my v4 series that raised my
> attention to the issue.
> 
> https://patchwork.dpdk.org/project/dpdk/list/?series=29086
> 
> We really can't solve the problem by not talking about atomics in the
> API because of the performance requirements of the APIs in question.
> 
> e.g. It is stuff like rte_rwlock, rte_spinlock, rte_pause all stuff that
> can't have additional levels of indirection because of the overhead
> involved.

I strongly support this position. Hiding all atomic operations in non-inline functions will have an unacceptable performance impact! (I know Bruce wasn't suggesting this with his question; but someone once suggested this on a techboard meeting, arguing that the overhead of calling a function is very small.)

> 
> >
> > Thanks,
> > /Bruce