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>
Contributor
|
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 i8042 and RTC (pl031) legacy devices from a module-global metrics static to the per-device metrics model already used by the virtio devices (net/block, and vsock/rng in #5599).
Each device now owns an
Arc<XxxDeviceMetrics>and registers a clone in a module-levelRwLock<Option<Arc<XxxDeviceMetrics>>>on construction; counters are incremented through the device's ownArcinstead of the shared global.legacy::flush_metricsserializes the registered instance (falling back to adefault()when no device has been built yet), so the serialized metrics output shape is unchanged.For the RTC this leans on
vm-superio'simpl<EV: RtcEvents> RtcEvents for Arc<EV>, so the innerRtcholds the sameArcthe module tracks — no extra field needed.Why
This is a step towards #4709 (running
vmmunit tests in parallel). With a process-wide metrics global, tests that assert on metric values race each other when run concurrently, which is whyRUST_TEST_THREADS=1is currently forced. Tests here 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 intentionally left for a follow-up: converting it means threading the metrics
ArcthroughSerialOutandSerialEventsWrapperat their call sites, which is better reviewed on its own. This PR does not flipRUST_TEST_THREADS; that stays until the remaining logger-global users (serial, 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— cleancargo test -p vmm devices::legacy— 12 passed (i8042 + serial), incl. newtest_i8042_metricstest_rtc_device_invalid_write,test_rtc_invalid_buf_len,test_rtc_dev_metrics) — 3 passed (verified on x86 by temporarily lifting theaarch64cfg gate, then reverted)cargo test -p vmm devices::legacy -- --test-threads=16, 5 consecutive runs — 12 passed / 0 failed each time, confirming the conversion is parallel-safeRelated: #4709