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

Jakub Kicinski
2026-04-30T02:34:48Z

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...