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

[Nicolai Buchwitz <nb@tipi-net.de>]

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