Re: [PATCH v4 next 3/9] locking/osq_lock: Set prev_cpu=0 instead of locked=1

[David Laight]

On Mon, 14 Sep 2026 14:03:52 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Mon, Sep 07, 2026 at 09:41:27AM +0100, David Laight wrote:
> 
> > -	for (;;) {
> > -		/*
> > -		 * cpu_relax() below implies a compiler barrier which would
> > -		 * prevent this comparison being optimized away.
> > -		 */
> > +	for (;; prev = READ_ONCE(node->prev)) {
> > +		if (!prev)
> > +			/* Lock acquired */
> > +			return true;  
> 
> This lacks {}, but also, is not ACQUIRE like the return you remove
> below. Did you want smp_acquire__after_ctrl_dep() in there?

I need to get my head around the acquire/release logic.
I think the READ_ONCE() in the for(;;) needs to be an acquire
(matching the smp_cond_load - which seems to have been wrong for ages).
I may have decided that because the smp_cond_load was relaxed this
read could be as well.
(They both need to be the same.)

> 
> > +
> > +		prev_ptr = decode_cpu(prev);
> > +
> >  		if (data_race(prev_ptr->next) == node &&

I'm also going to remove that 'optimisation' check.
It is there to avoid the atomic below - but it only fails under
race conditions.

David

> >  		    cmpxchg(&prev_ptr->next, node, NULL) == node)
> >  			break;
> >  
> >  		/*
> > -		 * We can only fail the cmpxchg() racing against an unlock(),
> > -		 * in which case we should observe @node->locked becoming
> > -		 * true.
> > +		 * 'prev' must have unlinked (or be in the process of unlinking)
> > +		 * itself from the list.
> >  		 */
> > -		if (smp_load_acquire(&node->locked))
> > -			return true;  
> 
> This is an ACQUIRE.
> 
> >  
> >  		cpu_relax();
> > -
> > -		/*
> > -		 * Or we race against a concurrent unqueue()'s step-B, in which
> > -		 * case its step-C will write us a new @node->prev pointer.
> > -		 */
> > -		prev = READ_ONCE(node->prev);
> > -		prev_ptr = decode_cpu(prev);
> >  	}