Conversation
The i8042 and RTC devices kept their metrics in a module-global static, shared process-wide. Because unit tests assert on metric values and each test builds its own device, that shared state forces the vmm unit tests to run single-threaded (RUST_TEST_THREADS=1) to avoid spurious failures. Move both devices to the per-device metrics model already used by the virtio devices: the metrics live behind a `RwLock<Option<Arc<XxxDeviceMetrics>>>` that the device populates on construction, and the device increments counters through its own `Arc<XxxDeviceMetrics>` rather than the global. `flush_metrics` reads the registered instance (falling back to a default when no device has been built yet), so the serialized output shape is unchanged. For the RTC this relies on vm-superio's `impl<EV: RtcEvents> RtcEvents for Arc<EV>`, letting the inner `Rtc` hold the same `Arc` the module tracks. Tests now build a device with a caller-owned metrics instance and assert on absolute values instead of reading-then-diffing a global. The UART (serial) device is left on its module-global metrics; it will be converted in a follow-up because it needs the metrics threaded through `SerialOut` and `SerialEventsWrapper` at their call sites. Progresses firecracker-microvm#4709. Signed-off-by: shaolila <shaolila@buaa.edu.cn>
The serial (UART) device kept its metrics in a module-global static, shared process-wide, so unit tests that assert on metric values could race each other and had to run single-threaded. Move it to the per-device metrics model used by the other devices: the device owns an `Arc<SerialDeviceMetrics>` and registers it in a module `RwLock<Option<...>>` on construction. Because the serial metrics are touched from three places, the `Arc` is threaded to each: - `SerialEventsWrapper` gains a `metrics` field (and a `new()` constructor) and increments through it. - `SerialOut` (built by the caller before the device exists) starts with a private default instance; `SerialDevice::new` swaps in the shared, registered metrics via `set_metrics` so the rate-limiter counter is aggregated with the rest. - The `BusDevice` impl reaches the metrics through `self.serial.events().metrics`. `legacy::flush_metrics` now serializes the registered UART instance (falling back to a default when none has been built), so the output shape is unchanged. Tests build a device with a caller-owned metrics instance and assert on absolute values. With this, all three legacy devices are off the process-wide metrics global. Removing RUST_TEST_THREADS=1 still depends on the remaining logger globals (mmds, msix), so this does not close the issue yet. Progresses firecracker-microvm#4709. Signed-off-by: shaolila <shaolila@buaa.edu.cn>
AdaAibaby
force-pushed
the
feat/serial-per-device-metrics
branch
from
September 8, 2026 03:31
b59a2d1 to
c8a2cbe
Compare
Contributor
|
Same comment as #6190 We're definitely interested in this, and especially the final outcome of being able to run unit tests in parallel. However, this PR seems premature since the approach is still being discussed in #5599. I suggest waiting for #5599 to be merged, and then we can apply the same approach to other devices |
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
3 participants
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.
Changes
Moves the serial (UART) device off its module-global metrics static to the per-device metrics model, completing the legacy-device conversion started in #6190 (i8042 + RTC).
The UART is trickier than i8042/RTC because its metrics are touched from three places, so the device's
Arc<SerialDeviceMetrics>is threaded to each:SerialEventsWrappergains a privatemetricsfield and anew()constructor; itsSerialEventsimpl increments through it.SerialOutis built by the caller before the owningSerialDeviceexists, so it starts with a private default metrics instance;SerialDevice::newswaps in the shared, registered instance viaset_metricsso the rate-limiter counter aggregates with the rest.BusDeviceimpl reaches the metrics viaself.serial.events().metrics.legacy::flush_metricsserializes the registered UART instance (falling back to adefault()when none has been built), so the serialized metrics output shape is unchanged.Stacking
Why
Another step towards #4709. With all three legacy devices (i8042, RTC, UART) off the process-wide metrics global, their unit tests can build isolated per-device metrics and run concurrently. This PR still does not flip
RUST_TEST_THREADS=1— that stays until the remaining logger globals (mmds, msix) are converted.Testing
Built and tested in the
fcuvm:v93dev container (x86_64):cargo build -p vmm— cleancargo clippy -p vmm --all-targets— no warningscargo fmt --check— clean (no lines >100 chars, safe under nightlywrap_comments)cargo test -p vmm devices::legacy— 12 passed (i8042 + serial)cargo test -p vmm --test devices(external integration test) — 3 passedcargo test -p vmm devices::legacy -- --test-threads=16, 3 consecutive runs — 12 passed / 0 failed each, confirming parallel-safetyRelated: #4709