Re: [PATCH] locking/osq_lock: Use READ_ONCE() for node->prev

[David Laight <david.laight.linux@gmail.com>]

On Mon, 18 May 2026 11:29:53 +0100
Will Deacon <will@kernel.org> wrote:

> On Mon, Mar 30, 2026 at 09:32:55AM +0800, Yu Peng wrote:
> > osq_lock() consults node->prev in the vcpu_is_preempted() heuristic while
> > a concurrent predecessor may update it via WRITE_ONCE(next->prev, prev)
> > during unqueue.
> > 
> > This read only affects the decision to abort optimistic spinning; stale
> > values do not affect queue linkage or lock correctness. Use READ_ONCE()
> > to mark the shared read and match the concurrent WRITE_ONCE() update.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Yu Peng <pengyu@kylinos.cn>
> > ---
> >  kernel/locking/osq_lock.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> > index b4233dc2c2b04..db4545e7bb72c 100644
> > --- a/kernel/locking/osq_lock.c
> > +++ b/kernel/locking/osq_lock.c
> > @@ -144,7 +144,7 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> >  	 * polling, be careful.
> >  	 */
> >  	if (smp_cond_load_relaxed(&node->locked, VAL || need_resched() ||
> > -				  vcpu_is_preempted(node_cpu(node->prev))))
> > +				  vcpu_is_preempted(node_cpu(READ_ONCE(node->prev)))))
> >  		return true;  
> 
> Hmm, I wonder whether this is actually sufficient...
> 
> Architectures with relaxed memory models won't order plain reads to
> different addresses, so the read of 'node->locked' is unordered wrt the
> read of 'node->prev' in this condition. Given that we're using
> smp_cond_load_relaxed(), can we end up using a value of 'node->prev'
> that was loaded in a previous iteration of the loop?


I've just found my unposted patches (later than the v3 ones posted mid march).
This all got sorted.
Basically node->prev can be replaced by node->prev_cpu and then node->locked
is equivalent to node->prev_cpu == 0.
It ends up with vcpu_is_preempted(VAL - 1).
The final struct optimistic_spin_node just has two 'int' members for the next
and prev cpu numbers.
I think I got bogged down trying to fix the comments.

Although the vcpu_is_preempted() return value is always stale.
So it just can't matter if the code does 'return false' at any time.
Otherwise it would all be terribly broken anyway.
(I've never looked at the callers of this code.)

	David

> 
> I'd be much more comfortable if this was smp_cond_load_acquire(), in
> addition to the READ_ONCE() that you are proposing.
> 
> Will
>