[PATCH] spinlock: remove volatile qualifier

[PATCH] spinlock: remove volatile qualifier

[Thomas Monjalon]

The user and count fields of rte_spinlock_recursive_t
do not need the volatile qualifier
because they are only accessed by the thread holding the lock,
which already provides the necessary memory ordering.

Removing volatile aligns with a C++20 deprecation
for increment and decrement of volatile variables.

This issue was seen with GCC 16 which changes the default C++ version
from -std=gnu++17 to -std=gnu++20.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/eal/include/generic/rte_spinlock.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/eal/include/generic/rte_spinlock.h b/lib/eal/include/generic/rte_spinlock.h
index c907d4e45c..19c0e34f0a 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 */
+	int user; /**< core id using lock, -1 for unused */
+	int count; /**< count of time this lock has been called */
 } rte_spinlock_recursive_t;
 
 /**
-- 
2.54.0

Re: [PATCH] spinlock: remove volatile qualifier

[Thomas Monjalon]

04/05/2026 10:37, Thomas Monjalon:
> The user and count fields of rte_spinlock_recursive_t
> do not need the volatile qualifier
> because they are only accessed by the thread holding the lock,
> which already provides the necessary memory ordering.
> 
> Removing volatile aligns with a C++20 deprecation
> for increment and decrement of volatile variables.
> 
> This issue was seen with GCC 16 which changes the default C++ version
> from -std=gnu++17 to -std=gnu++20.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

I've just found this has been discussed 4 years ago:
https://inbox.dpdk.org/dev/20221221083717.135c3f81@hermes.local

Stephen had found some comments issues that I will fix in another patch.

[PATCH] spinlock: fix API comments

[Thomas Monjalon]

This is not a read-write lock.

The user field stores the thread ID, not the core ID.

The implementation is architecture-specific in some cases only.

Reported-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/eal/include/generic/rte_spinlock.h | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lib/eal/include/generic/rte_spinlock.h b/lib/eal/include/generic/rte_spinlock.h
index 19c0e34f0a..d9f255f8c4 100644
--- a/lib/eal/include/generic/rte_spinlock.h
+++ b/lib/eal/include/generic/rte_spinlock.h
@@ -7,12 +7,16 @@
 
 /**
  * @file
+ * DPDK spinlocks
  *
- * RTE Spinlocks
+ * This is an API for spinlocks.
+ * This kind of lock simply waits in a loop
+ * repeatedly checking until the lock becomes available.
  *
- * This file defines an API for read-write locks, which are implemented
- * in an architecture-specific way. This kind of lock simply waits in
- * a loop repeatedly checking until the lock becomes available.
+ * Some functions may have an architecture-specific implementation
+ * if RTE_FORCE_INTRINSICS is disabled.
+ * The hardware transactional memory (lock elision) functions have _tm suffix
+ * and are implemented in architecture-specific files.
  *
  * All locks must be initialised before use, and only initialised once.
  */
@@ -197,7 +201,7 @@ rte_spinlock_trylock_tm(rte_spinlock_t *sl)
  */
 typedef struct {
 	rte_spinlock_t sl; /**< the actual spinlock */
-	int user; /**< core id using lock, -1 for unused */
+	int user; /**< thread id using lock, -1 for unused */
 	int count; /**< count of time this lock has been called */
 } rte_spinlock_recursive_t;
 
-- 
2.54.0

Re: [PATCH] spinlock: fix API comments

[Thomas Monjalon]

04/05/2026 11:12, Thomas Monjalon:
> This is not a read-write lock.
> 
> The user field stores the thread ID, not the core ID.
> 
> The implementation is architecture-specific in some cases only.
> 
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Fixes: ca2e2dab079a ("spinlock: support non-EAL thread")
Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Re: [PATCH] spinlock: remove volatile qualifier

[Stephen Hemminger]

On Mon,  4 May 2026 10:37:14 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> diff --git a/lib/eal/include/generic/rte_spinlock.h b/lib/eal/include/generic/rte_spinlock.h
> index c907d4e45c..19c0e34f0a 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 */
> +	int user; /**< core id using lock, -1 for unused */
> +	int count; /**< count of time this lock has been called */

It might make sense to use unsigned for count, and sized types.
I.e do you really need 32 bit values?

Only in tree use of recursive spinlock in vdev code.

PS: I wonder if we really need to keep the transactional memory versions of stuff.
I 

Re: [PATCH] spinlock: remove volatile qualifier

[Thomas Monjalon]

04/05/2026 19:02, Stephen Hemminger:
> On Mon,  4 May 2026 10:37:14 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > diff --git a/lib/eal/include/generic/rte_spinlock.h b/lib/eal/include/generic/rte_spinlock.h
> > index c907d4e45c..19c0e34f0a 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 */
> > +	int user; /**< core id using lock, -1 for unused */
> > +	int count; /**< count of time this lock has been called */
> 
> It might make sense to use unsigned for count, and sized types.
> I.e do you really need 32 bit values?

Such a change would need to be evaluated in a performance benchmark.

> Only in tree use of recursive spinlock in vdev code.
> 
> PS: I wonder if we really need to keep the transactional memory versions of stuff.

I don't know, it's part of the API and has no maintenance cost.

Re: [PATCH] spinlock: remove volatile qualifier

[Stephen Hemminger]

On Tue, 05 May 2026 09:01:43 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> > PS: I wonder if we really need to keep the transactional memory versions of stuff.  
> 
> I don't know, it's part of the API and has no maintenance cost.


It is not tested, only worked on x86 and most of the CPU's and tooling
dropped support for transactional memory years ago. It was a failed idea.