Received-SPF: pass (google.com: domain of linux-kernel+bounces-169937-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Praveen Kannoju <praveen.kannoju@oracle.com>
To: Jakub Kicinski <kuba@kernel.org>
CC: "jhs@mojatatu.com" <jhs@mojatatu.com>,
        "xiyou.wangcong@gmail.com"
	<xiyou.wangcong@gmail.com>,
        "jiri@resnulli.us" <jiri@resnulli.us>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "netdev@vger.kernel.org"
	<netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>,
        Rajesh Sivaramasubramaniom
	<rajesh.sivaramasubramaniom@oracle.com>,
        Rama Nichanamatlu
	<rama.nichanamatlu@oracle.com>,
        Manjunath Patil
	<manjunath.b.patil@oracle.com>
Subject: RE: [PATCH RFC] net/sched: adjust device watchdog timer to detect
 stopped queue at right time
Thread-Topic: [PATCH RFC] net/sched: adjust device watchdog timer to detect
 stopped queue at right time
Thread-Index: AQHamweIx8KdUY9mQ0GzMC27Mq44X7GC81qAgAKganCAAFa5AIAEWw2Q
Date: Mon, 6 May 2024 14:02:42 +0000
Message-ID: 
 <SA1PR10MB6445252B67E90F92DC87B28A8C1C2@SA1PR10MB6445.namprd10.prod.outlook.com>
References: <20240430140010.5005-1-praveen.kannoju@oracle.com>
	<20240501151251.2eccb4d0@kernel.org>
	<SA1PR10MB644567B49D44BA641CF63E828C1F2@SA1PR10MB6445.namprd10.prod.outlook.com>
 <20240503122954.58bee752@kernel.org>
In-Reply-To: <20240503122954.58bee752@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?U5ukOAUUxGTMxpGgddP5eI9BNtHrHNAvb3pm5reWFz3zQU7ButFwNpjCWrL0?=
 =?us-ascii?Q?9KsvKf15db6k3sSt7Ld/PAP23xtJ3oyei40CqxJ593rgMVEA0Ximoj71SS/N?=
 =?us-ascii?Q?Ghu3iRxSF+nfa8bD4XhkuWorA5FQtZeMuCv0btmhyvux8CSBp/qPzPrvEv+A?=
 =?us-ascii?Q?5MmCGUpBQ46nqp/JiSIO9yvANtFlboR74CaXwnil/qajNqcXD7hdu1sc+FeB?=
 =?us-ascii?Q?+IsiKrbEpfx/U6fY2DYHlC6a3WNp4RRIuOJLLgpqxsuRcIQ1qn5s0jA62tIJ?=
 =?us-ascii?Q?YMXch0RflnrchfZNo0JAdiguQdtdIJOq1np3NZmQiUYwwrOXq6kY+oEqeKQ5?=
 =?us-ascii?Q?1pDkYlQO8wH5r+yDvzqxHyVGuIrBWCP7MqI7NhGDNPGqPtQZSM9vbYMClB1V?=
 =?us-ascii?Q?jMySnaCA7bLdrkbpE27yhDiqOYwPDJrqLnOnQ4WkChEhZamOpPXWlyunxgvd?=
 =?us-ascii?Q?FF+Rxx9F3ZsbxNsrvWJ+400kOSgpuLo+IDGVPWcXaLww8kkUxaj/fB1MdkAI?=
 =?us-ascii?Q?kRgNfQAxdNQndndwQNxgKKHtwpbPeE4MwJfxZvvJHWtarYHu3KM09PtZhUIK?=
 =?us-ascii?Q?g9ux7jqU7dmE1Sa0TA7cy6NtTcJVdYx4iM/0HgplFzP9QmNE/XoyZj3eX4rJ?=
 =?us-ascii?Q?dZv4Cr1NcKH3u/QY85DM3j08oc/O77Kc93qNdDm/MXt79e11GO9c2+5OVwBv?=
 =?us-ascii?Q?4BUSw5Ez9RtNe8yhVOsw/Z9qxODmbesTsIjBd5a2xbaa9MFup9y4cnMDIWus?=
 =?us-ascii?Q?L+G1ND2hzfKle7H3U6ckkkO6uA6sBJ99jCxhW/fZduKTo3jVUd5KuYM5CIzi?=
 =?us-ascii?Q?tRIiGpBB+Kl1UHDApO9ld/8zHH5a2BYgVrEau89YXcUUvo8yloj7VF7isS6U?=
 =?us-ascii?Q?gqXZzs0GhiujlIbwa+nXbvrnvWBAWPbEm62z3UuA852d9QlrqwpFlwlLE6RS?=
 =?us-ascii?Q?wGuQFABuBRQDxJsOvfU5WxwCXvgoU5KldjIOD/FDZde/Kx/SnSyj4L7WK86R?=
 =?us-ascii?Q?rU3kIqlA5eSwcwoprW4uT1BWKepsP28BT4GC7tV2JfxFYtWg/66J/2EliEL4?=
 =?us-ascii?Q?k98I0/viGNgRplJ7r/zycPbpikQDwAxwCJLPez++iKgrh57NcXufTzIhoORY?=
 =?us-ascii?Q?SdgldIFc8vqrhz8V/tP9Qph6I5AXZjpn5ObBW3nE9GtU3jae5ZHC2kjtz6fE?=
 =?us-ascii?Q?QW51HHCsBtNo+cgj6a/OiKqVyy0fBFnwv0HjQvHMjFtLVeeEp6kxtgz/uTGN?=
 =?us-ascii?Q?CdAiV2lnV85a3YLGo85Ky9WerFokxPCKaPGi4fu8dXIGBmvml4PG0lbHXzU6?=
 =?us-ascii?Q?4Kx6V6CYrjf2LB5GxWGtZ2PVvpZoPNpaMYouDHM5wFVwKMrvshGUCrfB8wOq?=
 =?us-ascii?Q?uxZ83Er+j80o8h4P+R9ar8l8KIryLxsPo3dWuu1qHMgcvqrJMyUdVFaSTa50?=
 =?us-ascii?Q?6tZRzUi3UEKmW3CX1FStd0Y1+nxlZvNd2udMefUc407uv/kMlyt+3wTHfdkL?=
 =?us-ascii?Q?wokdYV2yyNlrpKEsTyQD7wrIcNueBh7QRbCjkUUMZaEtU6byHSsAwp+WF7eA?=
 =?us-ascii?Q?MfojNI5FHMubshhDP7/3uyVF1Yz5sr8I1DCC2l/Y?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	+/WhXFTJZJq30WduvfHobYgFARWqMaqg90mW3fKolPHWSJxpchHuhdYMoqLc3jTQ0wic3wp19rg9RUkWLJuycqn7rgX2OdNAt2A7yVUZl5fHBt7kByWh6YdFtdtwIVgAD5Y1higLg++HBlZvI8tO8sNcaUoIixa0aBalc84XnJbiK08JhSNN4fkS1Be4Otq4OZv34LkgHfEovPkZn9wcal339AeSr1Zcq+mWvXqDfmYCCTkzmaFAF63ZndF1hIaKwXG9p4R/1x0QjijtgnRmh3g4wuwqH9xr+MS9zq/6Qt/ZpBP3GzMhThoRmw9h4G6NdxDH0uOYW9OpxwVR7IkYfqdH5eNJyMkqrGIPgHKU+SbI5ZLClwtFjHdPxTOojuChBJ5BhGuuje+bxSQswOLVlvsR1rnT7CxGthz7VmARIbhEFV39I/YqGIOPdH4gAPiuDH1aKlVRDTSqcdZ3hrggwC95CqDr9w/82a2gFXwmzmMNP/JRAhw6hwz30625Tzb8/PSMRUq/BKVMq2Uo+ISHIXQ8PUYuUDs0rGHGT4ZB6ic59Ql9zqZ1FIN2tQC9RIhsXZWMbx3nUnn1JfXjwBxgce3hzQPvpB0YetVGziNjVUE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR10MB6445.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0628140c-fd5d-4eb4-714e-08dc6dd532f9
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 May 2024 14:02:43.0221
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 2bBNc5vGGIktHggbg0h2Fd5qn1v2XtazRV7PlHTch+ThkX10BqN4pim6gVV4yBEHZVj5lM5V5s3glXtEWWvNM+5n1skqdREYXtHEuciioQw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB7684
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1011,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-06_08,2024-05-06_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxscore=0 spamscore=0
 suspectscore=0 adultscore=0 phishscore=0 mlxlogscore=999 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000
 definitions=main-2405060096
X-Proofpoint-GUID: xEI5vBblAVvRZybi5WuI9WVc8bSujCkn
X-Proofpoint-ORIG-GUID: xEI5vBblAVvRZybi5WuI9WVc8bSujCkn

Thank you, Jakub.
Your comments have been addressed in the v2 patch. We've tested it internal=
ly and the patch works as expected. Please review and let us know If any ad=
ditional changes are needed.=20

-
Praveen.

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 04 May 2024 01:00 AM
> To: Praveen Kannoju <praveen.kannoju@oracle.com>
> Cc: jhs@mojatatu.com; xiyou.wangcong@gmail.com; jiri@resnulli.us; davem@d=
avemloft.net; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Rajesh Sivaramasubramaniom <rajesh.sivaramasubram=
aniom@oracle.com>; Rama Nichanamatlu
> <rama.nichanamatlu@oracle.com>; Manjunath Patil <manjunath.b.patil@oracle=
com>
> Subject: Re: [PATCH RFC] net/sched: adjust device watchdog timer to detec=
t stopped queue at right time
>=20
> On Fri, 3 May 2024 14:28:13 +0000 Praveen Kannoju wrote:
> > > >  				txq =3D netdev_get_tx_queue(dev, i);
> > > >  				trans_start =3D READ_ONCE(txq->trans_start);
> > > > -				if (netif_xmit_stopped(txq) &&
> > > > -				    time_after(jiffies, (trans_start +
> > > > -							 dev->watchdog_timeo))) {
> > > > -					timedout_ms =3D jiffies_to_msecs(jiffies - trans_start);
> > > > -					atomic_long_inc(&txq->trans_timeout);
> > > > -					break;
> > > > +				if (netif_xmit_stopped(txq)) {
> > >
> > > please use continue instead of adding another indentation level
> >
> > We need to take decision on whether to break out of loop or modify
> > "oldest_start" only when Queue is stopped. Hence one more level of
> > indentation is needed. Can you please elaborate on using "continue" in =
existing condition instead of adding a new indentation level.
>=20
> If the queue is not stopped, continue. Split the condition into multiple =
ifs.
>=20
> > > > +								   dev->watchdog_timeo))) {
> > > > +						timedout_ms =3D jiffies_to_msecs(current_jiffies -
> > > > +										trans_start);
> > > > +						atomic_long_inc(&txq->trans_timeout);
> > > > +						break;
> > > > +					}
> > > > +					next_check =3D trans_start + dev->watchdog_timeo -
> > > > +									current_jiffies;
> > >
> > > this will give us "next_check" for last queue. Let's instead find the=
 oldest trans_start in the loop. Do:
> > >
> > > 		unsigned long oldest_start =3D jiffies;
> > >
> > > then in the loop:
> > >
> > > 		oldest_start =3D min(...)
>=20
> BTW, the min() I suggested here needs to be a if (time_after(...)), we ca=
n't use bare min() to compare jiffies, because they may wrap.