fix: keep dirty memory bitmaps until snapshot is made #4383

Grandmother · 2024-01-19T14:49:30Z

Changes

Merge KVM bitmap of dirty pages into Firecracker internal one before going through dirty all pages and making a diff snapshot.
Erase Firecracker bitmap only after all snapshots are done to avoid loosing dirty pages in case when error happens while we're making a snapshot.

Reason

KVM bitmap of dirty pages got erased at the moment we read it so if we read it and fail to use (for creating diff snapshot) we want to keep it somewhere until then next attempt of making snapshot.

Closes #4545.

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

If a specific issue led to this PR, this PR closes the issue.
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
API changes follow the Runbook for Firecracker API changes.
User-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.
New TODOs link to an issue.
Commits meet contribution quality standards.

This functionality cannot be added in rust-vmm.

JonathanWoollett-Light · 2024-01-19T15:27:35Z

src/vmm/src/vstate/memory.rs

+    fn erase_dirty<>(&self) -> Result<(), MemoryError> {
+        self.iter()
+            .enumerate()
+            // FIXME: unused `slot`. Can we avoid having it at all instead of marking it with
+            // underscore?
+            .try_for_each(|(_slot, region)| {
+                let firecracker_bitmap = region.bitmap();
+                if let Some(bitmap) = firecracker_bitmap {
+                    bitmap.reset();
+                }
+
+                Ok(())
+            })
+            // FIXME: don't know which error would be better
+            .map_err(MemoryError::MmapRegionError)
+    }


Suggested change

fn erase_dirty<>(&self) -> Result<(), MemoryError> {

self.iter()

.enumerate()

// FIXME: unused `slot`. Can we avoid having it at all instead of marking it with

// underscore?

.try_for_each(|(_slot, region)| {

let firecracker_bitmap = region.bitmap();

if let Some(bitmap) = firecracker_bitmap {

bitmap.reset();

}

Ok(())

})

// FIXME: don't know which error would be better

.map_err(MemoryError::MmapRegionError)

}

fn erase_dirty(&self) {

for region in self.iter() {

if let Some(bitmap) = region.bitmap() {

bitmap.reset();

}

}

}

JonathanWoollett-Light · 2024-01-19T15:28:33Z

src/vmm/src/vstate/memory.rs

@@ -274,6 +277,26 @@ impl GuestMemoryExtension for GuestMemoryMmap {
            .map_err(MemoryError::WriteMemory)
    }

+    /// Erases the firecracker internal bitmap of dirty pages
+    /// FIXME: do we need to return error here?
+    /// FIXME: can we make it something like a private method? Or it's already not exposed anyway?


fn is the closest thing to private in Rust. There is only pub fn and fn.

JonathanWoollett-Light · 2024-01-19T15:28:45Z

src/vmm/src/vstate/memory.rs

@@ -274,6 +277,26 @@ impl GuestMemoryExtension for GuestMemoryMmap {
            .map_err(MemoryError::WriteMemory)
    }

+    /// Erases the firecracker internal bitmap of dirty pages
+    /// FIXME: do we need to return error here?


No, see https://github.com/firecracker-microvm/firecracker/pull/4383/files#r1459201110

JonathanWoollett-Light · 2024-01-19T15:31:28Z

src/vmm/src/vstate/memory.rs

        self.iter()
            .enumerate()
            .try_for_each(|(slot, region)| {
+                let firecracker_bitmap = region.bitmap();
                let kvm_bitmap = dirty_bitmap.get(&slot).unwrap();
+                for (i, v) in kvm_bitmap.iter().enumerate() {
+                    // FIXME: looks like this loop can be replaced with some
+                    // `(kvm_dirty_pages | fc_dirty_pages)`
+                    for j in 0..64 {
+                        let is_kvm_page_dirty = ((v >> j) & 1u64) != 0u64;
+                        let page_offset = ((i * 64) + j) * page_size;
+
+                        if is_kvm_page_dirty {
+                            // FIXME: not sure about passing page_size as len
+                            firecracker_bitmap.mark_dirty(page_offset, page_size)
+                        }
+                    }
+                }
+
+                Ok(())
+            })
+            .map_err(MemoryError::WriteMemory)


Suggested change

self.iter()

.enumerate()

.try_for_each(|(slot, region)| {

let firecracker_bitmap = region.bitmap();

let kvm_bitmap = dirty_bitmap.get(&slot).unwrap();

for (i, v) in kvm_bitmap.iter().enumerate() {

// FIXME: looks like this loop can be replaced with some

// `(kvm_dirty_pages | fc_dirty_pages)`

for j in 0..64 {

let is_kvm_page_dirty = ((v >> j) & 1u64) != 0u64;

let page_offset = ((i * 64) + j) * page_size;

if is_kvm_page_dirty {

// FIXME: not sure about passing page_size as len

firecracker_bitmap.mark_dirty(page_offset, page_size)

}

}

}

Ok(())

})

.map_err(MemoryError::WriteMemory)

for (slot, region) in self.iter().enumerate() {

let firecracker_bitmap = region.bitmap();

let kvm_bitmap = dirty_bitmap.get(&slot).unwrap();

for (i, v) in kvm_bitmap.iter().enumerate() {

for j in 0..64 {

let is_kvm_page_dirty = ((v >> j) & 1u64) != 0u64;

let page_offset = ((i * 64) + j) * page_size;

if is_kvm_page_dirty {

firecracker_bitmap.mark_dirty(page_offset, page_size);

}

}

}

pb8o · 2024-01-19T15:23:19Z

tools/devtool

@@ -309,7 +309,7 @@ run_devctr() {
        --rm \
        --volume /dev:/dev \
        --volume "$FC_ROOT_DIR:$CTR_FC_ROOT_DIR:z" \
-        --tmpfs /srv:exec,dev,size=32G \
+        --tmpfs /srv:exec,dev,size=1G \


hmm I will need to rethink this bit before it gets merged as it will affect other tests.

This will probably cause a lot of other tests to fail. You can drop this commit for now and we can run the tests.

pb8o · 2024-01-19T15:26:07Z

src/vmm/src/vstate/memory.rs

@@ -274,6 +277,26 @@ impl GuestMemoryExtension for GuestMemoryMmap {
            .map_err(MemoryError::WriteMemory)
    }

+    /// Erases the firecracker internal bitmap of dirty pages
+    /// FIXME: do we need to return error here?
+    /// FIXME: can we make it something like a private method? Or it's already not exposed anyway?


Yeah I think it's not exported unless you mark it as pub

pb8o · 2024-01-19T15:33:33Z

src/vmm/src/vstate/memory.rs

                for (i, v) in kvm_bitmap.iter().enumerate() {
+                    // FIXME: looks like this loop can be replaced with some


you mean fc_dirty = fc_dirty | kvm_dirty? Yeah, I think that would be simpler than iterating over each bit

Yes, something like that. Not sure yet what are the types of the fc_dirty and kvm_dirty.

codecov · 2024-01-19T16:33:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6414a07) 81.53% compared to head (11a2a5f) 81.55%.

❗ Current head 11a2a5f differs from pull request most recent head 2470ab4. Consider uploading reports for the commit 2470ab4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4383      +/-   ##
==========================================
+ Coverage   81.53%   81.55%   +0.01%     
==========================================
  Files         241      241              
  Lines       29333    29364      +31     
==========================================
+ Hits        23916    23947      +31     
  Misses       5417     5417

Flag	Coverage Δ
4.14-c7g.metal	`76.94% <100.00%> (+0.02%)`	⬆️
4.14-m5d.metal	`78.87% <100.00%> (+0.01%)`	⬆️
4.14-m6a.metal	`77.99% <100.00%> (+0.02%)`	⬆️
4.14-m6g.metal	`76.94% <100.00%> (+0.02%)`	⬆️
4.14-m6i.metal	`78.86% <100.00%> (+0.02%)`	⬆️
5.10-c7g.metal	`79.84% <100.00%> (+0.02%)`	⬆️
5.10-m5d.metal	`81.52% <100.00%> (+0.01%)`	⬆️
5.10-m6a.metal	`80.74% <100.00%> (+0.02%)`	⬆️
5.10-m6g.metal	`79.84% <100.00%> (+0.02%)`	⬆️
5.10-m6i.metal	`81.52% <100.00%> (+0.02%)`	⬆️
6.1-c7g.metal	`79.84% <100.00%> (+0.02%)`	⬆️
6.1-m5d.metal	`81.53% <100.00%> (+0.02%)`	⬆️
6.1-m6a.metal	`80.74% <100.00%> (+0.02%)`	⬆️
6.1-m6g.metal	`79.84% <100.00%> (+0.02%)`	⬆️
6.1-m6i.metal	`81.52% <100.00%> (+0.02%)`	⬆️

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.

mancio-aws · 2024-01-19T17:31:47Z

src/vmm/src/vstate/memory.rs

                let firecracker_bitmap = region.bitmap();
+                let kvm_bitmap = dirty_bitmap.get(&slot).unwrap();


do we need this at all? can't we just iterate over firecracker_bitmap?

Yes, we don't need KVM bitmap after we merged it into the FC one.

During snapshot there might be an error. If it happens, all the dirty pages bitmaps were lost. Now all the dirty pages bitmaps will be stored in firecracker internal bitmap untill next successfull snapshot. Signed-off-by: Roman Kovtyukh <HelloDearGrandma@gmail.com>

After having a failure during snapshot we're trying to do it again and expect all the dirty pages to be snapshotted on the second attempt if it succeeds. Signed-off-by: Roman Kovtyukh <HelloDearGrandma@gmail.com> Co-authored-by: Pablo Barbáchano <pablob@amazon.com>

Grandmother force-pushed the feature/keep-dirty-memory-bitmaps-until-snapshot-is-made branch 3 times, most recently from 6a46793 to 41ac44d Compare January 19, 2024 15:19

JonathanWoollett-Light reviewed Jan 19, 2024

View reviewed changes

pb8o reviewed Jan 19, 2024

View reviewed changes

Grandmother force-pushed the feature/keep-dirty-memory-bitmaps-until-snapshot-is-made branch 2 times, most recently from 8bc31ce to be5e263 Compare January 19, 2024 16:28

Grandmother changed the title ~~Feature/keep dirty memory bitmaps until snapshot is made~~ fix: keep dirty memory bitmaps until snapshot is made Jan 19, 2024

mancio-aws reviewed Jan 19, 2024

View reviewed changes

Grandmother force-pushed the feature/keep-dirty-memory-bitmaps-until-snapshot-is-made branch from 11a2a5f to 97ad1c8 Compare January 19, 2024 18:23

Grandmother force-pushed the feature/keep-dirty-memory-bitmaps-until-snapshot-is-made branch from 97ad1c8 to 2470ab4 Compare January 19, 2024 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: keep dirty memory bitmaps until snapshot is made #4383

fix: keep dirty memory bitmaps until snapshot is made #4383

Grandmother commented Jan 19, 2024 •

edited by JonathanWoollett-Light

JonathanWoollett-Light Jan 19, 2024 •

edited

JonathanWoollett-Light Jan 19, 2024

JonathanWoollett-Light Jan 19, 2024

JonathanWoollett-Light Jan 19, 2024

pb8o Jan 19, 2024

pb8o Jan 19, 2024

pb8o Jan 19, 2024

Grandmother Jan 19, 2024

codecov bot commented Jan 19, 2024 •

edited

mancio-aws Jan 19, 2024

Grandmother Jan 19, 2024

		for (i, v) in kvm_bitmap.iter().enumerate() {
		// FIXME: looks like this loop can be replaced with some

		let firecracker_bitmap = region.bitmap();
		let kvm_bitmap = dirty_bitmap.get(&slot).unwrap();

fix: keep dirty memory bitmaps until snapshot is made #4383

Are you sure you want to change the base?

fix: keep dirty memory bitmaps until snapshot is made #4383

Conversation

Grandmother commented Jan 19, 2024 • edited by JonathanWoollett-Light

Changes

Reason

License Acceptance

PR Checklist

JonathanWoollett-Light Jan 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 19, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Grandmother commented Jan 19, 2024 •

edited by JonathanWoollett-Light

JonathanWoollett-Light Jan 19, 2024 •

edited

codecov bot commented Jan 19, 2024 •

edited