Re: [PATCH net v2 2/4] net: macb: drop in-flight Tx SKBs on close

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Jakub,

On Thu Apr 30, 2026 at 4:34 AM CEST, Jakub Kicinski wrote:
> On Tue, 28 Apr 2026 18:32:58 +0200 Théo Lebrun wrote:
>>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>> -		kfree(queue->tx_skb);
>> -		queue->tx_skb = NULL;
>> +		if (queue->tx_skb) {
>> +			unsigned int dropped = 0, tail;
>> +
>> +			for (tail = queue->tx_tail; tail != queue->tx_head;
>> +			     tail++) {
>> +				if (macb_tx_skb(queue, tail)->skb)
>> +					dropped++;
>> +				macb_tx_unmap(bp, macb_tx_skb(queue, tail), 0,
>> +					      SKB_DROP_REASON_NOT_SPECIFIED);
>> +			}
>> +
>> +			queue->stats.tx_dropped += dropped;
>> +			bp->dev->stats.tx_dropped += dropped;
>
> I'm slightly baffled by the stats in this driver.
>
> Incrementing of both device and queue stats is highly unusual.
> The driver seems to already have the values for the per-queue drops
> but currently never increments it (did I miss it?) It does for Rx
> stats but not for Tx stats.
>
> As sashiko correctly points out incrementing dev stats will lead
> to races and lass of increments for multi-queue devices.
>
> Since there are no increments for tx_dropped stat today - could you
> please delete it from ethtool -S, migrate the only existing
> dev->stats.tx_dropped++; to increment the per-queue stat and make 
> macb_get_stats() collect the tx_dropped from all queues, instead
> of relying on the device-level stat?
>
> This should be patch 2 in this series, and then subsequent patches
> don't have to do this double-counting dance.
>
> I suppose you may want to migrate the byte and packet counters
> while at it, and add a u64 sync...

Agreed. Here is the plan for next revision. It goes further than your
proposal on some aspects and less so on others.

 - Stop using `netdev->stats`. Not even on MACB (single queue) or from
   at91 code (single queue, custom functions for a lot of things). This
   will drop all the double-counting; I added some in this series but
   there is lot in the driver to drop.

   sed 's/netdev->stats/queue->stats/' **/macb_main.c  # -ish

 - All stats that used to land in netdev->stats will instead land in
   queue->stats. For that we need to add two fields:
    - multicast, incremented by at91ether_rx()
    - tx_errors, incremented by at91ether_interrupt()

 - Make queue->stats u64 values (getting inspiration from nstat).

 - In macb_get_stats(), replace:

      netdev_stats_to_stats64(nstat, &bp->dev->stats);

   by:

      for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
         u64_stats_fetch_begin(...);
         nstat->rx_packets += queue->stats.rx_packets;
         nstat->tx_packets += queue->stats.tx_packets;
         // ... same for all stats ...
      }

 - Also the struct name (struct queue_stats) deserves a driver prefix.

Notice we don't drop tx_dropped from `ethtool -S`. It might be useful to
get per-queue stats and it doesn't cost much. We need per-queue
counters anyway, let's keep exposing them.

I don't have time to test enough, next revision will wait next week.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com