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>
Subject: RE: [PATCH] rcu-tasks: Check the atomic variable
 trc_n_readers_need_end again when wait timeout
Thread-Topic: [PATCH] rcu-tasks: Check the atomic variable
 trc_n_readers_need_end again when wait timeout
Thread-Index: AQHYRCgbTB0vKm9Y50G6+1/r5OpdTqzYSg4AgAA0FUCAADoYEIABKWoAgABS7dA=
Date:   Fri, 1 Apr 2022 00:23:18 +0000
Message-ID: <PH0PR11MB58806F59248B6FE5FE7FFAE3DAE09@PH0PR11MB5880.namprd11.prod.outlook.com>
References: <20220330112014.2587148-1-qiang1.zhang@intel.com>
 <20220330190556.GL4285@paulmck-ThinkPad-P17-Gen-1>
 <PH0PR11MB5880D6B11469266144C58032DA1F9@PH0PR11MB5880.namprd11.prod.outlook.com>
 <MW5PR11MB58585FA7332315F46FA36CD0DAE19@MW5PR11MB5858.namprd11.prod.outlook.com>
 <20220331192446.GW4285@paulmck-ThinkPad-P17-Gen-1>
In-Reply-To: <20220331192446.GW4285@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?c5Vk16aaWWhxVYI+gjngN6gbU6HAo3jKuItwI5Ne2FjI08fBGeqRO2I+kznx?=
 =?us-ascii?Q?SujCVTnwdTFjgBhUYHsetLI8wI6IsJyb/pEtQnPc8SwrduAajkz4Y4iNDe23?=
 =?us-ascii?Q?3s05ixcUnfBNCDja3yNKuJC/I53OTR3sF8tJSjvgd/Zrm+IRKiv8xhGJCDun?=
 =?us-ascii?Q?T4cY5R0oIknGHwPz9OiA+1XBX6YuwztyWg8zTuY/1AcBLzYr5oPcIzU+qiqc?=
 =?us-ascii?Q?acyRi9K1/oVYjlmD3iAUnAg17bzuF6BoEvg6sJZ+NCTUNo5Cug3C2PaTbHZ4?=
 =?us-ascii?Q?0tOBMP+0hlP2J968NdT3k4jMjTu/ZFxQ1ZoHP8ewGm4qWgq3fRfPH/5DbPAo?=
 =?us-ascii?Q?ArhSTXvepGimTFK4wYBIyNFMGXenJWbKz2NSQzqAkVM/G1JO9greS6R5nWh1?=
 =?us-ascii?Q?cvJIuKETLaEwoJHbZqFtNOp4tgsVVZdiLxYYjfiU4FPvSXQGqwOCdxgf2qxP?=
 =?us-ascii?Q?qJ9FOTIP47/JPEEl46trmB75E0UuPo0Ur5EQVJ60+75tatYmQwWGgtZ0xswz?=
 =?us-ascii?Q?w8Ha81O0vDfVZh57GrLoc6aqV0jnM41IrKdz3abdOh8YWFaQec5Wny1U0v3M?=
 =?us-ascii?Q?sedF1kYIqBUPl7H+ruy0+ICXZpgeGqPIXnbBmdG6Jb9VdHnt8BHHSsM37p5J?=
 =?us-ascii?Q?OSZ+3xzJ1UZaghIcTiGbndtMOIUb4CCoLitpPNCgPAfmLi466QeibhxxiAUy?=
 =?us-ascii?Q?bNkz7HrCWE9a8GXXtVDWI1pd+1zHtAAeOQ/GxfYiTwsWN8itP7Ssi8AM8FtM?=
 =?us-ascii?Q?M0Aycp9kSD00MjlIpJD4XtQA/LmJ3G44NiDuLQGcg/3Qaq1E6PoXWXUzznCL?=
 =?us-ascii?Q?9LX9ImvFAqn/nUaLy0JHnmFWKDGjHfJB5yOBn9hcBjPmxOVmvAfHls9bPTd2?=
 =?us-ascii?Q?7Akuy1WeNtcZo8SdXzP3eARZc4tIb/pZIdkTMPYZE9x9cL+cjlAmrfXAezfW?=
 =?us-ascii?Q?CJkrEhjAoUmepjyn9uEe58vyUKXU2IGnNFoc39w8y4rhAfVLeTeuf1+xlyys?=
 =?us-ascii?Q?vLoYHkkkk3hmgIpr0EH1OgytxmEbcTbjGK3crajK6+s2QrVz+8PWtW9aTwie?=
 =?us-ascii?Q?FYA3c86Or8ipsjIa1JOFAB4ilv5UI6hIhx0jYdphzd7+OHlT+snwHSQzWntG?=
 =?us-ascii?Q?2NRiODT0echd9K2DET/c/mfOzuIMB6/gkZK/zgBLoqVrXdQAJE+auhtJAMDj?=
 =?us-ascii?Q?EBOote1bj5RUnhd/S3SLx5dFP05n1Y7ry36Drn3j1IEiR6XbHBtOFo8Q+moZ?=
 =?us-ascii?Q?KcAqcL7PEMEJi7CLecUylSd8YkXg8yC4i015YZ67xSBVa/oOjVy+hU4gQb7Z?=
 =?us-ascii?Q?JDX5UBhlJOeZKipSwFFb+iKZbHmswgMI7BDzKVoKw/jgjqwZCjqXLHdV/lYs?=
 =?us-ascii?Q?zCHyMKvbxXGXIYgaLuVRsR1ZmuW6gE+SmOxfpI6VbhSam7C+v5nB0Hqtle5J?=
 =?us-ascii?Q?4DY++DP34tDJ2IbPbd2mpdN+GHbg8UDmi657soa2EM/BJM3HCobLa5ROtJiH?=
 =?us-ascii?Q?/Y4SiQYmwRhvTNj32jDD5wNWNTRwyKA0paUzW79UsFoKfvv7J6bU8sPMVe4n?=
 =?us-ascii?Q?TK+UeUAczLm3EDObdUcl/JswVzjFbn7c8O6jk11xjrDgBSlaIy23ptiJW9k4?=
 =?us-ascii?Q?dGir8hXw45IiFb9mO6O9laWIpHourEZ8AjUSjbQujwg8srv+qaimeujgQlGw?=
 =?us-ascii?Q?2CDCcEmO049PlRcdx3foXQEC4y5K8Sl6fVZsf7PfoWsXD66QwrDPDPqsSS+p?=
 =?us-ascii?Q?9ZQhybt7GA=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: c198132c-6e5f-4486-f116-08da1375d26b
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Apr 2022 00:23:18.7799
 (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: XSnpdmTrCLvlzUFUlx0oiORYc1XOfz7IQEVRpTTdxoIN9T27rf4U9/nd5crbkJeWDa5nVY4v8Y1vxMenyndVUg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5241
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        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 Thu, Mar 31, 2022 at 02:03:26AM +0000, Zhang, Qiang1 wrote:
>=20
> On Wed, Mar 30, 2022 at 07:20:14PM +0800, Zqiang wrote:
> > When the trc_wait waitqueue timeout, the atomic variable=20
> > trc_n_readers_need_end need to be checked again, perhaps the=20
> > conditions have been established at this time, avoid invalid stall=20
> > information output.
> >
> >But are you actually seeing this invalid stall information?  Either way,=
 please seem my comments and question below.
> >
> >Please don't get me wrong, we do have similar checks for normal vanilla =
RCU stall warnings, for example, this statement in print_other_cpu_stall() =
in kernel/rcu/tree_stall.h:
> >
> >	pr_err("INFO: Stall ended before state dump start\n");
> >
> >However, the approach used there did benefit from significant=20
> >real-world experience with its absence.  ;-)
> >
> >							Thanx, Paul
> >
> > Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> > ---
> >  kernel/rcu/tasks.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >=20
> > diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h index=20
> > 65d6e21a607a..b73a2b362d6b 100644
> > --- a/kernel/rcu/tasks.h
> > +++ b/kernel/rcu/tasks.h
> > @@ -1544,7 +1544,7 @@ static void rcu_tasks_trace_postgp(struct rcu_tas=
ks *rtp)
> >  				trc_wait,
> >  				atomic_read(&trc_n_readers_need_end) =3D=3D 0,
> >  				READ_ONCE(rcu_task_stall_timeout));
>=20
> >If I understand correctly, this patch is intended to handle a situation =
where the wait_event_idle_exclusive_timeout() timed out, but where the valu=
e of trc_n_readers_need_end decreased to zero just at this point.
> >>
> >>Yes,  this patch is intended to handle a situation.
>=20
> >If so, please see my question below.  If not, please show me the exact s=
equence of events leading up to the problem.
>=20
> > -		if (ret)
> > +		if (ret || !atomic_read(&trc_n_readers_need_end))
> >  			break;  // Count reached zero.
>=20
> >Couldn't the value of trc_n_readers_need_end decrease to zero right here=
, still resulting in invalid stall information?
> >
> >>The value of trc_n_readers_need_end decrease to zero right here, indica=
tes that the current grace period has been completed.
> >>Even if we go to print some task with condition 't->trc_reader_special.=
b.need_qs' as true,  and these task  will not affect the current grace peri=
od. Is my understanding correct?
> >>
>=20
> Is my commit  information not clear? Is this description more clearly
>=20
> When the wait_event_idle_exclusive_timeout() timed out, if the value=20
> of trc_n_readers_need_end  decrease to zero just at this point this=20
> indicates that the current grace period is just completed at this=20
> point, direct break  and  avoid printing stall information.

>Lack of clarity is not the problem.  The problem is instead lack of justif=
ication.  My questions were intended to help you understand how best to jus=
tify this sort of change, and also to help you determine on your own whethe=
r or not future changes can be justified.
>
>Please keep in mind that we reach that code when the grace period has been=
 stalled for ten full -minutes-.  If the system has 64 CPUs, this means tha=
t the interarrival rate of decrements to trc_n_readers_need_end is roughly =
0.1 per second.  The time between the wakeup-timeout test of trc_n_readers_=
need_end and the "if (ret)" will normally be small numbers of microseconds,=
 let's say ten of them.  The probability of this change making any differen=
ce is therefore on the order of one in a million.
>
>And the next step is to traverse the entire tasks list, which could take q=
uite a few milliseconds.  So there is is something like 1000 times more lik=
ely that the value of trc_n_readers_need_end will change in that loop than =
before the "if (ret)" check.
>
>Therefore, I am having a very hard time believing that this change is wort=
hwhile.
>
>My turn.  Is this explanation clear?  ;-)

Thanks, this explanation was very detailed.

>
>							Thanx, Paul

> >>Thanks
> >>Zqiang
>=20
> >  		// Stall warning time, so make a list of the offenders.
> >  		rcu_read_lock();
> > --
> > 2.25.1
> >=20