Start dirty page tracking implementation in vm-memory. #2149

sandreim · 2020-09-29T11:04:35Z

Reason for This PR

Bootstrap implementation for dirty page tracking in vm-memory - #1997

Thanks @alexandruag for providing insight/PoC on the proxy crate approach.

Description of Changes

Create a "proxy" crate that Firecracker will use as a dependency for all current vm-memory imports. This crate will export local GuestMemoryMmap and GuestRegionMap implementations that will contain dirty page tracking functionality. All other re-exports will be fetched from upstream vm-memory.

This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The reason for this PR is clearly provided (issue no. or explanation).
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes are reflected in firecracker/swagger.yaml.
Any user-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.

src/arch/Cargo.toml

alxiord · 2020-09-29T12:12:00Z

I may have gotten it backwards - I was thinking that Firecracker will continue consuming rust-vmm/vm-memory, but instead of using rust-vmm/vm-memory::GuestMemoryMmap all over the place, Firecracker would add its own struct GuestMemoryMmapWithTracking (or something) and impl rust-vmm/vm-memory::GuestMemory for firecracker::GuestMemoryMmapWithTracking (disregard rustlang blasphemy). That way we don't have to kill the dependency. Did we move away from that? What's the advantage of removing the dependency, other than renaming GuestMemoryMmap everywhere?

sandreim · 2020-09-29T12:26:05Z

I may have gotten it backwards - I was thinking that Firecracker will continue consuming rust-vmm/vm-memory, but instead of using rust-vmm/vm-memory::GuestMemoryMmap all over the place, Firecracker would add its own struct GuestMemoryMmapWithTracking (or something) and impl rust-vmm/vm-memory::GuestMemory for firecracker::GuestMemoryMmapWithTracking (disregard rustlang blasphemy). That way we don't have to kill the dependency. Did we move away from that? What's the advantage of removing the dependency, other than renaming GuestMemoryMmap everywhere?

The dependency to vm-memory from upstream is kept in the local proxy crate vm-memory-proxy and we override only what is needed with a local implementation. The rest of the FC code depends on the proxy crate which re-exports the upstream stuff.

alxiord · 2020-09-29T14:17:46Z

we override only what is needed with a local implementation

Why not create a new implementor for GuestMemory?

raduweiss · 2020-09-29T14:22:58Z

we override only what is needed with a local implementation

Why not create a new implementor for GuestMemory?

FWIW I also understood the coupling in the way @aghecenco described it above.

sandreim · 2020-09-29T14:37:37Z

we override only what is needed with a local implementation

Why not create a new implementor for GuestMemory?

FWIW I also understood the coupling in the way @aghecenco described it above.

The goal here is to do as few changes as possible to the interfaces we currently use in Firecracker with the hope that tracking will be available in upstream. The Firecracker code uses both the GuestMemory interface and also the GuestMemoryMmap backend directly. If we implement a new backend like GuestMemoryMmapTracked we would need to do more changes in Firecracker.

The approach in this PR is less invasive and completely transparent to Firecracker as we re-export all the stuff from upstream and we override only what's needed to support our usecase.

src/vm-memory/Cargo.toml

- Import mmap.rs from rust-vmm/vm-memory v0.2.2. - Added markers where write access needs to mark dirty bits in bitmap. Signed-off-by: Andrei Sandu <[email protected]>

src/vm-memory/src/bitmap.rs

Signed-off-by: Andrei Sandu <[email protected]>

- update Cargo.toml for all crates - implement AsSlice interface for GuestRegionMmap. Signed-off-by: Andrei Sandu <[email protected]>

Signed-off-by: Andrei Sandu <[email protected]>

luminitavoicu · 2020-10-02T05:59:26Z

src/vm-memory/src/mmap.rs

+    // Windows needs a dedicated test where it will retrieve the allocation
+    // granularity to determine a proper offset (other than 0) that can be
+    // used for the backing file. Refer to Microsoft docs here:
+    // https://docs.microsoft.com/en-us/windows/desktop/api/memoryapi/nf-memoryapi-mapviewoffile


I don't think we should we keep this indication.

You are right.

acatangiu · 2020-10-02T11:18:08Z

src/vm-memory/src/lib.rs

+// Portions Copyright 2017 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the THIRD-PARTY file.


nit: is this still the case?

acatangiu · 2020-10-02T11:19:48Z

src/vm-memory/src/mmap.rs

+// Copyright (C) 2019 Alibaba Cloud Computing. All rights reserved.
+//
+// Portions Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.


nit: For this local file where we add dirty tracking support, these two might be better reversed.

acatangiu · 2020-10-02T11:29:26Z

src/vm-memory/src/bitmap.rs

+    pub fn new(byte_size: usize, page_size: usize) -> Self {
+        // Bit size is the number of bits in the bitmap, always at least 1 (to store the state of
+        // the '0' address).
+        let bit_size = std::cmp::max(1, byte_size / page_size);


nit:

Suggested change

let bit_size = std::cmp::max(1, byte_size / page_size);

let num_pages = std::cmp::max(1, byte_size / page_size);

for clarity.

acatangiu · 2020-10-02T11:33:40Z

src/vm-memory/src/mmap.rs

+    // TODO: This implementation is temporary.
+    // We need to return None here once we refactor vsock.


Let's add an issue to track this.

Created #2153

sandreim changed the title ~~Start dirty page tracking implementation.~~ Start dirty page tracking implementation in vm-memory. Sep 29, 2020

sandreim force-pushed the local_memory_backend branch 2 times, most recently from 0e20ced to d6f1935 Compare September 29, 2020 11:27

serban300 reviewed Sep 29, 2020

View reviewed changes

src/arch/Cargo.toml Outdated Show resolved Hide resolved

sandreim force-pushed the local_memory_backend branch from d6f1935 to 46bf383 Compare September 29, 2020 12:27

sandreim requested a review from serban300 September 29, 2020 12:31

sandreim force-pushed the local_memory_backend branch from 46bf383 to baa0a01 Compare September 29, 2020 12:36

sandreim requested a review from acatangiu September 29, 2020 12:37

alxiord reviewed Sep 29, 2020

View reviewed changes

src/vm-memory/Cargo.toml Outdated Show resolved Hide resolved

sandreim force-pushed the local_memory_backend branch 2 times, most recently from de40fbc to fe74a58 Compare September 30, 2020 06:10

Create the vm-memory proxy crate.

3db9e5d

- Import mmap.rs from rust-vmm/vm-memory v0.2.2. - Added markers where write access needs to mark dirty bits in bitmap. Signed-off-by: Andrei Sandu <[email protected]>

sandreim force-pushed the local_memory_backend branch from fe74a58 to 919410c Compare September 30, 2020 06:36

sandreim requested a review from alxiord September 30, 2020 06:37

sandreim force-pushed the local_memory_backend branch from 919410c to ba4635c Compare September 30, 2020 06:39

luminitavoicu reviewed Sep 30, 2020

View reviewed changes

src/vm-memory/src/bitmap.rs Outdated Show resolved Hide resolved

sandreim force-pushed the local_memory_backend branch from ba4635c to 31b92ea Compare September 30, 2020 09:01

sandreim requested a review from luminitavoicu September 30, 2020 09:02

sandreim added 3 commits September 30, 2020 13:31

Implement dirty Bitmap object.

7aa10bc

Signed-off-by: Andrei Sandu <[email protected]>

Switch to local vm-memory proxy.

2002ffd

- update Cargo.toml for all crates - implement AsSlice interface for GuestRegionMmap. Signed-off-by: Andrei Sandu <[email protected]>

Update AMD coverage.

d086a32

Signed-off-by: Andrei Sandu <[email protected]>

sandreim force-pushed the local_memory_backend branch from 31b92ea to d086a32 Compare September 30, 2020 10:31

sandreim self-assigned this Sep 30, 2020

luminitavoicu approved these changes Oct 1, 2020

View reviewed changes

dianpopa added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Oct 1, 2020

luminitavoicu reviewed Oct 2, 2020

View reviewed changes

acatangiu approved these changes Oct 2, 2020

View reviewed changes

sandreim merged commit c468676 into firecracker-microvm:feature/diff-snapshot Oct 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start dirty page tracking implementation in vm-memory. #2149

Start dirty page tracking implementation in vm-memory. #2149

sandreim commented Sep 29, 2020 •

edited

Loading

alxiord commented Sep 29, 2020

sandreim commented Sep 29, 2020

alxiord commented Sep 29, 2020

raduweiss commented Sep 29, 2020

sandreim commented Sep 29, 2020 •

edited

Loading

luminitavoicu Oct 2, 2020

sandreim Oct 2, 2020

acatangiu Oct 2, 2020

acatangiu Oct 2, 2020

acatangiu Oct 2, 2020

acatangiu Oct 2, 2020

sandreim Oct 2, 2020

	let bit_size = std::cmp::max(1, byte_size / page_size);
	let num_pages = std::cmp::max(1, byte_size / page_size);

		// TODO: This implementation is temporary.
		// We need to return None here once we refactor vsock.

Start dirty page tracking implementation in vm-memory. #2149

Start dirty page tracking implementation in vm-memory. #2149

Conversation

sandreim commented Sep 29, 2020 • edited Loading

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

alxiord commented Sep 29, 2020

sandreim commented Sep 29, 2020

alxiord commented Sep 29, 2020

raduweiss commented Sep 29, 2020

sandreim commented Sep 29, 2020 • edited Loading

luminitavoicu Oct 2, 2020

Choose a reason for hiding this comment

sandreim Oct 2, 2020

Choose a reason for hiding this comment

acatangiu Oct 2, 2020

Choose a reason for hiding this comment

acatangiu Oct 2, 2020

Choose a reason for hiding this comment

acatangiu Oct 2, 2020

Choose a reason for hiding this comment

acatangiu Oct 2, 2020

Choose a reason for hiding this comment

sandreim Oct 2, 2020

Choose a reason for hiding this comment

sandreim commented Sep 29, 2020 •

edited

Loading

sandreim commented Sep 29, 2020 •

edited

Loading