[PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Daniel Gregory]

The ARM implementation of rte_pause uses RTE_BUILD_BUG_ON to check
memorder, which is not constant. This causes compile errors when it is
enabled with RTE_ARM_USE_WFE. eg.

../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
../lib/eal/include/rte_common.h:530:56: error: expression in static assertion is not constant
  530 | #define RTE_BUILD_BUG_ON(condition) do { static_assert(!(condition), #condition); } while (0)
      |                                                        ^~~~~~~~~~~~
../lib/eal/arm/include/rte_pause_64.h:156:9: note: in expansion of macro ‘RTE_BUILD_BUG_ON’
  156 |         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
      |         ^~~~~~~~~~~~~~~~

This has been the case since the switch to C11 assert (537caad2). Fix
the compile errors by replacing the check with an RTE_ASSERT.

Signed-off-by: Daniel Gregory <daniel.gregory@bytedance.com>
---
 lib/eal/arm/include/rte_pause_64.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/eal/arm/include/rte_pause_64.h b/lib/eal/arm/include/rte_pause_64.h
index 5cb8b59056..98e10e91c4 100644
--- a/lib/eal/arm/include/rte_pause_64.h
+++ b/lib/eal/arm/include/rte_pause_64.h
@@ -11,6 +11,7 @@ extern "C" {
 #endif
 
 #include <rte_common.h>
+#include <rte_debug.h>
 
 #ifdef RTE_ARM_USE_WFE
 #define RTE_WAIT_UNTIL_EQUAL_ARCH_DEFINED
@@ -153,7 +154,7 @@ rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected,
 {
 	uint16_t value;
 
-	RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
+	RTE_ASSERT(memorder != rte_memory_order_acquire &&
 		memorder != rte_memory_order_relaxed);
 
 	__RTE_ARM_LOAD_EXC_16(addr, value, memorder)
@@ -172,7 +173,7 @@ rte_wait_until_equal_32(volatile uint32_t *addr, uint32_t expected,
 {
 	uint32_t value;
 
-	RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
+	RTE_ASSERT(memorder != rte_memory_order_acquire &&
 		memorder != rte_memory_order_relaxed);
 
 	__RTE_ARM_LOAD_EXC_32(addr, value, memorder)
@@ -191,7 +192,7 @@ rte_wait_until_equal_64(volatile uint64_t *addr, uint64_t expected,
 {
 	uint64_t value;
 
-	RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
+	RTE_ASSERT(memorder != rte_memory_order_acquire &&
 		memorder != rte_memory_order_relaxed);
 
 	__RTE_ARM_LOAD_EXC_64(addr, value, memorder)
-- 
2.39.2

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Stephen Hemminger]

On Thu,  2 May 2024 15:21:16 +0100
Daniel Gregory <daniel.gregory@bytedance.com> wrote:

> The ARM implementation of rte_pause uses RTE_BUILD_BUG_ON to check
> memorder, which is not constant. This causes compile errors when it is
> enabled with RTE_ARM_USE_WFE. eg.
> 
> ../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
> ../lib/eal/include/rte_common.h:530:56: error: expression in static assertion is not constant
>   530 | #define RTE_BUILD_BUG_ON(condition) do { static_assert(!(condition), #condition); } while (0)
>       |                                                        ^~~~~~~~~~~~
> ../lib/eal/arm/include/rte_pause_64.h:156:9: note: in expansion of macro ‘RTE_BUILD_BUG_ON’
>   156 |         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
>       |         ^~~~~~~~~~~~~~~~
> 
> This has been the case since the switch to C11 assert (537caad2). Fix
> the compile errors by replacing the check with an RTE_ASSERT.
> 
> Signed-off-by: Daniel Gregory <daniel.gregory@bytedance.com>

The memory order passed to those routines must be constant.


Why not:
diff --git a/lib/eal/arm/include/rte_pause_64.h b/lib/eal/arm/include/rte_pause_64.h
index 5cb8b59056..81987de771 100644
--- a/lib/eal/arm/include/rte_pause_64.h
+++ b/lib/eal/arm/include/rte_pause_64.h
@@ -172,6 +172,8 @@ rte_wait_until_equal_32(volatile uint32_t *addr, uint32_t expected,
 {
        uint32_t value;
 
+       static_assert(__builtin_constant_p(memorder), "memory order is not a constant");
+
        RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
                memorder != rte_memory_order_relaxed);
 
@@ -191,6 +193,8 @@ rte_wait_until_equal_64(volatile uint64_t *addr, uint64_t expected,
 {
        uint64_t value;
 
+       static_assert(__builtin_constant_p(memorder), "memory order is not a constant");
+
        RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
                memorder != rte_memory_order_relaxed);
 

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Daniel Gregory]

On Thu, May 02, 2024 at 09:20:45AM -0700, Stephen Hemminger wrote:
> Why not:
> diff --git a/lib/eal/arm/include/rte_pause_64.h b/lib/eal/arm/include/rte_pause_64.h
> index 5cb8b59056..81987de771 100644
> --- a/lib/eal/arm/include/rte_pause_64.h
> +++ b/lib/eal/arm/include/rte_pause_64.h
> @@ -172,6 +172,8 @@ rte_wait_until_equal_32(volatile uint32_t *addr, uint32_t expected,
>  {
>         uint32_t value;
>  
> +       static_assert(__builtin_constant_p(memorder), "memory order is not a constant");
> +
>         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
>                 memorder != rte_memory_order_relaxed);
>  
> @@ -191,6 +193,8 @@ rte_wait_until_equal_64(volatile uint64_t *addr, uint64_t expected,
>  {
>         uint64_t value;
>  
> +       static_assert(__builtin_constant_p(memorder), "memory order is not a constant");
> +
>         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
>                 memorder != rte_memory_order_relaxed);
>  

What toolchain are you using? With your change I still get errors about
the expression not being constant:

In file included from ../lib/eal/arm/include/rte_pause.h:13,
                 from ../lib/eal/include/generic/rte_spinlock.h:25,
                 from ../lib/eal/arm/include/rte_spinlock.h:17,
                 from ../lib/telemetry/telemetry.c:20:
../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
../lib/eal/arm/include/rte_pause_64.h:156:23: error: expression in static assertion is not constant
156 |           static_assert(__builtin_constant_p(memorder), "memory order is not a constant");
    |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm cross-compiling with GCC v12.2 using the
config/arm/arm64_armv8_linux_gcc cross-file, and enabling
RTE_ARM_USE_WFE by uncommenting it in config/arm/meson.build and setting
its value to true.

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Stephen Hemminger]

On Thu, 2 May 2024 18:44:20 +0100
Daniel Gregory <daniel.gregory@bytedance.com> wrote:

> What toolchain are you using? With your change I still get errors about
> the expression not being constant:
> 
> In file included from ../lib/eal/arm/include/rte_pause.h:13,
>                  from ../lib/eal/include/generic/rte_spinlock.h:25,
>                  from ../lib/eal/arm/include/rte_spinlock.h:17,
>                  from ../lib/telemetry/telemetry.c:20:
> ../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
> ../lib/eal/arm/include/rte_pause_64.h:156:23: error: expression in static assertion is not constant
> 156 |           static_assert(__builtin_constant_p(memorder), "memory order is not a constant");
>     |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I'm cross-compiling with GCC v12.2 using the
> config/arm/arm64_armv8_linux_gcc cross-file, and enabling
> RTE_ARM_USE_WFE by uncommenting it in config/arm/meson.build and setting
> its value to true.


The problem with your patch is that checking at runtime generates additional
instructions in the critical path. Looks like more of a compiler bug.

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Stephen Hemminger]

On Thu, 2 May 2024 18:44:20 +0100
Daniel Gregory <daniel.gregory@bytedance.com> wrote:

> On Thu, May 02, 2024 at 09:20:45AM -0700, Stephen Hemminger wrote:
> > Why not:
> > diff --git a/lib/eal/arm/include/rte_pause_64.h b/lib/eal/arm/include/rte_pause_64.h
> > index 5cb8b59056..81987de771 100644
> > --- a/lib/eal/arm/include/rte_pause_64.h
> > +++ b/lib/eal/arm/include/rte_pause_64.h
> > @@ -172,6 +172,8 @@ rte_wait_until_equal_32(volatile uint32_t *addr, uint32_t expected,
> >  {
> >         uint32_t value;
> >  
> > +       static_assert(__builtin_constant_p(memorder), "memory order is not a constant");
> > +
> >         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
> >                 memorder != rte_memory_order_relaxed);
> >  
> > @@ -191,6 +193,8 @@ rte_wait_until_equal_64(volatile uint64_t *addr, uint64_t expected,
> >  {
> >         uint64_t value;
> >  
> > +       static_assert(__builtin_constant_p(memorder), "memory order is not a constant");
> > +
> >         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
> >                 memorder != rte_memory_order_relaxed);
> >    
> 
> What toolchain are you using? With your change I still get errors about
> the expression not being constant:
> 
> In file included from ../lib/eal/arm/include/rte_pause.h:13,
>                  from ../lib/eal/include/generic/rte_spinlock.h:25,
>                  from ../lib/eal/arm/include/rte_spinlock.h:17,
>                  from ../lib/telemetry/telemetry.c:20:
> ../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
> ../lib/eal/arm/include/rte_pause_64.h:156:23: error: expression in static assertion is not constant
> 156 |           static_assert(__builtin_constant_p(memorder), "memory order is not a constant");
>     |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I'm cross-compiling with GCC v12.2 using the
> config/arm/arm64_armv8_linux_gcc cross-file, and enabling
> RTE_ARM_USE_WFE by uncommenting it in config/arm/meson.build and setting
> its value to true.

I don't do ARM any more, suppose could build on Raspberry Pi but havent.

There are already constant checks like this elsewhere in the file. Why not this:
diff --git a/lib/eal/arm/include/rte_pause_64.h b/lib/eal/arm/include/rte_pause_64.h
index 5cb8b59056..4f54f5dac3 100644
--- a/lib/eal/arm/include/rte_pause_64.h
+++ b/lib/eal/arm/include/rte_pause_64.h
@@ -153,6 +153,7 @@ rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected,
 {
        uint16_t value;
 
+       RTE_BUILD_BUG_ON(!__builtin_constant_p(memorder));
        RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
                memorder != rte_memory_order_relaxed);
 
@@ -172,6 +173,7 @@ rte_wait_until_equal_32(volatile uint32_t *addr, uint32_t expected,
 {
        uint32_t value;
 
+       RTE_BUILD_BUG_ON(!__builtin_constant_p(memorder));
        RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
                memorder != rte_memory_order_relaxed);
 
@@ -191,6 +193,7 @@ rte_wait_until_equal_64(volatile uint64_t *addr, uint64_t expected,
 {
        uint64_t value;
 
+       RTE_BUILD_BUG_ON(!__builtin_constant_p(memorder));
        RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
                memorder != rte_memory_order_relaxed);

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Daniel Gregory]

On Thu, May 02, 2024 at 02:48:26PM -0700, Stephen Hemminger wrote:
> There are already constant checks like this elsewhere in the file.

Yes, but they're in macros, rather than inlined functions, so my
understanding was that at compile time, macro expansion has put the
memorder constant in the _Static_assert call as opposed to still being
a function parameter in the inline definition.

This is also the same approach used by the generic implementation
(lib/eal/include/generic/rte_pause.h), the inline functions use assert
and the macros use RTE_BUILD_BUG_ON.

To give a minimal example, the following inline function doesn't
compile (Godbolt demo here https://godbolt.org/z/aPqTf3v4o ):

static inline __attribute__((always_inline)) void
add(int *dst, int val)
{
	_Static_assert(val != 0, "adding zero does nothing");
	*dst += val;
}

But as a macro it does ( https://godbolt.org/z/x4a8fTf8h ):

#define add(dst, val) do {                                    \
	_Static_assert(val != 0, "adding zero does nothing"); \
	*(dst) += (val);                                      \
} while(0);

I don't believe this is a compiler bug as both GCC and Clang produce the
same error message.

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[David Marchand]

On Thu, May 2, 2024 at 4:22 PM Daniel Gregory
<daniel.gregory@bytedance.com> wrote:
>
> The ARM implementation of rte_pause uses RTE_BUILD_BUG_ON to check
> memorder, which is not constant. This causes compile errors when it is
> enabled with RTE_ARM_USE_WFE. eg.
>
> ../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
> ../lib/eal/include/rte_common.h:530:56: error: expression in static assertion is not constant
>   530 | #define RTE_BUILD_BUG_ON(condition) do { static_assert(!(condition), #condition); } while (0)
>       |                                                        ^~~~~~~~~~~~
> ../lib/eal/arm/include/rte_pause_64.h:156:9: note: in expansion of macro ‘RTE_BUILD_BUG_ON’
>   156 |         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
>       |         ^~~~~~~~~~~~~~~~
>
> This has been the case since the switch to C11 assert (537caad2). Fix
> the compile errors by replacing the check with an RTE_ASSERT.
>
> Signed-off-by: Daniel Gregory <daniel.gregory@bytedance.com>

- RTE_BUILD_BUG_ON() should not be used indeed.
IIRC, this issue was introduced with 875f350924b8 ("eal: add a new
helper for wait until scheme").
Please add a corresponding Fixes: tag in next revision.

Rather than use RTE_ASSERT(), you can simply revert to a assert()
call, like what was done before this change (and this is what is done
for the non WFE/generic implementation too, see
lib/eal/include/generic/rte_pause.h).


- This ARM specific implementation should take a rte_memory_order type
instead of a int type for the memorder input variable.
This was missed in 1ec6a845b5cb ("eal: use stdatomic API in public headers").

Could you send a fix for this small issue too?


Thanks!

-- 
David Marchand

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Daniel Gregory]

On Fri, May 03, 2024 at 03:32:20PM +0200, David Marchand wrote:
> - RTE_BUILD_BUG_ON() should not be used indeed.
> IIRC, this issue was introduced with 875f350924b8 ("eal: add a new
> helper for wait until scheme").
> Please add a corresponding Fixes: tag in next revision.

Will do. Should I CC stable@dpdk.org too?
 
> - This ARM specific implementation should take a rte_memory_order type
> instead of a int type for the memorder input variable.
> This was missed in 1ec6a845b5cb ("eal: use stdatomic API in public headers").
> 
> Could you send a fix for this small issue too?

Yes, sure thing.

Thanks, Daniel

[PATCH v2] eal/arm: replace RTE_BUILD_BUG on non-constant

[Daniel Gregory]

The ARM implementation of rte_pause uses RTE_BUILD_BUG_ON to check
memorder, which is not constant. This causes compile errors when it is
enabled with RTE_ARM_USE_WFE. eg.

../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
../lib/eal/include/rte_common.h:530:56: error: expression in static assertion is not constant
  530 | #define RTE_BUILD_BUG_ON(condition) do { static_assert(!(condition), #condition); } while (0)
      |                                                        ^~~~~~~~~~~~
../lib/eal/arm/include/rte_pause_64.h:156:9: note: in expansion of macro ‘RTE_BUILD_BUG_ON’
  156 |         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
      |         ^~~~~~~~~~~~~~~~

Fix the compile errors by replacing the check with an assert, like in
the generic implementation (lib/eal/include/generic/rte_pause.h).

Fixes: 875f350924b8 ("eal: add a new helper for wait until scheme")

Signed-off-by: Daniel Gregory <daniel.gregory@bytedance.com>
---
Cc: feifei.wang2@arm.com
---
 lib/eal/arm/include/rte_pause_64.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/eal/arm/include/rte_pause_64.h b/lib/eal/arm/include/rte_pause_64.h
index 5cb8b59056..852660091a 100644
--- a/lib/eal/arm/include/rte_pause_64.h
+++ b/lib/eal/arm/include/rte_pause_64.h
@@ -10,6 +10,8 @@
 extern "C" {
 #endif
 
+#include <assert.h>
+
 #include <rte_common.h>
 
 #ifdef RTE_ARM_USE_WFE
@@ -153,7 +155,7 @@ rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected,
 {
 	uint16_t value;
 
-	RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
+	assert(memorder != rte_memory_order_acquire &&
 		memorder != rte_memory_order_relaxed);
 
 	__RTE_ARM_LOAD_EXC_16(addr, value, memorder)
@@ -172,7 +174,7 @@ rte_wait_until_equal_32(volatile uint32_t *addr, uint32_t expected,
 {
 	uint32_t value;
 
-	RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
+	assert(memorder != rte_memory_order_acquire &&
 		memorder != rte_memory_order_relaxed);
 
 	__RTE_ARM_LOAD_EXC_32(addr, value, memorder)
@@ -191,7 +193,7 @@ rte_wait_until_equal_64(volatile uint64_t *addr, uint64_t expected,
 {
 	uint64_t value;
 
-	RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
+	assert(memorder != rte_memory_order_acquire &&
 		memorder != rte_memory_order_relaxed);
 
 	__RTE_ARM_LOAD_EXC_64(addr, value, memorder)
-- 
2.39.2

Re: [PATCH v2] eal/arm: replace RTE_BUILD_BUG on non-constant

[Daniel Gregory]

Apologies, mis-sent this before attaching a changelog:

v2:
* replaced RTE_ASSERT with assert
* added Fixes: tag

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Stephen Hemminger]

On Fri, 3 May 2024 10:46:05 +0100
Daniel Gregory <daniel.gregory@bytedance.com> wrote:

> On Thu, May 02, 2024 at 02:48:26PM -0700, Stephen Hemminger wrote:
> > There are already constant checks like this elsewhere in the file.  
> 
> Yes, but they're in macros, rather than inlined functions, so my
> understanding was that at compile time, macro expansion has put the
> memorder constant in the _Static_assert call as opposed to still being
> a function parameter in the inline definition.

Gcc and clang are smart enough that it is possible to use the internal
__builtin_constant_p() in the function. Some examples in DPDK:

static __rte_always_inline int
rte_mempool_do_generic_get(struct rte_mempool *mp, void **obj_table,
			   unsigned int n, struct rte_mempool_cache *cache)
{
	int ret;
	unsigned int remaining;
	uint32_t index, len;
	void **cache_objs;

	/* No cache provided */
	if (unlikely(cache == NULL)) {
		remaining = n;
		goto driver_dequeue;
	}

	/* The cache is a stack, so copy will be in reverse order. */
	cache_objs = &cache->objs[cache->len];

	if (__extension__(__builtin_constant_p(n)) && n <= cache->len) {

It should be possible to use RTE_BUILD_BUG_ON() or static_assert here.

Changing a compile time check into a runtime check means that buggy programs
blow up much later and in a confusing manner. And it impacts all code that
is doing a spin lock, certainly one of the hottest paths in DPDK.

Re: [PATCH v2] eal/arm: replace RTE_BUILD_BUG on non-constant

[Stephen Hemminger]

On Fri,  3 May 2024 19:27:30 +0100
Daniel Gregory <daniel.gregory@bytedance.com> wrote:

> The ARM implementation of rte_pause uses RTE_BUILD_BUG_ON to check
> memorder, which is not constant. This causes compile errors when it is
> enabled with RTE_ARM_USE_WFE. eg.
> 
> ../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
> ../lib/eal/include/rte_common.h:530:56: error: expression in static assertion is not constant
>   530 | #define RTE_BUILD_BUG_ON(condition) do { static_assert(!(condition), #condition); } while (0)
>       |                                                        ^~~~~~~~~~~~
> ../lib/eal/arm/include/rte_pause_64.h:156:9: note: in expansion of macro ‘RTE_BUILD_BUG_ON’
>   156 |         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
>       |         ^~~~~~~~~~~~~~~~
> 
> Fix the compile errors by replacing the check with an assert, like in
> the generic implementation (lib/eal/include/generic/rte_pause.h).

No, don't hide the problem.

What code is calling these. Looks like a real bug. Could be behind layers of wrappers.

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Stephen Hemminger]

On Thu,  2 May 2024 15:21:16 +0100
Daniel Gregory <daniel.gregory@bytedance.com> wrote:

> The ARM implementation of rte_pause uses RTE_BUILD_BUG_ON to check
> memorder, which is not constant. This causes compile errors when it is
> enabled with RTE_ARM_USE_WFE. eg.
> 
> ../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
> ../lib/eal/include/rte_common.h:530:56: error: expression in static assertion is not constant
>   530 | #define RTE_BUILD_BUG_ON(condition) do { static_assert(!(condition), #condition); } while (0)
>       |                                                        ^~~~~~~~~~~~
> ../lib/eal/arm/include/rte_pause_64.h:156:9: note: in expansion of macro ‘RTE_BUILD_BUG_ON’
>   156 |         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
>       |         ^~~~~~~~~~~~~~~~
> 
> This has been the case since the switch to C11 assert (537caad2). Fix
> the compile errors by replacing the check with an RTE_ASSERT.
> 
> Signed-off-by: Daniel Gregory <daniel.gregory@bytedance.com>

The only calls to rte_wait_until_equal_16 in upstream code
are in the test_bbdev_perf.c and test_timer.c.  Looks like
these test never got fixed to use rte_memory_order instead of __ATOMIC_ defines.

And there should be a CI test for ARM that enables the WFE code at least
to ensure it works!

Re: [PATCH v2] eal/arm: replace RTE_BUILD_BUG on non-constant

[Ruifeng Wang]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

+ Arm team to the loop.
Removed invalid email address.

From: Daniel Gregory <daniel.gregory@bytedance.com>
Date: Saturday, May 4, 2024 at 2:27 AM
To: Ruifeng Wang <Ruifeng.Wang@arm.com>
Cc: dev@dpdk.org <dev@dpdk.org>, Punit Agrawal <punit.agrawal@bytedance.com>, Liang Ma <liangma@bytedance.com>, Daniel Gregory <daniel.gregory@bytedance.com>, Feifei Wang <Feifei.Wang2@arm.com>
Subject: [PATCH v2] eal/arm: replace RTE_BUILD_BUG on non-constant
The ARM implementation of rte_pause uses RTE_BUILD_BUG_ON to check
memorder, which is not constant. This causes compile errors when it is
enabled with RTE_ARM_USE_WFE. eg.

../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
../lib/eal/include/rte_common.h:530:56: error: expression in static assertion is not constant
  530 | #define RTE_BUILD_BUG_ON(condition) do { static_assert(!(condition), #condition); } while (0)
      |                                                        ^~~~~~~~~~~~
../lib/eal/arm/include/rte_pause_64.h:156:9: note: in expansion of macro ‘RTE_BUILD_BUG_ON’
  156 |         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
      |         ^~~~~~~~~~~~~~~~

Fix the compile errors by replacing the check with an assert, like in
the generic implementation (lib/eal/include/generic/rte_pause.h).

Fixes: 875f350924b8 ("eal: add a new helper for wait until scheme")

Signed-off-by: Daniel Gregory <daniel.gregory@bytedance.com>
---
Cc: feifei.wang2@arm.com
---
 lib/eal/arm/include/rte_pause_64.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)



[-- Attachment #2: Type: text/html, Size: 4372 bytes --]

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Daniel Gregory]

On Fri, May 03, 2024 at 05:56:24PM -0700, Stephen Hemminger wrote:
> On Fri, 3 May 2024 10:46:05 +0100
> Daniel Gregory <daniel.gregory@bytedance.com> wrote:
> 
> > On Thu, May 02, 2024 at 02:48:26PM -0700, Stephen Hemminger wrote:
> > > There are already constant checks like this elsewhere in the file.  
> > 
> > Yes, but they're in macros, rather than inlined functions, so my
> > understanding was that at compile time, macro expansion has put the
> > memorder constant in the _Static_assert call as opposed to still being
> > a function parameter in the inline definition.
> 
> Gcc and clang are smart enough that it is possible to use the internal
> __builtin_constant_p() in the function. Some examples in DPDK:
> 
> static __rte_always_inline int
> rte_mempool_do_generic_get(struct rte_mempool *mp, void **obj_table,
> 			   unsigned int n, struct rte_mempool_cache *cache)
> {
> 	int ret;
> 	unsigned int remaining;
> 	uint32_t index, len;
> 	void **cache_objs;
> 
> 	/* No cache provided */
> 	if (unlikely(cache == NULL)) {
> 		remaining = n;
> 		goto driver_dequeue;
> 	}
> 
> 	/* The cache is a stack, so copy will be in reverse order. */
> 	cache_objs = &cache->objs[cache->len];
> 
> 	if (__extension__(__builtin_constant_p(n)) && n <= cache->len) {
> 
> It should be possible to use RTE_BUILD_BUG_ON() or static_assert here.

Yes, it's possible to use RTE_BUILD_BUG_ON(!__builtin_constant_p(n)) on
Clang, I am simply not seeing it succeed. In fact, the opposite check,
that the memorder is not constant, builds just fine with Clang 16.


diff --git a/lib/eal/arm/include/rte_pause_64.h b/lib/eal/arm/include/rte_pause_64.h
index 5cb8b59056..d0646320e6 100644
--- a/lib/eal/arm/include/rte_pause_64.h
+++ b/lib/eal/arm/include/rte_pause_64.h
@@ -153,8 +153,7 @@ rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected,
 {
 	uint16_t value;
 
-	RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
-		memorder != rte_memory_order_relaxed);
+	RTE_BUILD_BUG_ON(__builtin_constant_p(memorder));
 
 	__RTE_ARM_LOAD_EXC_16(addr, value, memorder)
 	if (value != expected) {
@@ -172,8 +171,7 @@ rte_wait_until_equal_32(volatile uint32_t *addr, uint32_t expected,
 {
 	uint32_t value;
 
-	RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
-		memorder != rte_memory_order_relaxed);
+	RTE_BUILD_BUG_ON(__builtin_constant_p(memorder));
 
 	__RTE_ARM_LOAD_EXC_32(addr, value, memorder)
 	if (value != expected) {
@@ -191,8 +189,7 @@ rte_wait_until_equal_64(volatile uint64_t *addr, uint64_t expected,
 {
 	uint64_t value;
 
-	RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
-		memorder != rte_memory_order_relaxed);
+	RTE_BUILD_BUG_ON(__builtin_constant_p(memorder));
 
 	__RTE_ARM_LOAD_EXC_64(addr, value, memorder)
 	if (value != expected) {
diff --git a/lib/eal/include/generic/rte_pause.h b/lib/eal/include/generic/rte_pause.h
index f2a1eadcbd..3973488865 100644
--- a/lib/eal/include/generic/rte_pause.h
+++ b/lib/eal/include/generic/rte_pause.h
@@ -80,6 +80,7 @@ static __rte_always_inline void
 rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected,
 		rte_memory_order memorder)
 {
+	RTE_BUILD_BUG_ON(__builtin_constant_p(memorder));
 	assert(memorder == rte_memory_order_acquire || memorder == rte_memory_order_relaxed);
 
 	while (rte_atomic_load_explicit((volatile __rte_atomic uint16_t *)addr, memorder)
@@ -91,6 +92,7 @@ static __rte_always_inline void
 rte_wait_until_equal_32(volatile uint32_t *addr, uint32_t expected,
 		rte_memory_order memorder)
 {
+	RTE_BUILD_BUG_ON(__builtin_constant_p(memorder));
 	assert(memorder == rte_memory_order_acquire || memorder == rte_memory_order_relaxed);
 
 	while (rte_atomic_load_explicit((volatile __rte_atomic uint32_t *)addr, memorder)
@@ -102,6 +104,7 @@ static __rte_always_inline void
 rte_wait_until_equal_64(volatile uint64_t *addr, uint64_t expected,
 		rte_memory_order memorder)
 {
+	RTE_BUILD_BUG_ON(__builtin_constant_p(memorder));
 	assert(memorder == rte_memory_order_acquire || memorder == rte_memory_order_relaxed);
 
 	while (rte_atomic_load_explicit((volatile __rte_atomic uint64_t *)addr, memorder)


This seemed odd, and it doesn't line up with what the GCC documentation
says about __builtin_constant_p:

> [__builtin_constant_p] does not return 1 when you pass a constant
> numeric value to the inline function unless you specify the -O option.
https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

So I did some more looking and the behaviour I've seen is that both
Clang and GCC don't allow for static checks on inline function
parameters:

static inline __attribute__((always_inline)) int
fn(int val)
{
	_Static_assert(val == 0, "val nonzero");
	return 0;
}

int main(void) {
	return fn(0);
}

( https://godbolt.org/z/TrfWqYoGo )

Both GCC and Clang's __builtin_constant_p return 1 at runtime when
called on an inline function parameter

static inline __attribute__((always_inline)) int
fn(int val)
{
	return __builtin_constant_p(val);
}

int main(void) {
	return fn(0);
}

( https://godbolt.org/z/hTGqvj91K )

But GCC doesn't allow for static assertions on the return value of
__builtin_constant_p, error-ing that the expression is not constant:

static inline __attribute__((always_inline)) int
fn(int val)
{
	_Static_assert(__builtin_constant_p(val), "v isn't constant");
	return 0;
}

int main(void) {
	return fn(0);
}

( https://godbolt.org/z/4799nEK1T )

And on Clang these static assertions are allowed, but always fail
( https://godbolt.org/z/6j16c5MbT ).

I don't really have the experience with Clang or GCC to assess whether
this is intended behaviour or a bug (I've not seen evidence this
behaviour is new or different from previous versions), it's just what
I've observed.
 
> Changing a compile time check into a runtime check means that buggy programs
> blow up much later and in a confusing manner. And it impacts all code that
> is doing a spin lock, certainly one of the hottest paths in DPDK.

True, buggy programs will error later, but other than converting the
rte_wait_until_equal_* functions to macros, I haven't seen an approach
that will let us use a static assert on the passed in memory order
value right now.

However, new enough GCC and Clang versions will optimise out runtime
checks that are guaranteed to pass, such as in this case when an inline
functions is called with a constant value. To give a reduced example:

#include <assert.h>

enum E {
	X,
	Y,
	Z,
};

static inline __attribute__((always_inline)) void
add(int *dst, enum E e)
{
	assert(e == X || e == Y);
	if (e == X)
		(*dst)++;
	else
		(*dst) += 4;
}
}

int
fn(int num)
{
	add(&num, Y);
	return num;
}

(GCC https://godbolt.org/z/GKGEa7Wdd )
(Clang https://godbolt.org/z/1vYv8MYcs )

With optimisations they both produce a fn that doesn't perform any check
on the value of e, and simply return the input+4.

Therefore, this patch shouldn't have an impact on the performance of
locks when built in release mode.

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Daniel Gregory]

On Fri, May 03, 2024 at 06:02:36PM -0700, Stephen Hemminger wrote:
> On Thu,  2 May 2024 15:21:16 +0100
> Daniel Gregory <daniel.gregory@bytedance.com> wrote:
> 
> > The ARM implementation of rte_pause uses RTE_BUILD_BUG_ON to check
> > memorder, which is not constant. This causes compile errors when it is
> > enabled with RTE_ARM_USE_WFE. eg.
> > 
> > ../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
> > ../lib/eal/include/rte_common.h:530:56: error: expression in static assertion is not constant
> >   530 | #define RTE_BUILD_BUG_ON(condition) do { static_assert(!(condition), #condition); } while (0)
> >       |                                                        ^~~~~~~~~~~~
> > ../lib/eal/arm/include/rte_pause_64.h:156:9: note: in expansion of macro ‘RTE_BUILD_BUG_ON’
> >   156 |         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
> >       |         ^~~~~~~~~~~~~~~~
> > 
> > This has been the case since the switch to C11 assert (537caad2). Fix
> > the compile errors by replacing the check with an RTE_ASSERT.
> > 
> > Signed-off-by: Daniel Gregory <daniel.gregory@bytedance.com>
> 
> The only calls to rte_wait_until_equal_16 in upstream code
> are in the test_bbdev_perf.c and test_timer.c.  Looks like
> these test never got fixed to use rte_memory_order instead of __ATOMIC_ defines.

Apologies, the commit message could make it clearer, but this is also an
issue for rte_wait_until_equal_32 and rte_wait_until_equal_64.

rte_wait_until_equal_32 is used in a dozen or so lock tests with the old
__ATOMIC_ defines, as well as rte_ring_generic_pvt.h and
rte_ring_c11_pvt.h, where it's used with the new rte_memorder_order
values. Correct me if I'm wrong, but shouldn't the static assertions in
rte_stdatomic.h ensure that mixed usage doesn't cause any issues, even
if using the older __ATOMIC_ defines isn't ideal?
 
> And there should be a CI test for ARM that enables the WFE code at least
> to ensure it works!

Yes, that could've caught this sooner.

Re: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Tyler Retzlaff]

On Thu, May 09, 2024 at 12:11:50PM +0100, Daniel Gregory wrote:
> On Fri, May 03, 2024 at 06:02:36PM -0700, Stephen Hemminger wrote:
> > On Thu,  2 May 2024 15:21:16 +0100
> > Daniel Gregory <daniel.gregory@bytedance.com> wrote:
> > 
> > > The ARM implementation of rte_pause uses RTE_BUILD_BUG_ON to check
> > > memorder, which is not constant. This causes compile errors when it is
> > > enabled with RTE_ARM_USE_WFE. eg.
> > > 
> > > ../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
> > > ../lib/eal/include/rte_common.h:530:56: error: expression in static assertion is not constant
> > >   530 | #define RTE_BUILD_BUG_ON(condition) do { static_assert(!(condition), #condition); } while (0)
> > >       |                                                        ^~~~~~~~~~~~
> > > ../lib/eal/arm/include/rte_pause_64.h:156:9: note: in expansion of macro ‘RTE_BUILD_BUG_ON’
> > >   156 |         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
> > >       |         ^~~~~~~~~~~~~~~~
> > > 
> > > This has been the case since the switch to C11 assert (537caad2). Fix
> > > the compile errors by replacing the check with an RTE_ASSERT.
> > > 
> > > Signed-off-by: Daniel Gregory <daniel.gregory@bytedance.com>
> > 
> > The only calls to rte_wait_until_equal_16 in upstream code
> > are in the test_bbdev_perf.c and test_timer.c.  Looks like
> > these test never got fixed to use rte_memory_order instead of __ATOMIC_ defines.
> 
> Apologies, the commit message could make it clearer, but this is also an
> issue for rte_wait_until_equal_32 and rte_wait_until_equal_64.
> 
> rte_wait_until_equal_32 is used in a dozen or so lock tests with the old
> __ATOMIC_ defines, as well as rte_ring_generic_pvt.h and
> rte_ring_c11_pvt.h, where it's used with the new rte_memorder_order
> values. Correct me if I'm wrong, but shouldn't the static assertions in
> rte_stdatomic.h ensure that mixed usage doesn't cause any issues, even
> if using the older __ATOMIC_ defines isn't ideal?

this is just informational.

the static assertions are intended to make sure there is alignment
between the value produced by the rte_memory_order and __ATOMIC_ constant
expressions. so you can expect that intermixing __ATOMIC_ and rte_memory_order
should work.

the older __ATOMIC_ are still used in tests because i just haven't had
time to finish conversion. i have a series up now that makes most of the
conversions, it is waiting for review.

>  
> > And there should be a CI test for ARM that enables the WFE code at least
> > to ensure it works!
> 
> Yes, that could've caught this sooner.

RE: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant

[Wathsala Wathawana Vithanage]

> Subject: [PATCH] eal/arm: replace RTE_BUILD_BUG on non-constant
> 
> The ARM implementation of rte_pause uses RTE_BUILD_BUG_ON to check
> memorder, which is not constant. This causes compile errors when it is
> enabled with RTE_ARM_USE_WFE. eg.

Use of wfe based rte_wait_until_equal_ implementations  had performance issues
when used in lock free data structures like rte_ring. 
To prevent these functions from getting included RTE_ARM_USE_WFE was
introduced some time back and this issue has been hiding so far because of that.
Solution looks good to me.  

> 
> ../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
> ../lib/eal/include/rte_common.h:530:56: error: expression in static assertion
> is not constant
>   530 | #define RTE_BUILD_BUG_ON(condition) do {
> static_assert(!(condition), #condition); } while (0)
>       |                                                        ^~~~~~~~~~~~
> ../lib/eal/arm/include/rte_pause_64.h:156:9: note: in expansion of macro
> ‘RTE_BUILD_BUG_ON’
>   156 |         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
>       |         ^~~~~~~~~~~~~~~~
> 
> This has been the case since the switch to C11 assert (537caad2). Fix the
> compile errors by replacing the check with an RTE_ASSERT.
> 
> Signed-off-by: Daniel Gregory <daniel.gregory@bytedance.com>

Acked-by: Wathsala Vithanage <wathsala.vithanage@arm.com>

RE: [PATCH v2] eal/arm: replace RTE_BUILD_BUG on non-constant

[Wathsala Wathawana Vithanage]

> ../lib/eal/arm/include/rte_pause_64.h: In function ‘rte_wait_until_equal_16’:
> ../lib/eal/include/rte_common.h:530:56: error: expression in static assertion
> is not constant
>   530 | #define RTE_BUILD_BUG_ON(condition) do {
> static_assert(!(condition), #condition); } while (0)
>       |                                                        ^~~~~~~~~~~~
> ../lib/eal/arm/include/rte_pause_64.h:156:9: note: in expansion of macro
> ‘RTE_BUILD_BUG_ON’
>   156 |         RTE_BUILD_BUG_ON(memorder != rte_memory_order_acquire &&
>       |         ^~~~~~~~~~~~~~~~
> 
> Fix the compile errors by replacing the check with an assert, like in the generic
> implementation (lib/eal/include/generic/rte_pause.h).
> 
> Fixes: 875f350924b8 ("eal: add a new helper for wait until scheme")
> 
> Signed-off-by: Daniel Gregory <daniel.gregory@bytedance.com>

Acked-by: Wathsala Vithanage <wathsala.vithanage@arm.com>