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

[Boqun Feng]

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