|SUCCESS| [v2,2/2] spinlock: fix API comments

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164085

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-18

commit d3f8a12c4e5b7a90f823456789abcdef01234567
Author: Thomas Monjalon <thomas at monjalon.net>
Subject: [PATCH v2 1/2] spinlock: remove volatile qualifier

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.

[ ... ]

> 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;

The memory ordering for the count field needs careful review. Since
count is incremented and decremented without atomic operations, is this
safe when rte_spinlock_recursive_unlock() decrements it to zero? Can the
count decrement reorder with the subsequent user store in
rte_spinlock_recursive_unlock()?

>  
>  /**
> @@ -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) {
>  		rte_spinlock_lock(&slr->sl);
> -		slr->user = id;
> +		rte_atomic_store_explicit(&slr->user, id, rte_memory_order_relaxed);

Is relaxed ordering sufficient here? The spinlock provides acquire
semantics, but does the user store need to be ordered with respect to the
lock acquisition? Should this be rte_memory_order_release to ensure that
the user assignment happens-after the lock is acquired?

>  	}
>  	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) {
> -		slr->user = -1;
> +		rte_atomic_store_explicit(&slr->user, -1, rte_memory_order_relaxed);
>  		rte_spinlock_unlock(&slr->sl);

Does the non-atomic decrement of count race with the atomic store to user?
In rte_spinlock_recursive_unlock(), the count-- happens first, then the
user store. Can another thread observe user == -1 before the count reaches
zero due to reordering?

Also, should the user store use rte_memory_order_release to ensure proper
ordering with the subsequent unlock?

>  	}
> -
>  }
>  
>  /**
> @@ -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) {
>  		if (rte_spinlock_trylock(&slr->sl) == 0)
>  			return 0;
> -		slr->user = id;
> +		rte_atomic_store_explicit(&slr->user, id, rte_memory_order_relaxed);
>  	}
>  	slr->count++;
>  	return 1;

Same question as in rte_spinlock_recursive_lock() - should this store use
release ordering?

[ ... ]