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

Théo Lebrun
2026-04-30T16:20:17Z

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