From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751405AbcBPFAW (ORCPT ); Tue, 16 Feb 2016 00:00:22 -0500 Received: from mail-pa0-f54.google.com ([209.85.220.54]:35191 "EHLO mail-pa0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750719AbcBPFAU (ORCPT ); Tue, 16 Feb 2016 00:00:20 -0500 Date: Tue, 16 Feb 2016 12:59:46 +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: <20160216045946.GB27347@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> <20160216030143.GA27347@fixme-laptop> <20160216040840.GP6719@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="s/l3CgOIzMHHjg/5" Content-Disposition: inline In-Reply-To: <20160216040840.GP6719@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 --s/l3CgOIzMHHjg/5 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 tuni= ng. > > > >=20 > > > > This patch therefore uses the LOCKED_ACCESS to collect the informat= ion > > > > 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 critic= al > > > > 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(voi= d) > > > > #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_derefere= nce(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/linu= x/notifier.h:15:0, > > > from /home/paulmck/public_git/linux-rcu/arch/x86/inc= lude/asm/kdebug.h:4, > > > from /home/paulmck/public_git/linux-rcu/include/linu= x/kdebug.h:4, > > > from /home/paulmck/public_git/linux-rcu/kernel/notif= ier.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: warnin= g: missing 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 = expansion 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: warnin= g: (near initialization for =E2=80=98__srcu_key.subkeys=E2=80=99) [-Wmissin= g-braces] > > > static struct lock_class_key __srcu_key =3D RCU_KEY; \ > > > ^ > > > /home/paulmck/public_git/linux-rcu/kernel/notifier.c:526:6: note: in = expansion 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 > >=20 > > Oh.. this is embarrassing ;-( > >=20 > > Hmm.. when CONFIG_RCU_LOCKED_ACCESS=3Dn and CONFIG_DEBUG_LOCK_ALLOC=3Dy, > > this line becomes: > >=20 > > static struct lock_class_key __srcu_key =3D { 0 }; > >=20 > > 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? > >=20 > > And may I ask your compiler's version? Because looks to me this may > > be a compiler bug according to: > >=20 > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D53119 > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D64709 >=20 > gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04) (KSIC) >=20 > > 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. > >=20 > > 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 ;-) >=20 > 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... >=20 Fair enough! > Thanx, Paul >=20 > > Regards, > > Boqun > >=20 > > > 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=3Dy and CONFIG_RCU_LOCKED_ACCESS=3Dn, will fix that too and send out a new series. Regards, Boqun > > >=20 > > > Thanx, Paul > > >=20 --s/l3CgOIzMHHjg/5 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAABCAAGBQJWwqy9AAoJEEl56MO1B/q4/3kIAIxHOd0o2Df7or+bRUyBcjjz U6VDj4FQUFwSv/blteSp1WUbyT1NFIZN3Az+y0EFVvZzl7+SjoCzlvf2KCLZKzYk o0AjJa8gF2u/fd6inkgo5swzDzeKHSTY4iFpbfZe1+cOATkrPYv/rj4XBFfiW4zM jKpc+jMSXw6FxxeF6zhKuw1sDNjX1GFGd5f229EvbUG3/TqU/qulOmxxrVH49bep FO9IL08UiFPVGoofTACw6mcgmEJ/b0n5v2AQ+s8iAZP4pMPFzev2oXjTYZ/UtWxx P2IMnENELIHD90pw2dklGQ3u0v4rX7AWRza47q5JgsWf6ScPjjxKWaOWMUCTENc= =fapi -----END PGP SIGNATURE----- --s/l3CgOIzMHHjg/5--