From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932246AbcCWQip (ORCPT ); Wed, 23 Mar 2016 12:38:45 -0400 Received: from e17.ny.us.ibm.com ([129.33.205.207]:48688 "EHLO e17.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755886AbcCWQin (ORCPT ); Wed, 23 Mar 2016 12:38:43 -0400 X-IBM-Helo: d01dlp02.pok.ibm.com X-IBM-MailFrom: paulmck@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Date: Wed, 23 Mar 2016 09:38:46 -0700 From: "Paul E. McKenney" To: Boqun Feng Cc: linux-kernel@vger.kernel.org, Josh Triplett , Steven Rostedt , Mathieu Desnoyers , Lai Jiangshan , Frederic Weisbecker Subject: Re: [PATCH] rcu: Remove superfluous versions of rcu_read_lock_sched_held() Message-ID: <20160323163846.GD4287@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <1458745908-30431-1-git-send-email-boqun.feng@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1458745908-30431-1-git-send-email-boqun.feng@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16032316-0041-0000-0000-000003AD8AC1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Mar 23, 2016 at 11:11:48PM +0800, Boqun Feng wrote: > Currently, we have four versions of rcu_read_lock_sched_held(), > depending on the combined choices on PREEMPT_COUNT and DEBUG_LOCK_ALLOC. > But we actually don't need to specialize those for PREEMPT_COUNT=n > kernel. Because: > > 1. For the implementations in DEBUG_LOCK_ALLOC=n kernel, we can use > preemptible() to implement one rcu_read_lock_sched_held(), which > gives us the same behavior as the current two. > > 2. For the implementations in DEBUG_LOCK_ALLOC=y kernel, even when > PREEMPT_COUNT=n, one CPU may block no grace period because of > the same reason for the PREEMPT_COUNT=y and !PREEMPT kernel. > (e.g. dynticks or cpu hotplug) > > So unify the implementations of rcu_read_lock_sched_held() by using > macro preemptible() and tightening up the lock-held checking for > PREEMPT_COUNT=n kernel. And this will improve the readability, make the > debug checking as expected and save several lines of code. > > Signed-off-by: Boqun Feng Looks like a nice consolidation! I have queued this for review and testing, updating the commit log as follows: Currently, we have four versions of rcu_read_lock_sched_held(), depending on the combined choices on PREEMPT_COUNT and DEBUG_LOCK_ALLOC. However, there is an existing function preemptible() that already distinguishes between the PREEMPT_COUNT=y and PREEMPT_COUNT=n cases, and allows these four implementations to be consolidated down to two. This commit therefore uses preemptible() to achieve this consolidation. Note that there could be a small performance regression in the case of CONFIG_DEBUG_LOCK_ALLOC=y && PREEMPT_COUNT=n. However, given the overhead associated with CONFIG_DEBUG_LOCK_ALLOC=y, this should be down in the noise. Does that capture it? Thanx, Paul > --- > include/linux/rcupdate.h | 17 +---------------- > kernel/rcu/update.c | 4 ++-- > 2 files changed, 3 insertions(+), 18 deletions(-) > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index b5d48bd..e3f845b 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -510,14 +510,7 @@ int rcu_read_lock_bh_held(void); > * CONFIG_DEBUG_LOCK_ALLOC, this assumes we are in an RCU-sched read-side > * critical section unless it can prove otherwise. > */ > -#ifdef CONFIG_PREEMPT_COUNT > int rcu_read_lock_sched_held(void); > -#else /* #ifdef CONFIG_PREEMPT_COUNT */ > -static inline int rcu_read_lock_sched_held(void) > -{ > - return 1; > -} > -#endif /* #else #ifdef CONFIG_PREEMPT_COUNT */ > > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > @@ -534,18 +527,10 @@ static inline int rcu_read_lock_bh_held(void) > return 1; > } > > -#ifdef CONFIG_PREEMPT_COUNT > static inline int rcu_read_lock_sched_held(void) > { > - return preempt_count() != 0 || irqs_disabled(); > + return !preemptible(); > } > -#else /* #ifdef CONFIG_PREEMPT_COUNT */ > -static inline int rcu_read_lock_sched_held(void) > -{ > - return 1; > -} > -#endif /* #else #ifdef CONFIG_PREEMPT_COUNT */ > - > #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > #ifdef CONFIG_PROVE_RCU > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c > index ca828b4..3ccdc8e 100644 > --- a/kernel/rcu/update.c > +++ b/kernel/rcu/update.c > @@ -67,7 +67,7 @@ static int rcu_normal_after_boot; > module_param(rcu_normal_after_boot, int, 0); > #endif /* #ifndef CONFIG_TINY_RCU */ > > -#if defined(CONFIG_DEBUG_LOCK_ALLOC) && defined(CONFIG_PREEMPT_COUNT) > +#ifdef CONFIG_DEBUG_LOCK_ALLOC > /** > * rcu_read_lock_sched_held() - might we be in RCU-sched read-side critical section? > * > @@ -111,7 +111,7 @@ int rcu_read_lock_sched_held(void) > return 0; > if (debug_locks) > lockdep_opinion = lock_is_held(&rcu_sched_lock_map); > - return lockdep_opinion || preempt_count() != 0 || irqs_disabled(); > + return lockdep_opinion || !preemptible(); > } > EXPORT_SYMBOL(rcu_read_lock_sched_held); > #endif > -- > 2.7.3 >