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 DD1DD428C4; Mon, 3 Apr 2023 23:11:40 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 65A374067E; Mon, 3 Apr 2023 23:11:40 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 2C5B940156 for ; Mon, 3 Apr 2023 23:11:39 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id A97BD15229 for ; Mon, 3 Apr 2023 23:11:38 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id A89181530B; Mon, 3 Apr 2023 23:11:38 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-2.1 required=5.0 tests=ALL_TRUSTED, AWL, NICE_REPLY_A autolearn=disabled version=3.4.6 X-Spam-Score: -2.1 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 1868B15481; Mon, 3 Apr 2023 23:11:36 +0200 (CEST) Message-ID: <5c2ab2bd-6a73-6c58-e789-83326d7ee129@lysator.liu.se> Date: Mon, 3 Apr 2023 23:11:35 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH v2] eal: introduce atomics abstraction To: Tyler Retzlaff , dev@dpdk.org Cc: david.marchand@redhat.com, thomas@monjalon.net, mb@smartsharesystems.com, Honnappa.Nagarahalli@arm.com, bruce.richardson@intel.com References: <1673558785-24992-1-git-send-email-roretzla@linux.microsoft.com> <1675892618-31755-1-git-send-email-roretzla@linux.microsoft.com> <1675892618-31755-2-git-send-email-roretzla@linux.microsoft.com> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= In-Reply-To: <1675892618-31755-2-git-send-email-roretzla@linux.microsoft.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Virus-Scanned: ClamAV using ClamSMTP 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 2023-02-08 22:43, Tyler Retzlaff wrote: > Introduce atomics abstraction that permits optional use of standard C11 > atomics when meson is provided the new enable_stdatomics=true option. > Terminology nitpicking: I don't think these functions provide any abstraction at all. They are just wrappers. > Signed-off-by: Tyler Retzlaff > --- > config/meson.build | 11 ++++ > lib/eal/arm/include/rte_atomic_32.h | 6 ++- > lib/eal/arm/include/rte_atomic_64.h | 6 ++- > lib/eal/include/generic/rte_atomic.h | 96 +++++++++++++++++++++++++++++++++- > lib/eal/loongarch/include/rte_atomic.h | 6 ++- > lib/eal/ppc/include/rte_atomic.h | 6 ++- > lib/eal/riscv/include/rte_atomic.h | 6 ++- > lib/eal/x86/include/rte_atomic.h | 8 ++- > meson_options.txt | 2 + > 9 files changed, 139 insertions(+), 8 deletions(-) > > diff --git a/config/meson.build b/config/meson.build > index 26f3168..25dd628 100644 > --- a/config/meson.build > +++ b/config/meson.build > @@ -255,6 +255,17 @@ endif > # add -include rte_config to cflags > add_project_arguments('-include', 'rte_config.h', language: 'c') > > +stdc_atomics_enabled = get_option('enable_stdatomics') > +dpdk_conf.set('RTE_STDC_ATOMICS', stdc_atomics_enabled) > + > +if stdc_atomics_enabled > +if cc.get_id() == 'gcc' or cc.get_id() == 'clang' > + add_project_arguments('-std=gnu11', language: 'c') > +else > + add_project_arguments('-std=c11', language: 'c') > +endif > +endif > + > # enable extra warnings and disable any unwanted warnings > # -Wall is added by default at warning level 1, and -Wextra > # at warning level 2 (DPDK default) > diff --git a/lib/eal/arm/include/rte_atomic_32.h b/lib/eal/arm/include/rte_atomic_32.h > index c00ab78..7088a12 100644 > --- a/lib/eal/arm/include/rte_atomic_32.h > +++ b/lib/eal/arm/include/rte_atomic_32.h > @@ -34,9 +34,13 @@ > #define rte_io_rmb() rte_rmb() > > static __rte_always_inline void > -rte_atomic_thread_fence(int memorder) > +rte_atomic_thread_fence(rte_memory_order memorder) > { > +#ifdef RTE_STDC_ATOMICS > + atomic_thread_fence(memorder); > +#else > __atomic_thread_fence(memorder); > +#endif > } > > #ifdef __cplusplus > diff --git a/lib/eal/arm/include/rte_atomic_64.h b/lib/eal/arm/include/rte_atomic_64.h > index 6047911..7f02c57 100644 > --- a/lib/eal/arm/include/rte_atomic_64.h > +++ b/lib/eal/arm/include/rte_atomic_64.h > @@ -38,9 +38,13 @@ > #define rte_io_rmb() rte_rmb() > > static __rte_always_inline void > -rte_atomic_thread_fence(int memorder) > +rte_atomic_thread_fence(rte_memory_order memorder) > { > +#ifdef RTE_STDC_ATOMICS > + atomic_thread_fence(memorder); > +#else > __atomic_thread_fence(memorder); > +#endif > } > > /*------------------------ 128 bit atomic operations -------------------------*/ > diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h > index f5c49a9..392d928 100644 > --- a/lib/eal/include/generic/rte_atomic.h > +++ b/lib/eal/include/generic/rte_atomic.h > @@ -110,6 +110,100 @@ > > #endif /* __DOXYGEN__ */ > > +#ifdef RTE_STDC_ATOMICS > + > +#if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L || defined(__STDC_NO_ATOMICS__) > +#error compiler does not support C11 standard atomics > +#else > +#include > +#endif > + > +#define __rte_atomic _Atomic > + > +typedef int rte_memory_order; > + > +#define rte_memory_order_relaxed memory_order_relaxed > +#define rte_memory_order_consume memory_order_consume > +#define rte_memory_order_acquire memory_order_acquire > +#define rte_memory_order_release memory_order_release > +#define rte_memory_order_acq_rel memory_order_acq_rel > +#define rte_memory_order_seq_cst memory_order_seq_cst > + Would this be better of as an enum, rather than a typedef? If typedef, it should have the "_t" postfix. Also, the #define should be all-caps. > +#define rte_atomic_store_explicit(obj, desired, order) \ > + atomic_store_explicit(obj, desired, order) > + Drop "explicit" from all the names. It's just noise. Also, the memory orders have very long names. We haven't even move all DPDK code over from the old API, to using GCC C11 built-ins, and now we are switching to a new API? > +#define rte_atomic_load_explicit(obj, order) \ > + atomic_load_explicit(obj, order) > + > +#define rte_atomic_exchange_explicit(obj, desired, order) \ > + atomic_exchange_explicit(obj, desired, order) > + > +#define rte_atomic_compare_exchange_strong_explicit(obj, expected, desired, success, fail) \ > + atomic_compare_exchange_strong_explicit(obj, expected, desired, success, fail) > + > +#define rte_atomic_compare_exchange_weak_explicit(obj, expected, desired, success, fail) \ > + atomic_compare_exchange_weak_explicit(obj, expected, desired, success, fail) > + > +#define rte_atomic_fetch_add_explicit(obj, arg, order) \ > + atomic_fetch_add_explicit(obj, arg, order) > + > +#define rte_atomic_fetch_sub_explicit(obj, arg, order) \ > + atomic_fetch_sub_explicit(obj, arg, order) > + > +#define rte_atomic_fetch_or_explicit(obj, arg, order) \ > + atomic_fetch_or_explicit(obj, arg, order) > + > +#define rte_atomic_fetch_xor_explicit(obj, arg, order) \ > + atomic_fetch_xor_explicit(obj, arg, order) > + > +#define rte_atomic_fetch_and_explicit(obj, arg, order) \ > + atomic_fetch_and_explicit(obj, arg, order) > + > +#else > + > +#define __rte_atomic > + > +typedef int rte_memory_order; > + > +#define rte_memory_order_relaxed __ATOMIC_RELAXED > +#define rte_memory_order_consume __ATOMIC_CONSUME > +#define rte_memory_order_acquire __ATOMIC_ACQUIRE > +#define rte_memory_order_release __ATOMIC_RELEASE > +#define rte_memory_order_acq_rel __ATOMIC_ACQ_REL > +#define rte_memory_order_seq_cst __ATOMIC_SEQ_CST > + > +#define rte_atomic_store_explicit(obj, desired, order) \ > + __atomic_store_n(obj, desired, order) > + > +#define rte_atomic_load_explicit(obj, order) \ > + __atomic_load_n(obj, order) > + > +#define rte_atomic_exchange_explicit(obj, desired, order) \ > + __atomic_exchange_n(obj, desired, order) > + > +#define rte_atomic_compare_exchange_strong_explicit(obj, expected, desired, success, fail) \ > + __atomic_compare_exchange_n(obj, expected, desired, 0, success, fail) > + > +#define rte_atomic_compare_exchange_weak_explicit(obj, expected, desired, success, fail) \ > + __atomic_compare_exchange_n(obj, expected, desired, 1, success, fail) > + > +#define rte_atomic_fetch_add_explicit(obj, arg, order) \ > + __atomic_fetch_add(obj, arg, order) > + > +#define rte_atomic_fetch_sub_explicit(obj, arg, order) \ > + __atomic_fetch_sub(obj, arg, order) > + > +#define rte_atomic_fetch_or_explicit(obj, arg, order) \ > + __atomic_fetch_or(obj, arg, order) > + > +#define rte_atomic_fetch_xor_explicit(obj, arg, order) \ > + __atomic_fetch_xor(obj, arg, order) > + > +#define rte_atomic_fetch_and_explicit(obj, arg, order) \ > + __atomic_fetch_and(obj, arg, order) > + > +#endif > + > /** > * Compiler barrier. > * > @@ -123,7 +217,7 @@ > /** > * Synchronization fence between threads based on the specified memory order. > */ > -static inline void rte_atomic_thread_fence(int memorder); > +static inline void rte_atomic_thread_fence(rte_memory_order memorder); > > /*------------------------- 16 bit atomic operations -------------------------*/ > > diff --git a/lib/eal/loongarch/include/rte_atomic.h b/lib/eal/loongarch/include/rte_atomic.h > index 3c82845..66aa0c8 100644 > --- a/lib/eal/loongarch/include/rte_atomic.h > +++ b/lib/eal/loongarch/include/rte_atomic.h > @@ -35,9 +35,13 @@ > #define rte_io_rmb() rte_mb() > > static __rte_always_inline void > -rte_atomic_thread_fence(int memorder) > +rte_atomic_thread_fence(rte_memory_order memorder) > { > +#ifdef RTE_STDC_ATOMICS > + atomic_thread_fence(memorder); > +#else > __atomic_thread_fence(memorder); > +#endif > } > > #ifdef __cplusplus > diff --git a/lib/eal/ppc/include/rte_atomic.h b/lib/eal/ppc/include/rte_atomic.h > index 663b4d3..a428a83 100644 > --- a/lib/eal/ppc/include/rte_atomic.h > +++ b/lib/eal/ppc/include/rte_atomic.h > @@ -38,9 +38,13 @@ > #define rte_io_rmb() rte_rmb() > > static __rte_always_inline void > -rte_atomic_thread_fence(int memorder) > +rte_atomic_thread_fence(rte_memory_order memorder) > { > +#ifdef RTE_STDC_ATOMICS > + atomic_thread_fence(memorder); > +#else > __atomic_thread_fence(memorder); > +#endif > } > > /*------------------------- 16 bit atomic operations -------------------------*/ > diff --git a/lib/eal/riscv/include/rte_atomic.h b/lib/eal/riscv/include/rte_atomic.h > index 4b4633c..3c203a9 100644 > --- a/lib/eal/riscv/include/rte_atomic.h > +++ b/lib/eal/riscv/include/rte_atomic.h > @@ -40,9 +40,13 @@ > #define rte_io_rmb() asm volatile("fence ir, ir" : : : "memory") > > static __rte_always_inline void > -rte_atomic_thread_fence(int memorder) > +rte_atomic_thread_fence(rte_memory_order memorder) > { > +#ifdef RTE_STDC_ATOMICS > + atomic_thread_fence(memorder); > +#else > __atomic_thread_fence(memorder); > +#endif > } > > #ifdef __cplusplus > diff --git a/lib/eal/x86/include/rte_atomic.h b/lib/eal/x86/include/rte_atomic.h > index f2ee1a9..02d8b12 100644 > --- a/lib/eal/x86/include/rte_atomic.h > +++ b/lib/eal/x86/include/rte_atomic.h > @@ -87,12 +87,16 @@ > * used instead. > */ > static __rte_always_inline void > -rte_atomic_thread_fence(int memorder) > +rte_atomic_thread_fence(rte_memory_order memorder) > { > - if (memorder == __ATOMIC_SEQ_CST) > + if (memorder == rte_memory_order_seq_cst) > rte_smp_mb(); > else > +#ifdef RTE_STDC_ATOMICS > + atomic_thread_fence(memorder); > +#else > __atomic_thread_fence(memorder); > +#endif > } > > /*------------------------- 16 bit atomic operations -------------------------*/ > diff --git a/meson_options.txt b/meson_options.txt > index 0852849..acbcbb8 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -46,6 +46,8 @@ option('mbuf_refcnt_atomic', type: 'boolean', value: true, description: > 'Atomically access the mbuf refcnt.') > option('platform', type: 'string', value: 'native', description: > 'Platform to build, either "native", "generic" or a SoC. Please refer to the Linux build guide for more information.') > +option('enable_stdatomics', type: 'boolean', value: false, description: > + 'enable use of standard C11 atomics.') > option('enable_trace_fp', type: 'boolean', value: false, description: > 'enable fast path trace points.') > option('tests', type: 'boolean', value: true, description: