refactor: Unify methods of guest memory creation #5013
Merged
Conversation
This file contains 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
roypat
changed the title
roypat
force-pushed
the
memory-cleanup
branch
from
99afd42 to
6115abf
Compare
roypat
added
the
Status: Awaiting review
roypat
force-pushed
the
memory-cleanup
branch
2 times, most recently
from
93e03be to
8a20ec7
Compare
kalyazin
reviewed
Jan 28, 2025
roypat
force-pushed
the
memory-cleanup
branch
2 times, most recently
from
89efde4 to
cdb2603
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5013 +/- ##
==========================================
+ Coverage 83.14% 83.16% +0.02%
==========================================
Files 245 245
Lines 26699 26647 -52
==========================================
- Hits 22198 22161 -37
+ Misses 4501 4486 -15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
roypat
force-pushed
the
memory-cleanup
branch
from
5d5d63d to
2b08197
Compare
In addition to being unused, it was also wrong, because it only updated the flag on KVM's side, but kept firecracker's tracking disabled. Signed-off-by: Patrick Roy <[email protected]>
We already know about dirty page tracking inside that function, based on whether the memory regions have a bitmap associated with them or not. So drop passing this information in again via a parameter, which saves us quite a bit of plumbing. Suggested-by: Nikita Kalyazin <[email protected]> Signed-off-by: Patrick Roy <[email protected]>
In praxis, the way we wrote our snapshot files has always been just writing all regions in-order. This mean that the offset of a region is simply the sum of the sizes of the preceding regions. The new `GuestMemoryMmap::create` code already computes the offsets for mapping the memory file this way, so drop the explicit calculation at snapshot creation time (as the calculated value isnt used by the restoration anymore). Do not bump the snapshot version number, because we already did so since the last release. Signed-off-by: Patrick Roy <[email protected]>
We forgot to include this in the 1.9.0 changelog. Let's retroactively do it. Signed-off-by: Patrick Roy <[email protected]>
Some tests in memory.rs were runnign effectively the same test scenario twice, the only difference being the state of dirty page tracking. Just use a loop over the two boolean values here to avoid the copy-paste. Also remove a leftover test that was referring to "guard pages", but actually only repeated one of the dirty page tracking blocks. Guard pages were removed in 71cf036. Signed-off-by: Patrick Roy <[email protected]>
In this day and age, Firecracker supports theoretically 4 different ways of backing guest memory: 1. Normal MAP_ANONYMOUS | MAP_PRIVATE memory 2. memfd backed memory, mapped as shared 3. direct mapping of a snapshot file 4. MAP_ANONYMOUS again, but this time regions are described by snapshot file. We have 3 different functions for creating these different backing stores, which then call each other and vm_memory's APIs. Clean this up by consolidating these into just one function that can be called with generic memory backing options, plus 3 wrappers for the three actually used ways of backing memory. For this, hoist up the hugepages/file-based restore incompatibility check, as with a dedicated function for dealing with the "snapshot restored by mapping file" case, this function simply will not take a huge pages argument, so we have to check this somewhere else. Signed-off-by: Patrick Roy <[email protected]>
roypat
force-pushed
the
memory-cleanup
branch
from
2b08197 to
6a14208
Compare
kalyazin
approved these changes
Jan 31, 2025
bchalios
approved these changes
Feb 3, 2025
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.
In this day and age, Firecracker supports theoretically 4 different ways of backing guest memory:
We have 3 different functions for creating these different backing stores, which then call each other and vm_memory's APIs.
In light of #4522, which will add yet another way of backing virtual machine guests, this was starting to make my head hurt a bit.
Clean this up by consolidating these into just one function (
GuestMemoryExtensions::create) that can be called with a description of the memory regions, plus an enum argument stating how each region should be backed.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 checkstyleto 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.