iflib: restore TX watchdog functionality

[netchild]

Summary

Since f6afed726b00 the TX-hang escalation 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.

Mark a queue as working when the transmit path has handed new
descriptors to the hardware, and only from the idle state, so that
enqueueing more work never demotes a queue already marked hung; only
completion progress may do that. When escalating a stalled queue to
hung, check the link state explicitly instead of asserting it: the
timer deliberately runs unlocked and can race the link-down demotion,
and that benign race would trip the assertion on an INVARIANTS
kernel.

The demotions to idle (reclaim progress, link down, controller stop)
are unchanged and the drain path cannot hand out descriptors while
the link is down, so a link-down queue still cannot be marked hung -
preserving what f6afed726b00 fixed. With this change a stalled TX
queue is again marked hung within one timer period and reset by the
existing watchdog machinery one period later, restoring for all
iflib(4) drivers the behavior lost in 2020.

PR: 220997, 239240
Fixes: f6afed726b00 ("iflib: Prevent watchdog from resetting idle queues")
MFC after: 1 month
Assisted-by: Claude Code (Fable 5)

[markj]

Thank you, I was looking at this too.

sys/net/iflib.c

4003

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

4004

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.

[gallatin]

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.

[netchild]

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.

[netchild]

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

[gallatin]

In D58266#1336569, @netchild wrote:

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