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] [thread-next>] [day] [month] [year] [list]

Date: Fri, 3 May 2024 12:29:54 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Praveen Kannoju <praveen.kannoju@...cle.com>
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

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.