From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753185AbcBPDCX (ORCPT ); Mon, 15 Feb 2016 22:02:23 -0500 Received: from mail-pf0-f169.google.com ([209.85.192.169]:32971 "EHLO mail-pf0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752572AbcBPDCV (ORCPT ); Mon, 15 Feb 2016 22:02:21 -0500 Date: Tue, 16 Feb 2016 11:01:43 +0800 From: Boqun Feng To: "Paul E. McKenney" Cc: linux-kernel@vger.kernel.org, Peter Zijlstra , Ingo Molnar , Josh Triplett , Steven Rostedt , Mathieu Desnoyers , Lai Jiangshan , sasha.levin@oracle.com Subject: Re: [RFC 6/6] RCU: Track rcu_dereference() in RCU read-side critical section Message-ID: <20160216030143.GA27347@fixme-laptop> References: <1454517912-10457-1-git-send-email-boqun.feng@gmail.com> <1454517912-10457-7-git-send-email-boqun.feng@gmail.com> <20160216010553.GA18422@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="2fHTh5uZTiUOsy+g" Content-Disposition: inline In-Reply-To: <20160216010553.GA18422@linux.vnet.ibm.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --2fHTh5uZTiUOsy+g Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. > >=20 > > This patch therefore uses the LOCKED_ACCESS to collect the information > > of relationship between rcu_dereference*() and rcu_read_lock*() by > > doing: > >=20 > > Step 0: define a locked_access_class for RCU. > >=20 > > Step 1: set the content of rcu_*_lock_key and __srcu_key to the > > address of the locked_access_class for RCU. > >=20 > > Step 2: add locked_access_point() in __rcu_dereference_check() > >=20 > > 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. > >=20 > > This feature is controlled by a config option RCU_LOCKED_ACCESS. > >=20 > > Also clean up the initialization code of lockdep_maps for different > > flavors of RCU a little bit. > >=20 > > Signed-off-by: Boqun Feng > > --- > > 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(-) > >=20 > > 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__ */ > >=20 > > +#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 =3D (typeof(*p) *__force)READ_ONCE(p); \ > > @@ -622,6 +629,7 @@ static inline void rcu_preempt_sleep_check(void) > > typeof(*p) *________p1 =3D (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 { > >=20 > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > >=20 > > +# ifdef CONFIG_RCU_LOCKED_ACCESS > > +# define RCU_KEY { .laclass =3D &rcu_laclass } > > +# else > > +# define RCU_KEY { 0 } > > +# endif > > + > > int __init_srcu_struct(struct srcu_struct *sp, const char *name, > > struct lock_class_key *key); > >=20 > > #define init_srcu_struct(sp) \ > > ({ \ > > - static struct lock_class_key __srcu_key; \ > > + static struct lock_class_key __srcu_key =3D RCU_KEY; \ >=20 > This gets me the following for the TASKS01, TINY02, TREE02, TREE05, > TREE06, and TREE08 configurations: >=20 > CC mm/mmap.o > In file included from /home/paulmck/public_git/linux-rcu/include/linux/no= tifier.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/kd= ebug.h:4, > from /home/paulmck/public_git/linux-rcu/kernel/notifier.= c:1: > /home/paulmck/public_git/linux-rcu/kernel/notifier.c: In function =E2=80= =98srcu_init_notifier_head=E2=80=99: > /home/paulmck/public_git/linux-rcu/include/linux/srcu.h:78:16: warning: m= issing braces around initializer [-Wmissing-braces] > static struct lock_class_key __srcu_key =3D RCU_KEY; \ > ^ > /home/paulmck/public_git/linux-rcu/kernel/notifier.c:526:6: note: in expa= nsion of macro =E2=80=98init_srcu_struct=E2=80=99 > if (init_srcu_struct(&nh->srcu) < 0) > ^ > /home/paulmck/public_git/linux-rcu/include/linux/srcu.h:78:16: warning: (= near initialization for =E2=80=98__srcu_key.subkeys=E2=80=99) [-Wmissing-br= aces] > static struct lock_class_key __srcu_key =3D RCU_KEY; \ > ^ > /home/paulmck/public_git/linux-rcu/kernel/notifier.c:526:6: note: in expa= nsion of macro =E2=80=98init_srcu_struct=E2=80=99 > if (init_srcu_struct(&nh->srcu) < 0) > ^ >=20 > These have the following in their .config files: >=20 > CONFIG_DEBUG_LOCK_ALLOC=3Dy >=20 > 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=3Dy, but others do not. >=20 Oh.. this is embarrassing ;-( Hmm.. when CONFIG_RCU_LOCKED_ACCESS=3Dn and CONFIG_DEBUG_LOCK_ALLOC=3Dy, this line becomes: static struct lock_class_key __srcu_key =3D { 0 }; IIUC, "=3D{ 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=3D53119 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D64709 Also I can't reproduce this with CONFIG_RCU_LOCKED_ACCESS=3Dn and CONFIG_DEBUG_LOCK_ALLOC=3Dy on my machine, whose gcc version is 5.3.0. Of course I can use a different way here and do not need a "=3D{ 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 ;-) Regards, Boqun > I have dequeued these for the moment. Please send an updated patch > series when you have this fixed. >=20 > Thanx, Paul >=20 > > \ > > __init_srcu_struct((sp), #sp, &__srcu_key); \ > > }) > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c > > index 996c2d5..36bd0cc 100644 > > --- a/kernel/locking/lockdep.c > > +++ b/kernel/locking/lockdep.c > > @@ -4623,5 +4623,8 @@ void locked_access(struct locked_access_class *la= class, > > correlate_locked_access(laclass, acqchain, loc, type); > > } > > EXPORT_SYMBOL(locked_access); > > +#ifdef CONFIG_RCU_LOCKED_ACCESS > > +DEFINE_LACLASS(rcu); > > +#endif > >=20 > > #endif /* CONFIG_LOCKED_ACCESS */ > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c > > index 76b94e1..ba2ded6 100644 > > --- a/kernel/rcu/update.c > > +++ b/kernel/rcu/update.c > > @@ -235,20 +235,23 @@ EXPORT_SYMBOL_GPL(__rcu_read_unlock); > > #endif /* #ifdef CONFIG_PREEMPT_RCU */ > >=20 > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > > -static struct lock_class_key rcu_lock_key; > > -struct lockdep_map rcu_lock_map =3D > > - STATIC_LOCKDEP_MAP_INIT("rcu_read_lock", &rcu_lock_key); > > -EXPORT_SYMBOL_GPL(rcu_lock_map); > > - > > -static struct lock_class_key rcu_bh_lock_key; > > -struct lockdep_map rcu_bh_lock_map =3D > > - STATIC_LOCKDEP_MAP_INIT("rcu_read_lock_bh", &rcu_bh_lock_key); > > -EXPORT_SYMBOL_GPL(rcu_bh_lock_map); > > - > > -static struct lock_class_key rcu_sched_lock_key; > > -struct lockdep_map rcu_sched_lock_map =3D > > - STATIC_LOCKDEP_MAP_INIT("rcu_read_lock_sched", &rcu_sched_lock_key); > > -EXPORT_SYMBOL_GPL(rcu_sched_lock_map); > > + > > +# ifdef CONFIG_RCU_LOCKED_ACCESS > > +# define RCU_KEY { .laclass =3D &rcu_laclass } > > +# else > > +# define RCU_KEY { 0 } > > +# endif > > + > > +#define DEFINE_RCU_LOCKDEP_MAP(flavor) \ > > + static struct lock_class_key rcu##flavor##_lock_key =3D RCU_KEY; \ > > + struct lockdep_map rcu##flavor##_lock_map =3D \ > > + STATIC_LOCKDEP_MAP_INIT("rcu" #flavor "_read_lock", \ > > + &rcu##flavor##_lock_key); \ > > + EXPORT_SYMBOL_GPL(rcu##flavor##_lock_map) > > + > > +DEFINE_RCU_LOCKDEP_MAP(); > > +DEFINE_RCU_LOCKDEP_MAP(_bh); > > +DEFINE_RCU_LOCKDEP_MAP(_sched); > >=20 > > static struct lock_class_key rcu_callback_key; > > struct lockdep_map rcu_callback_map =3D > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > index 178f288..b6003d4 100644 > > --- a/lib/Kconfig.debug > > +++ b/lib/Kconfig.debug > > @@ -1410,6 +1410,18 @@ config RCU_EQS_DEBUG > > Say N here if you need ultimate kernel/user switch latencies > > Say Y if you are unsure > >=20 > > +config RCU_LOCKED_ACCESS > > + bool "Track data access in RCU read-side critical sections" > > + depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPO= RT && LOCKDEP_SUPPORT > > + select LOCKED_ACCESS > > + default n > > + help > > + Track data acces in RCU read-side critical sections, > > + by doing so, one can examine which rcu_dereference() and its > > + friends are called in which RCU read-side critical sections, > > + and even more detailed, after which rcu_read_lock() and > > + its friends are called. > > + > > endmenu # "RCU Debugging" > >=20 > > config DEBUG_BLOCK_EXT_DEVT > > --=20 > > 2.7.0 > >=20 >=20 --2fHTh5uZTiUOsy+g Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJWwpETAAoJEEl56MO1B/q4JS0H/An2+ebLexV8f9xUCKAoBRDL F8kef48X3qbpQRIu4453c/vJRj7dUnJ+3AYcxETwf0ZkAyqYOdl5Nf7WP9v5n3Md Wi82R2w3BIxfdPO/6nAc7HjbG0P539mPNDdYusgdN/AmJFXJ74gdo+n9hHPqLoDJ B8mmdN8uvYR4MKXQj6WP/ErDnIs6L+JKaS5zOFZRZY7AngtKTKynzBGHd1gldy79 Mm2H9HO1yQ7hTwyNLKcHHeoJXMq/jG5W7kKS++7i0zUytrG4khHzmM0wSshQYqhv Qa72Iq0PfGeqj/Q/6u2oFTz0OJJS/zsUUaiaM5kc5GOVkbd07/KT5XwtfwX2TPs= =oKD4 -----END PGP SIGNATURE----- --2fHTh5uZTiUOsy+g--