Repository navigation
vmcore: fix per-VP task pool thread/fd leak on guest reset - #3901
Conversation
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
There was a problem hiding this comment.
Pull request overview
Fixes a regression where per-VP IoPool threads (created via ThreadDriverBackend / VmTaskDriverSource::current()) could never exit after guest reset because a thread-local strong DefaultDriver clone kept the pool’s task queue alive indefinitely, leaking threads and IO backend fds.
Changes:
- Switch the
vm_taskthread-localCURRENT_DRIVERfrom a strongDefaultDriverto a weakWeakDefaultDriver, upgrading only transiently when needed. - Add
IoDriver::downgrade()andWeakIoDrivertopal_async, plus platform type aliases (Weak*Driver/WeakDefaultDriver). - Add a regression test ensuring a dedicated pool thread exits once the last strong driver is dropped.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/vmcore/src/vm_task.rs | Use weak TLS driver for per-VP pool threads; upgrade on demand; add regression test for thread exit after driver drop. |
| support/pal/pal_async/src/windows/mod.rs | Re-export WeakDefaultDriver for Windows default backend. |
| support/pal/pal_async/src/windows/iocp.rs | Add WeakIocpDriver alias for IoDriver weak refs. |
| support/pal/pal_async/src/unix/mod.rs | Re-export WeakDefaultDriver for epoll/kqueue backends. |
| support/pal/pal_async/src/unix/kqueue.rs | Add WeakKqueueDriver alias for IoDriver weak refs. |
| support/pal/pal_async/src/unix/epoll.rs | Add WeakEpollDriver alias for IoDriver weak refs. |
| support/pal/pal_async/src/lib.rs | Export WeakDefaultDriver from the platform sys module. |
| support/pal/pal_async/src/io_pool.rs | Implement IoDriver::downgrade() and WeakIoDriver::upgrade() to avoid self-referential pool lifetimes. |
31fefb3 to
303309d
Compare
|
Robert Nowotny (@bitranox), this PR looks close but has been inactive for a while. Is it ready for review or merge, or is anything still pending? Thanks. |
303309d to
dc73bd4
Compare
|
Ready from my side, nothing pending. I re-checked it against current main: the strong |
dc73bd4 to
8529c9b
Compare
|
Agreed, the test did not cover the production path. It built its own pool and hand-wrote the lend sequence, so it only characterised the pattern the test itself wrote. I confirmed that by reintroducing the strong self-reference in |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The shutdown test does not reliably verify thread completion, and the weak-reference lifetime documentation needs correction.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
|
All three failing checks on
The shape is identical in all of them. The guest logs an RCU stall, prints This change also cannot reach that VM. The thread local it touches is lent only on a dedicated pool thread, and Could you re-run the |
A device that builds a target-VP task driver (ThreadDriverBackend) spawns a dedicated IO pool thread and, since the thread-local IO dispatch change, lent a clone of that pool's own driver into CURRENT_DRIVER for the whole lifetime of pool.run(). IoPool::run only returns once the last strong IoDriver is dropped, so that self-reference kept the pool's task queue open forever: after a device closed its channel and dropped its driver, the thread parked permanently, never releasing its epoll fd and wakeup eventfd. A guest reset (reboot) is the steady-state path that repeatedly rebuilds these per-channel drivers, so every reboot stacked a fresh set of parked threads and fds. On a Linux guest with VMBus netvsp/storvsp devices this leaked ~132 threads and ~247 fds per reboot until the process hit its fd limit. Publish a weak reference instead. It is only borrowed by code running on the pool's own thread while a task is executing, so a strong driver provably still exists at borrow time and the transient upgrade succeeds; at rest the thread holds no strong reference, so the pool shuts down and the thread exits once the device drops its driver. Add WeakIoDriver / IoDriver::downgrade to pal_async and a regression test that drives ThreadDriverBackend::build and waits for the pool to release its IO backend. The test waits on WeakIoDriver::is_backend_alive rather than upgrade. IoPool::run drops the pool's scheduler on entry, so upgrade starts returning None as soon as the caller drops its driver, which is shutdown requested and not shutdown done: measured, it returns None while the dedicated thread is still inside run(). The pool holds its IO backend until run() returns, so the backend going away is what shows the fds came back.
8529c9b to
fe17255
Compare
|
Robert Nowotny (@bitranox) you should hop in our new Discord, https://aka.ms/openvmmdiscord. Probably a better way to get John Starks (@jstarks)' attention lol |
uh, I dont need attention, just colaboration. the only help I need is the hvci issue - it needs a kvm kernel patch as far as I researched, there seem to be no way around it. For that I really need help, to get that kernel patch into the linux kernel, since I am not a kernel developer. So all I really ask is, to get that done. Everything else I can do on myprivate fork for proxmox, donating it back to this project. I emailed john, unfortunately no answer ... |
Steven Malis (smalis-msft)
left a comment
Steven Malis (smalis-msft)
left a comment
There was a problem hiding this comment.
Some minor thoughts on the code itself, mostly I think it's fine. I need to think more on if there are any potential issues that could lead to this change dropping things too soon though.
|
AI review finding: The weak upgrade in vm_task.rs exposes the field-drop order in TaskFuture: _scheduler is dropped before the user’s future. When the device driver has already been dropped and the last task completes or is cancelled: Dropping _scheduler releases the last scheduler reference. I did not find an existing production destructor in the current-driver consumers that triggers this, so this is a verified API-behavior regression—not an observed guest failure. Suggested fix: place future before _scheduler in TaskFuture, retaining the scheduler until future destruction finishes. I tested this experimentally: it fixes both failing probes, and the thread-leak regression test still passes. |
|
More AI: The earlier field-order fix is incomplete It destroys the result of a completed, detached task. Correction to my previous recommendation: moving the scheduler field after the future is not sufficient. The scheduler needs to survive the entire runnable execution, including output destruction and completion notification. I tested holding a temporary scheduler reference around task.run() in TaskQueue::run. This covers all four completion/cancellation probes while still allowing idle pools to shut down. 186 tests passed with that experimental change. These extend the existing finding; I would not count them as unrelated defects. |
do I need to do smth, or You take care of it ? I am ok with everything when the fd leak is gone ... |
|
If you could combine that fix with this PR it'd be appreciated. We don't want to risk regressing other functionality. |
on it - i will do it over the weekend... |
When a task completes or is cancelled, Runnable::run drops its future, then drops the output if the task was detached, then wakes the task's awaiter. The scheduler reference held in TaskFuture goes away with the future. If it was the last one, the two later steps run with the scheduler already gone, and so does the future's own destructor, since TaskFuture drops its scheduler field first. On a dedicated VP pool thread, CurrentThreadDriver finds the pool through a weak reference. In those teardown steps the upgrade fails and anything created there falls back to the default driver, even though it is running on the pool thread. TaskQueue::run now upgrades the task's scheduler before calling run() and drops it when run() returns. An idle pool still shuts down: the extra reference lasts for one call only. Add a test for each teardown path: a completed future's destructor, a cancelled future's destructor, a detached task's output destructor and the awaiter wake. All four fail without the change. Also remove WeakIoDriver::is_backend_alive, which only the leak test used, and stop the vm_task tests from polling. Each test now learns that the pool thread has exited from a thread-local that is dropped when the thread ends, and waits for its probe report on a channel. The 60 second timeout only bounds how long a regression takes to fail.
|
Folded in as 62bd37b. There is a test for each of the four teardown paths: a completed future's destructor, a cancelled future's destructor, a detached task's output destructor, and the awaiter wake. Each one checks on the pool thread that the weak upgrade The same push also covers your two inline comments: |
There was a problem hiding this comment.
🔵 Needs a closer look
The fix changes cross-platform executor shutdown and concurrent task-destruction lifetime semantics, warranting final human review.
0 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
The tests waited for the pool thread's exit signal and for the probe reports with a 60 second timeout. Block on the channels instead and leave a wedged test to the test runner. A signal that is dropped without being sent still fails the test through expect.
There was a problem hiding this comment.
🟢 Approval recommended
The lifecycle fix is coherent across platforms and is covered by targeted regression tests.
0 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
13660c2
into
microsoft:main
Problem
A device that builds a target-VP task driver via
ThreadDriverBackend(netvsp, storvsp, and any resource usingVmTaskDriverSource::current()on a per-VP thread) spawns a dedicatedIoPoolthread. Since #3310 that thread lends a clone of its own pool's driver into theCURRENT_DRIVERthread-local for the entire lifetime ofpool.run().IoPool::runreturns only once the last strongIoDriveris dropped. The lent clone is a strong self-reference held across the whole run, so after the device closes its channel and drops its ownThreadDriver, the pool's task queue never closes: the thread parks forever inrecv().await, never releasing its epoll fd and wakeup eventfd.A guest reset (reboot) repeatedly rebuilds these per-channel drivers, so every reboot stacks a fresh set of parked threads and fds. On a Linux guest with VMBus netvsp/storvsp devices, the worker leaked ~132 threads and ~247 fds per reboot until it hit its fd limit.
Regressed in #3310 (thread-local IO dispatch).
Fix
Publish a weak reference into
CURRENT_DRIVER. It is only borrowed by code running on the pool's own thread while a task executes, so a strong driver provably still exists and the transient upgrade succeeds; at rest the thread holds no strong reference, so the pool shuts down and the thread exits once the device drops its driver. AddsWeakIoDriver/IoDriver::downgradeto pal_async and a regression test.