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

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

[Benoit DE RANCOURT]

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
-- 
2.55.0

[PATCH iwl-net 1/2] igc: Fix Tx stop threshold to cover empty frame descriptors

[Benoit DE RANCOURT]

Commit db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag
bit") raised the number of free descriptors that igc_xmit_frame_ring()
requires before mapping a packet from count + 3 to count + 5, to leave
room for the empty frame (one context and one data descriptor) that may
be inserted ahead of a launch time packet. DESC_NEEDED was left at
MAX_SKB_FRAGS + 4. igc_tx_map() uses it to stop the queue in advance,
and TX_WAKE_THRESHOLD is derived from it.

A queue left running after a transmit therefore only guarantees
MAX_SKB_FRAGS + 4 free descriptors, while the next skb with a linear
part and MAX_SKB_FRAGS fragments, each fitting in one data descriptor,
requires MAX_SKB_FRAGS + 6. igc_xmit_frame_ring() then returns
NETDEV_TX_BUSY, which the queue stop logic is meant to prevent.

With TSO this happens routinely. On an I226-V (8086:125c) running 7.2.8
with MAX_SKB_FRAGS = 17, routed and locally generated TCP traffic on a
CPU-saturated router produced 10890 NETDEV_TX_BUSY returns in three
runs of 180 seconds, all for TSO skbs with 16 or 17 fragments, with
21 or 22 free descriptors in all but 5 cases. Together with deferred
tail writes (xmit_more), these returns led to 25 Tx timeouts and
adapter resets in the same runs:

  igc 0000:08:00.0 terra: NETDEV WATCHDOG: CPU: 1: transmit queue 2 timed out 5353 ms
  igc 0000:08:00.0 terra: Reset adapter

The following patch addresses the lost tail write itself.

Raise DESC_NEEDED to MAX_SKB_FRAGS + 6 to match the admission check.
The additional reservation is currently unconditional, including when
launch time is disabled; match that existing admission policy in the
proactive stop threshold. With MAX_SKB_FRAGS = 17, the queue is now
stopped below 23 free descriptors instead of 21. This also raises the
completion-based wake threshold from 42 to 46 free descriptors,
preserving the existing two-times-stop-threshold policy. Throughput
and latency effects were not measured.

With this change alone, three runs of the same test on 7.2.8 produced
no NETDEV_TX_BUSY and no Tx timeout over 8.8M NETDEV_TX_OK returns,
including 613k TSO skbs with 17 fragments.

As before commit db0b124f02ba, an skb whose linear part or fragments
exceed IGC_MAX_DATA_PER_TXD may still need more descriptors than
DESC_NEEDED accounts for.

Fixes: db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag bit")
Cc: stable@vger.kernel.org
Assisted-by: LLM bpftrace
Signed-off-by: Benoit DE RANCOURT <b2rancourt@gmail.com>
---
 drivers/net/ethernet/intel/igc/igc.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 17f213cc93e4..f1efadbd9f74 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -575,9 +575,13 @@ enum igc_boards {
 #define IGC_MAX_TXD_PWR		15
 #define IGC_MAX_DATA_PER_TXD	BIT(IGC_MAX_TXD_PWR)
 
-/* Tx Descriptors needed, worst case */
 #define TXD_USE_COUNT(S)	DIV_ROUND_UP((S), IGC_MAX_DATA_PER_TXD)
-#define DESC_NEEDED	(MAX_SKB_FRAGS + 4)
+
+/* Tx descriptor budget for a head and MAX_SKB_FRAGS fragments,
+ * each fitting in one data descriptor: one context descriptor,
+ * two descriptors for an optional empty frame, and two spare entries.
+ */
+#define DESC_NEEDED	(MAX_SKB_FRAGS + 6)
 
 struct igc_rx_buffer {
 	union {

base-commit: a83267db14681b3be481e02a4d5a38177507006c
-- 
2.55.0

[PATCH iwl-net 2/2] igc: Flush pending Tx descriptors before returning NETDEV_TX_BUSY

[Benoit DE RANCOURT]

When the stack sends a burst with xmit_more set, igc_tx_map() defers
the tail register write until the last skb of the burst or until the
queue is stopped. If igc_xmit_frame_ring() then refuses an skb with
NETDEV_TX_BUSY, it returns without flushing any pending tail write.
Descriptors of packets already accepted in the burst can therefore
remain unexposed to the hardware.

The queue is stopped at that point and igc_clean_tx_irq() only wakes
it once TX_WAKE_THRESHOLD descriptors are free. The hardware can only
complete what the tail exposes, so if the unsignalled descriptors keep
the free count below the threshold, the queue is never woken and the
Tx watchdog resets the adapter.

On an I226-V running 7.2.8, in all 25 Tx timeout episodes of the test
described in the previous patch, the stalled queue had pending
descriptors after a NETDEV_TX_BUSY return: the probes inferred at
least 214 of the 256 ring descriptors beyond the last tail write. For
that queue, the register dump showed TDH = TDT = the next_to_clean
value recorded by the probe: the hardware had completed everything it
had been given. For example, with queue 2 stopped, next_to_clean = 190
and next_to_use = 167:

  igc 0000:08:00.0 terra: NETDEV WATCHDOG: CPU: 1: transmit queue 2 timed out 5353 ms
  igc 0000:08:00.0 terra: TDH[0-3]        00000099 00000091 000000be 000000f5
  igc 0000:08:00.0 terra: TDT[0-3]        00000099 00000091 000000be 000000f5

Write the tail before returning NETDEV_TX_BUSY. The refused skb is
left untouched; only descriptors of already accepted packets are
exposed to the hardware. Those packets have already been accounted to
BQL, so flushing their descriptors lets completion processing
progress; the refused skb is neither mapped nor accounted here.

The previous patch makes this path rare again for common skbs, but it
remains reachable, for instance when the linear part or a fragment of
an skb exceeds IGC_MAX_DATA_PER_TXD.

With this change alone (without the previous patch), three runs of the
same test on 7.2.8 still produced 10751 NETDEV_TX_BUSY returns, but no
Tx timeout, against 25 on the unpatched kernel.

Fixes: 0507ef8a0372 ("igc: Add transmit and receive fastpath and interrupt handlers")
Cc: stable@vger.kernel.org
Assisted-by: LLM bpftrace
Signed-off-by: Benoit DE RANCOURT <b2rancourt@gmail.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index b94a08791102..16f2ca32f5f1 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1621,7 +1621,11 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 						&skb_shinfo(skb)->frags[f]));
 
 	if (igc_maybe_stop_tx(tx_ring, count + 5)) {
-		/* this is a hard error */
+		/* This is a hard error. Write the tail for packets deferred
+		 * by xmit_more before returning busy, otherwise the stopped
+		 * queue may never get enough completions to be woken.
+		 */
+		igc_flush_tx_descriptors(tx_ring);
 		return NETDEV_TX_BUSY;
 	}
 
-- 
2.55.0