linux-kernel - Re: [PATCH] rwsem: Support optimistic spinning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 28 Apr 2014 10:18:02 -0700
From:	Davidlohr Bueso <davidlohr@...com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Tim Chen <tim.c.chen@...ux.intel.com>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Alex Shi <alex.shi@...aro.org>,
	Andi Kleen <andi@...stfloor.org>,
	Michel Lespinasse <walken@...gle.com>,
	Rik van Riel <riel@...hat.com>,
	Peter Hurley <peter@...leysoftware.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Paul E.McKenney" <paulmck@...ux.vnet.ibm.com>,
	Aswin Chandramouleeswaran <aswin@...com>,
	"Norton, Scott J" <scott.norton@...com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rwsem: Support optimistic spinning

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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/