[v6,1/6] eal: provide rte stdatomics optional atomics API

  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 <roretzla@linux.microsoft.com>
Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
---
 config/meson.build                     |   1 +
 doc/api/doxy-api.conf.in               |   1 +
 lib/eal/include/generic/rte_atomic.h   |   1 +
 lib/eal/include/generic/rte_pause.h    |   1 +
 lib/eal/include/generic/rte_rwlock.h   |   1 +
 lib/eal/include/generic/rte_spinlock.h |   1 +
 lib/eal/include/meson.build            |   1 +
 lib/eal/include/rte_mcslock.h          |   1 +
 lib/eal/include/rte_pflock.h           |   1 +
 lib/eal/include/rte_seqcount.h         |   1 +
 lib/eal/include/rte_stdatomic.h        | 198 +++++++++++++++++++++++++++++++++
 lib/eal/include/rte_ticketlock.h       |   1 +
 lib/eal/include/rte_trace_point.h      |   1 +
 meson_options.txt                      |   2 +
 14 files changed, 212 insertions(+)
 create mode 100644 lib/eal/include/rte_stdatomic.h
  

22/08/2023 23:00, Tyler Retzlaff:
> --- a/lib/eal/include/generic/rte_rwlock.h
> +++ b/lib/eal/include/generic/rte_rwlock.h
> @@ -32,6 +32,7 @@
>  #include <rte_common.h>
>  #include <rte_lock_annotations.h>
>  #include <rte_pause.h>
> +#include <rte_stdatomic.h>

I'm not sure about adding the include in patch 1 if it is not used here.


> --- /dev/null
> +++ b/lib/eal/include/rte_stdatomic.h
> @@ -0,0 +1,198 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Microsoft Corporation
> + */
> +
> +#ifndef _RTE_STDATOMIC_H_
> +#define _RTE_STDATOMIC_H_
> +
> +#include <assert.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#ifdef RTE_ENABLE_STDATOMIC
> +#ifdef __STDC_NO_ATOMICS__
> +#error enable_stdatomics=true but atomics not supported by toolchain
> +#endif
> +
> +#include <stdatomic.h>
> +
> +/* RTE_ATOMIC(type) is provided for use as a type specifier
> + * permitting designation of an rte atomic type.
> + */
> +#define RTE_ATOMIC(type) _Atomic(type)
> +
> +/* __rte_atomic is provided for type qualification permitting
> + * designation of an rte atomic qualified type-name.

Sorry I don't understand this comment.

> + */
> +#define __rte_atomic _Atomic
> +
> +/* The memory order is an enumerated type in C11. */
> +typedef 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");

Not sure about using static_assert or RTE_BUILD_BUG_ON

> +#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
> +
> +#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_weak_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_explicit(ptr, memorder) \
> +	atomic_flag_test_and_set_explicit(ptr, memorder)
> +
> +#define rte_atomic_flag_clear_explicit(ptr, memorder) \
> +	atomic_flag_clear_explicit(ptr, memorder)
> +
> +/* We provide internal macro here to allow conditional expansion
> + * in the body of the per-arch rte_atomic_thread_fence inline functions.
> + */
> +#define __rte_atomic_thread_fence(memorder) \
> +	atomic_thread_fence(memorder)
> +
> +#else

Better to add some context in comment of this "else": /* !RTE_ENABLE_STDATOMIC */

> +
> +/* RTE_ATOMIC(type) is provided for use as a type specifier
> + * permitting designation of an rte atomic type.
> + */

The comment should say it has no effect.
Or no comment at all for this part.

> +#define RTE_ATOMIC(type) type
> +
> +/* __rte_atomic is provided for type qualification permitting
> + * designation of an rte atomic qualified type-name.
> + */
> +#define __rte_atomic
> +
> +/* 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
> +
> +#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)
> +
> +/* We provide internal macro here to allow conditional expansion
> + * in the body of the per-arch rte_atomic_thread_fence inline functions.
> + */
> +#define __rte_atomic_thread_fence(memorder) \
> +	__atomic_thread_fence(memorder)
> +
> +#endif
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_STDATOMIC_H_ */
  

On Thu, Sep 28, 2023 at 10:06 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 22/08/2023 23:00, Tyler Retzlaff:
> > --- a/lib/eal/include/generic/rte_rwlock.h
> > +++ b/lib/eal/include/generic/rte_rwlock.h
> > @@ -32,6 +32,7 @@
> >  #include <rte_common.h>
> >  #include <rte_lock_annotations.h>
> >  #include <rte_pause.h>
> > +#include <rte_stdatomic.h>
>
> I'm not sure about adding the include in patch 1 if it is not used here.

Yes, this is something I had already fixed locally.

>
> > --- /dev/null
> > +++ b/lib/eal/include/rte_stdatomic.h
> > @@ -0,0 +1,198 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2023 Microsoft Corporation
> > + */
> > +
> > +#ifndef _RTE_STDATOMIC_H_
> > +#define _RTE_STDATOMIC_H_
> > +
> > +#include <assert.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#ifdef RTE_ENABLE_STDATOMIC
> > +#ifdef __STDC_NO_ATOMICS__
> > +#error enable_stdatomics=true but atomics not supported by toolchain
> > +#endif
> > +
> > +#include <stdatomic.h>
> > +
> > +/* RTE_ATOMIC(type) is provided for use as a type specifier
> > + * permitting designation of an rte atomic type.
> > + */
> > +#define RTE_ATOMIC(type) _Atomic(type)
> > +
> > +/* __rte_atomic is provided for type qualification permitting
> > + * designation of an rte atomic qualified type-name.
>
> Sorry I don't understand this comment.

The difference between atomic qualifier and atomic specifier and the
need for exposing those two notions are not obvious to me.

One clue I have is with one use later in the series:
+rte_mcslock_lock(RTE_ATOMIC(rte_mcslock_t *) *msl, rte_mcslock_t *me)
...
+    prev = rte_atomic_exchange_explicit(msl, me, rte_memory_order_acq_rel);

So at least RTE_ATOMIC() seems necessary.


>
> > + */
> > +#define __rte_atomic _Atomic
> > +
> > +/* The memory order is an enumerated type in C11. */
> > +typedef 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");
>
> Not sure about using static_assert or RTE_BUILD_BUG_ON

Do you mean you want no check at all in a public facing header?

Or is it that we have RTE_BUILD_BUG_ON and we should keep using it
instead of static_assert?

I remember some problems with RTE_BUILD_BUG_ON where the compiler
would silently drop the whole expression and reported no problem as it
could not evaluate the expression.
At least, with static_assert (iirc, it is new to C11) the compiler
complains with a clear "error: expression in static assertion is not
constant".
We could fix RTE_BUILD_BUG_ON, but I guess the fix would be equivalent
to map it to static_assert(!condition).
Using language standard constructs seems a better choice.
  

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Friday, 29 September 2023 10.04
> 
> On Thu, Sep 28, 2023 at 10:06 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 22/08/2023 23:00, Tyler Retzlaff:

[...]

> > > +/* The memory order is an enumerated type in C11. */
> > > +typedef 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");
> >
> > Not sure about using static_assert or RTE_BUILD_BUG_ON
> 
> Do you mean you want no check at all in a public facing header?
> 
> Or is it that we have RTE_BUILD_BUG_ON and we should keep using it
> instead of static_assert?
> 
> I remember some problems with RTE_BUILD_BUG_ON where the compiler
> would silently drop the whole expression and reported no problem as it
> could not evaluate the expression.
> At least, with static_assert (iirc, it is new to C11) the compiler
> complains with a clear "error: expression in static assertion is not
> constant".
> We could fix RTE_BUILD_BUG_ON, but I guess the fix would be equivalent
> to map it to static_assert(!condition).
> Using language standard constructs seems a better choice.

+1 to using language standard constructs.

static_assert became standard in C11. (Formally, _Static_assert is standard C11, and static_assert is available through a convenience macro in C11 [1].)

In my opinion, our move to C11 thus makes RTE_BUILD_BUG_ON obsolete.

We should mark RTE_BUILD_BUG_ON as deprecated, and disallow RTE_BUILD_BUG_ON in new code. Perhaps checkpatches could catch this?

[1]: https://en.cppreference.com/w/c/language/_Static_assert

PS: static_assert also has the advantage that it can be used directly in header files. RTE_BUILD_BUG_ON can only be used in functions, and thus needs to be wrapped in a dummy (static inline) function when used in a header file.

> 
> 
> --
> David Marchand
  

On Fri, Sep 29, 2023 at 10:54 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> In my opinion, our move to C11 thus makes RTE_BUILD_BUG_ON obsolete.

That's my thought too.

>
> We should mark RTE_BUILD_BUG_ON as deprecated, and disallow RTE_BUILD_BUG_ON in new code. Perhaps checkpatches could catch this?

For a clear deprecation of a part of DPDK API, I don't see a need to
add something in checkpatch.
Putting a RTE_DEPRECATED in RTE_BUILD_BUG_ON directly triggers a build
warning (caught by CI since we run with Werror).


diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 771c70f2c8..40542629c1 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -495,7 +495,7 @@ rte_is_aligned(const void * const __rte_restrict
ptr, const unsigned int align)
 /**
  * Triggers an error at compilation time if the condition is true.
  */
-#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#define RTE_BUILD_BUG_ON(condition) RTE_DEPRECATED(RTE_BUILD_BUG_ON)
((void)sizeof(char[1 - 2*!!(condition)]))

 /*********** Cache line related macros ********/



$ ninja -C build-mini
...
[18/333] Compiling C object lib/librte_eal.a.p/eal_common_eal_common_trace.c.o
../lib/eal/common/eal_common_trace.c: In function ‘eal_trace_init’:
../lib/eal/common/eal_common_trace.c:44:20: warning:
"RTE_BUILD_BUG_ON" is deprecated
   44 |         RTE_BUILD_BUG_ON((offsetof(struct __rte_trace_header,
mem) % 8) != 0);
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[38/333] Compiling C object lib/librte_eal.a.p/eal_common_malloc_heap.c.o
../lib/eal/common/malloc_heap.c: In function ‘malloc_heap_destroy’:
../lib/eal/common/malloc_heap.c:1398:20: warning: "RTE_BUILD_BUG_ON"
is deprecated
 1398 |         RTE_BUILD_BUG_ON(offsetof(struct malloc_heap, lock) != 0);
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[50/333] Compiling C object lib/librte_eal.a.p/eal_unix_rte_thread.c.o
../lib/eal/unix/rte_thread.c: In function ‘rte_thread_self’:
../lib/eal/unix/rte_thread.c:239:20: warning: "RTE_BUILD_BUG_ON" is deprecated
  239 |         RTE_BUILD_BUG_ON(sizeof(pthread_t) > sizeof(uintptr_t));
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  

On Fri, Sep 29, 2023 at 11:02:38AM +0200, David Marchand wrote:
> On Fri, Sep 29, 2023 at 10:54 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > In my opinion, our move to C11 thus makes RTE_BUILD_BUG_ON obsolete.
> 
> That's my thought too.
> 
> >
> > We should mark RTE_BUILD_BUG_ON as deprecated, and disallow RTE_BUILD_BUG_ON in new code. Perhaps checkpatches could catch this?
> 
> For a clear deprecation of a part of DPDK API, I don't see a need to
> add something in checkpatch.
> Putting a RTE_DEPRECATED in RTE_BUILD_BUG_ON directly triggers a build
> warning (caught by CI since we run with Werror).
> 

Would it not be sufficient to just make it an alias for the C11 static
assertions? It's not like its a lot of code to maintain, and if app users
have it in their code I'm not sure we get massive benefit from forcing them
to edit their code. I'd rather see it kept as a one-line macro purely from
a backward compatibility viewpoint. We can replace internal usages, though
- which can be checked by checkpatch.

/Bruce
  

On Fri, Sep 29, 2023 at 11:26 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Sep 29, 2023 at 11:02:38AM +0200, David Marchand wrote:
> > On Fri, Sep 29, 2023 at 10:54 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > In my opinion, our move to C11 thus makes RTE_BUILD_BUG_ON obsolete.
> >
> > That's my thought too.
> >
> > >
> > > We should mark RTE_BUILD_BUG_ON as deprecated, and disallow RTE_BUILD_BUG_ON in new code. Perhaps checkpatches could catch this?
> >
> > For a clear deprecation of a part of DPDK API, I don't see a need to
> > add something in checkpatch.
> > Putting a RTE_DEPRECATED in RTE_BUILD_BUG_ON directly triggers a build
> > warning (caught by CI since we run with Werror).
> >
>
> Would it not be sufficient to just make it an alias for the C11 static
> assertions? It's not like its a lot of code to maintain, and if app users
> have it in their code I'm not sure we get massive benefit from forcing them
> to edit their code. I'd rather see it kept as a one-line macro purely from
> a backward compatibility viewpoint. We can replace internal usages, though
> - which can be checked by checkpatch.

No, there is no massive benefit, just trying to reduce our ever
growing API surface.

Note, this macro should have been kept internal but it was introduced
at a time such matter was not considered...
  

29/09/2023 11:34, David Marchand:
> On Fri, Sep 29, 2023 at 11:26 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Fri, Sep 29, 2023 at 11:02:38AM +0200, David Marchand wrote:
> > > On Fri, Sep 29, 2023 at 10:54 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > > In my opinion, our move to C11 thus makes RTE_BUILD_BUG_ON obsolete.
> > >
> > > That's my thought too.
> > >
> > > >
> > > > We should mark RTE_BUILD_BUG_ON as deprecated, and disallow RTE_BUILD_BUG_ON in new code. Perhaps checkpatches could catch this?
> > >
> > > For a clear deprecation of a part of DPDK API, I don't see a need to
> > > add something in checkpatch.
> > > Putting a RTE_DEPRECATED in RTE_BUILD_BUG_ON directly triggers a build
> > > warning (caught by CI since we run with Werror).
> > >
> >
> > Would it not be sufficient to just make it an alias for the C11 static
> > assertions? It's not like its a lot of code to maintain, and if app users
> > have it in their code I'm not sure we get massive benefit from forcing them
> > to edit their code. I'd rather see it kept as a one-line macro purely from
> > a backward compatibility viewpoint. We can replace internal usages, though
> > - which can be checked by checkpatch.
> 
> No, there is no massive benefit, just trying to reduce our ever
> growing API surface.
> 
> Note, this macro should have been kept internal but it was introduced
> at a time such matter was not considered...

I agree with all.
Now taking techboard hat, we agreed to avoid breaking API if possible.
So we should keep RTE_BUILD_BUG_ON forever even if not used.
Internally we can replace its usages.
  

On Fri, Sep 29, 2023 at 12:26 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 29/09/2023 11:34, David Marchand:
> > On Fri, Sep 29, 2023 at 11:26 AM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Fri, Sep 29, 2023 at 11:02:38AM +0200, David Marchand wrote:
> > > > On Fri, Sep 29, 2023 at 10:54 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > > > In my opinion, our move to C11 thus makes RTE_BUILD_BUG_ON obsolete.
> > > >
> > > > That's my thought too.
> > > >
> > > > >
> > > > > We should mark RTE_BUILD_BUG_ON as deprecated, and disallow RTE_BUILD_BUG_ON in new code. Perhaps checkpatches could catch this?
> > > >
> > > > For a clear deprecation of a part of DPDK API, I don't see a need to
> > > > add something in checkpatch.
> > > > Putting a RTE_DEPRECATED in RTE_BUILD_BUG_ON directly triggers a build
> > > > warning (caught by CI since we run with Werror).
> > > >
> > >
> > > Would it not be sufficient to just make it an alias for the C11 static
> > > assertions? It's not like its a lot of code to maintain, and if app users
> > > have it in their code I'm not sure we get massive benefit from forcing them
> > > to edit their code. I'd rather see it kept as a one-line macro purely from
> > > a backward compatibility viewpoint. We can replace internal usages, though
> > > - which can be checked by checkpatch.
> >
> > No, there is no massive benefit, just trying to reduce our ever
> > growing API surface.
> >
> > Note, this macro should have been kept internal but it was introduced
> > at a time such matter was not considered...
>
> I agree with all.
> Now taking techboard hat, we agreed to avoid breaking API if possible.
> So we should keep RTE_BUILD_BUG_ON forever even if not used.
> Internally we can replace its usages.

So back to the original topic, I get that static_assert is ok for this patch.
  

29/09/2023 13:38, David Marchand:
> On Fri, Sep 29, 2023 at 12:26 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 29/09/2023 11:34, David Marchand:
> > > On Fri, Sep 29, 2023 at 11:26 AM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > On Fri, Sep 29, 2023 at 11:02:38AM +0200, David Marchand wrote:
> > > > > On Fri, Sep 29, 2023 at 10:54 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > > > > In my opinion, our move to C11 thus makes RTE_BUILD_BUG_ON obsolete.
> > > > >
> > > > > That's my thought too.
> > > > >
> > > > > >
> > > > > > We should mark RTE_BUILD_BUG_ON as deprecated, and disallow RTE_BUILD_BUG_ON in new code. Perhaps checkpatches could catch this?
> > > > >
> > > > > For a clear deprecation of a part of DPDK API, I don't see a need to
> > > > > add something in checkpatch.
> > > > > Putting a RTE_DEPRECATED in RTE_BUILD_BUG_ON directly triggers a build
> > > > > warning (caught by CI since we run with Werror).
> > > > >
> > > >
> > > > Would it not be sufficient to just make it an alias for the C11 static
> > > > assertions? It's not like its a lot of code to maintain, and if app users
> > > > have it in their code I'm not sure we get massive benefit from forcing them
> > > > to edit their code. I'd rather see it kept as a one-line macro purely from
> > > > a backward compatibility viewpoint. We can replace internal usages, though
> > > > - which can be checked by checkpatch.
> > >
> > > No, there is no massive benefit, just trying to reduce our ever
> > > growing API surface.
> > >
> > > Note, this macro should have been kept internal but it was introduced
> > > at a time such matter was not considered...
> >
> > I agree with all.
> > Now taking techboard hat, we agreed to avoid breaking API if possible.
> > So we should keep RTE_BUILD_BUG_ON forever even if not used.
> > Internally we can replace its usages.
> 
> So back to the original topic, I get that static_assert is ok for this patch.

Yes we can use static_assert.