From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755558AbcLUESM (ORCPT ); Tue, 20 Dec 2016 23:18:12 -0500 Received: from mail-pg0-f67.google.com ([74.125.83.67]:34112 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754628AbcLUESJ (ORCPT ); Tue, 20 Dec 2016 23:18:09 -0500 Date: Wed, 21 Dec 2016 12:18:08 +0800 From: Boqun Feng To: "Paul E. McKenney" Cc: Colin Ian King , Mark Rutland , linux-kernel@vger.kernel.org, Josh Triplett , Steven Rostedt , Mathieu Desnoyers , Lai Jiangshan Subject: Re: [RFC v2 4/5] rcu: Use for_each_leaf_node_cpu() in force_qs_rnp() Message-ID: <20161221041808.GF1316@tardis.cn.ibm.com> References: <20161215024204.28620-5-boqun.feng@gmail.com> <20161215120459.GE21758@leverpostej> <20161215144242.GN9728@tardis.cn.ibm.com> <05a9953b-aaa4-6117-b120-85c12ad56ace@canonical.com> <20161219151515.GP9728@tardis.cn.ibm.com> <20161220050913.GP3924@linux.vnet.ibm.com> <20161220055914.GB1316@tardis.cn.ibm.com> <20161220152352.GQ3924@linux.vnet.ibm.com> <20161221023456.GE1316@tardis.cn.ibm.com> <20161221034024.GC3924@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="imjhCm/Pyz7Rq5F2" Content-Disposition: inline In-Reply-To: <20161221034024.GC3924@linux.vnet.ibm.com> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --imjhCm/Pyz7Rq5F2 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Dec 20, 2016 at 07:40:24PM -0800, Paul E. McKenney wrote: [...] > >=20 > > Agreed, my intent is to keep this overcare check for couples of releases > > and if no one shoots his/her foot, we can remove it, if not, it > > definitely means this part is subtle, and we need to pay more attention > > to it, maybe write some regression tests for this particular problem to > > help developers avoid it. > >=20 > > This check is supposed to be removed, so I'm not stick to keeping it. >=20 > I suggest keeping through validation. If it triggers during that time, > consider keeping it longer. If it does not trigger, remove it before > it goes upstream. >=20 Good point ;-) [...] > > > >=20 > > > > But this brings a side question, is the callsite of rcu_cpu_startin= g() > > > > is correct? Given rcu_cpu_starting() ignores the @cpu parameter and= only > > > > set _this_ cpu's bit in a leaf node? > > >=20 > > > The calls from notify_cpu_starting() are called from the various > > > start_kernel_secondary(), secondary_start_kernel(), and similarly > > > named functions. These are called on the incoming CPU early in that > > > CPU's execution. The call from rcu_init() is correct until such time > > > as more than one CPU can be running at rcu_init() time. And that > > > day might be coming, so please see the untested patch below. > >=20 > > Looks better than mine ;-) > >=20 > > But do we need to worry that we start rcu on each CPU twice, which may > > slow down the boot? >=20 > We only start a given CPU once. The boot CPU at rcu_init() time, and > the rest at CPU-hotplug time. Unless of course a CPU is later taken Confused... we call rcu_cpu_starting() in a for_each_online_cpu() loop in rcu_init(), so we basically start all online CPUs there after applying your patch. And all the rest CPUs will get themselves start again at CPU-hotplug time, right? Besides, without your patch, we started the boot CPU many times in the for_each_online_cpu() loop. Am I missing something subtle? Regards, Boqun > offline, in which case we start it again when it comes back online. >=20 > Thanx, Paul >=20 > > Regards, > > Boqun > >=20 > > > Thanx, Paul > > >=20 > > > ---------------------------------------------------------------------= --- > > >=20 > > > commit 1e84402587173d6d4da8645689f0e24c877b3269 > > > Author: Paul E. McKenney > > > Date: Tue Dec 20 07:17:58 2016 -0800 > > >=20 > > > rcu: Make rcu_cpu_starting() use its "cpu" argument > > > =20 > > > The rcu_cpu_starting() function uses this_cpu_ptr() to locate the > > > incoming CPU's rcu_data structure. This works for the boot CPU a= nd for > > > all CPUs onlined after rcu_init() executes (during very early boo= t). > > > Currently, this is the full set of CPUs, so all is well. But if > > > anyone ever parallelizes boot before rcu_init() time, it will fai= l. > > > This commit therefore substitutes the rcu_cpu_starting() function= 's > > > this_cpu_pointer() for per_cpu_ptr(), future-proofing the code and > > > (arguably) improving readability. > > > =20 > > > Reported-by: Boqun Feng > > > Signed-off-by: Paul E. McKenney > > >=20 > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index b9d3c0e30935..083cb8a6299c 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -4017,7 +4017,7 @@ void rcu_cpu_starting(unsigned int cpu) > > > struct rcu_state *rsp; > > > =20 > > > for_each_rcu_flavor(rsp) { > > > - rdp =3D this_cpu_ptr(rsp->rda); > > > + rdp =3D per_cpu_ptr(rsp->rda, cpu); > > > rnp =3D rdp->mynode; > > > mask =3D rdp->grpmask; > > > raw_spin_lock_irqsave_rcu_node(rnp, flags); > > >=20 >=20 >=20 --imjhCm/Pyz7Rq5F2 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAlhaAnsACgkQSXnow7UH +rhWIQf/YMKtNrZEPgPTvH2mzBj6FS+Hq6g1vqCwOELAUPxhku8IkMnY7aebwJS+ nqSkis9qhW2VDwoiUvtF3NycyyMSCnHu5C/xO4dQegZ2Qq8b13xYhSGMNMdaqRaO lLQVjxlV0efyvrHdyovtpL8GIGSBj+6nwLKy0lCaFOYYzPH7CVsQqs1N/Bn2kgLe w0i9yI+kIhdYnGw7Oimw56duF3Rha5A2Io54OpV92JBpss3XAyR0ne/3xZwtIOS5 tcb7kAseUhCtMtUBSgzs2Zduzd9VDDbJP7dFF8P9LJHlSpvyD80eaSmYKvEsz8vX mUi/5Q7cLvGE/NK3S04DsIAp5j27tw== =LQ+7 -----END PGP SIGNATURE----- --imjhCm/Pyz7Rq5F2--