Repository navigation
watchdog_core: poll the timeout action instead of blocking on it - #4634
Open
Robert Nowotny (bitranox) wants to merge 4 commits into
Open
Robert Nowotny (bitranox) wants to merge 4 commits into
Robert Nowotny (bitranox) wants to merge 4 commits into
Conversation
WatchdogServices::poll ran the platform's on_timeout with pal_async::local::block_on inside the device's poll. For the guest watchdog, on_timeout first writes the boot-failure flag to its VMGS store, and the VMGS broker runs on the same thread that polls chipset devices. The first timeout therefore parked that thread for good: the VPs were never halted, the configured action never ran, and the guest froze with every VP idle. The UEFI watchdog was not affected because its store completes without waiting. The timeout now moves the platform into a future that poll() drives with its own context, and the future hands the platform back when it is done. A new saved-state field, timeout_pending (false for older states), records that the action is still owed, so a state saved while it runs still carries it out after a restore. reset() leaves a running action alone. pal_async is now only a dev-dependency.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Reset followed by save can lose an in-flight timeout action during restore.
1 open finding
What changed in this PR
Reworks watchdog timeout handling to avoid blocking the device polling thread.
Changes:
- Polls timeout actions asynchronously.
- Persists pending timeout actions across save/restore.
- Adds regression tests and scopes
pal_asyncto tests.
| File | Description |
|---|---|
watchdog_core/src/lib.rs |
Adds asynchronous timeout state management and tests. |
watchdog_core/Cargo.toml |
Moves pal_async to dev-dependencies. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Drop the timeout_pending saved-state field. Both watchdog devices now await WatchdogServices::finish_timeout_action() in stop() and reset(), so state is never saved while a timeout action is still running, and the saved state stays as it was. WatchdogServices::reset() on its own still leaves a running action alone, because the action owns the platform. The device reset finishes it first.
WatchdogServices::reset() put the registers back to the unconfigured state but left the vmtime deadline armed. After a VM reset the guest didn't see coming (sysrq b, or a panic with reboot), the old deadline fired in the next boot, although nothing there had configured the watchdog. reset() now cancels it. reset_cancels_the_armed_timer arms the watchdog, resets it, moves the clock past the deadline and requires no timeout.
Comment on lines
+154
to
+155
| // An in-flight timeout action keeps running: it owns the platform, so | ||
| // dropping it would lose the platform, and the timeout did happen. |
There was a problem hiding this comment.
Can we actually get here with an in-flight action? Shouldn't stop have run by now?
There was a problem hiding this comment.
If so, I'd rather we assert that as an invariant here.
Contributor
Author
There was a problem hiding this comment.
No, it can't. Devices are always stopped before they're reset, and stop already runs finish_timeout_action(), so there's never an action left by the time reset() runs. Added the assert in 7e6d205 and dropped the extra finish_timeout_action() calls from both devices' reset(). The test that reset with an action still in flight is now a should_panic test for the assert.
Devices are always stopped before they are reset, and stop already finishes the timeout action, so reset never sees one in flight. Assert that instead of handling it, and drop the extra finish_timeout_action() calls from the guest watchdog's and UEFI device's reset.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WatchdogServices::pollruns the platform's timeout action withpal_async::local::block_on, on the thread that polls chipset devices. For the guest watchdog,BaseWatchdogPlatform::on_timeoutfirst writes the boot-failure flag to the guest watchdog's VMGS store, and the VMGS broker is spawned on that same thread. So the first timeout blocks the thread on work that only it can do. The configured action (reset, halt or exit) never runs, nothing is logged after "Encountered a watchdog timeout", and the guest stays frozen with all VPs idle, because its timers are served by that thread too.The UEFI watchdog doesn't hit this, since its store completes without waiting.
This PR moves the platform into a future when the timer fires, and
polldrives that future with its own context. The future hands the platform back when it finishes. Both watchdog devices wait for a running action instop(WatchdogServices::finish_timeout_action), the same way #4633 finishes queued NVRAM writes, so state is never saved halfway through an action and the saved state doesn't change. A device is always stopped before it is reset, soWatchdogServices::resetasserts that no action is in flight.pal_asyncis now only a dev-dependency.How we found it
We run a lot of end-to-end testing with OpenVMM on KVM. While working on #4633 we noticed the same
block_onpattern here and wrote a test for it: an Ubuntu guest started with--guest-watchdog --guest-watchdog-action reset,wdat_wdtloaded (the firmware publishes a WDAT table), and the watchdog armed with a single write and never fed. Without this change the log stops at "Encountered a watchdog timeout" and the guest stays frozen; an inspect of the VM comes back with only unit metadata at its deadline, because the device thread never answers. With the change, the guest halts with reason Watchdog about 50 ms after the timeout, resets and boots again. 25 reboot and 25 reset cycles on a Windows guest and on a Linux guest showed no fd, thread or device regressions. After the follow-up commit we ran the watchdog test again, plus suspend/resume, a live snapshot, and 10 more reboot/reset cycles on each guest, all clean.Tests
Five tests in
watchdog_coredrivepollwith a platform whoseon_timeoutwaits on a channel for permission, which stands in for a store served by the polling thread:pollwith the action pending instead of blocking, and the action runs once the store answers,finish_timeout_actionruns an action that is still in flight,A test in
guest_watchdogchecks that stopping the device finishes a running action.The timer fires by restoring the stopped
VmTimeKeeperpast the deadline, so nothing waits on real time. With theblock_onput back, thewatchdog_coretests that fire a timeout hang. Without the call instop, theguest_watchdogtest fails, without the cancel inreset,reset_cancels_the_armed_timerfails, and without the assert,reset_with_an_in_flight_action_panicsfails.Stale timer after a reset
WatchdogServices::resetput the registers back to the unconfigured state but left the timer armed, so after a reset the guest didn't see coming, the old deadline fired in the next boot although nothing there had configured the watchdog.resetnow cancels the timer. An orderly reboot doesn't hit this, because Linux'swdat_wdtstops the watchdog on the way down, butsysrq b(or a panic with reboot) does. VM time restarts at zero on reset, so the stale timeout comes back late: on an Ubuntu guest that armed a 120 s watchdog at 10 s of uptime and then ransysrq b, it fired 141.6 s into the next boot without this change, and stayed quiet for the 188 s we watched with it.