Add dirty bitmap tracking abstractions #125

alexandruag · 2020-12-01T15:41:34Z

This PR contains an initial proposal to tackle the problem of dirty page/region tracking within a GuestMemory object, which happens at the level of each individual region here. We add the Bitmap interface, the BitmapSlice abstraction (which is passed to objects such as VolatileSlices to enable tracking local accesses in the context of the outer GuestMemoryRegion), and begin to add the necessary invocations of tracking functionality around write accesses. Here are some details and open questions:

The PR also removes several things (for example, the VolatileMemory impls for MmapRegion and &mut [u8]). I did a quick investigation and it looked like these implementations where not specifically used outside the crate, and I think this also simplifies things a bit. They can be added back and made compatible with the current changes if necessary.
An open question is what to do (w.r.t. tracking) with methods that return mutable slices or references (the bigger question is whether we should actually have them at all, but that's outside of the current scope :D). My preference is to add the lack of tracking to the safety contract of methods like as_mut_slice (i.e. the caller must manually mark the areas that get written to), and to remove (either completely or just from the public interface) methods such as get_atomic_ref and aligned_as_mut from VolatileMemory. One alternative is to treat them like as_mut_slice, but this kinda means we're increasing the number of moving parts that have to be taken into account, and most of their use cases are already covered by the Bytes interface (there's just load and store for atomic ops right now, but we can add the rest as well if required). WDYT?
A potentially significant (and breaking) change is the additional generic type parameter for Volatile{Slice,Ref,ArrayRef}. Its purpose is turning this into a zero-cost abstraction (as far as accesses are concerned), and to allow different tracking implementations to be used. The change can become unwieldy if there are a lot of fields/variable definitions that explicitly mention the type, and one way around this is to create a type alias instead of replacing everything. GuestMemoryMmap and GuestRegionMmap also get an additional type parameter which represents the bitmap implementation in use.

Looking forward to any comments! If this makes sense, I'll start adding the scaffolding required to test things for genericBytes implementations as well as specific mutable accesses that are enabled by the various vm-memory primitives.

rbradford · 2020-12-01T15:46:01Z

@alexandruag We have live migration working in Cloud Hypervisor now taking advantage of the KVM region tracking. I did wonder if we could be in a situation where updates from the VMM side i.e. via GuestMemory might be an issue. The fact you've created this PR makes me think you're seeing this in practice.

rbradford · 2020-12-01T15:51:22Z

@alexandruag It looks good. But don't need a way to atomically clear the bitmap? Like how the KVM_GET_DIRTY_LOG ioctl does so that for live migration you can only transfer the needed pages.

alexandruag · 2020-12-01T16:59:44Z

Hi Rob, thanks for the quick reply! Yeah, during previous experiments we noticed the KVM dirty log did not catch some of the write accesses performed by the VMM (which makes sense if stuff like PML is used). Regarding your second question, the Bitmap interface here is mainly trying to define the methods required by GuestMemory backends to record tracking information. Concrete Bitmap implementations will most likely have additional functionality like the one you mention, and we can def augment the interface to support those as well; I just wanted to start by looking at what it takes to interact with GuestMemory implementations.

src/bitmap.rs

src/bytes.rs

src/guest_memory.rs

src/mmap_unix.rs

jiangliu · 2021-01-04T17:22:32Z

Seems it's a chance to get rid of some unused interfaces, we could add them back later when they are really used.
I think the overall design to extend VolatileXxx to assist dirty track should be ok, it's much more safe to let the user of Volatile interfaces do dirty tracking.
For unsafe interfaces returns *mut u8, it should be ok to let the user track dirty status.

alexandruag · 2021-01-18T11:34:22Z

Thanks for the comments Gerry, I'll start addressing them soon.

luminitavoicu · 2021-01-18T11:36:47Z

src/volatile_memory.rs

+        self.bitmap.mark_dirty(addr, count);
+        src.read_exact(dst).map_err(Error::IOError)


In case the read_exact function fails here, we still mark the pages as dirty. Wouldn't it be best to update the bitmap only in case the write succeeds?

Suggested change

self.bitmap.mark_dirty(addr, count);

src.read_exact(dst).map_err(Error::IOError)

src.read_exact(dst).map_err(Error::IOError)?;

self.bitmap.mark_dirty(addr, count)

I've added the mark_dirty first since even if the memory operation fails, I think we can still have partial writes to guest memory, and they are implicitly covered if the entire range was marked already. It is possible to generate false positives this way, but I think they are not significant errors in the context of tracking dirty memory areas.

alxiord · 2021-01-22T10:14:18Z

src/bitmap.rs

+}
+
+/// Represents a slice into a `Bitmap` object, starting at `base_offset`.
+pub struct BitmapSlice<'a, B> {


Why doesn't a BitmapSlice have a size?

Hmm, I tried to keep the interface to a minimum, and I'd leave it as is for now until we identify a use case that explicitly needs this (sorry if I missed any).

alxiord · 2021-01-22T10:16:47Z

src/bitmap.rs

+    fn new(size: usize) -> Self;
+
+    /// Mark the memory range specified by the given `offset` and `len` as dirtied.
+    fn mark_dirty(&self, offset: usize, len: usize);


What would a impl Bitmap for Foo look like - more precisely, what would Foo contain to represent the bitmap? Mult be something with interior mutability so you can get away with &self here - are you thinking direct pointer manipulation? Splitting the bitmap representation into 64-bit chunks and using Atomics? Something else?

It's up to the implementer; anything goes as long as it can implement the interface. Using a sequence of AtomicU64s does seem like one of the options that first come to mind, and IIRC the implementation used for tracking by Firecracker does this.

alexandruag

Hi everyone, thanks for the comments! I've updated the PR, and the biggest change is that I used HRTBs (drawing inspiration from @Daniel-Aaron-Bloom's PR that was merged a while ago) to turn the tracking into a zero-cost abstraction (no additional operations are performed or space required when a trivial Bitmap implementation is used). This does make some signatures a bit more complex, but I've also added some type aliases to help with that.

Instead of the Bitmap trait having constructors itself, I've defined a NewBitmap trait meant to be used by GuestRegionMmap to instantiate bitmap objects without having to change its current interface (i.e. such that bitmaps are moved over at construction time). Another thing I'm considering is to rename GuestMemoryMmap and GuestRegionMmap to something else, and use the original names for type aliases that imply using a trivial bitmap implementation by default. This should minimize the amount of changes required by vm-memory consumers that do not need the new functionality.

The next steps are to fix the unit tests, add some generic tests that can be used with any Bitmap implementation, and provide a backend implementation as well. I'll update this PR with the tests as soon as they're done, and I'm looking forward to any feedback regarding the changes so far.

src/bitmap.rs

alexandruag · 2021-02-22T17:45:38Z

src/bitmap.rs

+    fn new(size: usize) -> Self;
+
+    /// Mark the memory range specified by the given `offset` and `len` as dirtied.
+    fn mark_dirty(&self, offset: usize, len: usize);


It's up to the implementer; anything goes as long as it can implement the interface. Using a sequence of AtomicU64s does seem like one of the options that first come to mind, and IIRC the implementation used for tracking by Firecracker does this.

src/bitmap.rs

alexandruag · 2021-02-22T17:54:43Z

src/bitmap.rs

+}
+
+/// Represents a slice into a `Bitmap` object, starting at `base_offset`.
+pub struct BitmapSlice<'a, B> {


Hmm, I tried to keep the interface to a minimum, and I'd leave it as is for now until we identify a use case that explicitly needs this (sorry if I missed any).

src/bitmap.rs

src/guest_memory.rs

src/mmap_unix.rs

alexandruag · 2021-02-22T18:03:50Z

src/volatile_memory.rs

+        self.bitmap.mark_dirty(addr, count);
+        src.read_exact(dst).map_err(Error::IOError)


I've added the mark_dirty first since even if the memory operation fails, I think we can still have partial writes to guest memory, and they are implicitly covered if the entire range was marked already. It is possible to generate false positives this way, but I think they are not significant errors in the context of tracking dirty memory areas.

rbradford · 2021-03-02T15:01:34Z

@alexandruag Would you also be willing to include a simple "off the shelf" Bitmap tracking implementation so every user does not need to construct their own?

bonzini · 2021-03-02T15:41:39Z

Sorry for not noticing this PR until now.

There is one important issue with attaching the bitmap to the GuestMemory, which is that the memory addresses can change but the dirty bitmap should not. Think for example of a large RAM region for an emulated graphics card; because it is a PCI BAR, the base address can move freely.

So I'm thinking of a different design where the bitmap is attached to the GuestRegion instead, for example as a decorator where you'd create a GuestMemory<DirtyTrackedRegion<MmapRegion>>. Do you think this would be hard to implement?

Regarding removing the impl VolatileMemory for MmapRegion, I'm not sure what you gain from removing it; you are basically inlining two methods, but there is almost no simplification. Likewise, impl VolatileMemory for &'a mut [u8] has at least a didactic value, as it shows that VolatileMemory is something that applies to sequences of bytes. In any case there was an agreement to move those to a separate PR, so there's no need to worry about it now.

alexandruag · 2021-03-05T07:25:56Z

@rbradford Yes, the plan is to include a backend implementation as well (guarded by a feature, like backend-mmap for example).

@bonzini Thanks for having a look! The bitmap(s) are already attached to individual regions, and the tracking is done relative to the start of each particular region. Is this different from what you were describing?

bonzini · 2021-03-05T08:28:50Z

The bitmap(s) are already attached to individual regions

You're right, I misread the code. I have now looked at Firecracker too, and the main difference with my proposal in #140 is that Firecracker handles external bitmaps (such as KVM's) by hand in SnapshotMemory::dump_dirty.

alexandruag · 2021-03-15T14:01:32Z

Hi! I’ve updated the PR with backend implementations (RefSlice based on the previous BitmapSlice struct, and AtomicBitmap which is lifted from the current Firecracker Bitmap object implementation) and testing. I’ve added generic unit test methods which can be exported at some point so they can be used by other crates/implementations as well.

I’ve reverted the code removals with the exception of the original VolatileMemory implementation for MmapRegion. The main reason is that VolatileMemory now has the B: Bitmap associated type which doesn’t seem to fit the MmapRegion semantics very well IMO. At the same time, if anyone would like to see this change reverted as well, I’ll add the VolatileMemory implementation back, and change MmapRegion to MmapRegion<B> (similar to what happened to GuestRegionMmap).

alexandruag · 2021-03-15T14:11:02Z

@bonzini @jiangliu @aghecenco @rbradford @luminitavoicu can you please take another look? I'll go over the orderings in the AtomicBitmap implementation again as well, and some doc tests still need to be fixed.

alxiord

Did a first pass and overall LGTM! I find the naming scheme a bit confusing with all the new traits and implementors, as it's hard to figure out which is which, but I have no better suggestions (others than those in comments). When this gets "promoted" to non-RFC, I think dedicated benchmarks could be useful in demonstrating the zero part of the zero cost abstractions.

alxiord · 2021-03-15T15:45:55Z

src/bitmap/mod.rs

+{
+}
+
+/// Common bitmap operations. Using HRTBs to effectively define an associated type that


nit: I had to search what HRTB stands for; maybe add the full term in parentheses when you first introduce the abbreviation?

alxiord · 2021-03-15T15:49:22Z

src/bitmap/mod.rs

+
+/// Helper type alias for referring to the `BitmapSlice` concrete type associated with
+/// an object `B: WithBitmapSlice<'a>`.
+pub type BS<'a, B> = <B as WithBitmapSlice<'a>>::S;


nit: I'd be more verbose with these exported type names, rather than the very abbreviated form (particularly BS). Maybe BitmapSliceT?

Yeah, I don't know what the best solution is here. Having the very abbreviated forms helps with keeping type declarations or function return types shorter whenever we need to explicitly mention them. I prefer the shorthand notation, but we can def think about different possibilities here some more until the PR is ready to be merged otherwise.

alxiord · 2021-03-15T15:56:39Z

src/bitmap/backend/atomic_bitmap.rs

+#[cfg(feature = "backend-mmap")]
+use crate::mmap::NewBitmap;
+
+/// `Bitmap` implements a simple bit map on the page level with test and set operations. It is


s/Bitmap/AtomicBitmap

src/bitmap/backend/atomic_bitmap.rs

bonzini · 2021-03-15T16:20:51Z

I'm not sure why VolatileMemory would necessarily have a bitmap associated type; the tests show that even an [u8] makes sense as a VolatileMemory instance.

I'll take a look at making a decorator-like implementation where you have DirtyTrackedRegion<MmapRegion>, but being able to use either MmapRegion<()> or MmapRegion<AtomicBitmap> would be fine as well.

alexandruag · 2021-03-16T09:49:19Z

The B associated type for VolatileMemory is used to determine what kind of dirty tracking slice is embedded into the VolatileSlice, VolatileRef, and VolatileArrayRef objects that are returned by methods such as get_slice, get_ref, etc.

alexandruag

Hi again! I've addressed the recently added comments, and I also pushed a new version where the previous removals around MmapRegion are reverted, and the struct now has an inner object used for tracking (i.e. it's now MmapRegion<B>). I have to fix some of the doc tests and Windows-specific logic as well, and then I'll bump the status to an actual PR.

I think that adding benchmarks to verify that the newly added abstractions really are zero-cost is a bit heavy-weight; so far I've validated the expectations in this regard mainly by looking at snippets of compiler output (with the regular release optimizations enabled). We can def revisit the idea if folks think it's valuable going forward.

Looking forward to any additional feedback!

alexandruag · 2021-03-30T18:42:35Z

src/bitmap/mod.rs

+{
+}
+
+/// Common bitmap operations. Using HRTBs to effectively define an associated type that


alexandruag · 2021-03-30T18:45:04Z

src/bitmap/mod.rs

+
+/// Helper type alias for referring to the `BitmapSlice` concrete type associated with
+/// an object `B: WithBitmapSlice<'a>`.
+pub type BS<'a, B> = <B as WithBitmapSlice<'a>>::S;


Yeah, I don't know what the best solution is here. Having the very abbreviated forms helps with keeping type declarations or function return types shorter whenever we need to explicitly mention them. I prefer the shorthand notation, but we can def think about different possibilities here some more until the PR is ready to be merged otherwise.

alexandruag · 2021-03-30T18:47:52Z

src/bitmap/backend/atomic_bitmap.rs

+#[cfg(feature = "backend-mmap")]
+use crate::mmap::NewBitmap;
+
+/// `Bitmap` implements a simple bit map on the page level with test and set operations. It is


src/bitmap/backend/atomic_bitmap.rs

As the first step to complete live-migration with tracking dirty-pages written by the VMM, this patch patches the dependent vm-memory crate to the development branch with dirty-page-tracking capability [1]. For now, we are using our own fork of the vm-memory crate [2] before the feature is landed in the upstream. Most changes are due to the updated `GuestMemoryMmap`, `GuestRegionMmap`, and `MmapRegion` structs which are taking an additional generic type parameter (to specify what 'bitmap backend' is used). For the same reason, similar changes are required in the dependent 'vfio-ioctls' crate (that uses the vm-memory crate). We are also using our own fork of the vfio-ioctls crate before the upstream is updated [3]. The above changes should be transparent to the rest of the code base, e.g. all unit/integration tests should pass without additional changes. [1] rust-vmm/vm-memory#125 [2] https://github.com/cloud-hypervisor/vm-memory/tree/ch [3] https://github.com/cloud-hypervisor/vfio-ioctls/tree/ch Signed-off-by: Bo Chen <chen.bo@intel.com>

jiangliu · 2021-05-27T03:24:16Z

@bonzini so from what I understand, you're proposing to use the decorator approach with objects such as VolatileSlice as well. The issue here is that unlike decorators for GuestMemoryMmap and region (which are still abstracted by the GuestMemory etc. interfaces), VolatileSlice is itself part of the vm-memory interface, so it has to support tracking for the underlying memory range. Right now, the status of this PR w.r.t. to high level changes that affect all users is as follows:

The VolatileSlice, VolatileRef, and VolatileArrayRef objects are essentially the same (after using Gerry's suggestion to leverage the default generic parameters feature). The tracking logic is there, but it's implicitly using the zero-cost trivial implementation and the default generic parameters ensure older type signatures that don't care about the new functionality are still valid.

The GuestMemoryRegion and VolatileMemory traits have a new associated type, but it can simply be set to () when not interesting for a particular implementation.

GuestMemoryMmap, GuestRegionMmap, and MmapRegion now have a generic B: Bitmap type parameter. The preferred way, from my point of view, to work around this is to use type aliases (for example, something like type GuestMemoryMmap = vm_memory::GuestMemoryMmap<()>), which can also be placed project-wide to re-export a definition for an entire code base. The extra type parameter is also transparent in generic usage contexts (i.e. for objects behind a M: GuestMemory bound). If we want to reduce the number of visible changes to the absolute minimum, then we can provide upstream. For example, we can rename the generic GuestMemoryMmap<B> to something like GuestMemoryMmapBase<B> and export a type GuestMemoryMmap = GuestMemoryMmapBase<()> alias together with the base type itself directly from vm_memory (same for GuestRegionMmap and MmapRegion). This looks more cumbersome overall to me, but it's def an option to be considered.

Things like "GuestMemoryMmapBase" may be a burden for maintenance.
I think current solution is OK. With the default generic parameter applied, it should need too much effort to migrate existing code.

As the first step to complete live-migration with tracking dirty-pages written by the VMM, this patch patches the dependent vm-memory crate to the development branch with dirty-page-tracking capability [1]. For now, we are using our own fork of the vm-memory crate [2] before the feature is landed in the upstream. Most changes are due to the updated `GuestMemoryMmap`, `GuestRegionMmap`, and `MmapRegion` structs which are taking an additional generic type parameter (to specify what 'bitmap backend' is used). For the same reason, similar changes are required in the dependent 'vfio-ioctls' crate (that uses the vm-memory crate). We are also using our own fork of the vfio-ioctls crate before the upstream is updated [3]. The above changes should be transparent to the rest of the code base, e.g. all unit/integration tests should pass without additional changes. [1] rust-vmm/vm-memory#125 [2] https://github.com/cloud-hypervisor/vm-memory/tree/ch [3] https://github.com/cloud-hypervisor/vfio-ioctls/tree/ch Signed-off-by: Bo Chen <chen.bo@intel.com>

Signed-off-by: Alexandru Agache <aagch@amazon.com>

alexandruag · 2021-05-31T07:10:56Z

You're right; I experimented a bit more and looks like the default generic parameter works automatically in most cases. The only places where something needs to be explicitly specified are free standing statements (i.e. like the examples in the doc comments). Whenever there's context that already specifies (for example) the GuestMemoryMmap type as part of a struct field definition, return value or parameter type, etc (which happens most of the time) the default generic parameter works without additional changes. Thanks again for the idea Gerry!

Right now, the two suggestions about improving the AtomicBitmap implementation are still pending, as they need a bit more work on my side. Since they don't significantly impact the rest of the code, are you folks ok with merging this PR and then adding one commit for those changes as part of the PR for publishing a new version that will follow? @jiangliu @likebreath @rbradford

jiangliu · 2021-05-31T07:15:58Z

You're right; I experimented a bit more and looks like the default generic parameter works automatically in most cases. The only places where something needs to be explicitly specified are free standing statements (i.e. like the examples in the doc comments). Whenever there's context that already specifies (for example) the GuestMemoryMmap type as part of a struct field definition, return value or parameter type, etc (which happens most of the time) the default generic parameter works without additional changes. Thanks again for the idea Gerry!

Right now, the two suggestions about improving the AtomicBitmap implementation are still pending, as they need a bit more work on my side. Since they don't significantly impact the rest of the code, are you folks ok with merging this PR and then adding one commit for those changes as part of the PR for publishing a new version that will follow? @jiangliu @likebreath @rbradford

I'm OK with that because it's just an optimization:)

As the first step to complete live-migration with tracking dirty-pages written by the VMM, this patch patches the dependent vm-memory crate to the development branch with dirty-page-tracking capability [1]. For now, we are using our own fork of the vm-memory crate [2] before the feature is landed in the upstream. Most changes are due to the updated `GuestMemoryMmap`, `GuestRegionMmap`, and `MmapRegion` structs which are taking an additional generic type parameter (to specify what 'bitmap backend' is used). For the same reason, similar changes are required in the dependent 'vfio-ioctls' crate (that uses the vm-memory crate). We are also using our own fork of the vfio-ioctls crate before the upstream is updated [3]. The above changes should be transparent to the rest of the code base, e.g. all unit/integration tests should pass without additional changes. [1] rust-vmm/vm-memory#125 [2] https://github.com/cloud-hypervisor/vm-memory/tree/ch [3] https://github.com/cloud-hypervisor/vfio-ioctls/tree/ch Signed-off-by: Bo Chen <chen.bo@intel.com>

lauralt · 2021-06-02T06:48:08Z

src/guest_memory.rs

-                }
+                };
+
+                region.bitmap().mark_dirty(start, len);


Can we use result instead of len?

alexandruag mentioned this pull request Dec 1, 2020

mmap: try THP via madvise #113

Open