Re: [PATCH 4/5] locking/rwsem: Avoid deceiving lock spinners

[Tim Chen <tim.c.chen@linux.intel.com>]

On Tue, 2015-02-03 at 13:04 -0800, Jason Low wrote:
> On Tue, 2015-02-03 at 11:43 -0800, Tim Chen wrote:
> > On Tue, 2015-02-03 at 09:54 -0800, Jason Low wrote:
> > > On Tue, 2015-02-03 at 09:16 -0800, Tim Chen wrote:
> > > > > > >  
> > > > > > > +	if (READ_ONCE(sem->owner))
> > > > > > > +		return true; /* new owner, continue spinning */
> > > > > > > +
> > > > > > 
> > > > > > Do you have some comparison data of whether it is more advantageous
> > > > > > to continue spinning when owner changes?  After the above change, 
> > > > > > rwsem will behave more like a spin lock for write lock and 
> > > > > > will keep spinning when the lock changes ownership.
> > > > > 
> > > > > But recall we still abort when need_resched, so the spinning isn't
> > > > > infinite. Never has been.
> > > > > 
> > > > > >  Now during heavy
> > > > > > lock contention, if we don't continue spinning and sleep, we may use the
> > > > > > clock cycles for actually running other threads. 
> > > > > 
> > > > > Under heavy contention, time spinning will force us to ultimately block
> > > > > anyway.
> > > > 
> > > > The question is under heavy contention, if we are going to block anyway,
> > > > won't it be more advantageous not to continue spinning so we can use
> > > > the cycles for useful task?
> > > 
> > > Hi Tim,
> > > 
> > > Now that we have the OSQ logic, under heavy contention, there will still
> > > only be 1 thread that is spinning on owner at a time. 
> > 
> > That's true.  We cannot have the lock grabbed by a new write 
> > contender as any new writer contender of the lock will be 
> > queued by the OSQ logic. Only the
> > thread doing the optimistic spin is attempting write lock.  
> > In other word, switching of write owner of the rwsem to a new
> > owner cannot happen.
> 
> Another thread can still obtain the write lock in the fast path though
> right? We try to obtain the write lock once before calling
> rwsem_down_write_failed().
> 
> 

True. The change owner check is still needed then.  

Thinking more about this, I now agree that continue spinning is the 
right thing. The possible number of threads contending for write 
locking has been greatly reduced by OSQ logic.  Most of the time 
any new threads doing write locking attempt will do that only once
and then go directly to the OSQ. The probability of success of retrying
write lock by the thread at head of OSQ is high so we should do it.

Davidlohr, you can add my Ack for this patch.

Thanks.

Tim