Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751926AbbDIGkQ (ORCPT ); Thu, 9 Apr 2015 02:40:16 -0400 Received: from g2t2352.austin.hp.com ([15.217.128.51]:59980 "EHLO g2t2352.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751818AbbDIGkO (ORCPT ); Thu, 9 Apr 2015 02:40:14 -0400 Message-ID: <1428561611.3506.78.camel@j-VirtualBox> Subject: Re: [PATCH 2/2] locking/rwsem: Use a return variable in rwsem_spin_on_owner() From: Jason Low To: Ingo Molnar Cc: Peter Zijlstra , Linus Torvalds , Davidlohr Bueso , Tim Chen , Aswin Chandramouleeswaran , LKML , jason.low2@hp.com Date: Wed, 08 Apr 2015 23:40:11 -0700 In-Reply-To: <20150409053725.GB13871@gmail.com> References: <1428521960-5268-1-git-send-email-jason.low2@hp.com> <1428521960-5268-3-git-send-email-jason.low2@hp.com> <20150409053725.GB13871@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3205 Lines: 102 On Thu, 2015-04-09 at 07:37 +0200, Ingo Molnar wrote: > The 'break' path does not seem to be equivalent, we used to do: > > > - rcu_read_unlock(); > > - return false; > > and now we'll do: > > > + ret = false; > ... > > + if (!READ_ONCE(sem->owner)) { > > + long count = READ_ONCE(sem->count); > > it's harmless (we do one more round of checking), but that's not an > equivalent transformation and slows down the preemption trigger a > (tiny) bit, because the chance that we actually catch the lock when > breaking out early is vanishingly small. (It might in fact do the > wrong thing in returning true if need_resched() is set and we've > switched owners in that small window.) > > Given how dissimilar the return path is in this case, I'm not sure > it's worth sharing it. This might be one of the few cases where > separate return statements is the better solution. I also preferred the multiple returns for the rwsem variant to avoid needing to check sem->owner when it should go to slowpath, as you mentioned. Now that I think of it though, if we want to have just one return path, we can still do that if we add an "out" label. --- kernel/locking/rwsem-xadd.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-) diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c index 3417d01..e74240f 100644 --- a/kernel/locking/rwsem-xadd.c +++ b/kernel/locking/rwsem-xadd.c @@ -327,38 +327,39 @@ done: static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem, struct task_struct *owner) { - long count; + bool ret = true; rcu_read_lock(); while (sem->owner == owner) { /* * Ensure we emit the owner->on_cpu, dereference _after_ - * checking sem->owner still matches owner, if that fails, - * owner might point to free()d memory, if it still matches, + * checking sem->owner still matches owner. If that fails, + * owner might point to freed memory. If it still matches, * the rcu_read_lock() ensures the memory stays valid. */ barrier(); - /* abort spinning when need_resched or owner is not running */ + /* Abort spinning when need_resched or owner is not running. */ if (!owner->on_cpu || need_resched()) { rcu_read_unlock(); - return false; + ret = false; + goto out; } cpu_relax_lowlatency(); } rcu_read_unlock(); - if (READ_ONCE(sem->owner)) - return true; /* new owner, continue spinning */ - /* * When the owner is not set, the lock could be free or - * held by readers. Check the counter to verify the - * state. + * held by readers. Check the counter to verify the state. */ - count = READ_ONCE(sem->count); - return (count == 0 || count == RWSEM_WAITING_BIAS); + if (!READ_ONCE(sem->owner)) { + long count = READ_ONCE(sem->count); + ret = (count == 0 || count == RWSEM_WAITING_BIAS); + } +out: + return ret; } static bool rwsem_optimistic_spin(struct rw_semaphore *sem) -- 1.7.2.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/