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

[David Laight]

On Tue, 15 Sep 2026 10:40:47 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

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

The existing ordering seems to have been broken.
I'll rework it for the next version.
(It rather depends on what happens to the patch to change the smp_cond_load.)


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

I know. I think it really depends on the likelyhood of the compare failing.
It there is a reasonable chance of it failing you definitely need the compare.

David