From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760098AbbAaC2P (ORCPT ); Fri, 30 Jan 2015 21:28:15 -0500 Received: from smtp2.provo.novell.com ([137.65.250.81]:39909 "EHLO smtp2.provo.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756205AbbAaC2O (ORCPT ); Fri, 30 Jan 2015 21:28:14 -0500 Message-ID: <1422671289.28351.1.camel@stgolabs.net> Subject: Re: [PATCH 4/5] locking/rwsem: Avoid deceiving lock spinners From: Davidlohr Bueso To: Tim Chen Cc: Peter Zijlstra , Ingo Molnar , "Paul E. McKenney" , Jason Low , Michel Lespinasse , linux-kernel@vger.kernel.org Date: Fri, 30 Jan 2015 18:28:09 -0800 In-Reply-To: <1422669098.9530.33.camel@schen9-desk2.jf.intel.com> References: <1422609267-15102-1-git-send-email-dave@stgolabs.net> <1422609267-15102-5-git-send-email-dave@stgolabs.net> <1422669098.9530.33.camel@schen9-desk2.jf.intel.com> 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: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2015-01-30 at 17:51 -0800, Tim Chen wrote: > On Fri, 2015-01-30 at 01:14 -0800, Davidlohr Bueso wrote: > > When readers hold the semaphore, the ->owner is nil. As such, > > and unlike mutexes, '!owner' does not necessarily imply that > > the lock is free. This will cause writers to potentially spin > > excessively as they've been mislead to thinking they have a > > chance of acquiring the lock, instead of blocking. > > > > This patch therefore enhances the counter check when the owner > > is not set by the time we've broken out of the loop. Otherwise > > we can return true as a new owner has the lock and thus we want > > to continue spinning. While at it, we can make rwsem_spin_on_owner() > > less ambiguos and return right away under need_resched conditions. > > > > Signed-off-by: Davidlohr Bueso > > --- > > kernel/locking/rwsem-xadd.c | 21 +++++++++++++++------ > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c > > index 07713e5..1c0d11e 100644 > > --- a/kernel/locking/rwsem-xadd.c > > +++ 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)) { > > - if (need_resched()) > > - break; > > + /* 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 */ > > + > > Do you have some comparison data of whether it is more advantageous > to continue spinning when owner changes? After the above change, > rwsem will behave more like a spin lock for write lock and > will keep spinning when the lock changes ownership. But recall we still abort when need_resched, so the spinning isn't infinite. Never has been. > Now during heavy > lock contention, if we don't continue spinning and sleep, we may use the > clock cycles for actually running other threads. Under heavy contention, time spinning will force us to ultimately block anyway. Thanks, Davidlohr