Re: [PATCH iwl-net 0/2] igc: Fix Tx hangs after NETDEV_TX_BUSY with TSO

[Benoit 2r]

Hi Dima,

Thank you for the review and for the explanation.

Yes, going with patch 1 only is fine with me. It is the actual fix for
the problem I hit: in my tests, it removed every NETDEV_TX_BUSY return
and every Tx timeout on its own.

I agree that patch 2 is a broader question. The same pattern exists in
other Intel drivers, so it makes more sense for your team to decide
whether and how to handle it consistently across them. I'm happy to
leave that to you.

Thanks again for taking care of my contribution.

Best regards,
Benoit


Le lun. 26 oct. 2026 à 10:04, Ruinskiy, Dima <dima.ruinskiy@xxxxxxxxx> a écrit :
>
> On 04/10/2026 18:28, Benoit DE RANCOURT wrote:
> > Since commit db0b124f02ba ("igc: Enhance Qbv scheduling by using first
> > flag bit"), igc_xmit_frame_ring() requires count + 5 free descriptors,
> > while the queue is only stopped in advance below DESC_NEEDED =
> > MAX_SKB_FRAGS + 4. TSO skbs with 16 or 17 fragments then hit
> > NETDEV_TX_BUSY. That return skips the tail write deferred by
> > xmit_more, so the queue can stay stopped with descriptors the hardware
> > never saw, until the Tx watchdog resets the adapter.
> >
> > Patch 1 aligns DESC_NEEDED with the admission check. Patch 2 writes the
> > tail before returning NETDEV_TX_BUSY, which remains reachable for skbs
> > with buffers larger than IGC_MAX_DATA_PER_TXD. Each patch was tested
> > alone; together they keep the queue stop logic consistent and the
> > remaining busy path safe.
> >
> > Setup: CWWK router, Pentium Gold 8505, six I226-V (8086:125c rev 04,
> > NVM 2017:888d), 7.2.8, default MAX_SKB_FRAGS = 17. Load: CPU saturated
> > with busy loops, a remote build routed through the port, two SSH bulk
> > streams in each direction and short TCP bursts, 180 seconds of load
> > per run observed during a 195-second probe window, TSO enabled on the
> > egress port. bpftrace probes on igc_xmit_frame, __igc_maybe_stop_tx
> > and igc_poll recorded NETDEV_TX_BUSY returns, the inferred last tail
> > write and next_to_clean stalls.
> >
> >                         runs  Tx timeouts  NETDEV_TX_BUSY  TX_OK returns
> >    unpatched 7.2.8         3           25           10890       7.3M
> >    patch 2 only            3            0           10751       9.5M
> >    patch 1 only            3            0               0       8.8M
> >    both patches            3            0               0       9.4M
> >    unpatched, TSO off      1            0               0      21.5M
> >
> > In all 25 timeout episodes, the stalled queue had pending descriptors
> > after a NETDEV_TX_BUSY return. The probes inferred at least 214
> > descriptors beyond the last tail write. For that queue, the hardware
> > register dump showed TDH = TDT = the next_to_clean value recorded by
> > the probe.
> >
> > With both patches applied, no NETDEV_TX_BUSY occurred, so the flush
> > path of patch 2 was not exercised in that run; its effect is shown by
> > the patch 2 only runs.
> >
> > TX_OK returns count igc_xmit_frame() calls returning NETDEV_TX_OK,
> > excluding NETDEV_TX_BUSY retries. These are software call counts,
> > not hardware packet counters; with TSO disabled, segmentation occurs
> > before the driver.
> >
> > Related code, not addressed in this series and not tested:
> >
> > - bnxt: commit e8d8c5d80f5e ("bnxt: make sure xmit_more + errors does
> >    not miss doorbells") rings a pending doorbell on the drop paths. It
> >    leaves the busy path alone, reasoning that busy can only happen if
> >    start_xmit races with completions that both enable the queue, in
> >    which case no kick can be pending; the current code warns "ring busy
> >    w/ flush pending!" if that assumption breaks. In igc, patch 1
> >    restores that property for skbs whose buffers each fit in one
> >    descriptor, and patch 2 covers the remaining case.
> >
> > - igc drop paths (skb_put_padto() failure in igc_xmit_frame(), out_drop
> >    in igc_xmit_frame_ring(), the DMA mapping error path of igc_tx_map())
> >    also return without writing a pending tail, the case bnxt fixed in
> >    e8d8c5d80f5e and 00eeab0c644a ("bnxt_en: Write doorbell when
> >    linearizing skb fails"). The queue is not stopped there, so pending
> >    descriptors are delayed until the next transmit on that queue rather
> >    than stranded.
> >
> > - igb, ixgbe, fm10k, i40e, iavf, ice, e1000e and e1000 also defer the
> >    tail write with xmit_more and return NETDEV_TX_BUSY from their
> >    admission check without writing it. Their stop thresholds match
> >    their admission checks, so this should only be reachable for skbs
> >    needing more descriptors than the stop threshold assumes.
> >
> > Not done:
> > - runtime test on a kernel built from this tree; the patches were
> >    tested on 7.2.8, where the affected code is identical, and apply
> >    cleanly here;
> > - launch time (ETF/taprio) and XDP/AF_XDP traffic, which share the
> >    ring and the wake threshold;
> > - longer runs and other I226/I225 boards;
> > - only the igc objects were built with W=1 under allmodconfig, not
> >    the full tree.
> >
> > The analysis and the patches were prepared with an LLM-based coding
> > assistant, which also wrote the bpftrace probes; the results above come
> > from those probes and the kernel log on real hardware.
> >
> > Benoit DE RANCOURT (2):
> >    igc: Fix Tx stop threshold to cover empty frame descriptors
> >    igc: Flush pending Tx descriptors before returning NETDEV_TX_BUSY
> >
> >   drivers/net/ethernet/intel/igc/igc.h      | 8 ++++++--
> >   drivers/net/ethernet/intel/igc/igc_main.c | 6 +++++-
> >   2 files changed, 11 insertions(+), 3 deletions(-)
> >
> >
> > base-commit: a83267db14681b3be481e02a4d5a38177507006c
>
> Hi Benoit,
> Thank you for this fix. Much appreciated.
>
> I wonder if we can go just with patch 1 for now. Reasons:
> - As per the analysis, just Patch 1 resolves all the issues encountered
> (the corner case resolved by patch 2 is not reached anymore, although it
> remains theoretically possible)
> - As explained in the cover letter, Patch 2 changes the behavior in a
> way that makes it inconsistent with what other drivers are doing. Maybe
> the new behavior is more correct in all cases, maybe not, but in general
> I prefer to avoid introducing inconsistencies, especially when I don't
> fully understand the effects.
>
> Best,
> Dima