-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[virtio-mem] Add hotplug/hotunplug support with statically allocated memory region #5462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/virtio-mem
Are you sure you want to change the base?
[virtio-mem] Add hotplug/hotunplug support with statically allocated memory region #5462
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/virtio-mem #5462 +/- ##
======================================================
+ Coverage 82.70% 83.11% +0.41%
======================================================
Files 270 271 +1
Lines 27850 28315 +465
======================================================
+ Hits 23033 23534 +501
+ Misses 4817 4781 -36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ddb9041
to
dab0533
Compare
Allocate the memory that will be used for hotplugging. Initially, this memory will be registered with KVM, but that will change later when we add dynamic slot support. Signed-off-by: Riccardo Mancini <[email protected]>
Wire up PATCH requests with the virtio-mem device. All the validation is performed in the device, but the actual operation is not yet implemented. Signed-off-by: Riccardo Mancini <[email protected]>
Add entry for the patch API in Swagger and in the docs. Signed-off-by: Riccardo Mancini <[email protected]>
Test that the new PATCH API behaves as expected. Also updates expected metrics and fixes memory monitor to account for hotplugging. Signed-off-by: Riccardo Mancini <[email protected]>
Parse virtio requests over the queue and always ack them. Following commits will add the state management inside the device. Signed-off-by: Riccardo Mancini <[email protected]>
This commit adds block state management and implements the virtio requests for the virtio-mem device. Block state is tracked using a BitVec, each bit representing a single block. Plug/Unplug requests are validated before being executed to verify the range is valid (aligned and within range), and that all blocks in range are unplugged/plugged, as per the virtio spec. UplugAll is the only request where usable_region_size can be lowered. This commit is missing the dynamic KVM slot management which will be added later. Signed-off-by: Riccardo Mancini <[email protected]>
Adds unit tests using VirtioTestHelper to verify correct functioning of the new device. Signed-off-by: Riccardo Mancini <[email protected]>
Add the virtio-mem device metrics to the integ test validation. Signed-off-by: Riccardo Mancini <[email protected]>
If the handler receives a UFFD remove event, it currently stores the PFN and will reply with a zero page whenever it receives a pagefault event for that page. This works well with 4k pages, but zeropage is not supported on hugepages. In order to support hugepages, let's just unregister from UFFD whenever we get a remove event. By doing so, the handler won't receive a notification for the removed page, and the VM will get a new zero page from the kernel. Signed-off-by: Riccardo Mancini <[email protected]>
This moves the logic to measure RSS to framework.utils and adds a logic to also include huge pages in the measurement. Furthermore, this also adds caching for the firecracker_pid, as well as a new property to get the corresponding psutil.Process. Signed-off-by: Riccardo Mancini <[email protected]>
Move the logic to get the MemAvailable from /proc/meminfo inside the guest to a new guest_stats module in the test framework. This provides a new class MeminfoGuest that can be used to retrieve this information (and more!). Signed-off-by: Riccardo Mancini <[email protected]>
Add integration tests for the new device: - check that the device is detected - check that hotplugging and unplugging works - check that memory can be used after hotplugging - check that memory is freed on hotunplug - check different config combinations - check different uvm types - check that contents are preserved across snapshot-restore Signed-off-by: Riccardo Mancini <[email protected]>
Since these tests need to be run on an ag=1 host, move them under the "performance" folder. Signed-off-by: Riccardo Mancini <[email protected]>
These tests add unit test coverage to the builder.rs and vm.rs files which where previously untested in the memory hotplug case. Signed-off-by: Riccardo Mancini <[email protected]>
c507d69
to
370eb4e
Compare
#[derive(Debug, Clone, Copy)] | ||
pub enum ResponseType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed? This enum just repeats what VIRTIO_MEM_RESP_
consts are. If you use constants directly, there is no need for this enum or any conversion code.
Or at least you can use repr(u16)
to remove to u16 conversion and totally skip VIRTIO_MEM_RESP_
values since thes enum will have same values.
base64 = "0.22.1" | ||
bincode = { version = "2.0.1", features = ["serde"] } | ||
bitflags = "2.9.4" | ||
bitvec = { version = "1.0.1", features = ["atomic", "serde"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must ask: can we have a local implementation for this? Looking at the usage it seems quite simple to implement ourselfs.
nb_blocks * u64_to_usize(self.config.block_size) | ||
} | ||
|
||
fn is_range_plugged(&self, range: &RequestedRange) -> BlockRangeState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe name the functino range_state
?
start_block..(start_block + range.nb_blocks) | ||
} | ||
|
||
fn do_plug_request(&mut self, range: &RequestedRange) -> Result<(), VirtioMemError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
process_plug_request
?
let response = self.do_plug_request(range).map_or_else( | ||
|err| { | ||
METRICS.plug_fails.inc(); | ||
error!("virtio-mem: Failed to plug range: {}", err); | ||
Response::error() | ||
}, | ||
|_| { | ||
METRICS | ||
.plug_bytes | ||
.add(usize_to_u64(self.nb_blocks_to_len(range.nb_blocks))); | ||
Response::ack() | ||
}, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use usual match
instaed of this convotuted mapping?
/// Invalid requested range: {0:?}. | ||
InvalidRange(RequestedRange), | ||
/// The requested range cannot be plugged because it's {0:?}. | ||
PlugRequestBlockStateInvalid(BlockRangeState), | ||
/// Plug request rejected as plugged_size would be greater than requested_size | ||
PlugRequestIsTooBig, | ||
/// The requested range cannot be unplugged because it's {0:?}. | ||
UnplugRequestBlockStateInvalid(BlockRangeState), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use {0}
in error messages to use Display
instaed of Debug
printing
&self.activate_event | ||
} | ||
|
||
fn plug_range(&mut self, range: &RequestedRange, plug: bool) -> Result<(), VirtioMemError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/plug_range/update_range ?
raise ValueError("value_kib must be non-negative") | ||
return ByteUnit(value_kib * 1024) | ||
|
||
def bytes(self) -> float: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these floats?
Changes
Adds support for PATCH API to
/hotplug/memory
which enables users to plug and unplug memory. The hotpluggable memory region is statically allocated at startup, and unplugged blocks are discarded (madvise(MADV_DONTNEED
), but they are not removed from the KVM user memory slots. This will be done in a follow up PR.Also adds a set of basic integration tests to test the functionality of virtio-mem.
Reason
Virtio-mem support for memory hotplugging.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md
.PR Checklist
tools/devtool checkbuild --all
to verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyle
to verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md
.Runbook for Firecracker API changes.
integration tests.
TODO
.rust-vmm
.