Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Zhang, Qiang1" <qiang1.zhang@intel.com>
To:     "paulmck@kernel.org" <paulmck@kernel.org>
CC:     "frederic@kernel.org" <frederic@kernel.org>,
        "quic_neeraju@quicinc.com" <quic_neeraju@quicinc.com>,
        "joel@joelfernandes.org" <joel@joelfernandes.org>,
        "rcu@vger.kernel.org" <rcu@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] rcu: Remove duplicate RCU exp QS report from
 rcu_report_dead()
Thread-Topic: [PATCH] rcu: Remove duplicate RCU exp QS report from
 rcu_report_dead()
Thread-Index: AQHYvEojjFZtiK5khEuSJPg6VsxqKq3HJvmAgAAZmXA=
Date:   Tue, 30 Aug 2022 11:30:39 +0000
Message-ID: <PH0PR11MB58809A3EB14FDBD408292446DA799@PH0PR11MB5880.namprd11.prod.outlook.com>
References: <20220830083151.396561-1-qiang1.zhang@intel.com>
 <20220830090623.GR6159@paulmck-ThinkPad-P17-Gen-1>
In-Reply-To: <20220830090623.GR6159@paulmck-ThinkPad-P17-Gen-1>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-version: 11.6.500.17
dlp-reaction: no-action
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?xPw+1mARyguIdBy7ON9XMwFpqzTLXNd5SMWnoRyVeMW76KjfkcAAyibNq51Q?=
 =?us-ascii?Q?EaAaRz5m44fAjeVnip/D9Zf0WxYdfuyU/vOCHQXLYF7LqjeVR37nw0XwiLRp?=
 =?us-ascii?Q?u4kpWbTWRNxLEzIimG0ddW97enU/q9AMQhScQJM3RfQAJXT7jpafzGMhTVP8?=
 =?us-ascii?Q?6Xx9In0dbUCrFieix4DNT7/TJkBtvFaBBY8LqoeN7ziy3QjaMTuMgXJbgUMz?=
 =?us-ascii?Q?jTX5vaMHgy6YIz2NlhE8lHodIEje0jqlv3ONIX1Yy0mfIrzR+AK792Gmc25m?=
 =?us-ascii?Q?MFUce6DuMnrg6AlC6AGtCzBHcDxwb6ftX4LrntSSxkrKPJG+1MxW/9i9sAVp?=
 =?us-ascii?Q?B3PeWQcDozbFcOupdtfzh2ABqPyC2bAki5eVz2UPpnfKiWE6FYjfXnkrZVyV?=
 =?us-ascii?Q?Yr4a5VLPzaKSPOAHTyeYupSjm2m00A9AH4xrtljQtEqjLMnKjzBRlOk56xcy?=
 =?us-ascii?Q?QBm+aO44IMtGvwHoPPZ7AJ8gBzV3WictgH7f4MerkgLD7+JWdlOccP/adOVP?=
 =?us-ascii?Q?Uj75qtpHutdU7lI+BJWNOhIF1tJkRhVmg63qzs4qE501ZQcs044JcG6oTXrq?=
 =?us-ascii?Q?+bv4FEQFySyKjxnXqHbMypN1Tgxh6yRMLVy7W+xt+Xd/ymwkd5I68Uhg2e8u?=
 =?us-ascii?Q?Wih62ZZgcWIyrSJj3X8yUzz8OhMPkwJ0d3yfpx+v5jSUoLCq8nlwwBgkvqiK?=
 =?us-ascii?Q?WJvyENxAJW+qd/pOWKh9SOdS+n5ikyUUpr8BRk23qOR+ERIIur3GQXB3ICiQ?=
 =?us-ascii?Q?gVkzOwVfxA0VMyzLIqLWDMRb1eqlUa5lzH3KWamRIxPe4htD0KOeEwH9/5dR?=
 =?us-ascii?Q?64dZSSU8p8m7/UP80Jyvm+t5lH3OD03o+7qIt7UL8szpVocQNC+FbTyuyLw6?=
 =?us-ascii?Q?Mrta8vp9drC2APlXs0FlBhpik+WdQZg2O9/Om562lU60nyq1z9vXUoEhO55n?=
 =?us-ascii?Q?SPw+TQ6Uy8IlPAkbR2ZjetvtrGgnU+p/lKaNqlshOSiGGuIoqC09uDL9QtPj?=
 =?us-ascii?Q?zEsw3c0QCWEzQWx1cBshBoF+cfHWQ41gFn03Md9DVC58derXUmIavYQlVNZR?=
 =?us-ascii?Q?cn7Cqt/hRelsrrZskSdyRmHIBD/7PjpbXap8+YV76pYWHG0JWo7Q3Yk0lj3U?=
 =?us-ascii?Q?afkUyrZYMjvG/ZS7I3P0j28UGcsQ3A8if5aNVpmgw0MC4zPUbPJpk7HT0NYF?=
 =?us-ascii?Q?YamkSL3PtPay7CMmMwCM4gWIFHVWlCRJ0urwIJN0UuTwFIj67zVwS8IvrQMf?=
 =?us-ascii?Q?ZhONTkB084m+bM51Y0Z1VYqt/MX09sipYPLWwRKX/o8YbpccHFBbdROeALOm?=
 =?us-ascii?Q?dR2gSCWtTdhqFHf6+d0DB4R290HaGbbxmYoJriTotPoTxNuM4j6lDLKMny5k?=
 =?us-ascii?Q?kJkNO6UPNZ1LH9+2uJwXlgoptm9eiAQktWGAEndlNqcZiwMfBHl2Xr/n8Y/P?=
 =?us-ascii?Q?9igAn9J6Zmwdh4QvYGmgefeyUIdiVFPq/5MMdFQeTL2FxelBY/IJCfTch9Wf?=
 =?us-ascii?Q?eGrA+T6d0X1ac/la+4LSC7iGanUZu3KWVMzYMLEGjeUqOjT7ZNaBaouMq+kX?=
 =?us-ascii?Q?/RV7SVea7CY0NdkO8ACcjSyQsD5ZKbj2Ty066zIR?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5880.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1277bf61-e337-49fb-314c-08da8a7b1101
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Aug 2022 11:30:39.7091
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: SpleY6zIGYMOHcIr+4eYbLK+sL3AQi8wKklVllyEvppXUG7nIfQzclI+EC9g4E1wbLXzjBVFEfUAqlzNBj6nNg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6925
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Aug 30, 2022 at 04:31:51PM +0800, Zqiang wrote:
> For PREEMPT_RCU, the rcu_report_dead() is invoked means that the
> outgoing CPU mask is clear from leaf rcu_node and has no further
> need of RCU, so invoke rcu_preempt_depth() return value is always
> zero in rcu_report_dead(), if the current outgoing CPU rcu_data
> structure's cpu_no_qs.b.exp is true, the rcu_preempt_deferred_qs()
> will invoke rcu_report_exp_rdp() to report exp QS.
>=20
> for non-PREEMPT_RCU, the rcu_preempt_deferred_qs() is equivalent to
> rcu_report_exp_rdp().
>=20
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
>
>Nice! =20
>
>One question... Currently, for PREEMPT_RCU, the outgoing CPU silently
>reports a quiescent state even if there was a bug that resulted in that
>CPU still being in an RCU read-side critical section.  With your change,
>the outgoing CPU would silently refuse to report a quiescent state.
>
>Is there something along the CPU-offline code path that already complains
>about this situation?  If not, I believe that the first WARN_ON_ONCE()
>in rcu_implicit_dynticks_qs() would complain.

In the following code, the current CPU will report QS if (rnp->qsmask & mas=
k) return true.
it means that the WARN_ON_ONCE(!rcu_rdp_cpu_online(rdp)) is not trigger.

if (rnp->qsmask & mask) { /* RCU waiting on outgoing CPU? */
                /* Report quiescent state -before- changing ->qsmaskinitnex=
t! */
                rcu_disable_urgency_upon_qs(rdp);
                rcu_report_qs_rnp(mask, rnp, rnp->gp_seq, flags);
                raw_spin_lock_irqsave_rcu_node(rnp, flags);
        }
 WRITE_ONCE(rnp->qsmaskinitnext, rnp->qsmaskinitnext & ~mask);

>
>Could you please try this, just so we know what happens in this case?
>One way of forcing this would be to do rcu_read_lock() just before the
>call to rcu_report_dead(), though other diagnostics might require that
>rcu_read_lock() to be earlier in the code.
>
>
>Another question in both cases...  There is a more subtle change where the
>old code ignores rdp->cpu_no_qs.b.exp (thus invoking rcu_report_exp_rdp()
>unconditionally) and the new code avoids invoking rcu_report_exp_rdp()
>unless this is set.  How does this interact with a new expedited
>grace period that starts just as this CPU calls rcu_report_dead()?

1.When a new expedited grace period that starts just as this CPU call rcu_r=
eport_dead(),
if in this time, this CPU rcu_data structure's cpu_no_qs.b.exp is not set t=
rue,=20
the rcu_preempt_deferred_qs() will not call rcu_report_exp_rdp().  but when=
 call
rcu_report_dead(),  this CPU have been offline(cpu_is_offline(this CPU) ret=
urn true).

2.In __sync_rcu_exp_select_node_cpus(), invoke smp_call_function_single() f=
or this
CPU will return -ENXIO,  and then check  (rnp->qsmaskinitnext & mask) and (=
rnp->expmask & mask)=20

3.If in this time, the rcu_report_dead() has not yet clear CPU mask from rn=
p->qsmaskinitnext,
we will redo 2 step,  recall smp_call_function_single(), but is always retu=
rn -ENXIO,=20
and recheck (rnp->qsmaskinitnext & mask) and (rnp->expmask & mask), until r=
cu_report_dead()
clear CPU mask from rnp->qsmaskinitnext. =20

Therefore, the __sync_rcu_exp_select_node_cpus() will call rcu_report_exp_c=
pu_mult()
to report this offline CPU's exp QS=20


Thanks
Zqiang

>The expedited grace-period code in __sync_rcu_exp_select_node_cpus()
>is of special concern here.
>
>							Thanx, Paul

> ---
>  kernel/rcu/tree.c | 2 --
>  1 file changed, 2 deletions(-)
>=20
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 6bb8e72bc815..0ca21ac0f064 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -4276,8 +4276,6 @@ void rcu_report_dead(unsigned int cpu)
>  	// Do any dangling deferred wakeups.
>  	do_nocb_deferred_wakeup(rdp);
> =20
> -	/* QS for any half-done expedited grace period. */
> -	rcu_report_exp_rdp(rdp);
>  	rcu_preempt_deferred_qs(current);
> =20
>  	/* Remove outgoing CPU from mask in the leaf rcu_node structure. */
> --=20
> 2.25.1
>=20