rust-vmm / vm-memory

Virtual machine's guest memory crate
Apache License 2.0
305 stars 98 forks source link

Use pointers instead of slices when operating on guest memory #217

Closed roypat closed 1 year ago

roypat commented 1 year ago

Summary of the PR

Currently, vm-memory internally uses slices to operate on guest memory, in addition to providing methods for passing slices to guest memory to arbitrary user-code (via read_from and write_to). This PR makes sure that all internal operations on guest memory happen through pointers, to prevent accidental triggering of undefined behavior via violation of aliasing rules (either in rust-land, or due to the fact that the guest can modify its own memory at will while we hold references to guest memory).

As a consequence of this PR, the unittests of vm-memory/src/volatile_memory.rs pass under miri

Addresses #45

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

andreeaflorescu commented 1 year ago

@zachreizner @jiangliu @bonzini you were part of the initial discussions in #45. Can you also take a look at this PR?

roypat commented 1 year ago

We discussed offline about the performance impact of this change. Can you also please add the relevant data to this PR for tracking purposes?

Sure, I can summarize (firecracker's performance pipelines are not public, so I sadly cannot simply link).

The main performance impact here is the additional copy in the VolatileSlice functions that either read from a Read implementation to guest memory or write to guest memory using a Write implementation. We could observe the performance impact on a firecracker fork that simply updates its vm-memory dependency to point towards this patch using our performance test suite. Firecracker uses the above-mentioned functions for the vsock and block devices, where the Read/Write impl is a UnixStream and a File respectively. Here, we noticed a degradation of throughput between 5% and 10% compared to our baselines (the block device degradation was actually within statistical error bounds). All other test in the suite passed.

andreeaflorescu commented 1 year ago

@roypat I just realized that we also have some microbenchmarks in this repository. Do you think you could run them as well?

roypat commented 1 year ago

@roypat I just realized that we also have some microbenchmarks in this repository. Do you think you could run them as well?

I did run the microbenchmarks (first on main and then on this patch to get criterion to print the difference) and got

     Running benches/main.rs (target/release/deps/main-dd966502b4f16b8b)
WARNING: HTML report generation will become a non-default optional feature in Criterion.rs 0.4.0.
This feature is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'html_reports' feature in your Cargo.toml.

Gnuplot not found, using plotters backend
VolatileSlice::copy_to_u8                                                                             
                        time:   [15.029 ns 15.073 ns 15.129 ns]
                        change: [-2.6517% -2.2393% -1.7593%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 11 outliers among 200 measurements (5.50%)
  2 (1.00%) low mild
  3 (1.50%) high mild
  6 (3.00%) high severe

VolatileSlice::copy_to_u16                                                                             
                        time:   [106.06 ns 106.12 ns 106.20 ns]
                        change: [-0.0024% +0.0465% +0.1002%] (p = 0.09 > 0.05)
                        No change in performance detected.
Found 23 outliers among 200 measurements (11.50%)
  12 (6.00%) high mild
  11 (5.50%) high severe

VolatileSlice::copy_to_volatile_slice                                                                             
                        time:   [131.21 ns 131.56 ns 131.98 ns]
                        change: [+0.1682% +0.5149% +0.9015%] (p = 0.01 < 0.05)
                        Change within noise threshold.
Found 19 outliers among 200 measurements (9.50%)
  1 (0.50%) low mild
  7 (3.50%) high mild
  11 (5.50%) high severe

VolatileSlice::copy_from_u8                                                                             
                        time:   [13.817 ns 13.823 ns 13.831 ns]
                        change: [-0.1741% -0.0820% +0.0181%] (p = 0.10 > 0.05)
                        No change in performance detected.
Found 36 outliers among 200 measurements (18.00%)
  14 (7.00%) high mild
  22 (11.00%) high severe

VolatileSlice::copy_from_u16                                                                             
                        time:   [105.78 ns 105.82 ns 105.87 ns]
                        change: [-0.0194% +0.0627% +0.1505%] (p = 0.13 > 0.05)
                        No change in performance detected.
Found 36 outliers among 200 measurements (18.00%)
  7 (3.50%) high mild
  29 (14.50%) high severe