Repository navigation
Conversation
575bfa0 to
3352184
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6321 +/- ##
==========================================
+ Coverage 83.61% 83.65% +0.03%
==========================================
Files 282 282
Lines 33613 33665 +52
==========================================
+ Hits 28107 28162 +55
+ Misses 5506 5503 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
not4s
added
the
Status: Awaiting review
a118982 to
8dd5e23
Compare
97566c7 to
0d47025
Compare
| largest frame needs, instead of every buffer the guest has made available. | ||
| This lowers the cost of receiving each frame and improves receive throughput | ||
| for small packets. | ||
| - [#6321](https://github.com/firecracker-microvm/firecracker/pull/6321) Snapshot |
There was a problem hiding this comment.
Since today we don't have any other thread running while this happens, maybe we should skip a changelog entry here.
In single thread, this is an invisible change, right?
There was a problem hiding this comment.
Agreed that it shouldn't go under changed, but it does fix the failure case where FC used to lose dirty bitmaps apart from the atomic reset changes, so I'll rather put this under fixed with that content.
| /// slots are whole 2 MiB blocks, so with 4K pages each starts on a word. | ||
| pub(crate) fn slot_words(&self, index: usize) -> Range<usize> { | ||
| let pages = self.slot_size / host_page_size(); | ||
| assert_eq!(index * pages % 64, 0); |
There was a problem hiding this comment.
If I understand correctly, we have a new invariant that self.slot_size must be a multiple of 256KiB (with 4 KiB pages).
Can we make this explicit by both writing it into the documentation of GuestRegionMmapExt, and checking it when we construct it?
PS: is this actually always true? E.g. could a malformed snapshot return slots that aren't multiples? Maybe we should check the code to make sure this is enforced everywhere
There was a problem hiding this comment.
Good catch, this is guaranteed on boot config (slots are multiple of 2MB blocks) but isn't guaranteed on restore. I'm planning to open another PR first to merge it in before this one since there seems to be some test changes as well in virtio mem as well that seems out of scope for this PR
| .guest_memory() | ||
| .iter() | ||
| .any(|region| (**region).bitmap().is_some()); | ||
| let dirty_bitmap = match snapshot_type { |
There was a problem hiding this comment.
Can you look into this refactor (IDK if it will end up better or worst):
- here, we could call
take_dirty_bitmaponly in case oftracks_dirty_pages(even for Diff) - we could add a check that either all regions have a bitmap, or none has
- on
take_dirty, we can assume the region has a bitmap
Basically, the goal would be to avoid having to create an empty bitmap on take_dirty, which creates an unused code path that is pretty expensive.
There was a problem hiding this comment.
Makes sense, put the check inside take_dirty_bitmap rather than in its callers
| for (region, region_bitmap) in self.iter().zip(dirty_bitmap.iter_mut()) { | ||
| for (i, (mem_slot, plugged)) in region.slots().enumerate() { | ||
| if plugged { | ||
| let log = read_log(&mem_slot)?; |
There was a problem hiding this comment.
Can we add an assert_eq!(log.len(), words.len())? I think it's always true today.
zip stops at the shortest, so having an explicit check seems wiser
| if region_bitmap.len() > (region.size() / page_size).div_ceil(64) { | ||
| return Err(MemoryError::DirtyBitmapTooLarge); | ||
| } |
There was a problem hiding this comment.
I think this can be an assert. If we enable dirty bitmaps, they must be correct size per region, so I would even check for full equality region_bitmap.len() == (region.size() / page_size).div_ceil(64). This should also remove these .get calls.
There was a problem hiding this comment.
Agreed, it now asserts that and zips
| if dirty_bitmap.len() > expected_bitmap_array_len { | ||
| return Err(MemoryError::DirtyBitmapTooLarge); | ||
| } else if kvm_bitmap.len() < expected_bitmap_array_len { | ||
| } else if dirty_bitmap.len() < expected_bitmap_array_len { | ||
| return Err(MemoryError::DirtyBitmapTooSmall); | ||
| } |
There was a problem hiding this comment.
while we are at it, let's remove this abomination of a check and better yet assert the size and remove these errors.
| let page_size = host_page_size(); | ||
|
|
||
| let write_result = self.iter().enumerate().try_for_each(|(i, region)| { | ||
| self.iter().enumerate().try_for_each(|(i, region)| { |
There was a problem hiding this comment.
how about converting this to a simple for loop since we don't need the write_result anymore?
There was a problem hiding this comment.
Don’t use iterator combinator chains when direct iteration is clearer, agreed
|
|
||
| /// Writes guest memory to `file` as a snapshot of `snapshot_type`, taking the dirty pages it | ||
| /// covers. If writing fails, the pages taken are marked dirty again. | ||
| fn snapshot_memory<F: MemoryFile>( |
There was a problem hiding this comment.
It seems to me we are running out of names that basically say put memory into a file. How about:
snapshot_memory_to_file -> snapshot_memory_to_path
snapshot_memory -> snapshot_memory_to_file
and write_memory just merged back into snapshot_memory (before rename)?
Maybe there are better alternatives, but I think names should be improved here.
| let tracks_dirty_pages = self | ||
| .guest_memory() | ||
| .iter() | ||
| .any(|region| (**region).bitmap().is_some()); | ||
| let dirty_bitmap = match snapshot_type { | ||
| SnapshotType::Diff => self.take_dirty_bitmap()?, | ||
| SnapshotType::Full if tracks_dirty_pages => self.take_dirty_bitmap()?, | ||
| SnapshotType::Full => DirtyBitmap::new(), | ||
| }; | ||
| let result = self.write_memory(file, snapshot_type, &dirty_bitmap, sync_snapshot_files); | ||
| if result.is_err() { | ||
| self.guest_memory().store_dirty_bitmap(&dirty_bitmap); | ||
| } | ||
| result | ||
| } | ||
|
|
||
| /// Writes guest memory to `file`, all of it or the pages in `dirty_bitmap`, then flushes it | ||
| /// and, if asked, syncs it. | ||
| fn write_memory<F: MemoryFile>( | ||
| &self, | ||
| file: &mut F, | ||
| snapshot_type: SnapshotType, | ||
| dirty_bitmap: &DirtyBitmap, | ||
| sync_snapshot_files: bool, | ||
| ) -> Result<(), CreateSnapshotError> { | ||
| use self::CreateSnapshotError::MemoryBackingFile; | ||
|
|
||
| match snapshot_type { | ||
| SnapshotType::Diff => { | ||
| let dirty_bitmap = self.get_dirty_bitmap()?; | ||
| self.guest_memory().dump_dirty(&mut file, &dirty_bitmap)?; | ||
| } | ||
| SnapshotType::Full => { | ||
| self.guest_memory().dump(&mut file)?; | ||
| self.reset_dirty_bitmap(); | ||
| self.guest_memory().reset_dirty(); | ||
| } | ||
| SnapshotType::Diff => self.guest_memory().dump_dirty(file, dirty_bitmap)?, | ||
| SnapshotType::Full => self.guest_memory().dump(file)?, |
There was a problem hiding this comment.
Shouldn't something closer to the original code work a bit better:
match snapshot_type {
SnapshotType::Diff => {
let dirty_bitmap = self.take_dirty_bitmap()?;
if let Err(err) = self.guest_memory().dump_dirty(file, dirty_bitmap) {
self.guest_memory().store_dirty_bitmap(&dirty_bitmap)
} else if sync_snapshot_files && let Err(err) = file.sync_all() {
self.guest_memory().store_dirty_bitmap(&dirty_bitmap)
}
}
SnapshotType::Full => {
self.guest_memory().dump(file)?;
if sync_snapshot_files {
file.sync_all()
.map_err(|err| MemoryBackingFile("sync_all", err))?;
}
// only need to reset if dirty tracking is on
// can be done after memory write/flush since Full snaps can only be taken if VM is paused
let tracks_dirty_pages = self
.guest_memory()
.iter()
.any(|region| (**region).bitmap().is_some());
if tracks_dirty_pages {
_ = self.take_dirty_bitmap()?;
}
}
}There was a problem hiding this comment.
I'd still take first, they do the same thing bug Diff has to take first anyways, and it also works if something later writes during the copy. The idea is if a device thead ever wrote a page mid-copy, taking last would clear that mark and the next Diff would miss the page. If the dirty log read fails, it fails before we've written anything. Performance-wise should be the same unless it fails (which is still minimal)
| - [#6321](https://github.com/firecracker-microvm/firecracker/pull/6321) Snapshot | ||
| creation now takes the dirty page bitmaps before copying guest memory, | ||
| clearing each word atomically, and marks the taken pages dirty again if the | ||
| snapshot fails. |
There was a problem hiding this comment.
Maybe it is better to say something like: "Firecracker dose not lose dirty bitmaps on io failures anymore" and move it to fixed section?
There was a problem hiding this comment.
Makes sense, moved to fixed
|
|
||
| /// ORs into `dirty_bitmap` each plugged slot's log, as `read_log` returns it. Stops at the | ||
| /// first log that fails to read. | ||
| fn merge_slot_logs<E, F>(&self, dirty_bitmap: &mut DirtyBitmap, read_log: F) -> Result<(), E> |
There was a problem hiding this comment.
merge_slot_kvm_dirty_logs/merge_slot_kvm_dirty_bitmap
Also as I can see the read_log is used purely for the unit testing which is kinda sad.
Maybe if we could get a single Vec<u64> for the whole region composed of slots this would be much nicer api. What I am thinking about is bypassing kvm-ioctls and allocating a single Vec<u64> with size of slots * u64s_per_slot and then calling KVM_GET_DIRTY_LOG directly passing parts of this Vec into an ioctls. In the end we get a single Vec which represent the bitmap for the whole region which is trivial to merge.
Anyway, these are just my thoughts for now. Maybe it can be done in the future PRs.
There was a problem hiding this comment.
Went with merge_slot_kvm_dirty_logs. I'd still keep read_log, and the region sized buffer is a nice idea it was awkward and wasteful to have that additional translation step. Should this be upstreamed to kvm-ioctls or should we own the ioctls if it were to go through?
DirtyBitmap maps each KVM slot to its dirty log. Make it one bitmap per guest memory region, in region order, which is how Firecracker's bitmap, mincore and the memory file are laid out. Only KVM's log is per slot. This lets the next commits take Firecracker's bitmap of a region in one call to AtomicBitmap::get_and_reset. A slot's pages are then a slice of its region's bitmap (slot_words), so every hotpluggable slot must start on a bitmap word: its size must be a multiple of 64 host pages, 256 KiB with 4K pages. Configuration validation guarantees that, since hotpluggable slots are whole 2 MiB blocks. The constructor asserts it, and slot_size is now private so every region goes through a constructor. get_dirty_bitmap copies each plugged slot's log into its slice, dump_dirty writes each plugged slot from it, and store_dirty_bitmap marks a region's pages from it with set_bits. Firecracker always builds one full-size bitmap per region, so dump_dirty asserts the sizes instead of returning DirtyBitmapTooLarge or DirtyBitmapTooSmall, which are removed. Snapshots behave as before. Kani proves set_bits against a per-bit specification, for bitmaps of up to two words with at most two bits set per word, including that it keeps pages already marked. Signed-off-by: Jay Chung <jaehoc@amazon.com>
Add the two steps the next commit uses to take the dirty pages before a snapshot copies memory. take_dirty takes Firecracker's bitmap of each region with get_and_reset, which reads and clears each word in one atomic operation, so a page marked during the take is either taken or stays marked. It needs every region to track dirty pages. merge_slot_kvm_dirty_logs ORs KVM's log of each plugged slot, as the caller reads it, into its region's bitmap, and stops at the first log that fails to read, keeping the logs read before it. Each log must cover exactly its slot's words. The tests mark every page from another thread while takes run and check that no mark is lost, check that a take holds each region's KVM and Firecracker bits, and check what a failed log read leaves. Signed-off-by: Jay Chung <jaehoc@amazon.com>
A snapshot read Firecracker's dirty bitmap page by page while copying guest memory, then cleared it with a plain store. A page marked after its bit was read lost the mark to that store, and a page marked before the read but written again after its copy was cleared while the file held the older content. Both need a writer on another thread during the snapshot, which threaded device I/O and snapshots of a running microVM will bring. Two failures lose dirty pages today. The bitmaps were cleared before the memory file was flushed and synced, so a failed sync left the next Diff without those pages. And a failed read of KVM's dirty log dropped the slots already read, whose bits KVM had already cleared. Take the dirty pages before writing any memory, Full included: take Firecracker's bitmap of each region with take_dirty, then OR KVM's log of each plugged slot into it with merge_slot_kvm_dirty_logs. Write only the taken pages. If reading a log, writing, flushing or syncing the file fails, mark the taken pages dirty again. Without dirty tracking, a Diff writes the resident pages, as before. reset_dirty is now test-only. The tests make reading KVM's log, writing, flushing and syncing the file fail in turn and check the pages stay dirty. One writes a page right after the snapshot copied it and checks the next snapshot takes it again. Another writes pages from another thread while Diff snapshots run and checks that one more snapshot brings the file up to date. test_dirty_bitmap_success takes once before it counts, since a take also holds the pages Firecracker marked while booting. Signed-off-by: Jay Chung <jaehoc@amazon.com>
test_diff_snapshot_works_after_error fills the disk so that a Diff fails, then checks that a Diff after it restores a working microVM. A Full now takes the dirty bitmaps before writing memory too, so run the same test with a Full as the snapshot that fails. Signed-off-by: Jay Chung <jaehoc@amazon.com>
Snapshot creation now marks pages clean before it writes guest memory, and marks them dirty again if writing the memory file fails. Describe that, and list the fix under Fixed: a failed flush or sync of the memory file, or a failed read of KVM's dirty log, no longer loses dirty pages. Signed-off-by: Jay Chung <jaehoc@amazon.com>
0d47025 to
adb4e0c
Compare
Changes
Snapshot creation now takes the dirty pages before it copies guest memory. It takes Firecracker's bitmap of each region with
get_and_reset, which reads and clears each word in one atomic operation, then ORs KVM's dirty log of each plugged slot into it. If reading a log or writing the snapshot fails, it marks the taken pages dirty again.DirtyBitmapis now one bitmap per guest memory region, in region order, which is how Firecracker's bitmap,mincoreand the memory file are laid out. Only KVM's log is per slot.Reason
A snapshot read Firecracker's bitmap while it copied guest memory, then cleared the bitmap. A page that a device wrote during the copy lost its mark, and the next Diff snapshot missed it. No device writes during a snapshot today, but threaded device I/O will. A failed sync, or a KVM log read that failed partway, also dropped marks that the snapshot had already cleared.
Testing
Tests cover each failure path, a write during the copy, and concurrent marking. Kani proves
set_bits, which puts taken pages back.test_write_during_memory_snapshot_stays_dirtytest_write_during_memory_snapshot_stays_dirtytest_failed_memory_snapshot_keeps_dirty_pagestest_failed_dirty_log_read_keeps_firecracker_markstake_dirtyreading the bitmap, then clearing ittest_take_dirty_with_concurrent_marksPerformance
Time spent on dirty bitmaps in a 16 GiB snapshot, against main. It is the same whether devices (Firecracker's bitmap) or the guest (KVM's log) dirtied the pages.
Diff snapshots get faster because one atomic take per word replaces a read per page during the copy and a clear afterwards, and KVM's log is merged with plain ORs. Full snapshots with dirty tracking now take the bitmap instead of clearing it with a plain store, which costs more.
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 --allto verify that the PR passes build checks on allsupported architectures.
tools/devtool checkstyle --no-clippyto verify that the PR passes the automatedstyle checks.
the problem in a clear and encompassing way.
CHANGELOG.md.TODO.rust-vmm.