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

[Peter Zijlstra]

On Mon, Sep 14, 2026 at 02:08:32PM +0100, David Laight wrote:
> 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.)

You cannot change ordering and not mention in the changelog. If you're
unsure, retain existing ordering and make note in changelog.

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

Someone will put it back eventually. It is typically good form to have
this pre check. It improved the contended behaviour of these loops. The
regular load can be a forward, while CAS requires an exclusive load --
or something along those lines.