Re: [PATCH RFC] net/sched: adjust device watchdog timer to detect stopped queue at right time

[Jakub Kicinski]

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.