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

Nicolai Buchwitz
2026-04-30T07:14:35Z

Hi Théo and Jacub

On 30.4.2026 04:34, 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?

Would make sense, yes. While we're already cleaning this up, two
more things possibly worth touching:

1. macb_start_xmit() drops the skb on macb_clear_csum() and
macb_pad_and_fcs() failures without counting it. Both could
use a tx_dropped++.

2. tx_packets / tx_bytes already increment per-queue but never
rach nstat (rx side too). Could just pick them up in the same
loop.

> [...]

Thanks,
Nicolai