Page MenuHomeFreeBSD

iflib: restore TX watchdog functionality
ClosedPublic

Authored by netchild on Wed, Jul 15, 7:31 PM.
Tags
None
Referenced Files
F166087722: D58266.id183195.diff
Tue, Aug 11, 3:21 PM
F166086195: D58266.id182075.diff
Tue, Aug 11, 3:07 PM
F166056821: D58266.id183391.diff
Tue, Aug 11, 10:01 AM
F166040954: D58266.id182050.diff
Tue, Aug 11, 7:33 AM
F166024774: D58266.diff
Tue, Aug 11, 3:49 AM
F166014768: D58266.id183099.diff
Tue, Aug 11, 1:29 AM
F165998844: D58266.id183195.diff
Mon, Aug 10, 9:59 PM
F165978028: D58266.id182220.diff
Mon, Aug 10, 7:10 PM

Details

Summary

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)
MFC after: 1 month
Assisted-by: Claude Code (Fable 5, Opus 5)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

netchild held this revision as a draft.

Thank you, I was looking at this too.

sys/net/iflib.c
4042

What is "escalation"? It's a watchdog timer.

4043

This seems inadequate: suppose we enqueue two packets for transmission, and the queue transmits the first one, and we reclaim that descriptor. Then we will mark the queue as idle even though there is a packet pending, so the watchdog will never fire, no?

I guess in practice we won't hit that because this function is typically where we transition WORKING->IDLE (above, on line 3909), and this function will usually enqueue at least one more packet for transmission, but it's still kind of wrong.

The whole watchdog thing, like mp_ring, is absurdly over-complicated for no real reason. Just do it the mxge way: if we have pending transmits (eg, producer != consumer) *AND* the nic has not consumed any packets since the last time the watchdog callout ran, then reset. I get dizzy every time i look at the iflib queue-state stuff. Its just extra complexity that adds nothing.

Switch to the mxge way. Mxge seems to harvest the completions
eagerly per interrupt, while iflib seems to reclaim lazy. The
software counter of iflib would get into the watchdog firing
situation on a lightly loaded interface... if I get it correctly.

With this I get watchdog resets on the system which has issues
and which triggered this investigation.

This fires too often, maybe some false positives. I will rework.

This fires too often, maybe some false positives. I will rework.

I've frankly lost track of how exactly the mp_ring path cleans things since I've been entirely focused on the simple_tx path. In that context, i can see how the mxge method would lead to false positives. It would be great if you could also test your changes with dev.$DRIVER.$UNIT.iflib.simple_tx=1. I plan to make that the default and hopefully remove the overly complicated mp_ring path entirely, I think removing mp_ring will make iflib much simpler and easier to maintain

New tested version, on hardware which triggers the watchdog.

Results from the affected hardware (em0, 82541PI), ~44 h
on a v3 kernel. Summary: 16 watchdog fires, all true positives, 0 false
positives.

@gallatin — I did try the plain mxge predicate first (pidx != cidx with
no hardware progress between watchdog cycles); that was the previous
version of this diff and it FAILED its field test. On the 8254x, WTHRESH
writeback coalescing withholds the final report-status writeback on a
quiet queue, so a perfectly healthy idle queue shows pidx != cidx with no
visible DD for seconds at a time. Result was a reset storm: fires every
~4.4 s through nighttime lulls.

The growth arming is what makes that verdict trustworthy. Arming requires
NEW work (unharvested writebacks grew), and new completions flush the
previously withheld writebacks, so by verdict time a healthy queue always
has visible DD. Measured: over a 13.2 h night (92169 samples) the kernel
reached the verdict ~100 times on healthy hardware and EVERY peek found
credits > 0 — never a reset. peek == 0 after growth means nothing came
back despite new work, i.e. a genuine hang.

On simple_tx specifically, your case is stronger than mine: I re-ran the
full six-regime drip matrix under dev.em.0.iflib.simple_tx=1 and the
predicate never armed at all — max_armed = 0 in every regime, vs. a peak
of 2 under mp_ring (threshold is 4). The inline reclaim in
iflib_simple_transmit advances ift_processed in the same period as any
growth, so the frozen condition cannot hold. IFLIB_WDOG_ARMED_PERIODS=4
is calibrated by the mp_ring worst case and needs no retune when
simple_tx becomes the default.

Related, and it argues for your removal plan: the armed kick
(GROUPTASK_ENQUEUE) is effectively a reclaim no-op under mp_ring — with
ift_db_pending == 0, _task_fn_tx only calls ifmp_ring_check_drainage(),
which returns immediately on an idle ring (mp_ring.c:475). No correctness
issue (the next transmit reclaims; the queue is ~2% occupied), but the
lost-interrupt recovery is best-effort there. Under simple_tx,
_task_fn_tx calls iflib_completed_tx_reclaim() directly
(iflib.c:4094-4098), so the recovery path becomes fully functional once
mp_ring goes away.

@markj — the "mark the queue idle on partial reclamation" concern no
longer applies: v3 does not track queue state at all. D58282 deletes
ift_qstatus and the IFLIB_QUEUE_* states outright. The predicate is now
just (link up && unharvested writebacks grew && stayed frozen for 4 timer
periods) -> peek the descriptors for credits. Nothing is marked idle, so
there is no partial-reclaim edge case to miss.

True-positive data, from a 48 h soak: night 1 clean (0 fires). Night 2 had
four spontaneous wedge episodes (8/3/1/4 fires); the watchdog self-healed
three of them in <=35 s each. The fourth was reinit-resistant — repeated
resets did not cure it and the box needed a reboot. I have seen that shape
twice now (a separate episode took 48 fires without curing), so I would
like input on whether this should carry a reset backoff or a cap rather
than retrying indefinitely; I did not want to bundle a policy decision
into this diff.

Negative control: after that reboot the box sat through 7+ h of
provider-side IPv4-only loss to the gateway (50-100% loss, v6 clean, em0
counters healthy) with watchdog_timeouts = 0. The predicate does not fire
on external loss.

This in general looks good to me, thanks for tackling it!

sys/net/iflib.c
322

I suspect this should be a sysctl, like net.iflib.timer_default is.

372

Why did you put the fields here specifically? This is leaving a hole.

sys/net/iflib.c
372

They are put next to the vars which are related.

Yes, this adds padding, but the total size of the struct does not change. There is pre-existing padding. In the updated patch I made the padding explicit. The i386 size changes, but goes back with the follow-up patch to remove the "old" watchdog variables.

netchild edited the summary of this revision. (Show Details)

Move the variables instruct iflib_txq to a place which causes less padding. Make the existing padding in the struct visible. Checked for amd64, riscv64, arm64, and i386 (only arch where the sizeof() changes, follow-up commit to remove the "old" watchdog parts changes the size back.

netchild added inline comments.
sys/net/iflib.c
322

Changed to a sysctl.

The comments are simultaneously too verbose and not very helpful. They consistently refer to "writeback", which isn't mentioned anywhere else in iflib and only one or twice in the e1000 drivers. A couple of them imply that a queue being frozen for X timer periods is hung, but that's not true, as a different comment explains.

I think the whole mechanism can be described pretty simply: a transmit queue is frozen if there are some pending, uncompleted transmissions on the queue; the hardware is allowed to indefinitely defer marking tx descriptors as completed; thus, the watchdog fires when 1) the queue is frozen 2) iflib_tx_watchdog_periods timer ticks elapse where the number of pending descriptors goes up without any reclamation of completed descriptors.

sys/net/iflib.c
580

Probably this should be initialized to hz, btw. On systems with hz=100 the timer interval will be 10s.

2522–2523

I do not see why this code block is here, instead of where the goto hung statement is.

netchild added inline comments.
sys/net/iflib.c
580

Pre-existing code. I prefer to not touch that part in the watchdog patch.
Note, I think iflib_module_init() overwrites it with hz / 2 before any driver attaches.

2522–2523

Second goto went away with f6afed726b00, I will move that into the only place left above then.

netchild edited the summary of this revision. (Show Details)

Comments reworded, a part moved into the commit log. The single (after f6afed726b00 ) goto moved into the one place where it matters.

@markj iflib_timer_default deliberately not changed, see the comment there. Are the source comments ok for you now (commit ready?)?

I think the patch is ok with my comments addressed.

sys/net/iflib.c
317

I don't see the point of adding this comment, isn't D58282 going to remove it immediately?

591

This comment should reference iflib_timer(), where "frozen" is defined.

"Zero disables the check" is just repeating text from the sysctl description.

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Aug 4, 5:15 PM
This revision was automatically updated to reflect the committed changes.