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