vhost: Always enable vm-memory/backend-mmap #175

slp · 2023-07-12T16:26:25Z

Since 4029089 ('vhost: Add support for Xen memory mappings') the feature backend-mmap of the vm-memory crate is no longer just a dev-dependency. Make it a formal dependency.

slp · 2023-07-12T16:26:56Z

We should probably also do a quick 0.8.1 release after merging this one.

stefano-garzarella · 2023-07-13T07:23:43Z

@vireshk can you take a look?

Should we enable that feature depending on our xen feature or in any case?
We didn't see any error during publishing, how can we prevent it in the future?

vireshk · 2023-07-13T07:33:30Z

@slp I am not sure why this change is required though. If xen feature is enabled for vhost crate, then it enables vm-memory/xen too, which eventually enables backend-mmap. Why do we need to explictly add it here ?

I tried cargo build --release --features xen and worked just fine.

Just wondering what am I missing here ..

stefano-garzarella · 2023-07-13T07:34:59Z

@slp sorry, I can't understand the problem. What error do you see without this patch?

Maybe it's my limited knowledge of cargo, but in vm-memory/Cargo.toml the xen feature depends on backend-mmap. Here in vhost/Cargo.toml we have xen feature, which depends on vm-memory/xen, so I guess also on vm-memory/backend-mmap.

So IIUC, when we enable xen here, then we also enable backend-mmap implicitly. What did I miss?

slp · 2023-07-13T08:27:02Z

According to the cargo documentation:

Dev-dependencies are not used when compiling a package for building, but are used for compiling tests, examples, and benchmarks.

This means imports from dev-dependencies should be gated behind test or some other build-time conditional. In our case, we're now importing GuestMemoryMmap from the global namespace:

vhost/crates/vhost/src/backend.rs

Line 17 in 900b9a5

use vm_memory::{bitmap::Bitmap, Address, GuestMemoryRegion, GuestRegionMmap};

This implies backend-mmap is no longer a dev-dependency for us, but a general build dependency.

Maybe it's my limited knowledge of cargo, but in vm-memory/Cargo.toml the xen feature depends on backend-mmap. Here in vhost/Cargo.toml we have xen feature, which depends on vm-memory/xen, so I guess also on vm-memory/backend-mmap.

Even if one of your dependencies enables a feature, that feature doesn't appear in your namespace unless you also enable it in your own crate.

You can reproduce the issue by passing the -Z avoid-dev-deps flag to cargo (which some distros use to minimize the number of dependencies at build time and save some CPU cycles):

[root@f0e9515a5e2b vhost]# cd crates/vhost
[root@f0e9515a5e2b vhost]# RUSTC_BOOTSTRAP=1 cargo build -Z avoid-dev-deps
    Updating crates.io index
   Compiling proc-macro2 v1.0.64
   Compiling quote v1.0.29
   Compiling unicode-ident v1.0.10
   Compiling libc v0.2.147
   Compiling thiserror v1.0.43
   Compiling bitflags v1.3.2
   Compiling syn v2.0.25
   Compiling vmm-sys-util v0.11.1
   Compiling thiserror-impl v1.0.43
   Compiling vm-memory v0.12.0
   Compiling vhost v0.8.0 (/root/vhost/crates/vhost)
error[E0432]: unresolved import `vm_memory::GuestRegionMmap`
  --> crates/vhost/src/backend.rs:17:61
   |
17 | use vm_memory::{bitmap::Bitmap, Address, GuestMemoryRegion, GuestRegionMmap};
   |                                                             ^^^^^^^^^^^^^^^ no `GuestRegionMmap` in the root

warning: unused import: `GuestMemoryRegion`
  --> crates/vhost/src/backend.rs:17:42
   |
17 | use vm_memory::{bitmap::Bitmap, Address, GuestMemoryRegion, GuestRegionMmap};
   |                                          ^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused import: `Address`
  --> crates/vhost/src/backend.rs:17:33
   |
17 | use vm_memory::{bitmap::Bitmap, Address, GuestMemoryRegion, GuestRegionMmap};
   |                                 ^^^^^^^

warning: unused import: `std::os::unix::io::AsRawFd`
  --> crates/vhost/src/backend.rs:13:5
   |
13 | use std::os::unix::io::AsRawFd;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0432`.
warning: `vhost` (lib) generated 3 warnings
error: could not compile `vhost` (lib) due to previous error; 3 warnings emitted
[root@f0e9515a5e2b vhost]#

stefano-garzarella · 2023-07-13T08:42:02Z

Maybe it's my limited knowledge of cargo, but in vm-memory/Cargo.toml the xen feature depends on backend-mmap. Here in vhost/Cargo.toml we have xen feature, which depends on vm-memory/xen, so I guess also on vm-memory/backend-mmap.

Even if one of your dependencies enables a feature, that feature doesn't appear in your namespace unless you also enable it in your own crate.

Sure, of course I missed that we are now using GuestMemoryMmap without a guard, but it seems it is needed.
@vireshk can you confirm that?

@slp can you include in the commit description that the feature is needed because of GuestMemoryMmap?

Maybe we should add a test in rust-vmm-ci to cover this scenario.

slp · 2023-07-13T08:51:41Z

@slp can you include in the commit description that the feature is needed because of GuestMemoryMmap?

Done :-)

Since 4029089 ('vhost: Add support for Xen memory mappings') the feature backend-mmap of the vm-memory crate is no longer just a dev-dependency, as we're unconditionally importing GuestMemoryMmap from 'src/backend.rs'. Make it a general build dependency. Signed-off-by: Sergio Lopez <slp@redhat.com>

vireshk · 2023-07-13T09:40:54Z

Hi,

I am somehow not able to reproduce it on commit 900b9a5c4197a.

$ RUSTC_BOOTSTRAP=1  cargo +nightly build -Z avoid-dev-deps
    Updating crates.io index
   Compiling proc-macro2 v1.0.64
   Compiling unicode-ident v1.0.10
   Compiling quote v1.0.29
   Compiling libc v0.2.147
   Compiling thiserror v1.0.43
   Compiling arc-swap v1.6.0
   Compiling bitflags v1.3.2
   Compiling log v0.4.19
   Compiling virtio-bindings v0.2.1
   Compiling syn v2.0.25
   Compiling vmm-sys-util v0.11.1
   Compiling thiserror-impl v1.0.43
   Compiling vm-memory v0.12.0
   Compiling virtio-queue v0.9.0
   Compiling vhost v0.8.0 (/mnt/ssd/all/work/repos/virtio/rust/rust-vmm/vhost/crates/vhost)
   Compiling vhost-user-backend v0.10.0 (/mnt/ssd/all/work/repos/virtio/rust/rust-vmm/vhost/crates/vhost-user-backend)
    Finished dev [unoptimized + debuginfo] target(s) in 4.17s

stefano-garzarella · 2023-07-13T10:12:50Z

@vireshk I can reproduce adding -p vhost:

cargo +nightly build -Z avoid-dev-deps -p vhost   
   Compiling proc-macro2 v1.0.63
   Compiling quote v1.0.29
   Compiling unicode-ident v1.0.10
   Compiling libc v0.2.147
   Compiling thiserror v1.0.41
   Compiling bitflags v1.3.2
   Compiling syn v2.0.23
   Compiling vmm-sys-util v0.11.1
   Compiling thiserror-impl v1.0.41
   Compiling vm-memory v0.12.0
   Compiling vhost v0.8.0 (/home/stefano/repos/vhost/crates/vhost)
error[E0432]: unresolved import `vm_memory::GuestRegionMmap`
  --> crates/vhost/src/backend.rs:17:61
   |
17 | use vm_memory::{bitmap::Bitmap, Address, GuestMemoryRegion, GuestRegionMmap};
   |                                                             ^^^^^^^^^^^^^^^ no `GuestRegionMmap` in the root
   |
note: found an item that was configured out
  --> /home/stefano/.cargo/registry/src/index.crates.io-6f17d22bba15001f/vm-memory-0.12.0/src/lib.rs:60:40
   |
60 | pub use mmap::{Error, GuestMemoryMmap, GuestRegionMmap, MmapRegion};
   |                                        ^^^^^^^^^^^^^^^
   = note: the item is gated behind the `backend-mmap` feature

warning: unused import: `GuestMemoryRegion`
  --> crates/vhost/src/backend.rs:17:42
   |
17 | use vm_memory::{bitmap::Bitmap, Address, GuestMemoryRegion, GuestRegionMmap};
   |                                          ^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused import: `Address`
  --> crates/vhost/src/backend.rs:17:33
   |
17 | use vm_memory::{bitmap::Bitmap, Address, GuestMemoryRegion, GuestRegionMmap};
   |                                 ^^^^^^^

warning: unused import: `std::os::unix::io::AsRawFd`
  --> crates/vhost/src/backend.rs:13:5
   |
13 | use std::os::unix::io::AsRawFd;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0432`.
warning: `vhost` (lib) generated 3 warnings
error: could not compile `vhost` (lib) due to previous error; 3 warnings emitted

Maybe it doesn't show you because vhost-user-backed already have that dependency feature.

stefano-garzarella

@slp thanks!

I'll prepare v0.8.1 ASAP, but I'd wait @vireshk to understand if VhostUserMemoryRegionInfo::from_guest_region() is really needed, since it seems the only one using GuestRegionMmap.

vireshk · 2023-07-13T10:21:24Z

@slp thanks!

I'll prepare v0.8.1 ASAP, but I'd wait @vireshk to understand if VhostUserMemoryRegionInfo::from_guest_region() is really needed, since it seems the only one using GuestRegionMmap.

Hey. I am trying to fix it differently, give me few minutes.

stefano-garzarella · 2023-07-13T10:22:27Z

@slp thanks!
I'll prepare v0.8.1 ASAP, but I'd wait @vireshk to understand if VhostUserMemoryRegionInfo::from_guest_region() is really needed, since it seems the only one using GuestRegionMmap.

Hey. I am trying to fix it differently, give me few minutes.

Sure, take your time ;-)

vireshk · 2023-07-13T10:59:21Z

I couldn't get much on this. Just get this merged :)

slp · 2023-07-14T11:03:09Z

@eryugey @jiangliu @sboeuf could you please take a quick look at this one?

This version fixes an issue on how a dependency is defined in Cargo.toml. We need to include it to prevent build failure during packaging. rust-vmm/vhost#175 Signed-off-by: German Maglione <gmaglione@redhat.com>

slp requested review from eryugey, jiangliu, sboeuf and stefano-garzarella as code owners July 12, 2023 16:26

slp force-pushed the vm-memory-mmap branch from 710529d to d37650d Compare July 13, 2023 08:50

stefano-garzarella approved these changes Jul 13, 2023

View reviewed changes

vireshk approved these changes Jul 13, 2023

View reviewed changes

jiangliu approved these changes Jul 16, 2023

View reviewed changes

jiangliu merged commit 9306ce0 into rust-vmm:main Jul 16, 2023
23 checks passed

vireshk mentioned this pull request Jul 20, 2023

Don't always enable vm-memory/backend-mmap #179

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vhost: Always enable vm-memory/backend-mmap #175

vhost: Always enable vm-memory/backend-mmap #175

slp commented Jul 12, 2023

slp commented Jul 12, 2023

stefano-garzarella commented Jul 13, 2023

vireshk commented Jul 13, 2023

stefano-garzarella commented Jul 13, 2023

slp commented Jul 13, 2023

stefano-garzarella commented Jul 13, 2023

slp commented Jul 13, 2023

vireshk commented Jul 13, 2023

stefano-garzarella commented Jul 13, 2023

stefano-garzarella left a comment

vireshk commented Jul 13, 2023

stefano-garzarella commented Jul 13, 2023

vireshk commented Jul 13, 2023

slp commented Jul 14, 2023

vhost: Always enable vm-memory/backend-mmap #175

vhost: Always enable vm-memory/backend-mmap #175

Conversation

slp commented Jul 12, 2023

slp commented Jul 12, 2023

stefano-garzarella commented Jul 13, 2023

vireshk commented Jul 13, 2023

stefano-garzarella commented Jul 13, 2023

slp commented Jul 13, 2023

stefano-garzarella commented Jul 13, 2023

slp commented Jul 13, 2023

vireshk commented Jul 13, 2023

stefano-garzarella commented Jul 13, 2023

stefano-garzarella left a comment

Choose a reason for hiding this comment

vireshk commented Jul 13, 2023

stefano-garzarella commented Jul 13, 2023

vireshk commented Jul 13, 2023

slp commented Jul 14, 2023