nvme: fused keepalive (#3811)#4005
Open
babayet2 wants to merge 4 commits into
Open
Conversation
Backport of the nvme_workaround feature (squashed commit 28c70c99) onto release 1.7.2511. Includes: - Fused NVMe keepalive device detection and handling (mod.rs, device.rs, manager.rs, driver.rs, queue_pair.rs). - Concurrent NVMe device restore in NvmeManagerWorker::restore via try_join_all. Backport deviations from the feature branch (due to mainline drift absent from the release branch): - driver.rs restore uses the unconditional eager sorted_io path (the feature's actual runtime behavior, since allow_lazy_restore is always Some(false)); the allow_lazy_restore saved-state field and the lazy proto_io restore split are not introduced. - Unstable Vec::push_mut is replaced with push + last_mut. - The phantom-completion admin-CQ diagnostic is dropped, as it depends on the mainline request_diagnostic_dump plumbing not present on the release branch. - The added VMM servicing tests in openhcl_servicing.rs are dropped, as they depend on a newer fault-injection framework (HardwareConfigFaultConfig, TDISP, run_cpu_pinned_io, nvme_test resolver changes) absent from the release branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 002f00db-6e37-4182-b06a-57deb3e7c0a5
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad2f5a78-7441-43d3-9679-9de10f68a9c3
github-actions
Bot
added
the
release_1.7.2511
Contributor
There was a problem hiding this comment.
Pull request overview
This backport introduces “fused keepalive” support for NVMe devices that cannot rely on the admin completion queue remaining mapped after a servicing event. The implementation aims to keep the admin queue idle after initialization/restore by disabling AERs and eagerly creating I/O queues, while plumbing a fused_keepalive_device flag from Underhill’s NVMe manager into the NVMe driver.
Changes:
- Add a
fused_keepalive_devicemode flag toNvmeDriver, disabling AER handling and eagerly pre-creating I/O queues (plus save/restore support for “unmapped” queues). - Add “admin-queue must remain idle after restore” enforcement via
commands_forbiddenand wire it through queue-pair construction/restore. - Detect fused devices in Underhill via PCI vendor/device IDs and propagate the flag through driver creation and restore paths; update tests and fuzzing entry points accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/devices/storage/storage_tests/tests/scsidvd_nvme.rs | Updates test driver construction to include the new fused-mode parameter. |
| vm/devices/storage/disk_nvme/nvme_driver/src/tests.rs | Updates NVMe driver unit tests for the new constructor signature. |
| vm/devices/storage/disk_nvme/nvme_driver/src/queue_pair.rs | Adds fused-mode AER disabling and introduces commands_forbidden guard in the queue handler. |
| vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs | Implements fused keepalive behavior (disable AER task, eager I/O queues, save/restore changes). |
| vm/devices/storage/disk_nvme/nvme_driver/fuzz/fuzz_nvme_driver.rs | Extends fuzzing to cover both fused and non-fused modes. |
| openhcl/underhill_core/src/nvme_manager/save_restore_helpers.rs | Updates helper test saved-state construction for new unmapped field. |
| openhcl/underhill_core/src/nvme_manager/mod.rs | Adds fused-device detection via sysfs vendor/device IDs and extends driver creation API. |
| openhcl/underhill_core/src/nvme_manager/manager.rs | Propagates fused-device mode into device creation/restore and parallelizes restore creation. |
| openhcl/underhill_core/src/nvme_manager/device.rs | Plumbs fused-device mode into VFIO-backed driver creation/restore and manager state. |
Comment on lines
1259
to
+1265
| Cmd::Command(rpc) => { | ||
| if self.commands_forbidden { | ||
| panic!( | ||
| "attempted to submit a command to admin queue {} for device {} after restore; the admin queue must remain idle for fused keepalive devices", | ||
| self.qid, self.device_id | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
No. We want to panic, because we want to find this noisily.
Comment on lines
+910
to
+916
| if fused_keepalive_device && pending_commands_count > 0 { | ||
| panic!( | ||
| "fused keepalive device {pci_id} restored with a non-empty admin \ | ||
| queue ({pending_commands_count} pending commands); fused devices \ | ||
| must not issue admin commands after init" | ||
| ); | ||
| } |
Comment on lines
+203
to
+206
| /// WORKAROUND: for fused keepalive devices, we eagerly initialize | ||
| /// IO queues. However, we continue to wait for an IO before mapping | ||
| /// the interrupt to a CPU | ||
| unmapped: bool, |
Comment on lines
574
to
+577
| // Spawn a task to handle asynchronous events. | ||
| let async_event_task = self.driver.spawn("nvme_async_event", { | ||
| let admin = admin.issuer().clone(); | ||
| let rescan_notifiers = self.rescan_notifiers.clone(); | ||
| async move { | ||
| if let Err(err) = handle_asynchronous_events(&admin, rescan_notifiers).await { | ||
| tracing::error!( | ||
| error = err.as_ref() as &dyn std::error::Error, | ||
| "asynchronous event failure, not processing any more" | ||
| ); | ||
| // When fused_keepalive_device is set, no commands are issued on the admin queue after init. | ||
| let async_event_task = if !self.fused_keepalive_device { | ||
| Some(self.driver.spawn("nvme_async_event", { |
Comment on lines
948
to
+952
| // Spawn a task to handle asynchronous events. | ||
| let async_event_task = this.driver.spawn("nvme_async_event", { | ||
| let admin = admin.issuer().clone(); | ||
| let rescan_notifiers = this.rescan_notifiers.clone(); | ||
| async move { | ||
| if let Err(err) = handle_asynchronous_events(&admin, rescan_notifiers) | ||
| .instrument(tracing::info_span!("async_event_handler")) | ||
| .await | ||
| { | ||
| tracing::error!( | ||
| error = err.as_ref() as &dyn std::error::Error, | ||
| "asynchronous event failure, not processing any more" | ||
| ); | ||
| // When fused_keepalive_device is set, no commands are issued on the admin queue after init. | ||
| let async_event_task = if !fused_keepalive_device { | ||
| Some(this.driver.spawn("nvme_async_event", { | ||
| let admin = admin.issuer().clone(); |
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.
This is a backport of #3811 to 1.7. This backport has in-progress scale testing, but should be reviewed as an independent PR, it is an AI-assisted manual backport, not a clean cherry pick