8000 Use mincore(2) to create diff snapshots without dirty page tracking by roypat · Pull Request #5274 · firecracker-microvm/firecracker · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use mincore(2) to create diff snapshots without dirty page tracking #5274

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Use mincore(2) to create diff snapshots without dirty page tracking #5274

wants to merge 12 commits into from

Conversation

roypat
Copy link
Contributor
@roypat roypat commented Jun 19, 2025

Currently, Firecracker only supports creation of diff snapshots if dirty
page tracking is explicitly enabled. Allow creation of diff snapshots
even if it is not enabled, through the use of mincore(2). The mincore(2)
syscalls determines which pages of a VMA are "in core". For anonymous
mappings (as used by booted VMs without vhost-user devices), this refers
to all pages that are currently faulted in. For memfd (as used by booted
vms with vhost-user devices), this means all
pages that have been allocated into the memfd, regardless of whether
they were allocated through the VMA on which mincore(2) was called
(meaning creation of mincore-diff-snapshots will correctly account for
pages that were only touched by the vhost-user backend, but not by
Firecracker or KVM). For restored VMs, this means all pages of the
underlying snapshot file that have been faulted in.

Note that this only works if swap has been disabled, as pages currently
swapped to disk do not count as "in-core", yet obviously should be
included in a diff snapshot. If swap is used, dirty page tracking MUST
be enabled for diff snapshots to work correctly.

Compared to diff snapshots based on dirty page tracking, mincore-based
diff snapshots will be slightly larger. This is because dirty page
tracked diff snapshots only include pages that were actually written to,
while mincore-based snapshots will contain all pages that were accessed
at all, e.g. even if only for reading.

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

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • If a specific issue led to this PR, this PR closes the issue.
  • When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • I have linked an issue to every new TODO.
  • This functionality cannot be added in rust-vmm.

@roypat roypat force-pushed the mincore-diff-snapshots branch from 028794d to 56ca763 Compare June 19, 2025 13:28
roypat added 2 commits June 19, 2025 14:32
@roypat
doc: specify that diff snapshots produce sparse files
6bd5caf 
Include a sentence about diff snapshots producing sparse files in our
snapshot documentation.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat
doc: remove rebase-snap instructions from documentation
c8eb16f 
The rebase-snap tool is deprecated and snapshot-editor should be used.
Since the two ways of merging snapshot files are completely equivalent,
let's remove the rebase-snap instructions and only keep the
snapshot-editor ones, to avoid people using deprecated tools.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat roypat force-pushed the mincore-diff-snapshots branch 4 times, most recently from 0622764 to 7379503 Compare June 19, 2025 14:29
@codecov Codecov
8000
Copy link
codecov bot commented Jun 19, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 94.82759% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.93%. Comparing base (d5f3513) to head (31dc205).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/vmm/src/vstate/vm.rs 94.23% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5274      +/-   ##
==========================================
+ Coverage   82.84%   82.93%   +0.08%     
==========================================
  Files         250      250              
  Lines       26967    27015      +48     
==========================================
+ Hits        22342    22405      +63     
+ Misses       4625     4610      -15
Flag Coverage Δ
5.10-c5n.metal 83.37% <94.82%> (+0.04%) ⬆️
5.10-m5n.metal 83.37% <94.82%> (+0.04%) ⬆️
5.10-m6a.metal 82.58% <94.82%> (+0.03%) ⬆️
5.10-m6g.metal 79.01% <15.51%> (-0.16%) ⬇️
5.10-m6i.metal 83.37% <94.82%> (+0.04%) ⬆️
5.10-m7a.metal-48xl 82.58% <94.82%> (?)
5.10-m7g.metal 79.01% <15.51%> (-0.16%) ⬇️
5.10-m7i.metal-24xl 83.33% <94.82%> (?)
5.10-m7i.metal-48xl 83.33% <94.82%> (?)
5.10-m8g.metal-24xl 79.01% <15.51%> (?)
5.10-m8g.metal-48xl 79.01% <15.51%> (?)
6.1-c5n.metal 83.41% <94.82%> (+0.04%) ⬆️
6.1-m5n.metal 83.42% <94.82%> (+0.04%) ⬆️
6.1-m6a.metal 82.64% <94.82%> (+0.04%) ⬆️
6.1-m6g.metal 79.01% <15.51%> (-0.16%) ⬇️
6.1-m6i.metal 83.41% <94.82%> (+0.04%) ⬆️
6.1-m7a.metal-48xl 82.63% <94.82%> (?)
6.1-m7g.metal 79.01% <15.51%> (-0.16%) ⬇️
6.1-m7i.metal-24xl 83.43% <94.82%> (?)
6.1-m7i.metal-48xl 83.43% <94.82%> (?)
6.1-m8g.metal-24xl 79.01% <15.51%> (?)
6.1-m8g.metal-48xl 79.01% <15.51%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@roypat roypat force-pushed the mincore-diff-snapshots branch 2 times, most recently from 1a5e0ae to fa57cc6 Compare June 19, 2025 15:48
roypat added 4 commits June 20, 2025 08:47
@roypat
Use mincore(2) to create diff snapshots without dirty page tracking
e112f60 
Currently, Firecracker only supports creation of diff snapshots if dirty
page tracking is explicitly enabled. Allow creation of diff snapshots
even if it is not enabled, through the use of mincore(2). The mincore(2)
syscalls determines which pages of a VMA are "in core". For anonymous
mappings (as used by booted VMs without vhost-user devices), this refers
to all pages that are currently faulted in. For memfd (as used by booted
vms with vhost-user devices), this means all
pages that have been allocated into the memfd, regardless of whether
they were allocated through the VMA on which mincore(2) was called
(meaning creation of mincore-diff-snapshots will correctly account for
pages that were only touched by the vhost-user backend, but not by
Firecracker or KVM). For restored VMs, this means all pages of the
underlying snapshot file that have been faulted in.

Note that this only works if swap has been disabled, as pages currently
swapped to disk do not count as "in-core", yet obviously should be
included in a diff snapshot. If swap is used, dirty page tracking MUST
be enabled for diff snapshots to work correctly.

Compared to diff snapshots based on dirty page tracking, mincore-based
diff snapshots will be slightly larger. This is because dirty page
tracked diff snapshots only include pages that were actually written to,
while mincore-based snapshots will contain all pages that were accessed
at all, e.g. even if only for reading.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat
rename enable_diff_snapshots API paramter to track_dirty_pages
07aa7d6 
We have two different API parameters that do the same thing in two
different APIs: For booted VMs, we enable KVM dirty page tracking by
setting track_dirty_pages to true in /machine-config. For resumed VMs,
we enable KVM dirty page tracking by setting enable_diff_snapshots to
true in /snapshot/load. Apart from being inconsistent for no reason, one
of these is very badly named: With support for diff snapshots without
dirty page tracking, enable_diff_snapshots is a misnomer, because
setting it to false will still allow us to do diff snapshots, it'll just
fall back to mincore.

Rename enable_diff_snapshots to track_dirty_pages, so we're consistent
and also so that the parameter reflects what is actually happening. Go
through the whole deprecation cycle of deprecating enable_diff_snapshots
and adding the new track_dirty_pages parameter at the same time. We will
need to remove enable_diff_snapshots in 2.0

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat
doc: document mincore-based diff snapshots
acc17a9 
Document that diff snapshots can work even without dirty page tracking,
and what the drawbacks of this are (bigger snapshots, no swap support).

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat
refactor(test): Eliminate Snapshot.is_diff
9998fbb 
We were using this property as a proxy for two things: "Does this
snapshot rebase rebasing before resumptions?" and "do I need to enable
dirty page tracking to create another snapshot of this type?". With
mincore-snapshots, these two questions are no longer equivalent (mincore
snapshots need rebase, but do _not_ need dirty page tracking), so
untangle this property into two properties.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat roypat force-pushed the mincore-diff-snapshots branch from 7e06ee7 to 0f00c29 Compare June 20, 2025 07:48
roypat added 3 commits June 20, 2025 09:29
@roypat
refactor(test): Stop SnapshotType being a str enum
08a2b86 
With support for "mincore diff snapshots", we'll have two snapshot types
that map to "diff" in the firecracker API, so the enum can no longer be
string based. Instead just have a property that translates to
Firecracker API types.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat
refactor(test): stop accepting string in make_snapshot
887e912 < 8000 /code> 
Just always demand the enum. The internal conversion from str to enum
now doesnt work anymore with the enum no longer being str-based.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat
fix(test): clean up some open coded == SnapshotType.DIFF comparisons
4e22e28 
Have them use the correct needs_rebase/needs_dirty_tracking helpers
instead.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat roypat force-pushed the mincore-diff-snapshots branch from 828dd4b to 68541f7 Compare June 20, 2025 08:29
roypat added 2 commits June 20, 2025 09:48
@roypat
test: run all parametrized snapshot tests with mincore snapshots
8f78b7f 
Now that the test framework correctly differentiates between the need
for rebase and the need for dirty page tracking, start running tests
with mincore snapshots.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat
test: unify hugepage snapshot testing
8f6641c 
Just have a single test that is parametrized by snapshot type. This will
then automatically also test mincore diff snapshots.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat roypat force-pushed the mincore-diff-snapshots branch 2 times, most recently from fb6355d to 4cb5f51 Compare June 23, 2025 16:20
@roypat
tmp: swap in memory before taking snapshot
31dc205 
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat roypat force-pushed the mincore-diff-snapshots branch from 4cb5f51 to 31dc205 Compare June 23, 2025 16:24
8000
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xmarcalx xmarcalx Awaiting requested review from xmarcalx xmarcalx will be requested when the pull request is marked ready for review xmarcalx is a code owner

@kalyazin kalyazin Awaiting requested review from kalyazin kalyazin will be requested when the pull request is marked ready for review kalyazin is a code owner

@pb8o pb8o Awaiting requested review from pb8o pb8o will be requested when the pull request is marked ready for review pb8o is a code owner

@Manciukic Manciukic Awaiting requested review from Manciukic Manciukic will be requested when the pull request is marked ready for review Manciukic is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@roypat
0