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

[Davidlohr Bueso <dave@stgolabs.net>]

On Sat, 2015-01-31 at 10:29 +0100, Peter Zijlstra wrote:
> On Fri, Jan 30, 2015 at 01:14:26AM -0800, Davidlohr Bueso wrote:
> > +++ b/kernel/locking/rwsem-xadd.c
> > @@ -337,21 +337,30 @@ static inline bool owner_running(struct rw_semaphore *sem,
> >  static noinline
> >  bool rwsem_spin_on_owner(struct rw_semaphore *sem, struct task_struct *owner)
> >  {
> > +	long count;
> > +
> >  	rcu_read_lock();
> >  	while (owner_running(sem, owner)) {
> > +		/* abort spinning when need_resched */
> > +		if (need_resched()) {
> > +			rcu_read_unlock();
> > +			return false;
> > +		}
> >  
> >  		cpu_relax_lowlatency();
> >  	}
> >  	rcu_read_unlock();
> >  
> > +	if (READ_ONCE(sem->owner))
> > +		return true; /* new owner, continue spinning */
> > +
> 
> Same concern as Tim; also the mutex code seems to terminate the spin
> when owner changes. And I think we want to have writers behave similar
> to mutexes, no?
> 
> Does it make sense to change things to allow owner changes from NULL,
> but not to NULL?

I think it does, yes:

- owner changes to nil: readers can hold the lock. We know the rest.

- owner changes from nil: implies that a writer now holds the mutex. Why
should we want to block? We continue to apply the same reasoning why
we're spinning in the first place. This is very beneficial if, for
instance, we began polling on the owner when the lock is just about to
be released. So a few iterations later, the lock owner changes on us and
with the current code will make us sleep. With this change, after a few
spins it is very likely we'll get the lock. And if not, the need_resched
will ultimately kick in and block. Additionally, as Jason pointed out,
with osq we have no need to worry about simultaneously spinning on the
owner at the same time.

Or am I missing something?

Thanks,
Davidlohr