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 3F71343035; Fri, 11 Aug 2023 17:54:45 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BE5614325A; Fri, 11 Aug 2023 17:54:44 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 9337640144; Fri, 11 Aug 2023 17:54:42 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id C183820FD0DE; Fri, 11 Aug 2023 08:54:41 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com C183820FD0DE DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1691769281; bh=+rCsp6aEC3OJiRPDzq7+DTbNDyh0MMsSEWNeDtuRAHQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=m3gxY5vrMkZ+Iwq5BlwcJqBsOr0YTt2FMVteoip/T5MY0VqL653LmwhsZix1LMf3E aMB63BeiY2cXAyjCwIAKAEDFTJvlPVrATn7u16COxw1APoxdX4pOoxtP7a8OOnDE2+ zinR6NJgSHjx892nbDC7dMx+DlhhFgNB8H/xkyyY= Date: Fri, 11 Aug 2023 08:54:41 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: dev@dpdk.org, techboard@dpdk.org, Bruce Richardson , Honnappa Nagarahalli , Ruifeng Wang , Jerin Jacob , Sunil Kumar Kori , Mattias =?iso-8859-1?Q?R=F6nnblom?= , Joyce Kong , David Christensen , Konstantin Ananyev , David Hunt , Thomas Monjalon , David Marchand Subject: Re: [PATCH 1/6] eal: provide rte stdatomics optional atomics API Message-ID: <20230811155441.GB20040@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1691717521-1025-1-git-send-email-roretzla@linux.microsoft.com> <1691717521-1025-2-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35D87AE8@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87AE8@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 Fri, Aug 11, 2023 at 11:42:12AM +0200, Morten Brørup wrote: > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > Sent: Friday, 11 August 2023 03.32 > > > > Provide API for atomic operations in the rte namespace that may > > optionally be configured to use C11 atomics with meson > > option enable_stdatomics=true > > > > Signed-off-by: Tyler Retzlaff > > --- > > config/meson.build | 1 + > > config/rte_config.h | 1 + > > lib/eal/include/meson.build | 1 + > > lib/eal/include/rte_stdatomic.h | 162 > > ++++++++++++++++++++++++++++++++++++++++ > > meson_options.txt | 1 + > > 5 files changed, 166 insertions(+) > > create mode 100644 lib/eal/include/rte_stdatomic.h > > > > diff --git a/config/meson.build b/config/meson.build > > index d822371..ec49964 100644 > > --- a/config/meson.build > > +++ b/config/meson.build > > @@ -303,6 +303,7 @@ endforeach > > # set other values pulled from the build options > > dpdk_conf.set('RTE_MAX_ETHPORTS', get_option('max_ethports')) > > dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet')) > > +dpdk_conf.set('RTE_ENABLE_STDATOMIC', get_option('enable_stdatomic')) > > dpdk_conf.set('RTE_ENABLE_TRACE_FP', get_option('enable_trace_fp')) > > # values which have defaults which may be overridden > > dpdk_conf.set('RTE_MAX_VFIO_GROUPS', 64) > > diff --git a/config/rte_config.h b/config/rte_config.h > > index 400e44e..f17b6ae 100644 > > --- a/config/rte_config.h > > +++ b/config/rte_config.h > > @@ -13,6 +13,7 @@ > > #define _RTE_CONFIG_H_ > > > > #include > > +#include > > > > /* legacy defines */ > > #ifdef RTE_EXEC_ENV_LINUX > > diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build > > index b0db9b3..f8a47b3 100644 > > --- a/lib/eal/include/meson.build > > +++ b/lib/eal/include/meson.build > > @@ -43,6 +43,7 @@ headers += files( > > 'rte_seqlock.h', > > 'rte_service.h', > > 'rte_service_component.h', > > + 'rte_stdatomic.h', > > 'rte_string_fns.h', > > 'rte_tailq.h', > > 'rte_thread.h', > > diff --git a/lib/eal/include/rte_stdatomic.h > > b/lib/eal/include/rte_stdatomic.h > > new file mode 100644 > > index 0000000..832fd07 > > --- /dev/null > > +++ b/lib/eal/include/rte_stdatomic.h > > @@ -0,0 +1,162 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(c) 2023 Microsoft Corporation > > + */ > > + > > +#ifndef _RTE_STDATOMIC_H_ > > +#define _RTE_STDATOMIC_H_ > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +typedef int rte_memory_order; > > In C11 memory_order is an enumerated type, and in GCC built-ins it is an int. If possible, rte_memory_order should be too; i.e. remove the typedef here, and make two variants of it instead. will be in v2 > > > + > > +#ifdef RTE_ENABLE_STDATOMIC > > +#ifdef __STDC_NO_ATOMICS__ > > +#error enable_stdatomics=true but atomics not supported by toolchain > > +#endif > > + > > +#include > > + > > +#define __rte_atomic _Atomic > > Move the (changed) C11 memory order type definition here: > > /* The memory order is an enumerated type in C11. */ > #define memory_order rte_memory_order > > > + > > +#define rte_memory_order_relaxed memory_order_relaxed > > +#ifdef __ATOMIC_RELAXED > > +_Static_assert(rte_memory_order_relaxed == __ATOMIC_RELAXED, > > + "rte_memory_order_relaxed == __ATOMIC_RELAXED"); > > +#endif > > + > > +#define rte_memory_order_consume memory_order_consume > > +#ifdef __ATOMIC_CONSUME > > +_Static_assert(rte_memory_order_consume == __ATOMIC_CONSUME, > > + "rte_memory_order_consume == __ATOMIC_CONSUME"); > > +#endif > > + > > +#define rte_memory_order_acquire memory_order_acquire > > +#ifdef __ATOMIC_ACQUIRE > > +_Static_assert(rte_memory_order_acquire == __ATOMIC_ACQUIRE, > > + "rte_memory_order_acquire == __ATOMIC_ACQUIRE"); > > +#endif > > + > > +#define rte_memory_order_release memory_order_release > > +#ifdef __ATOMIC_RELEASE > > +_Static_assert(rte_memory_order_release == __ATOMIC_RELEASE, > > + "rte_memory_order_release == __ATOMIC_RELEASE"); > > +#endif > > + > > +#define rte_memory_order_acq_rel memory_order_acq_rel > > +#ifdef __ATOMIC_ACQ_REL > > +_Static_assert(rte_memory_order_acq_rel == __ATOMIC_ACQ_REL, > > + "rte_memory_order_acq_rel == __ATOMIC_ACQ_REL"); > > +#endif > > + > > +#define rte_memory_order_seq_cst memory_order_seq_cst > > +#ifdef __ATOMIC_SEQ_CST > > +_Static_assert(rte_memory_order_seq_cst == __ATOMIC_SEQ_CST, > > + "rte_memory_order_seq_cst == __ATOMIC_SEQ_CST"); > > +#endif > > Excellent idea adding these _Static_asserts! > > Have you tested (with the toolchain you are targeting with this _Static_assert) that e.g. __ATOMIC_RELAXED is actually #defined, so the preprocessor can see it? (I guess that being a built-it, it might not be a #define, it might be a magic value known by the compiler only.) * llvm and gcc both expose it as a built-in #define for test builds i have run. worst case the assert is lost if it isn't. * since i have to handle non-{clang,gcc} too i still guard with ifdef * i do need to switch to using assert.h static_assert macro to inter-operate with c++ in v2 > > > + > > +#define rte_atomic_load_explicit(ptr, memorder) \ > > + atomic_load_explicit(ptr, memorder) > > + > > +#define rte_atomic_store_explicit(ptr, val, memorder) \ > > + atomic_store_explicit(ptr, val, memorder) > > + > > +#define rte_atomic_exchange_explicit(ptr, val, memorder) \ > > + atomic_exchange_explicit(ptr, val, memorder) > > + > > +#define rte_atomic_compare_exchange_strong_explicit( \ > > + ptr, expected, desired, succ_memorder, fail_memorder) \ > > + atomic_compare_exchange_strong_explicit( \ > > + ptr, expected, desired, succ_memorder, fail_memorder) > > + > > +#define rte_atomic_compare_exchange_weak_explicit( \ > > + ptr, expected, desired, succ_memorder, fail_memorder) \ > > + atomic_compare_exchange_strong_explicit( \ > > + ptr, expected, desired, succ_memorder, fail_memorder) > > + > > +#define rte_atomic_fetch_add_explicit(ptr, val, memorder) \ > > + atomic_fetch_add_explicit(ptr, val, memorder) > > + > > +#define rte_atomic_fetch_sub_explicit(ptr, val, memorder) \ > > + atomic_fetch_sub_explicit(ptr, val, memorder) > > + > > +#define rte_atomic_fetch_and_explicit(ptr, val, memorder) \ > > + atomic_fetch_and_explicit(ptr, val, memorder) > > + > > +#define rte_atomic_fetch_xor_explicit(ptr, val, memorder) \ > > + atomic_fetch_xor_explicit(ptr, val, memorder) > > + > > +#define rte_atomic_fetch_or_explicit(ptr, val, memorder) \ > > + atomic_fetch_or_explicit(ptr, val, memorder) > > + > > +#define rte_atomic_fetch_nand_explicit(ptr, val, memorder) \ > > + atomic_fetch_nand_explicit(ptr, val, memorder) > > + > > +#define rte_atomic_flag_test_and_set_explict(ptr, memorder) \ > > + atomic_flag_test_and_set_explicit(ptr, memorder) > > + > > +#define rte_atomic_flag_clear_explicit(ptr, memorder) \ > > + atomic_flag_clear(ptr, memorder) > > + > > +#else > > + > > +#define __rte_atomic > > Move the built-ins memory order type definition here: > > /* The memory order is an integer type in GCC built-ins, > * not an enumerated type like in C11. > */ > 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 > > Agree; the memorder type is int, so no enum here. > > > + > > +#define rte_atomic_load_explicit(ptr, memorder) \ > > + __atomic_load_n(ptr, memorder) > > + > > +#define rte_atomic_store_explicit(ptr, val, memorder) \ > > + __atomic_store_n(ptr, val, memorder) > > + > > +#define rte_atomic_exchange_explicit(ptr, val, memorder) \ > > + __atomic_exchange_n(ptr, val, memorder) > > + > > +#define rte_atomic_compare_exchange_strong_explicit( \ > > + ptr, expected, desired, succ_memorder, fail_memorder) \ > > + __atomic_compare_exchange_n( \ > > + ptr, expected, desired, 0, succ_memorder, fail_memorder) > > + > > +#define rte_atomic_compare_exchange_weak_explicit( \ > > + ptr, expected, desired, succ_memorder, fail_memorder) \ > > + __atomic_compare_exchange_n( \ > > + ptr, expected, desired, 1, succ_memorder, fail_memorder) > > + > > +#define rte_atomic_fetch_add_explicit(ptr, val, memorder) \ > > + __atomic_fetch_add(ptr, val, memorder) > > + > > +#define rte_atomic_fetch_sub_explicit(ptr, val, memorder) \ > > + __atomic_fetch_sub(ptr, val, memorder) > > + > > +#define rte_atomic_fetch_and_explicit(ptr, val, memorder) \ > > + __atomic_fetch_and(ptr, val, memorder) > > + > > +#define rte_atomic_fetch_xor_explicit(ptr, val, memorder) \ > > + __atomic_fetch_xor(ptr, val, memorder) > > + > > +#define rte_atomic_fetch_or_explicit(ptr, val, memorder) \ > > + __atomic_fetch_or(ptr, val, memorder) > > + > > +#define rte_atomic_fetch_nand_explicit(ptr, val, memorder) \ > > + __atomic_fetch_nand(ptr, val, memorder) > > + > > +#define rte_atomic_flag_test_and_set_explicit(ptr, memorder) \ > > + __atomic_test_and_set(ptr, memorder) > > + > > +#define rte_atomic_flag_clear_explicit(ptr, memorder) \ > > + __atomic_clear(ptr, memorder) > > + > > +#endif > > + > > +#ifdef __cplusplus > > +} > > +#endif > > + > > +#endif /* _RTE_STDATOMIC_H_ */ > > diff --git a/meson_options.txt b/meson_options.txt > > index 621e1ca..7d6784d 100644 > > --- a/meson_options.txt > > +++ b/meson_options.txt > > @@ -46,6 +46,7 @@ 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_stdatomic', type: 'boolean', value: false, description: > > 'enable use of C11 stdatomic') > > 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 >