Re: [PATCH net-next 1/3] net: Introduce netif_xmit_time_out_duration() helper

[Shahar Shitrit <shshitrit@nvidia.com>]

On 28/11/2025 3:34, Jakub Kicinski wrote:
> On Tue, 25 Nov 2025 09:12:54 +0200 Tariq Toukan wrote:
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index e808071dbb7d..3cd73769fcfa 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
> 
> include/net/net_queue.h seems like a better place for new code
I don't see this header, even in older kernels.
> 
>> @@ -3680,6 +3680,21 @@ static inline bool netif_xmit_stopped(const struct netdev_queue *dev_queue)
>>  	return dev_queue->state & QUEUE_STATE_ANY_XOFF;
>>  }
>>  
>> +static inline unsigned int
>> +netif_xmit_timeout_ms(struct netdev_queue *txq, unsigned long *trans_start)
>> +{
>> +	unsigned long txq_trans_start = READ_ONCE(txq->trans_start);
>> +
>> +	if (trans_start)
>> +		*trans_start = txq_trans_start;
> 
> The drivers don't really care about this, AFAICT hns3 uses this
> to calculate the stall length (return value of this func.
ok
> 
>> +	if (netif_xmit_stopped(txq) &&
>> +	    time_after(jiffies, txq_trans_start + txq->dev->watchdog_timeo))
>> +		return jiffies_to_msecs(jiffies - txq_trans_start);
>> +
>> +	return 0;
>> +}
>> +
>>  static inline bool
>>  netif_xmit_frozen_or_stopped(const struct netdev_queue *dev_queue)
>>  {
>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>> index 852e603c1755..aa6192781a24 100644
>> --- a/net/sched/sch_generic.c
>> +++ b/net/sched/sch_generic.c
>> @@ -523,10 +523,9 @@ static void dev_watchdog(struct timer_list *t)
>>  				 * netdev_tx_sent_queue() and netif_tx_stop_queue().
>>  				 */
>>  				smp_mb();
>> -				trans_start = READ_ONCE(txq->trans_start);
>> -
>> -				if (time_after(jiffies, trans_start + dev->watchdog_timeo)) {
>> -					timedout_ms = jiffies_to_msecs(jiffies - trans_start);
>> +				timedout_ms = netif_xmit_timeout_ms(txq,
>> +								    &trans_start);
>> +				if (timedout_ms) {
> 
> The use of the new helper in the core feels a bit forced, I'd leave 
> the core as is. Otherwise you need the awkward output param, and
> core now duplicates the netif_xmit_stopped(txq) check
ok
> 
>>  					atomic_long_inc(&txq->trans_timeout);
>>  					break;
>>  				}