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

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 18 May 2026 17:14:54 +0200
"Robin Jarry" <rjarry@redhat.com> wrote:

> 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

No it does not. There is no dependency here.
The acquire is inside the spinlock.

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

Ditto.

> 
> >  	}
> >  	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 would make an RTE_ASSERT() that id matched current thread id.
Since caller holds lock, no atomic required for that.