Re: [PATCH v2 1/2] spinlock: remove volatile qualifier

["Robin Jarry" <rjarry@redhat.com>]

Hey Thomas,

Thomas Monjalon, May 18, 2026 at 17:07:
> When compiling with C++20 standard requirement (default in GCC 16),
> the increment and decrement of volatile variables are rejected:
>
> rte_spinlock.h:241:14: error:
> 	'++' expression of 'volatile'-qualified type is deprecated
> rte_spinlock.h:252:21: error:
> 	'--' expression of 'volatile'-qualified type is deprecated
> rte_spinlock.h:278:14: error:
> 	'++' expression of 'volatile'-qualified type is deprecated
>
> The count field of rte_spinlock_recursive_t
> does not need the volatile qualifier
> because it is only accessed by the thread holding the lock,
> which already provides the necessary memory ordering.
>
> The user field can be accessed outside of the lock,
> so it must handled as a C11 atomic variable.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> v1: drop volatile keyword
> v2: make user an atomic variable
> ---
>  lib/eal/include/generic/rte_spinlock.h | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/lib/eal/include/generic/rte_spinlock.h b/lib/eal/include/generic/rte_spinlock.h
> index c907d4e45c..5d810b682a 100644
> --- a/lib/eal/include/generic/rte_spinlock.h
> +++ b/lib/eal/include/generic/rte_spinlock.h
> @@ -197,8 +197,8 @@ rte_spinlock_trylock_tm(rte_spinlock_t *sl)
>   */
>  typedef struct {
>  	rte_spinlock_t sl; /**< the actual spinlock */
> -	volatile int user; /**< core id using lock, -1 for unused */
> -	volatile int count; /**< count of time this lock has been called */
> +	RTE_ATOMIC(int) user; /**< core id using lock, -1 for unused */
> +	int count; /**< count of time this lock has been called */
>  } rte_spinlock_recursive_t;
>  
>  /**
> @@ -215,7 +215,7 @@ typedef struct {
>  static inline void rte_spinlock_recursive_init(rte_spinlock_recursive_t *slr)
>  {
>  	rte_spinlock_init(&slr->sl);
> -	slr->user = -1;
> +	rte_atomic_store_explicit(&slr->user, -1, rte_memory_order_relaxed);
>  	slr->count = 0;
>  }
>  
> @@ -230,9 +230,9 @@ static inline void rte_spinlock_recursive_lock(rte_spinlock_recursive_t *slr)
>  {
>  	int id = rte_gettid();
>  
> -	if (slr->user != id) {
> +	if (rte_atomic_load_explicit(&slr->user, rte_memory_order_relaxed) != id) {

This needs to be rte_memory_order_acquire

>  		rte_spinlock_lock(&slr->sl);
> -		slr->user = id;
> +		rte_atomic_store_explicit(&slr->user, id, rte_memory_order_relaxed);

And rte_memory_order_release

>  	}
>  	slr->count++;
>  }
> @@ -246,10 +246,9 @@ static inline void rte_spinlock_recursive_unlock(rte_spinlock_recursive_t *slr)
>  	__rte_no_thread_safety_analysis
>  {
>  	if (--(slr->count) == 0) {

This code is completely broken. Any thread can unlock without any check.

I think it should be:

diff --git a/lib/eal/include/generic/rte_spinlock.h b/lib/eal/include/generic/rte_spinlock.h
index c907d4e45c39..b1058e4f8b4f 100644
--- a/lib/eal/include/generic/rte_spinlock.h
+++ b/lib/eal/include/generic/rte_spinlock.h
@@ -245,11 +247,17 @@ static inline void rte_spinlock_recursive_lock(rte_spinlock_recursive_t *slr)
 static inline void rte_spinlock_recursive_unlock(rte_spinlock_recursive_t *slr)
        __rte_no_thread_safety_analysis
 {
-       if (--(slr->count) == 0) {
-               slr->user = -1;
-               rte_spinlock_unlock(&slr->sl);
-       }
+       int id = rte_gettid();

+       if (rte_atomic_load_explicit(&slr->user, rte_memory_order_acquire) == id) {
+               RTE_ASSERT(slr->count > 0);
+               if (--(slr->count) == 0) {
+                       rte_atomic_store_explicit(&slr->user, -1, rte_memory_order_release);
+                       rte_spinlock_unlock(&slr->sl);
+               }
+       } else {
+               RTE_ASSERT(id != -1 && "unlocked from another thread");
+       }
 }

(not tested)

> -		slr->user = -1;
> +		rte_atomic_store_explicit(&slr->user, -1, rte_memory_order_relaxed);
>  		rte_spinlock_unlock(&slr->sl);
>  	}
> -
>  }
>  
>  /**
> @@ -266,10 +265,10 @@ static inline int rte_spinlock_recursive_trylock(rte_spinlock_recursive_t *slr)
>  {
>  	int id = rte_gettid();
>  
> -	if (slr->user != id) {
> +	if (rte_atomic_load_explicit(&slr->user, rte_memory_order_relaxed) != id) {

rte_memory_order_acquire

>  		if (rte_spinlock_trylock(&slr->sl) == 0)
>  			return 0;
> -		slr->user = id;
> +		rte_atomic_store_explicit(&slr->user, id, rte_memory_order_relaxed);

rte_memory_order_release

>  	}
>  	slr->count++;
>  	return 1;


-- 
Robin

> Monitored by the American Human Association.