From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DCF8843082; Wed, 16 Aug 2023 19:25:57 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 735B040693; Wed, 16 Aug 2023 19:25:57 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 6053E4003C; Wed, 16 Aug 2023 19:25:55 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id ADC14211F614; Wed, 16 Aug 2023 10:25:54 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com ADC14211F614 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1692206754; bh=LKqq/Wdgd4cZ0UnHixyq5twTfpdkVs56Gqw28m+E4Xo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=H6SrdSKTDgA8+2exhCohVbAuOdTiH9F9bzF3Wj+g9h4T2mcJ8RGOp6L/fKWteQfoa EGtbxz4xgsig8jGMwvaLTKYun72qBMV61l7kStgTYF62I4B3uKghOTY6oy/x2vo+8Q 6GIfy5f3aaOWjI8UJtjiS1nOFVtFjrAlega3c1EE= Date: Wed, 16 Aug 2023 10:25:54 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Thomas Monjalon , Bruce Richardson , dev@dpdk.org, techboard@dpdk.org, david.marchand@redhat.com, Honnappa.Nagarahalli@arm.com Subject: Re: C11 atomics adoption blocked Message-ID: <20230816172554.GA20093@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20230808175303.GA11006@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20230808204944.GA3335@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D87AD8@smartserver.smartshare.dk> <3629940.iIbC2pHGDl@thomas> <98CBD80474FA8B44BF855DF32C47DC35D87AF9@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87AF9@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Mon, Aug 14, 2023 at 05:13:04PM +0200, Morten Brørup wrote: > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > Sent: Monday, 14 August 2023 15.46 > > > > mercredi 9 août 2023, Morten Brørup: > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > > > Sent: Tuesday, 8 August 2023 22.50 > > > > > > > > On Tue, Aug 08, 2023 at 10:22:09PM +0200, Morten Brørup wrote: > > > > > > 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 > > > > > > > > > > > > > > > > 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. > > > > > > > > I'm shy to do anything in the rte_ namespace because I don't want to > > > > formalize it as an API. > > > > > > > > I was envisioning the following. > > > > > > > > Internally DPDK code just uses __atomic_fetch_add directly, the > > macros > > > > are provided for Windows targets to expand to __atomic_fetch_add. > > > > > > > > Externally DPDK applications that don't care about being portable > > may > > > > use __atomic_fetch_add (BSD/Linux) or atomic_fetch_add (Windows) > > > > directly. > > > > > > > > Externally DPDK applications that care to be portable may do what is > > > > done Internally and <> the __atomic_fetch_add directly. By > > > > including say rte_stdatomic.h indirectly (Windows) gets the macros > > > > expanded to atomic_fetch_add and for BSD/Linux it's a noop include. > > > > > > > > Basically I'm placing a little ugly into Windows built code and in > > trade > > > > we don't end up with a bunch of rte_ APIs that were strongly > > objected to > > > > previously. > > > > > > > > It's a compromise. > > > > > > OK, we probably need to offer a public header file to wrap the > > atomics, using either names prefixed with rte_ or names similar to the > > gcc builtin atomics. > > > > > > I guess the objections were based on the assumption that we were > > switching to C11 atomics with DPDK 23.11, so the rte_ prefixed atomic > > APIs would be very short lived (DPDK 23.07 to 23.11 only). But with this > > new information about GNU C++ incompatibility, that seems not to be the > > case, so the naming discussion can be reopened. > > > > > > If we don't introduce such a wrapper header, all portable code needs > > to surround the use of atomics with #ifdef USE_STDATOMIC_H. > > > > > > BTW: Can the compilers that understand both builtin atomics and C11 > > stdatomics.h handle code with #define __atomic_fetch_add > > atomic_fetch_add and #define __ATOMIC_SEQ_CST memory_order_seq_cst? If > > not, then we need to use rte_ prefixed atomics. > > > > > > And what about C++ atomics... Do we want (or need?) a third variant > > using C++ atomics, e.g. "atomic x;" instead of "_Atomic int x;"? (I > > hope not!) For reference, the "atomic_int" type is "_Atomic int" in C, > > but "std::atomic" in C++. > > > > > > C++23 provides the C11 compatibility macro "_Atomic(T)", which means > > "_Atomic T" in C and "std::atomic" in C++. Perhaps we can somewhat > > rely on this, and update our coding standards to require using e.g. > > "_Atomic(int)" for atomic types, and disallow using "_Atomic int". > > > > You mean the syntax _Atomic(T) is working well in both C and C++? > > This syntax is API compatible across C11 and C++23, so it would work with (C11 and C++23) applications building DPDK from scratch. > > But it is only "recommended" ABI compatible for compilers [1], so DPDK in distros cannot rely on. > > [1]: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0943r6.html > > It would be future-proofing for the benefit of C++23 based applications... I was mainly mentioning it for completeness, now that we are switching to a new standard for atomics. > > Realistically, considering that 1. such a coding standard (requiring "_Atomic(T)" instead of "_Atomic T") would only be relevant for a 2023 standard, and 2. that we are now upgrading to a standard from 2011, we would probably have to wait for a very distant future (12 years?) before C++ applications can reap the benefits of such a coding standard. > i just want to feedback on this coding convention topic here (in relation to the RFC patch series thread) i think the convention of using the macro should be adopted now. the main reason being that it is far easier that an atomic type is a type or a pointer type when the '*' is captured as a part of the macro parameter. please see the RFC patch thread for the details of how this was beneficial for rcs_mcslock.h and how the placement of the _Atomic keyword matters when applied to pointer types of incomplete types.