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

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

[Yu Peng]

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;
 
 	/* unqueue */
-- 
2.43.0

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

[Will Deacon]

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'd be much more comfortable if this was smp_cond_load_acquire(), in
addition to the READ_ONCE() that you are proposing.

Will

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

[David Laight]

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'd be much more comfortable if this was smp_cond_load_acquire(), in
> addition to the READ_ONCE() that you are proposing.

I've got a patch 'pending' for this file that tidied some things up.
In particular it saves the cpu numbers not the per-cpu addresses.
So the above check doesn't bounce a cache line.
Perhaps it is time to resend it.

Note that the vpu_is_preempted() path is horribly expensive and
can't actually work reliably.
And can't work at all on arm where 'mwait' (equivalent) is used.

-- David

> 
> Will
> 

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

[Waiman Long]

On 5/18/26 10:29 AM, David Laight wrote:
> 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'd be much more comfortable if this was smp_cond_load_acquire(), in
>> addition to the READ_ONCE() that you are proposing.
> I've got a patch 'pending' for this file that tidied some things up.
> In particular it saves the cpu numbers not the per-cpu addresses.
> So the above check doesn't bounce a cache line.
> Perhaps it is time to resend it.
>
> Note that the vpu_is_preempted() path is horribly expensive and
> can't actually work reliably.
> And can't work at all on arm where 'mwait' (equivalent) is used.

I believe the vcpu_is_preempted() check is only enabled in x86, 
loongarch, powerpc and s390. It is disabled (always returns false) in 
other arches. Yes, the vcpu_is_preempted() check, if implemented, can be 
expensive. Perhaps we can reduce the frequency that it is being called.

Cheers,
Longman

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

[David Laight]

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
>