linux-kernel - RE: [PATCH RFC] net/sched: adjust device watchdog timer to detect stopped queue at right time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Mon, 6 May 2024 14:02:42 +0000
From: Praveen Kannoju <praveen.kannoju@...cle.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "jhs@...atatu.com" <jhs@...atatu.com>,
        "xiyou.wangcong@...il.com"
	<xiyou.wangcong@...il.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        Rajesh Sivaramasubramaniom
	<rajesh.sivaramasubramaniom@...cle.com>,
        Rama Nichanamatlu
	<rama.nichanamatlu@...cle.com>,
        Manjunath Patil
	<manjunath.b.patil@...cle.com>
Subject: RE: [PATCH RFC] net/sched: adjust device watchdog timer to detect
 stopped queue at right time

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

-
Praveen.

> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: 04 May 2024 01:00 AM
> To: Praveen Kannoju <praveen.kannoju@...cle.com>
> Cc: jhs@...atatu.com; xiyou.wangcong@...il.com; jiri@...nulli.us; davem@...emloft.net; netdev@...r.kernel.org; linux-
> kernel@...r.kernel.org; Rajesh Sivaramasubramaniom <rajesh.sivaramasubramaniom@...cle.com>; Rama Nichanamatlu
> <rama.nichanamatlu@...cle.com>; Manjunath Patil <manjunath.b.patil@...clecom>
> Subject: Re: [PATCH RFC] net/sched: adjust device watchdog timer to detect stopped queue at right time
> 
> On Fri, 3 May 2024 14:28:13 +0000 Praveen Kannoju wrote:
> > > >  				txq = netdev_get_tx_queue(dev, i);
> > > >  				trans_start = READ_ONCE(txq->trans_start);
> > > > -				if (netif_xmit_stopped(txq) &&
> > > > -				    time_after(jiffies, (trans_start +
> > > > -							 dev->watchdog_timeo))) {
> > > > -					timedout_ms = 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.
> 
> If the queue is not stopped, continue. Split the condition into multiple ifs.
> 
> > > > +								   dev->watchdog_timeo))) {
> > > > +						timedout_ms = jiffies_to_msecs(current_jiffies -
> > > > +										trans_start);
> > > > +						atomic_long_inc(&txq->trans_timeout);
> > > > +						break;
> > > > +					}
> > > > +					next_check = 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 = jiffies;
> > >
> > > then in the loop:
> > >
> > > 		oldest_start = min(...)
> 
> BTW, the min() I suggested here needs to be a if (time_after(...)), we can't use bare min() to compare jiffies, because they may wrap.