* [PATCH] rcu: Remove superfluous versions of rcu_read_lock_sched_held() @ 2016-03-23 15:11 Boqun Feng 2016-03-23 16:38 ` Paul E. McKenney 0 siblings, 1 reply; 3+ messages in thread From: Boqun Feng @ 2016-03-23 15:11 UTC (permalink / raw) To: linux-kernel Cc: Paul E. McKenney, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Frederic Weisbecker, Boqun Feng 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 <boqun.feng@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] rcu: Remove superfluous versions of rcu_read_lock_sched_held() 2016-03-23 15:11 [PATCH] rcu: Remove superfluous versions of rcu_read_lock_sched_held() Boqun Feng @ 2016-03-23 16:38 ` Paul E. McKenney 2016-03-24 1:02 ` Boqun Feng 0 siblings, 1 reply; 3+ messages in thread From: Paul E. McKenney @ 2016-03-23 16:38 UTC (permalink / raw) To: Boqun Feng Cc: linux-kernel, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Frederic Weisbecker 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 <boqun.feng@gmail.com> 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] rcu: Remove superfluous versions of rcu_read_lock_sched_held() 2016-03-23 16:38 ` Paul E. McKenney @ 2016-03-24 1:02 ` Boqun Feng 0 siblings, 0 replies; 3+ messages in thread From: Boqun Feng @ 2016-03-24 1:02 UTC (permalink / raw) To: Paul E. McKenney Cc: linux-kernel, Josh Triplett, Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Frederic Weisbecker [-- Attachment #1: Type: text/plain, Size: 4469 bytes --] On Wed, Mar 23, 2016 at 09:38:46AM -0700, Paul E. McKenney wrote: > 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 <boqun.feng@gmail.com> > > 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? > Yes, thank you ;-) Regards, Boqun > 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 > > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-24 1:03 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-23 15:11 [PATCH] rcu: Remove superfluous versions of rcu_read_lock_sched_held() Boqun Feng 2016-03-23 16:38 ` Paul E. McKenney 2016-03-24 1:02 ` Boqun Feng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox