Re: [PATCH] rcu: Remove superfluous versions of rcu_read_lock_sched_held()

From: Boqun Feng
Date: Wed Mar 23 2016 - 21:03:39 EST


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@xxxxxxxxx>
>
> 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: signature.asc
Description: PGP signature