Date: Tue, 04 Aug 2026 17:15:23 +0000 From: Alexander Leidinger <netchild@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 69c3e0de01c1 - main - iflib: restore TX watchdog functionality Message-ID: <6a721e2b.222b2.69faa28e@gitrepo.freebsd.org>
index | next in thread | raw e-mail
The branch main has been updated by netchild: URL: https://cgit.FreeBSD.org/src/commit/?id=69c3e0de01c1938792d319f18ba0a9ffa60dfa96 commit 69c3e0de01c1938792d319f18ba0a9ffa60dfa96 Author: Alexander Leidinger <netchild@FreeBSD.org> AuthorDate: 2026-08-04 16:44:52 +0000 Commit: Alexander Leidinger <netchild@FreeBSD.org> CommitDate: 2026-08-04 17:13:43 +0000 iflib: restore TX watchdog functionality Since f6afed726b00 the TX-hang check in iflib_timer() has required a queue state other than IFLIB_QUEUE_IDLE, but nothing ever sets IFLIB_QUEUE_WORKING, so IFLIB_QUEUE_HUNG has been unreachable ever since: stalled TX queues are not detected, not reported, and not reset - the TX watchdog of every iflib(4) driver has been dead code. Instead of resurrecting the queue-state machine, detect the hang directly. A transmit queue is frozen while it holds descriptors the hardware has not reported as completed and none were reclaimed over a timer period. Being frozen is not a fault: the hardware may defer marking descriptors as completed indefinitely. The check therefore arms only when a frozen queue also takes on new work, while the link is up, no pause frames were received and no doorbell is pending; and it acts only after the queue has stayed frozen for net.iflib.tx_watchdog_periods consecutive periods. It then asks the hardware through the driver's read-only credits peek (isc_txd_credits_update with clear=false, the same call the mp_ring can_drain callback makes routinely): if completions are ready but were not harvested for this long, the completion interrupt went missing - kick the queue's task instead of resetting; if the hardware reports nothing although the queue kept receiving work, it is hung and the existing watchdog reset machinery takes over. Neither software counters alone nor mere persistence of unharvested work can make this decision. iflib reclaims lazily (up to isc_tx_nsegments completed descriptors stay unharvested indefinitely) and defers report-status requests, so "descriptors in use" and "no cleaning progress" are normal states of an idle healthy queue. And hardware that coalesces completion reports (e.g. 8254x, TXDCTL.WTHRESH) legitimately withholds the last one of a quiet queue indefinitely, so a zero credits peek is a normal idle state, not a hang indicator: arming on persistence alone reset healthy interfaces on every traffic lull (field-tested on 82541PI). Only growth across frozen periods separates a wedged queue from a coalescing one. The threshold is a threshold in time, not in device work: a period is one iflib_timer interval (hz/2 by default), so at the default of four periods the verdict falls after roughly two seconds. It was calibrated from counter traces on that old and slow hardware, where healthy coalescing always cleared within two periods; newer hardware reports completions far sooner and leaves the frozen state earlier, so the default needs no recalibration for more modern devices. Setting the sysctl to zero disables the check. A queue whose link is down is never flagged - preserving what f6afed726b00 fixed. The new per-queue state goes into padding the transmit queue structure already had, rather than next to the counters it is derived from: that region is packed, so an insertion there would grow the structure. What is left of that padding is now spelled out instead of being implicit. The size of the structure is unchanged on amd64, arm64, riscv64, i386 and armv7. The IFLIB_QUEUE_* states no longer participate in the watchdog decision; they will be removed in a followup commit. PR: 220997, 239240 Fixes: f6afed726b00 ("iflib: Prevent watchdog from resetting idle queues") Suggested by: gallatin (mxge-style detection) Reviewed by: adrian, markj MFC after: 1 month Differential Revision: https://reviews.freebsd.org/D58266 Assisted-by: Claude Code (Fable 5, Opus 5) --- share/man/man4/iflib.4 | 11 +++- sys/net/iflib.c | 135 +++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 118 insertions(+), 28 deletions(-) diff --git a/share/man/man4/iflib.4 b/share/man/man4/iflib.4 index 349fa402d319..b7e09de40875 100644 --- a/share/man/man4/iflib.4 +++ b/share/man/man4/iflib.4 @@ -1,4 +1,4 @@ -.Dd January 7, 2026 +.Dd August 4, 2026 .Dt IFLIB 4 .Os .Sh NAME @@ -128,6 +128,15 @@ Some NICs allow processing completed transmit descriptors in batches. Doing so usually increases the transmit throughput by reducing the number of transmit interrupts. Setting this to a non-zero value will disable the use of this feature. +.It Va net.iflib.tx_watchdog_periods +Number of consecutive +.Va net.iflib.timer_default +intervals for which a transmit queue must stay frozen, +that is, hold descriptors the hardware has not reported as completed +while none are reclaimed, +before iflib asks the hardware whether it has completions pending. +If it reports none, the interface is reset. +Setting this to zero disables the check. .El .Pp These diff --git a/sys/net/iflib.c b/sys/net/iflib.c index b6a8ec1eb6ce..1b43f5833d57 100644 --- a/sys/net/iflib.c +++ b/sys/net/iflib.c @@ -374,7 +374,7 @@ struct iflib_txq { uint8_t ift_txd_size[8]; uint64_t ift_processed; uint64_t ift_cleaned; - uint64_t ift_cleaned_prev; + uint64_t ift_processed_prev; #if MEMORY_LOGGING uint64_t ift_enqueued; uint64_t ift_dequeued; @@ -404,9 +404,20 @@ struct iflib_txq { #endif /* DEV_NETMAP */ if_txsd_vec_t ift_sds; + /* + * TX watchdog state, updated once per iflib_timer period. The + * period count saturates instead of wrapping, and is 16 bits + * wide so that it still reaches any value + * net.iflib.tx_watchdog_periods is plausibly set to; an 8-bit + * counter would silently disable the check for a threshold + * above 255. + */ + qidx_t ift_outstanding_prev; + uint16_t ift_wdog_armed; uint8_t ift_qstatus; uint8_t ift_closed; uint8_t ift_update_freq; + uint8_t ift_spare0; /* pad to the next pointer boundary */ struct iflib_filter_info ift_filter_info; bus_dma_tag_t ift_buf_tag; bus_dma_tag_t ift_tso_buf_tag; @@ -569,6 +580,20 @@ SYSCTL_INT(_net_iflib, OID_AUTO, no_tx_batch, CTLFLAG_RW, static int iflib_timer_default = 1000; SYSCTL_INT(_net_iflib, OID_AUTO, timer_default, CTLFLAG_RW, &iflib_timer_default, 0, "number of ticks between iflib_timer calls"); +/* + * Consecutive timer periods a TX queue must stay frozen - see + * iflib_timer(), which defines that state - before the hardware is + * asked whether it has completions pending. Four periods is roughly + * two seconds with the default timer interval: a healthy queue on + * hardware that coalesces completion reports (e.g. 8254x, + * TXDCTL.WTHRESH) stays frozen for at most two (measured on 82541PI), + * a wedged one until it is reset. + */ +static int iflib_tx_watchdog_periods = 4; +SYSCTL_INT(_net_iflib, OID_AUTO, tx_watchdog_periods, CTLFLAG_RWTUN, + &iflib_tx_watchdog_periods, 0, + "consecutive frozen timer periods before a TX queue is checked for " + "a hang (0 disables the check)"); #if IFLIB_DEBUG_COUNTERS @@ -2403,25 +2428,89 @@ iflib_timer(void *arg) return; /* - ** Check on the state of the TX queue(s), this - ** can be done without the lock because its RO - ** and the HUNG state will be static if set. - */ + * Check on the state of the TX queue(s); this can be done + * without the lock: the counters the check reads are only + * advanced by the queue's tx task and a stale read just + * delays the verdict by one timer period. + */ if (this_tick - txq->ift_last_timer_tick >= iflib_timer_default) { + qidx_t outstanding; + bool frozen; + txq->ift_last_timer_tick = this_tick; IFDI_TIMER(ctx, txq->ift_id); - if ((txq->ift_qstatus == IFLIB_QUEUE_HUNG) && - ((txq->ift_cleaned_prev == txq->ift_cleaned) || - (sctx->isc_pause_frames == 0))) - goto hung; - - if (txq->ift_qstatus != IFLIB_QUEUE_IDLE && - ifmp_ring_is_stalled(txq->ift_br)) { - KASSERT(ctx->ifc_link_state == LINK_STATE_UP, - ("queue can't be marked as hung if interface is down")); - txq->ift_qstatus = IFLIB_QUEUE_HUNG; + + /* + * Descriptors the hardware has not reported as + * completed: neither harvested as credits + * (ift_processed) nor reclaimed (ift_cleaned accounts + * the difference to ift_in_use). The tail whose + * report-status request is still deferred is never + * reported and must not count (ift_rs_pending + * over-counts it by one per packet). + */ + outstanding = txq->ift_in_use - + (qidx_t)(txq->ift_processed - txq->ift_cleaned); + + /* + * The queue is frozen while it has descriptors the + * hardware has not reported as completed and none + * were reclaimed over the period; the link must be + * up, with no pause frames and no pending doorbell + * (the laggard check below rings it). + * + * Being frozen is not a fault - the hardware may + * defer marking descriptors as completed + * indefinitely, and 8254x hardware does so for a + * quiet queue - therefore the check arms only when a + * frozen queue also takes on new work, and acts only + * once it has stayed frozen for + * net.iflib.tx_watchdog_periods consecutive periods. + */ + frozen = outstanding > txq->ift_rs_pending && + txq->ift_processed == txq->ift_processed_prev && + txq->ift_db_pending == 0 && + sctx->isc_pause_frames == 0 && + ctx->ifc_link_state == LINK_STATE_UP; + if (!frozen) + txq->ift_wdog_armed = 0; + else if (txq->ift_wdog_armed > 0 || + outstanding > txq->ift_outstanding_prev) { + if (txq->ift_wdog_armed < UINT16_MAX) + txq->ift_wdog_armed++; } - txq->ift_cleaned_prev = txq->ift_cleaned; + + /* + * Frozen long enough: ask the hardware. Completions + * ready but unharvested for this long mean the + * completion interrupt went missing - kick the + * queue's task. Nothing ready, although the queue + * kept taking on work, means it is hung. + */ + if (iflib_tx_watchdog_periods > 0 && + txq->ift_wdog_armed >= iflib_tx_watchdog_periods) { + bus_dmamap_sync(txq->ift_ifdi->idi_tag, + txq->ift_ifdi->idi_map, BUS_DMASYNC_POSTREAD); + if (ctx->isc_txd_credits_update(ctx->ifc_softc, + txq->ift_id, false) == 0) { + device_printf(ctx->ifc_dev, + "Watchdog timeout (TX: %d desc " + "avail: %d pidx: %d) -- resetting\n", + txq->ift_id, TXQ_AVAIL(txq), + txq->ift_pidx); + STATE_LOCK(ctx); + if_setdrvflagbits(ctx->ifc_ifp, + IFF_DRV_OACTIVE, IFF_DRV_RUNNING); + ctx->ifc_flags |= + (IFC_DO_WATCHDOG | IFC_DO_RESET); + iflib_admin_intr_deferred(ctx); + STATE_UNLOCK(ctx); + return; + } + GROUPTASK_ENQUEUE(&txq->ift_task); + } + txq->ift_outstanding_prev = outstanding; + txq->ift_processed_prev = txq->ift_processed; } /* handle any laggards */ if (txq->ift_db_pending) @@ -2431,17 +2520,6 @@ iflib_timer(void *arg) if (if_getdrvflags(ctx->ifc_ifp) & IFF_DRV_RUNNING) callout_reset_on(&txq->ift_timer, iflib_timer_default, iflib_timer, txq, txq->ift_timer.c_cpu); - return; - - hung: - device_printf(ctx->ifc_dev, - "Watchdog timeout (TX: %d desc avail: %d pidx: %d) -- resetting\n", - txq->ift_id, TXQ_AVAIL(txq), txq->ift_pidx); - STATE_LOCK(ctx); - if_setdrvflagbits(ctx->ifc_ifp, IFF_DRV_OACTIVE, IFF_DRV_RUNNING); - ctx->ifc_flags |= (IFC_DO_WATCHDOG | IFC_DO_RESET); - iflib_admin_intr_deferred(ctx); - STATE_UNLOCK(ctx); } static uint16_t @@ -2625,6 +2703,9 @@ iflib_stop(if_ctx_t ctx) iflib_txsd_free(ctx, txq, j); } txq->ift_processed = txq->ift_cleaned = txq->ift_cidx_processed = 0; + txq->ift_processed_prev = 0; + txq->ift_outstanding_prev = 0; + txq->ift_wdog_armed = 0; txq->ift_in_use = txq->ift_gen = txq->ift_no_desc_avail = 0; if (sctx->isc_flags & IFLIB_PRESERVE_TX_INDICES) txq->ift_cidx = txq->ift_pidx;home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6a721e2b.222b2.69faa28e>