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 <[email protected]>
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
>
