Re: [RFC 6/6] RCU: Track rcu_dereference() in RCU read-side critical section

From: Boqun Feng
Date: Tue Feb 16 2016 - 00:00:25 EST

Next message: Sanchayan Maity: "[PATCH v3 0/3] Allow iio_hwmon to accept hypen in node names"
Previous message: Kuninori Morimoto: "[PATCH] regmap: merge regmap_fields_update_bits() into macro"
Next in thread: Josh Triplett: "Re: [RFC 6/6] RCU: Track rcu_dereference() in RCU read-side critical section"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Feb 15, 2016 at 08:08:40PM -0800, Paul E. McKenney wrote:
> On Tue, Feb 16, 2016 at 11:01:43AM +0800, Boqun Feng wrote:
> > On Mon, Feb 15, 2016 at 05:05:53PM -0800, Paul E. McKenney wrote:
> > > On Thu, Feb 04, 2016 at 12:45:12AM +0800, Boqun Feng wrote:
> > > > The variables protected by an RCU read-side critical section are
> > > > sometimes hard to figure out, especially when the critical section is
> > > > long or has some function calls in it. However, figuring out which
> > > > variable a RCU read-side critical section protects could save
> > > > us a lot of time for code reviewing, bug fixing or performance tuning.
> > > >
> > > > This patch therefore uses the LOCKED_ACCESS to collect the information
> > > > of relationship between rcu_dereference*() and rcu_read_lock*() by
> > > > doing:
> > > >
> > > > Step 0: define a locked_access_class for RCU.
> > > >
> > > > Step 1: set the content of rcu_*_lock_key and __srcu_key to the
> > > > address of the locked_access_class for RCU.
> > > >
> > > > Step 2: add locked_access_point() in __rcu_dereference_check()
> > > >
> > > > After that we can figure out not only in which RCU read-side critical
> > > > section but also after which rcu_read_lock*() called an
> > > > rcu_dereference*() is called.
> > > >
> > > > This feature is controlled by a config option RCU_LOCKED_ACCESS.
> > > >
> > > > Also clean up the initialization code of lockdep_maps for different
> > > > flavors of RCU a little bit.
> > > >
> > > > Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> > > > ---
> > > > include/linux/rcupdate.h | 8 ++++++++
> > > > include/linux/srcu.h | 8 +++++++-
> > > > kernel/locking/lockdep.c | 3 +++
> > > > kernel/rcu/update.c | 31 +++++++++++++++++--------------
> > > > lib/Kconfig.debug | 12 ++++++++++++
> > > > 5 files changed, 47 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > > index 14e6f47..4bab658 100644
> > > > --- a/include/linux/rcupdate.h
> > > > +++ b/include/linux/rcupdate.h
> > > > @@ -610,6 +610,13 @@ static inline void rcu_preempt_sleep_check(void)
> > > > #define rcu_dereference_sparse(p, space)
> > > > #endif /* #else #ifdef __CHECKER__ */
> > > >
> > > > +#ifdef CONFIG_RCU_LOCKED_ACCESS
> > > > +extern struct locked_access_class rcu_laclass;
> > > > +#define rcu_dereference_access() \
> > > > + locked_access_point(&rcu_laclass, LOCKED_ACCESS_TYPE_READ)
> > > > +#else /* #ifdef CONFIG_LOCKED_ACCESS */
> > > > +#define rcu_dereference_access()
> > > > +#endif /* #else #ifdef CONFIG_LOCKED_ACCESS */
> > > > #define __rcu_access_pointer(p, space) \
> > > > ({ \
> > > > typeof(*p) *_________p1 = (typeof(*p) *__force)READ_ONCE(p); \
> > > > @@ -622,6 +629,7 @@ static inline void rcu_preempt_sleep_check(void)
> > > > typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
> > > > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
> > > > rcu_dereference_sparse(p, space); \
> > > > + rcu_dereference_access(); \
> > > > ((typeof(*p) __force __kernel *)(________p1)); \
> > > > })
> > > > #define __rcu_dereference_protected(p, c, space) \
> > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > > > index f5f80c5..ff048a2 100644
> > > > --- a/include/linux/srcu.h
> > > > +++ b/include/linux/srcu.h
> > > > @@ -64,12 +64,18 @@ struct srcu_struct {
> > > >
> > > > #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > >
> > > > +# ifdef CONFIG_RCU_LOCKED_ACCESS
> > > > +# define RCU_KEY { .laclass = &rcu_laclass }
> > > > +# else
> > > > +# define RCU_KEY { 0 }
> > > > +# endif
> > > > +
> > > > int __init_srcu_struct(struct srcu_struct *sp, const char *name,
> > > > struct lock_class_key *key);
> > > >
> > > > #define init_srcu_struct(sp) \
> > > > ({ \
> > > > - static struct lock_class_key __srcu_key; \
> > > > + static struct lock_class_key __srcu_key = RCU_KEY; \
> > >
> > > This gets me the following for the TASKS01, TINY02, TREE02, TREE05,
> > > TREE06, and TREE08 configurations:
> > >
> > > CC mm/mmap.o
> > > In file included from /home/paulmck/public_git/linux-rcu/include/linux/notifier.h:15:0,
> > > from /home/paulmck/public_git/linux-rcu/arch/x86/include/asm/kdebug.h:4,
> > > from /home/paulmck/public_git/linux-rcu/include/linux/kdebug.h:4,
> > > from /home/paulmck/public_git/linux-rcu/kernel/notifier.c:1:
> > > /home/paulmck/public_git/linux-rcu/kernel/notifier.c: In function âsrcu_init_notifier_headâ:
> > > /home/paulmck/public_git/linux-rcu/include/linux/srcu.h:78:16: warning: missing braces around initializer [-Wmissing-braces]
> > > static struct lock_class_key __srcu_key = RCU_KEY; \
> > > ^
> > > /home/paulmck/public_git/linux-rcu/kernel/notifier.c:526:6: note: in expansion of macro âinit_srcu_structâ
> > > if (init_srcu_struct(&nh->srcu) < 0)
> > > ^
> > > /home/paulmck/public_git/linux-rcu/include/linux/srcu.h:78:16: warning: (near initialization for â__srcu_key.subkeysâ) [-Wmissing-braces]
> > > static struct lock_class_key __srcu_key = RCU_KEY; \
> > > ^
> > > /home/paulmck/public_git/linux-rcu/kernel/notifier.c:526:6: note: in expansion of macro âinit_srcu_structâ
> > > if (init_srcu_struct(&nh->srcu) < 0)
> > > ^
> > >
> > > These have the following in their .config files:
> > >
> > > CONFIG_DEBUG_LOCK_ALLOC=y
> > >
> > > However, none of your new Kconfig options are set, which needs to be
> > > tested because this will be the common case. Several of them have
> > > CONFIG_PROVE_LOCKING=y, but others do not.
> > >
> >
> > Oh.. this is embarrassing ;-(
> >
> > Hmm.. when CONFIG_RCU_LOCKED_ACCESS=n and CONFIG_DEBUG_LOCK_ALLOC=y,
> > this line becomes:
> >
> > static struct lock_class_key __srcu_key = { 0 };
> >
> > IIUC, "={ 0 }" is the unverisal zero initializer in C, could be used for
> > zero initialising any structure or array, so it's OK here, right?
> >
> > And may I ask your compiler's version? Because looks to me this may
> > be a compiler bug according to:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64709
>
> gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04) (KSIC)
>
> > Also I can't reproduce this with CONFIG_RCU_LOCKED_ACCESS=n and
> > CONFIG_DEBUG_LOCK_ALLOC=y on my machine, whose gcc version is 5.3.0.
> >
> > Of course I can use a different way here and do not need a "={ 0 }"
> > here, because __srcu_key is static, and I just want to make sure we know
> > what's going on here before we use a workaround, or I just want to know
> > if I'm a bad C programmer ;-)
>
> Well, it is quite possible that your code is correct in theory, but
> we do need to make it so that compilers can deal with it. It would
> not be the first compiler-bug workaround in RCU...
>

Fair enough!

> Thanx, Paul
>
> > Regards,
> > Boqun
> >
> > > I have dequeued these for the moment. Please send an updated patch
> > > series when you have this fixed.

Moreover, I hit some "defined but not used" warnings with
CONFIG_DEBUG_LOCK_ALLOC=y and CONFIG_RCU_LOCKED_ACCESS=n, will fix that
too and send out a new series.

Regards,
Boqun

> > >
> > > Thanx, Paul
> > >

Attachment: signature.asc
Description: PGP signature

Next message: Sanchayan Maity: "[PATCH v3 0/3] Allow iio_hwmon to accept hypen in node names"
Previous message: Kuninori Morimoto: "[PATCH] regmap: merge regmap_fields_update_bits() into macro"
Next in thread: Josh Triplett: "Re: [RFC 6/6] RCU: Track rcu_dereference() in RCU read-side critical section"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]