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 953A1428C4; Mon, 3 Apr 2023 23:17:59 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1B9634067E; Mon, 3 Apr 2023 23:17:59 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 2C04040156 for ; Mon, 3 Apr 2023 23:17:58 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id D6A1C14D6F for ; Mon, 3 Apr 2023 23:17:57 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id D5A0414C69; Mon, 3 Apr 2023 23:17:57 +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)) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 6899F15484; Mon, 3 Apr 2023 23:17:56 +0200 (CEST) Message-ID: Date: Mon, 3 Apr 2023 23:17:56 +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 Content-Language: en-US To: =?UTF-8?Q?Morten_Br=c3=b8rup?= , Tyler Retzlaff , dev@dpdk.org Cc: david.marchand@redhat.com, thomas@monjalon.net, 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> <98CBD80474FA8B44BF855DF32C47DC35D87718@smartserver.smartshare.dk> From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87718@smartserver.smartshare.dk> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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-09 09:05, Morten Brørup wrote: >> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] >> Sent: Wednesday, 8 February 2023 22.44 >> >> Introduce atomics abstraction that permits optional use of standard C11 >> atomics when meson is provided the new enable_stdatomics=true option. >> >> Signed-off-by: Tyler Retzlaff >> --- > > Looks good. A few minor suggestions about implementation only. > > With or without suggested modifications, > > Acked-by: Morten Brørup > > >> 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; > > I would prefer enum for rte_memory_order: > > typedef enum { > rte_memory_order_relaxed = memory_order_relaxed, > rte_memory_order_consume = memory_order_consume, > rte_memory_order_acquire = memory_order_acquire, > rte_memory_order_release = memory_order_release, > rte_memory_order_acq_rel = memory_order_acq_rel, > rte_memory_order_seq_cst = memory_order_seq_cst > } 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 >> + >> +#define rte_atomic_store_explicit(obj, desired, order) \ >> + atomic_store_explicit(obj, desired, order) >> + >> +#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 > > Prefer enum for rte_memory_order: > > typedef enum { > rte_memory_order_relaxed = __ATOMIC_RELAXED, > rte_memory_order_consume = __ATOMIC_CONSUME, > rte_memory_order_acquire = __ATOMIC_ACQUIRE, > rte_memory_order_release = __ATOMIC_RELEASE, > rte_memory_order_acq_rel = __ATOMIC_ACQ_REL, > rte_memory_order_seq_cst = __ATOMIC_SEQ_CST > } rte_memory_order; > Minus the typedef, I would suggest. The DPDK coding conventions calls for enum values to be upper case. >> + >> +#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) > > The type of the "weak" parameter to __atomic_compare_exchange_n() is bool, not int, so use "false" instead of 0. There is probably no practical difference, so I'll leave it up to you. > > You might need to include for this... I haven't checked. > >> + >> +#define rte_atomic_compare_exchange_weak_explicit(obj, expected, >> desired, success, fail) \ >> + __atomic_compare_exchange_n(obj, expected, desired, 1, success, >> fail) > > Same as above: Use "true" instead of 1. > >> + >> +#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: >> -- >> 1.8.3.1 >> >