Re: [PATCH] eal: introduce atomics abstraction

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

On Tue, Feb 07, 2023 at 04:16:58PM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Friday, 3 February 2023 21.49
> > 
> > On Fri, Feb 03, 2023 at 12:19:13PM +0000, Bruce Richardson wrote:
> > > On Thu, Feb 02, 2023 at 11:00:23AM -0800, Tyler Retzlaff wrote:
> > > > 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.
> > > >
> > > What compiler is this? As far as I know, all our currently support
> > > compilers claim to support C99 fully. All should support C11 also,
> > > except for GCC 4.8 on RHEL/CentOS 7. Once we drop support for Centos
> > 7, I
> > > think we can require at minimum a c11 compiler for building DPDK
> > itself.
> > > I'm still a little uncertain about requiring that users build their
> > own
> > > code with -std=c11, though.
> > 
> > perhaps i'm mistaken but it was my understanding that the gcc version
> > on
> > RHEL 7 did not fully conform to C99? maybe i read C99 when it was
> > actually
> > C11.
> 
> RHEL does supports C99, it's C11 that it doesn't support [1].
> 
> [1]: http://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35D8762F@smartserver.smartshare.dk/
> 
> > 
> > regardless, even if every supported compiler for dpdk was C11
> > conformant
> > including stdatomics which are optional we can't just move from
> > intrinsic/builtins to standard C atomics (because of the compatibility
> > and performance issues mentioned previously).
> 
> For example, with C11, you can make structures atomic. And an atomic instance of a type can have a different size than the non-atomic type.
> 
> If do we make a shim, it will have some limitations compared to C11 atomics, e.g. it cannot handle atomic structures.

right, so it "looks" like standard but then doesn't work like standard.

> 
> Either we accept these limitations of the shim, or we use our own namespace. If we accept the limitations, we risk that someone with a C11 build environment uses them anyway, and it will not work in non-C11 build environments. So a shim is not a rose without thorns.

the standard says even integer types can have different alignment and size
so it isn't strictly portable to replace any integer type with the similar
_Atomic type. here is an example of something i would prefer not to have
to navigate which using a shim would sign us up for.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65146

> > 
> > so just re-orienting this discussion, the purpose of this abstraction
> > is
> > to allow the optional use of standard C atomics when a conformant
> > compiler
> > is available and satisfactory code is generated for the desired target.
> 
> I think it is more important getting this feature into DPDK than using the C11 stdatomic.h API for atomics in DPDK.
> 
> I don't feel strongly about this API, and will accept either the proposed patch series (with the C11-like API, but rte_ prefixed namespace), or a C11 stdatomic.h API shim.

let's just stay out of the standard namespace, it doesn't buy us the
forward compatibility we want and being explicit in the namespace makes
it obvious that we aren't.

ty