integrate vm-memory with dirty page tracking #2661

alindima · 2021-07-14T14:06:27Z

Reason for This PR

The vm-memory crate from rust-vmm has recently introduced abstractions for dirty page tracking, similar to the ones we had in Firecracker.
We need to consume them to avoid code duplication and to strip down the proxy crate we have in Firecracker.

Note that this PR is made on a feature branch.

Description of Changes

remove the Firecracker code responsible for dirty page tracking and the bitmap abstractions.
add a single function for creating the GuestMemoryMmap object, vm_memory::create_guest_memory. There were a lot of alternatives of creating the guest memory before this PR, which were hard to follow.
place and refactor the existing code for guard pages in the vm_memory::create_guest_memory function.
replace the deprecated usage of .with_regions() and .with_regions_mut() with .iter().
add a test utility for creating unguarded guest memory. This is needed because some tests need memory regions with sizes that are not multiples of 4096 (the default page size).
replace all test instantiations of GuestMemoryMmap with calls to create_guest_memory or the create_guest_memory_unguarded test utility (where needed).
port the unit tests for guard pages

The proxy crate is still needed to reexport the GuestMemory* types with the concrete type of B: Option<AtomicBitmap>. Otherwise we'd have to modify all the code that uses the GuestMemory* types, which wouldn't look nice.
It's also used for exporting the create_guest_memory() function.

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

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.

acatangiu · 2021-07-14T14:09:49Z

Seems like upstream feature is not stabilized yet: rust-vmm/vm-memory#158

alindima · 2021-07-14T14:28:08Z

Seems like upstream feature is not stabilized yet: rust-vmm/vm-memory#158

Thanks for pointing it out, that PR seems very fresh. I'm going to make this PR a draft and keep it this way until rust-vmm has a release

alindima · 2021-08-05T09:45:58Z

vm-memory 0.6.0 was released so I'll rebase this PR soon and mark it ready for review

alindima · 2021-08-06T12:02:22Z

PR is now ready for review

serban300

The PR looks good. I just have a couple of small comments, most of them related to the old code. As a high level comment, we usually make sure that each commit builds. I don't know of any exception so far. Can we do the same thing here while keeping the PR structure (keeping it split in multiple small commits in order to make it easy to review) ?

src/vm-memory/src/lib.rs

src/arch/src/aarch64/fdt.rs

src/vm-memory/src/lib.rs

sandreim

Changes look good overall. How far are we from dropping the local vm-memory crate from FC ?

src/vm-memory/src/lib.rs

tests/integration_tests/build/test_binary_size.py

dianpopa · 2021-08-11T14:19:15Z

src/vm-memory/Cargo.toml

@@ -5,6 +5,6 @@ authors = ["Amazon Firecracker team <[email protected]>"]
 edition = "2018"

 [dependencies]
-vm-memory-upstream = { package = "vm-memory", version = ">=0.2.2", features = ["backend-mmap"] }
+vm-memory-upstream = { package = "vm-memory",  version = "0.6.0", features = ["backend-mmap", "backend-bitmap"] }


So can t we use the vm-memory crate directly?

My take on this is that create_guest_memory should be placed in vmm since this is the glue code needed in order to make use of upstream vm-memory so i do not see the need to have a separate crate in fc for this

see this comment: #2661 (comment)

src/vm-memory/src/lib.rs

alindima · 2021-08-11T16:36:19Z

I see there are multiple comments related to dropping the vm-memory proxy crate from Firecracker.
I'll address them all in this one big comment, so that the answer is cohesive.

In a nutshell, what prevents us from removing the proxy crate are the following:

we need to have a wrapper for creating the guarded memory regions. This is currently only possible by using MmapRegion::build_raw. There is a very large number of tests which need to create guest memory objects. Moving the create_guest_memory function to the vmm crate would create a cyclic dependency and the code would not build. For example the arch crate needs this function, but vmm already depends on arch, so arch cannot depend on vmm as well.

The silver bullet here would be to leverage guarded memory regions from upstream vm-memory (which do not exist yet).

As per this closed RFC PR, @alexandruag said:

My current preference is to see what can be done in terms of simplications/performance improvements before changing how other things work (for example, if it makes sense to restrict the host addresses of the mapped regions to get contiguous areas, that might simplify using guard pages as well). Does the build_raw-based approach sound reasonable for the time being?

We could add a new feature request or contribute this to vm-memory, but I'd like your insight @alexandruag . Has anything changed from the time the previous PR was posted?
vm-memory v0.6.0 introduced a breaking change, since the GuestMemoryMmap type (and not only) is now parametrised with a Bitmap. Here is what the current PR does to hide this from the other parts of the codebase:
```
pub type GuestMemoryMmap = UpstreamGuestMemoryMmap<Option<AtomicBitmap>>;
pub type GuestRegionMmap = UpstreamGuestRegionMmap<Option<AtomicBitmap>>;
pub type MmapRegion = UpstreamMmapRegion<Option<AtomicBitmap>>;
```
The alternative is to change every single type and function in the codebase that has a member of the GuestMemoryMmap type to also be parametrised with the Bitmap type. IMHO this would make for some very ugly code, where types and functions that appear to have no business dealing with Bitmaps would be parametrised with a bitmap, also affecting code readability.

In my opinion, keeping the proxy crate implementation as it is now is the best way of currently handling the situation.

@sandreim @dianpopa @serban300 @alexandruag I'm more than happy to hear alternatives and please leave your thoughts here.

alindima · 2021-08-16T08:02:54Z

As a high level comment, we usually make sure that each commit builds. I don't know of any exception so far. Can we do the same thing here while keeping the PR structure (keeping it split in multiple small commits in order to make it easy to review) ?

Unfortunately, I can't do this without squashing into the first commit the commits adding modifications to the tests.
I'm inclined to keep the current commits, since they make for easier to review code and better structure.

WDYT @serban300 ?

alindima · 2021-08-16T08:03:05Z

We synced offline and decided that keeping the minimal proxy crate in the Firecracker is the right thing to do, for the reasons outlined above.

The PR is now ready for another review, I've addressed all of your comments @serban300 @dianpopa @sandreim

alindima · 2021-08-16T09:32:49Z

I've changed the target branch of this PR to be main, since vm-memory has had a release that we can consume.

src/vm-memory/src/lib.rs

src/arch/src/aarch64/fdt.rs

src/vm-memory/src/lib.rs

alindima · 2021-08-17T09:36:47Z

I am noticing some block throughput degradation with this PR, so let's wait until we conclude the investigation

dismissing review because we need further investigation for the block performance issue

serban300 · 2021-08-19T13:24:58Z

I am noticing some block throughput degradation with this PR, so let's wait until we conclude the investigation

This reminds me of an issue. Do you think we could try to address #2592 in this PR as well ? Or right after ? Should be easy to implement and it would confirm if the new vm-memory release offers the expected performance for read_from and write_to.

alindima · 2021-09-15T09:44:26Z

I am noticing some block throughput degradation with this PR, so let's wait until we conclude the investigation

This reminds me of an issue. Do you think we could try to address #2592 in this PR as well ? Or right after ? Should be easy to implement and it would confirm if the new vm-memory release offers the expected performance for read_from and write_to.

good call! This PR does in fact unblock the performance side of the vsock issue. The error handling is not solved so I posted an update and opened an issue in rust-vmm: rust-vmm/vm-memory#171

Signed-off-by: alindima <[email protected]>

This module exports a couple of test helpers for creating the guest memory. The first one is for creating the guest memory without guard pages. This is needed because the default `create_guest_memory` uses MmapRegionBuilder::build_raw() for setting up the memory with guard pages, which would error if the size is not a multiple of the page size. We need a test helper because We have unit tests which need a custom memory size, not a multiple of the page size. The second test helpers creates anonymous guest memory objects, that do not need a FileOffset object. It is just a little syntactic sugar that helps deduplicate test code. Signed-off-by: alindima <[email protected]>

Signed-off-by: alindima <[email protected]>

alindima · 2021-09-17T11:58:09Z

I am noticing some block throughput degradation with this PR, so let's wait until we conclude the investigation

Investigation was concluded. No issue is caused by vm-memory, so we are unblocking this PR

Signed-off-by: alindima <[email protected]>

alindima self-assigned this Jul 14, 2021

alindima marked this pull request as draft July 14, 2021 14:28

alindima mentioned this pull request Jul 14, 2021

Linux loader integration #2470

Merged

9 tasks

alindima added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Jul 14, 2021

dianpopa added the Status: Blocked Indicates that an issue or pull request cannot currently be worked on label Aug 5, 2021

alindima force-pushed the vm_memory_2 branch from 9f049a1 to 4b51b4a Compare August 6, 2021 11:52

alindima marked this pull request as ready for review August 6, 2021 11:58

alindima force-pushed the vm_memory_2 branch from 4b51b4a to 91e892e Compare August 6, 2021 12:00

alindima removed the Status: Blocked Indicates that an issue or pull request cannot currently be worked on label Aug 6, 2021

alindima requested a review from a team August 6, 2021 12:01

dianpopa self-requested a review August 6, 2021 13:51

serban300 self-requested a review August 11, 2021 08:28

serban300 reviewed Aug 11, 2021

View reviewed changes

sandreim reviewed Aug 11, 2021

View reviewed changes

dianpopa reviewed Aug 11, 2021

View reviewed changes

alindima mentioned this pull request Aug 13, 2021

Add codegen-units=1 #2699

Closed

alindima force-pushed the vm_memory_2 branch 2 times, most recently from 691f82f to 8a63ca0 Compare August 16, 2021 07:58

alindima force-pushed the vm_memory_2 branch 2 times, most recently from de09c1a to 2576452 Compare August 16, 2021 08:55

alindima changed the base branch from feature/rust_vmm_adoption to main August 16, 2021 09:30

alindima force-pushed the vm_memory_2 branch from 2576452 to dbb393b Compare August 16, 2021 09:31

alindima changed the title ~~[feature/rust_vmm_adoption] integrate vm-memory with dirty page tracking~~ integrate vm-memory with dirty page tracking Aug 16, 2021

serban300 mentioned this pull request Aug 16, 2021

bitmap: Calculate page count correctly #2695

Closed

9 tasks

serban300 reviewed Aug 16, 2021

View reviewed changes

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

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

src/arch/src/aarch64/fdt.rs Outdated Show resolved Hide resolved

src/vm-memory/src/lib.rs Show resolved Hide resolved

alindima force-pushed the vm_memory_2 branch from dbb393b to 8b051d2 Compare August 16, 2021 15:22

serban300 previously approved these changes Aug 16, 2021

View reviewed changes

sandreim previously approved these changes Aug 17, 2021

View reviewed changes

alindima requested a review from sandreim August 17, 2021 09:37

alindima mentioned this pull request Sep 15, 2021

use GuestMemory::read_from() and GuestMemory::write_to() in Vsock #2592

Closed

alindima added 5 commits September 17, 2021 12:15

vm-memory integration with dirty page tracking

b364005

Signed-off-by: alindima <[email protected]>

vm-memory: fix deprecated usage of with_regions*

8d78005

Signed-off-by: alindima <[email protected]>

vm-memory: update tests to use new interface

85ee031

Signed-off-by: alindima <[email protected]>

utils: add get_page_size function

ef5f012

Signed-off-by: alindima <[email protected]>

alindima dismissed serban300’s stale review via fcb695e September 17, 2021 11:56

alindima force-pushed the vm_memory_2 branch from 8b051d2 to fcb695e Compare September 17, 2021 11:56

update coverage

eb389b4

Signed-off-by: alindima <[email protected]>

alindima force-pushed the vm_memory_2 branch from fcb695e to eb389b4 Compare September 17, 2021 12:10

dianpopa approved these changes Sep 17, 2021

View reviewed changes

serban300 approved these changes Sep 17, 2021

View reviewed changes

alindima merged commit 830ee86 into firecracker-microvm:main Sep 17, 2021

AlexandruCihodaru mentioned this pull request Sep 18, 2021

kernel: fix bug in cmdline setup #2716

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrate vm-memory with dirty page tracking #2661

integrate vm-memory with dirty page tracking #2661

alindima commented Jul 14, 2021 •

edited

Loading

acatangiu commented Jul 14, 2021 •

edited

Loading

alindima commented Jul 14, 2021

alindima commented Aug 5, 2021

alindima commented Aug 6, 2021

serban300 left a comment

sandreim left a comment

dianpopa Aug 11, 2021

dianpopa Aug 11, 2021

alindima Aug 11, 2021

alindima commented Aug 11, 2021 •

edited

Loading

alindima commented Aug 16, 2021

alindima commented Aug 16, 2021

alindima commented Aug 16, 2021

alindima commented Aug 17, 2021

serban300 commented Aug 19, 2021

alindima commented Sep 15, 2021 •

edited

Loading

alindima commented Sep 17, 2021

integrate vm-memory with dirty page tracking #2661

integrate vm-memory with dirty page tracking #2661

Conversation

alindima commented Jul 14, 2021 • edited Loading

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

acatangiu commented Jul 14, 2021 • edited Loading

alindima commented Jul 14, 2021

alindima commented Aug 5, 2021

alindima commented Aug 6, 2021

serban300 left a comment

Choose a reason for hiding this comment

sandreim left a comment

Choose a reason for hiding this comment

dianpopa Aug 11, 2021

Choose a reason for hiding this comment

dianpopa Aug 11, 2021

Choose a reason for hiding this comment

alindima Aug 11, 2021

Choose a reason for hiding this comment

alindima commented Aug 11, 2021 • edited Loading

alindima commented Aug 16, 2021

alindima commented Aug 16, 2021

alindima commented Aug 16, 2021

alindima commented Aug 17, 2021

serban300 commented Aug 19, 2021

alindima commented Sep 15, 2021 • edited Loading

alindima commented Sep 17, 2021

alindima commented Jul 14, 2021 •

edited

Loading

acatangiu commented Jul 14, 2021 •

edited

Loading

alindima commented Aug 11, 2021 •

edited

Loading

alindima commented Sep 15, 2021 •

edited

Loading