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

["Ruinskiy, Dima" <dima.ruinskiy@intel.com>]

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