Re: [PATCH v2] eal: introduce atomics abstraction

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

On Mon, Apr 03, 2023 at 11:11:35PM +0200, Mattias Rönnblom wrote:
> 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 <roretzla@linux.microsoft.com>
> >---
> >  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 <stdatomic.h>
> >+#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?

This series has been marked obsolete/abandoned.  As per technical board
DPDK will move straight to standard C atomics without transitioning with
these macros.

Thanks