From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1756232AbbAaVPH (ORCPT <rfc822;w@1wt.eu>);
	Sat, 31 Jan 2015 16:15:07 -0500
Received: from smtp2.provo.novell.com ([137.65.250.81]:47076 "EHLO
	smtp2.provo.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1756035AbbAaVPF (ORCPT
	<rfc822;groupwise-linux-kernel@vger.kernel.org:0:0>);
	Sat, 31 Jan 2015 16:15:05 -0500
Message-ID: <1422738891.28351.4.camel@stgolabs.net>
Subject: Re: [PATCH 4/5] locking/rwsem: Avoid deceiving lock spinners
From: Davidlohr Bueso <dave@stgolabs.net>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        Jason Low <jason.low2@hp.com>, Michel Lespinasse <walken@google.com>,
        Tim Chen <tim.c.chen@linux.intel.com>, linux-kernel@vger.kernel.org
Date: Sat, 31 Jan 2015 13:14:51 -0800
In-Reply-To: <20150131092921.GB32343@twins.programming.kicks-ass.net>
References: <1422609267-15102-1-git-send-email-dave@stgolabs.net>
	 <1422609267-15102-5-git-send-email-dave@stgolabs.net>
	 <20150131092921.GB32343@twins.programming.kicks-ass.net>
Content-Type: text/plain; charset="UTF-8"
X-Mailer: Evolution 3.12.7 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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