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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/