|SUCCESS| spinlock: remove volatile qualifier

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

I've reviewed the patch and would like to raise some concerns about the
correctness of removing the volatile qualifier.

commit: 20260504083714.2904729-1-thomas at monjalon.net
Author: Thomas Monjalon <thomas at monjalon.net>
Subject: [PATCH] spinlock: remove volatile qualifier

This patch removes the volatile qualifier from the user and count fields
of rte_spinlock_recursive_t to address C++20 deprecation warnings with
GCC 16.

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

The commit message states that user and count are "only accessed by the
thread holding the lock" but can this be verified across all use cases?

In rte_spinlock_recursive_lock(), the user field is read first before the
lock is acquired to check if the calling thread already owns the lock.
Does this read-check-then-acquire sequence remain safe without volatile
when another thread might be updating user while releasing the lock?

Similarly, when a thread checks (slr->user != id) before attempting to
acquire the lock, can the compiler now legally cache the value of user
across the lock acquisition, potentially missing updates from other cores?

The justification mentions that the lock provides necessary memory
ordering, but the issue is that user is read BEFORE acquiring the lock to
determine if we need to acquire it. Can removing volatile allow the
compiler to optimize away redundant reads in a way that breaks the
recursive lock check?

Would using __atomic_load() and __atomic_store() with appropriate memory
ordering be a more correct solution that both satisfies C++20 requirements
and maintains the necessary memory semantics?