* [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