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>,
        "rcu@vger.kernel.org" <rcu@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        lkp <lkp@intel.com>
Subject: RE: [PATCH v2] rcu: Add nocb_cb_kthread check to
 rcu_is_callbacks_kthread()
Thread-Topic: [PATCH v2] rcu: Add nocb_cb_kthread check to
 rcu_is_callbacks_kthread()
Thread-Index: AQHYWTteAMSEHqddYEaNU2h/OC67jK0FioWAgAFGIhA=
Date:   Fri, 29 Apr 2022 12:24:39 +0000
Message-ID: <PH0PR11MB5880DBC7E302EFA3A552B6ABDAFC9@PH0PR11MB5880.namprd11.prod.outlook.com>
References: <20220426070031.3759998-1-qiang1.zhang@intel.com>
 <20220428163913.GI1790663@paulmck-ThinkPad-P17-Gen-1>
In-Reply-To: <20220428163913.GI1790663@paulmck-ThinkPad-P17-Gen-1>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-version: 11.6.401.20
dlp-reaction: no-action
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?CCMyWDM/8729skj7k16frg6QxaiWZmTKm/epdE8hxGff4YUh0FTiemTHegyM?=
 =?us-ascii?Q?oIFzsIXeR3Ccb+1iEgI4kaOP4d/OdiYzG8OU3Q/6+RwdWUBW/ZT9xKMvNmtu?=
 =?us-ascii?Q?DoTXqczZ12rX+wfeoftHrAvC8jvdzCZCmaazw0jVMd6w1qQZ7atHe7lqSHZn?=
 =?us-ascii?Q?Kr85YgracAWxLAFNTx5qM3wP9jvGNeonHFxZ//wAYBjY87QxtcEQBA3qV1q8?=
 =?us-ascii?Q?QzhZciMEFiQmALuiPPu5Efv515P5A5M+iqNCtdmfWRY1jDh0s2RTYU8Uk6Eg?=
 =?us-ascii?Q?zWxqrzivJcO6yLqiyQXGEyYoxz33K+vJ7+uWExPbv0zvc3xtD2FvwZha4Sk1?=
 =?us-ascii?Q?366+6pxaAhkjTn7JOc1CavCu02Ea8JetYyiq/NHtoaIVMEEsOo+/CcfNUO5i?=
 =?us-ascii?Q?RcdfEyd5pV9Jq6OLuZ3qgmrM2UOS7NABZCK0u96QPErF1xggCl0ftipHVUGB?=
 =?us-ascii?Q?wBaEvPO5q7uOJnQqy0JJN7qcXkhlcCwmx4xdPf1HLkMsD5ir31IprH7Gea27?=
 =?us-ascii?Q?e0cVCKKmzKfIDee50pWw5NZu1/7jZXne63sailChvDrhuE9eQkmxGbJOm7i+?=
 =?us-ascii?Q?bQXFoV0dlknBMFYlCyKMhj2T5EsgNV39q2csymCkei7Fdw3z8kAP2rNTkdUP?=
 =?us-ascii?Q?KAKkmr1F2Vsiq1eHbPdEzQGrW3LYvoEDdU830G0jPxgG1ot2h5sYhBQfgbVr?=
 =?us-ascii?Q?dQ0iqek6dJuXx67AFpaZOmUrrSImaHDBwiHq05fJwbiS7w+lEPG6Au5FzYx7?=
 =?us-ascii?Q?Rhy06eY/Y8DTy4akDQfHvieTglH73tQd/HOObNoP10UXLjwHo62Iup8Pnk7O?=
 =?us-ascii?Q?pLY7m1KJ8vmUc+qHwJ5NJlsh49YuNVqKzDci8fEnoYKInVFOLJQBU9IKRGdW?=
 =?us-ascii?Q?VX3wEhtta1f3aDSbJ13UHa/M0NB56h4Y8pgVTlJ2aLxV3gC1dbcxoZpEZSM6?=
 =?us-ascii?Q?QQR214zu6I2Xic5ZSXvvvVbjB0kfiMTmgv8z6IWcTRb9nTS6PALxvV9P7lGT?=
 =?us-ascii?Q?QiRprSKAKjppbrjM9DqUeVH1DPHTUbHkR/QCuoZUjD+Yygj+Aodz6h9GpihR?=
 =?us-ascii?Q?4XupxZiv6ydWJa3ra99dUO4/cCZp7Icuf5DaOvdpSHtYODzLD1X83iztKMi+?=
 =?us-ascii?Q?mMVeIlOMte2gUFwzOb93VANiVEf7geNg5aTTceJL5b8l41mhzeCcD2COhpoN?=
 =?us-ascii?Q?0vy2/5m6wPXF2zczr7rhzXlNvcB2XY8oW5G0T947s1oFFzDOdvRmkK8kMJ3y?=
 =?us-ascii?Q?5inn8MpSWUkgj5KNBqBDXpwfkcUhRM+mtLNrahiKHhLHvagccOm/iqfg/GJn?=
 =?us-ascii?Q?fxwJhWBxUzAzkx5nrXrfLBBO5a8QEkyKJrWiO1yD/spXHPR50Od1oHsgkTKG?=
 =?us-ascii?Q?n2/w9hGFp7fVwszMBb8gaN33uH8286bpJ+cIZEEhbpdJC0KIYkLDcWYctUho?=
 =?us-ascii?Q?LIiUnWDVtEpXKZbdZfprMMTTArwafprNINhPGgtZm/gD2FMrF2KMSKF5S6cz?=
 =?us-ascii?Q?zo94YKy/fuAgOY5lPHzTca9/ysRKXnsfE/H7fi9b2iLZLiLukPALbAVI79kZ?=
 =?us-ascii?Q?n9L/vRnFBBeZrxLcB9DjZiGbZfVRTqelB1ZpOcqRr/hghEXE9RwkLoLbS6M3?=
 =?us-ascii?Q?YsJYej+TsZUyS4720PxKDD187Q94wV0kANZwTT9xNVwLv+08OQ3+/9bvsomL?=
 =?us-ascii?Q?I36rgN1yaiiBeqA+LVAwQPvF7YPfO+TebqF7Pl/7lCN5SnpQVg0q3/5NeufB?=
 =?us-ascii?Q?cIhMQS6M1Q=3D=3D?=
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: 3262dd17-2a4b-4f86-e19b-08da29db3b3d
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Apr 2022 12:24:39.4521
 (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: uwMWKbkqJzlJ7+brq4hEufQ6xFV5CQS/wbDLV+WatYEcIFiX3QSUewfvRq5kTvFvD6Rnj3lzM2oSDEFSmjLjzQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5813
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        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, Apr 26, 2022 at 03:00:31PM +0800, Zqiang wrote:
> At present, there are two situations which the rcu callback function=20
> be exectued in the kthreads, one is if the use_softirq is set to zero,=20
> the RCU_SOFTIRQ processing is carried out by the per-CPU rcuc=20
> kthreads, for non-offload rdp, the rdp's rcu callback function be=20
> exectued in rcuc kthreads. another one is if the rdp is set to=20
> offloaded, the rdp's rcu callback function be exectued in the rcuop kthre=
ads.
>=20
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>

>Good catch!
>
>From what I can see, this affects only tracing.  Or did I miss a use case?

Yes it is affects tracing, it was add make tracing info more accurate.

>
>Also, should the two definitions of rcu_is_callbacks_kthread() be merged?
>It looks like the current situation dates back to when the only time rcuc =
kthreads were created was in kernels built with CONFIG_RCU_BOOST=3Dy.
>If so, the definition of this function should move from tree_plugin.h to t=
ree.c.

Yes it should not depend on CONFIG_RCU_BOOST option.

>
>							Thanx, Paul

> ---
>  v1->v2:
>  fix compilation error when CONFIG_RCU_NOCB_CPU is no define
>=20
>  kernel/rcu/tree.c        |  4 ++--
>  kernel/rcu/tree.h        |  2 +-
>  kernel/rcu/tree_plugin.h | 12 +++++++++---
>  3 files changed, 12 insertions(+), 6 deletions(-)
>=20
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index=20
> 5c587e00811c..9dc4c4e82db6 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2610,7 +2610,7 @@ static void rcu_do_batch(struct rcu_data *rdp)
>  		trace_rcu_batch_end(rcu_state.name, 0,
>  				    !rcu_segcblist_empty(&rdp->cblist),
>  				    need_resched(), is_idle_task(current),
> -				    rcu_is_callbacks_kthread());
> +				    rcu_is_callbacks_kthread(rdp));
>  		return;
>  	}
> =20
> @@ -2688,7 +2688,7 @@ static void rcu_do_batch(struct rcu_data *rdp)
>  	rcu_nocb_lock_irqsave(rdp, flags);
>  	rdp->n_cbs_invoked +=3D count;
>  	trace_rcu_batch_end(rcu_state.name, count, !!rcl.head, need_resched(),
> -			    is_idle_task(current), rcu_is_callbacks_kthread());
> +			    is_idle_task(current), rcu_is_callbacks_kthread(rdp));
> =20
>  	/* Update counts and requeue any remaining callbacks. */
>  	rcu_segcblist_insert_done_cbs(&rdp->cblist, &rcl); diff --git=20
> a/kernel/rcu/tree.h b/kernel/rcu/tree.h index=20
> 996387962de3..3cdc18997a38 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -433,7 +433,7 @@ static void rcu_flavor_sched_clock_irq(int user); =20
> static void dump_blkd_tasks(struct rcu_node *rnp, int ncheck);  static=20
> void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags); =20
> static void rcu_preempt_boost_start_gp(struct rcu_node *rnp); -static=20
> bool rcu_is_callbacks_kthread(void);
> +static bool rcu_is_callbacks_kthread(struct rcu_data *rdp);
>  static void rcu_cpu_kthread_setup(unsigned int cpu);  static void=20
> rcu_spawn_one_boost_kthread(struct rcu_node *rnp);  static bool=20
> rcu_preempt_has_tasks(struct rcu_node *rnp); diff --git=20
> a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h index=20
> 971bb6a00ede..120bade40e02 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -1155,9 +1155,15 @@ static void rcu_initiate_boost(struct rcu_node *rn=
p, unsigned long flags)
>   * Is the current CPU running the RCU-callbacks kthread?
>   * Caller must have preemption disabled.
>   */
> -static bool rcu_is_callbacks_kthread(void)
> +static bool rcu_is_callbacks_kthread(struct rcu_data *rdp)
>  {
> -	return __this_cpu_read(rcu_data.rcu_cpu_kthread_task) =3D=3D current;
> +	bool ret;
> +#ifdef CONFIG_RCU_NOCB_CPU
> +	ret =3D rdp->rcu_cpu_kthread_task =3D=3D current || rdp->nocb_cb_kthrea=
d=20
> +=3D=3D current; #else
> +	ret =3D rdp->rcu_cpu_kthread_task =3D=3D current; #endif
>
>The problem here is that the first part of that condition is duplicated.
>This is an accident waiting to happen when someone fixes one side of that =
#ifdef without also adjusting the other side.  One approach is to define a =
function that tests "nocb_cb_kthread =3D=3D current" in CONFIG_RCU_NOCB_CPU=
=3Dy kernels and just returns "false" otherwise.

Thanks for your suggestion, I will resend.

>
>Alternatively, you could make ->nocb_cb_kthread defined unconditionally, b=
ut left always NULL in CONFIG_RCU_NOCB_CPU=3Dn kernels.

> +	return ret;
>  }
> =20
>  #define RCU_BOOST_DELAY_JIFFIES DIV_ROUND_UP(CONFIG_RCU_BOOST_DELAY *=20
> HZ, 1000) @@ -1242,7 +1248,7 @@ static void rcu_initiate_boost(struct rcu=
_node *rnp, unsigned long flags)
>  	raw_spin_unlock_irqrestore_rcu_node(rnp, flags);  }
> =20
> -static bool rcu_is_callbacks_kthread(void)
> +static bool rcu_is_callbacks_kthread(struct rcu_data *rdp)
>  {
>  	return false;
>  }
> --
> 2.25.1
>=20