From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752663AbbDIHxR (ORCPT ); Thu, 9 Apr 2015 03:53:17 -0400 Received: from mail-wi0-f171.google.com ([209.85.212.171]:38900 "EHLO mail-wi0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751810AbbDIHxQ (ORCPT ); Thu, 9 Apr 2015 03:53:16 -0400 Date: Thu, 9 Apr 2015 09:53:11 +0200 From: Ingo Molnar To: Jason Low Cc: Peter Zijlstra , Linus Torvalds , Davidlohr Bueso , Tim Chen , Aswin Chandramouleeswaran , LKML Subject: Re: [PATCH 2/2] locking/rwsem: Use a return variable in rwsem_spin_on_owner() Message-ID: <20150409075311.GA4645@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> <1428561611.3506.78.camel@j-VirtualBox> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1428561611.3506.78.camel@j-VirtualBox> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Jason Low wrote: > 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. That's the usual pattern we use, but: > - /* 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; > } The point is to generally unify the 'out' paths - i.e. to merge it with the rcu_read_unlock() as well, so that we have really simple gotos and only a single exit path. That's not really doable here without extra overhead AFAICS, so I'd suggest we leave it alone ... I have applied your other patch. Thanks, Ingo