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

[Nicolai Buchwitz]

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