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

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]

Message-ID: <1422738891.28351.4.camel@stgolabs.net>
Date:	Sat, 31 Jan 2015 13:14:51 -0800
From:	Davidlohr Bueso <dave@...olabs.net>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Ingo Molnar <mingo@...nel.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Jason Low <jason.low2@...com>,
	Michel Lespinasse <walken@...gle.com>,
	Tim Chen <tim.c.chen@...ux.intel.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] locking/rwsem: Avoid deceiving lock spinners

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

--
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/