On Mon, 18 May 2026 17:14:54 +0200
"Robin Jarry" <[email protected]> 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: [email protected]
> >
> > Signed-off-by: Thomas Monjalon <[email protected]>
> > ---
> > 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.
