From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965044AbcLVBIw (ORCPT ); Wed, 21 Dec 2016 20:08:52 -0500 Received: from mail-pg0-f65.google.com ([74.125.83.65]:34087 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932961AbcLVBIu (ORCPT ); Wed, 21 Dec 2016 20:08:50 -0500 Date: Thu, 22 Dec 2016 09:08:49 +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: <20161222010849.GA1728@tardis.cn.ibm.com> References: <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> <20161221041808.GF1316@tardis.cn.ibm.com> <20161221164845.GH3924@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="VS++wcV0S1rZb1Fb" Content-Disposition: inline In-Reply-To: <20161221164845.GH3924@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 --VS++wcV0S1rZb1Fb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Dec 21, 2016 at 08:48:45AM -0800, Paul E. McKenney wrote: > On Wed, Dec 21, 2016 at 12:18:08PM +0800, Boqun Feng wrote: > > 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 rel= eases > > > > 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 atten= tion > > > > to it, maybe write some regression tests for this particular proble= m to > > > > help developers avoid it. > > > >=20 > > > > This check is supposed to be removed, so I'm not stick to keeping i= t. > > >=20 > > > I suggest keeping through validation. If it triggers during that tim= e, > > > consider keeping it longer. If it does not trigger, remove it before > > > it goes upstream. > >=20 > > Good point ;-) > >=20 > > [...] > > > > > >=20 > > > > > > But this brings a side question, is the callsite of rcu_cpu_sta= rting() > > > > > > 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 t= hat > > > > > 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 > >=20 > > 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? >=20 > At rcu_init() time, there is only one online CPU, namely the boot CPU. >=20 > Or perhaps your point is that if CPUs come online before rcu_init(), they > might do so via the normal online mechanism. I don't believe that this > is likely, because the normal online mechanism reaquires the scheduler > be running. But either way, my hope would be that whoever fires up CPUs > before rcu_init() asks a few questions when they run into bugs. ;-) >=20 ;-) > > Besides, without your patch, we started the boot CPU many times in the > > for_each_online_cpu() loop. >=20 > That is true. It is harmless because it just does a group of assignments > repeatedly, and because there is only one CPU and because interrupts > are disabled, this cannot have any effect. And my fix inadvertently > fixed this issue, didn't it? >=20 Yep! > So I do need to update the commit log accordingly. Done! >=20 > > Am I missing something subtle? >=20 > Given the nature of RCU, the only possible answer I can give to that > question is "probably". (Hey, you asked!!!) >=20 True, I misread the for_each_online_cpu() loop in rcu_init(), I thought at that time, CPUs other than boot cpu have already mask themselves in the cpu_online_mask. But that's not true..=20 Sorry for the noice, and thank you for explanation ;-) Regards, Boqun > Thanx, Paul >=20 > > Regards, > > Boqun > >=20 > > > 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 C= PU and for > > > > > all CPUs onlined after rcu_init() executes (during very early= boot). > > > > > Currently, this is the full set of CPUs, so all is well. But= if > > > > > anyone ever parallelizes boot before rcu_init() time, it will= fail. > > > > > This commit therefore substitutes the rcu_cpu_starting() func= tion's > > > > > this_cpu_pointer() for per_cpu_ptr(), future-proofing the cod= e 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 >=20 >=20 --VS++wcV0S1rZb1Fb Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAlhbJ5oACgkQSXnow7UH +rjbJwf5AdPflGqpzIhaKTVHVBs46dLdAdYTCZXhn8CJGl5ZvyEZLWJEOMBUFfHv PbCahL+siSEu0BzplHh4BoCYlPMZ3F8cCjovHxemjsth//0PBnSpH9baUZB4vQ9X a9vbv9f16oD6fKKJGBJjyXk4hes32SM4WX6B7siFrffToXEQc+qHBhPffFQkl4lA EWrIovsSIL9p4hz76IbT223QzcbibcHA79gR73nBc7xs93fiCiFWEx4hG5qdhHua kREiqXoQp9zN+D+NOxefS+wut8TajZI7N69RWq7f4QoqzO7u+vrjJ/6lGdBeutF5 nxHz9UZ+smUCbYVjvB6XLb7Ti4MDcQ== =OF1C -----END PGP SIGNATURE----- --VS++wcV0S1rZb1Fb--