|SUCCESS| [v3,3/3] spinlock: fix API comments

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

git sha: 20260519103640.3986710-1-thomas at monjalon.net
Author: Thomas Monjalon <thomas at monjalon.net>
Subject: [PATCH v3 1/3] spinlock: remove volatile qualifier

This patch removes the volatile qualifier from the rte_spinlock_recursive_t
structure fields and converts the user field to an atomic variable, renaming
it to owner. The count field no longer uses volatile as it is only accessed
by the lock-holding thread.

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

The field rename from user to owner changes the ABI. While the commit message
mentions this will break direct field access, does this structure need an ABI
versioning approach or compatibility shim? The structure is public API and
applications may be accessing the field despite it being internal state.

[ ... ]

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

Does rte_memory_order_relaxed provide sufficient ordering guarantees here?
In rte_spinlock_recursive_lock(), after acquiring sl with rte_spinlock_lock()
(which provides acquire semantics), storing owner with relaxed ordering seems
safe. However, should the relationship between sl acquisition and owner update
be documented to clarify the memory ordering rationale?

> @@ -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->owner, -1, rte_memory_order_relaxed);
>  		rte_spinlock_unlock(&slr->sl);
>  	}
> -
>  }

In rte_spinlock_recursive_unlock(), does the relaxed store of owner=-1
before the release of sl create a visibility issue? The unlock of sl provides
release semantics, but should owner be set with at least release ordering
to ensure other threads see the -1 value after the lock is released?

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

Similar to the lock function, does the relaxed ordering on owner load in
rte_spinlock_recursive_trylock() need to be acquire ordered? If another
thread just released the lock, relaxed loading might not see the updated
owner=-1 value immediately due to weak ordering guarantees.

---

git sha: 20260519103640.3986710-2-thomas at monjalon.net
Author: Thomas Monjalon <thomas at monjalon.net>
Subject: [PATCH v3 2/3] spinlock: add debug checks in recursive unlock

This patch adds RTE_ASSERT checks in rte_spinlock_recursive_unlock() to
verify the caller owns the lock and that the count is positive.

> diff --git a/lib/eal/include/generic/rte_spinlock.h b/lib/eal/include/generic/rte_spinlock.h
> index ffdcb8fa3d..e7cd18a8e2 100644
> --- a/lib/eal/include/generic/rte_spinlock.h
> +++ b/lib/eal/include/generic/rte_spinlock.h
> @@ -21,6 +21,7 @@
>  #ifdef RTE_FORCE_INTRINSICS
>  #include <rte_common.h>
>  #endif
> +#include <rte_debug.h>
>  #include <rte_lock_annotations.h>
>  #include <rte_pause.h>
>  #include <rte_stdatomic.h>
> @@ -245,6 +246,8 @@ 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
>  {
> +	RTE_ASSERT(rte_atomic_load_explicit(&slr->owner, rte_memory_order_relaxed) == rte_gettid());
> +	RTE_ASSERT(slr->count > 0);

Does the first assert line exceed 79 characters? The coding style guide
requires lines to wrap at 79 characters unless breaking would make the code
less readable. This assertion is 108 characters long.

---

git sha: 20260519103640.3986710-3-thomas at monjalon.net
Author: Thomas Monjalon <thomas at monjalon.net>
Subject: [PATCH v3 3/3] spinlock: fix API comments

This patch corrects documentation comments in the spinlock header, clarifying
that it's not a read-write lock, the owner field stores thread ID not core ID,
and that architecture-specific implementations are conditional.

The changes are documentation-only and appear correct.