Re: [PATCH] rwsem: Support optimistic spinning

[Davidlohr Bueso <davidlohr@hp.com>]

On Mon, 2014-04-28 at 09:52 +0200, Peter Zijlstra wrote:
> On Tue, Apr 22, 2014 at 03:19:26PM -0700, Davidlohr Bueso wrote:
> > ---
> >  include/linux/rwsem.h       |   9 +-
> >  kernel/locking/rwsem-xadd.c | 213 +++++++++++++++++++++++++++++++++++++++-----
> >  kernel/locking/rwsem.c      |  31 ++++++-
> >  3 files changed, 231 insertions(+), 22 deletions(-)
> 
> rwsem-spinlock.c doesn't need changes?

I had considered this, but the spinlock variant is, of course,
completely different to the xadd one (ie it doesn't even rely on the
->count). Furthermore, any systems that should benefit from optimistic
spinning, should already be using xadd. The same was true when the
writer lock stealing was done. Note that the rwsem structure in
rwsem-spinlock.h was not changed. I will explicitly mention the spinlock
method in the changelog in v2.

> > diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
> > index cfff143..a911dbf 100644
> > --- a/kernel/locking/rwsem.c
> > +++ b/kernel/locking/rwsem.c
> > @@ -12,6 +12,27 @@

err I also noticed that in:

@@ -58,7 +63,9 @@ static inline int rwsem_is_locked(struct rw_semaphore *sem)
 #define __RWSEM_INITIALIZER(name)                      \
        { RWSEM_UNLOCKED_VALUE,                         \
          __RAW_SPIN_LOCK_UNLOCKED(name.wait_lock),     \
-         LIST_HEAD_INIT((name).wait_list)              \
+         LIST_HEAD_INIT((name).wait_list),             \
+         NULL, /* owner */                             \
+         NULL /* mcs lock */        

This needs to depend on CONFIG_SMP.

> >  #include <linux/atomic.h>
> >  
> > +#ifdef CONFIG_SMP
> > +static inline void rwsem_set_owner(struct rw_semaphore *sem)
> > +{
> > +	sem->owner = current;
> > +}
> > +
> > +static inline void rwsem_clear_owner(struct rw_semaphore *sem)
> > +{
> > +	sem->owner = NULL;
> > +}
> > +
> > +#else
> > +static inline void rwsem_set_owner(struct rw_semaphore *sem)
> > +{
> > +}
> > +
> > +static inline void rwsem_clear_owner(struct rw_semaphore *sem)
> > +{
> > +}
> > +#endif
> > +
> >  /*
> >   * lock for reading
> >   */
> > @@ -48,6 +69,7 @@ void __sched down_write(struct rw_semaphore *sem)
> >  	rwsem_acquire(&sem->dep_map, 0, 0, _RET_IP_);
> >  
> >  	LOCK_CONTENDED(sem, __down_write_trylock, __down_write);
> > +	rwsem_set_owner(sem);
> >  }
> >  
> >  EXPORT_SYMBOL(down_write);
> > @@ -59,8 +81,11 @@ int down_write_trylock(struct rw_semaphore *sem)
> >  {
> >  	int ret = __down_write_trylock(sem);
> >  
> > -	if (ret == 1)
> > +	if (ret == 1) {
> >  		rwsem_acquire(&sem->dep_map, 0, 1, _RET_IP_);
> > +		rwsem_set_owner(sem);
> > +	}
> > +
> >  	return ret;
> >  }
> 
> So first acquire lock, then set owner.
> 
> > @@ -86,6 +111,7 @@ void up_write(struct rw_semaphore *sem)
> >  	rwsem_release(&sem->dep_map, 1, _RET_IP_);
> >  
> >  	__up_write(sem);
> > +	rwsem_clear_owner(sem);
> >  }
> >  
> >  EXPORT_SYMBOL(up_write);
> > @@ -100,6 +126,7 @@ void downgrade_write(struct rw_semaphore *sem)
> >  	 * dependency.
> >  	 */
> >  	__downgrade_write(sem);
> > +	rwsem_clear_owner(sem);
> >  }
> 
> But here you first release and then clear owner; this is buggy. The
> moment you release another acquire can happen and your clear will clear
> the new owner, not us.

Ah yes. The same should go for up_write(). We need to follow this order:

take lock
set owner

clear owner
release lock

Will update in v2.

Thanks,
Davidlohr