rust-vmm / vhost

Apache License 2.0
130 stars 69 forks source link

Adding POSTCOPY support #218

Closed ShadowCurse closed 8 months ago

ShadowCurse commented 9 months ago

Summary of the PR

Added ability to use VHOST_USER_POSTCOPY_ADVISE, VHOST_USER_POSTCOPY_LISTEN and VHOST_USER_POSTCOPY_END messages. This includes sending them from the frontend and processing them on the backend.

The POSTCOPY messages are only usable when VHOST_USER_PROTOCOL_F_PAGEFAULT feature is negotiated.

Messages and their descriptions are:

Requirements

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

Ablu commented 9 months ago

@germag: This may be related to your work on migration?

ShadowCurse commented 9 months ago

Hi @Ablu, I have addressed comments you left before, please take another look. Also @vireshk and @germag, would be nice to hear your thoughts as well.

Regarding ci not passing with musl toolchain, I am working on this.

stefano-garzarella commented 9 months ago

@ShadowCurse did you test the changes of rust-vmm-container locally? You can use ./docker.sh build to build a local image and try to build this branch through it.

For example, what I usually do is:

cd rust-vmm-container
./docker.sh build

cd vhost
# I'm using podman, but with docker should be similar
podman run -it --rm --security-opt=seccomp=unconfined --volume $(pwd):/workdir --workdir /workdir localhost/rustvmm/dev:v33_x86_64 
cargo build --target x86_64-unknown-linux-musl
Ablu commented 9 months ago

/cc @stsquad

ShadowCurse commented 8 months ago

@Ablu I have updated the PR to disable postcopy messages if xen feature is enabled. I think this should work for the majority of use cases, while xen + postcopy can be added later if need arises.

Ablu commented 8 months ago

@Ablu I have updated the PR to disable postcopy messages if xen feature is enabled. I think this should work for the majority of use cases, while xen + postcopy can be added later if need arises.

Hm. I must admit that I am not particular thrilled to add further Xen vs non-Xen flags. In fact, I would prefer this way of getting access to virtual addresses altogether as it seems to be confusing.

But I am happy to hear other peoples opinions.

stefano-garzarella commented 8 months ago

nit: I suggest to use imperative form in commit (title and description). https://www.kernel.org/doc/html/v4.10/process/submitting-patches.html#describe-your-changes . And possibly I'd use vhost: and vhost-user-backend: prefix in the title since we have multiple crates in this workspace and that allows us to better understand the git story.

ShadowCurse commented 8 months ago

nit: I suggest to use imperative form in commit (title and description). https://www.kernel.org/doc/html/v4.10/process/submitting-patches.html#describe-your-changes . And possibly I'd use vhost: and vhost-user-backend: prefix in the title since we have multiple crates in this workspace and that allows us to better understand the git story.

Updated commits. Do they look better now?

stefano-garzarella commented 8 months ago

Updated commits. Do they look better now?

Yep, thanks! I'd avoided "can" and used "support", for example: "vhost/frontend: support POSTCOPY messages", but I don't want to be too pedantic, it's fine even as it is now ;-)

BTW the code LGTM, but I'd wait a bit a comment from Viresh and Erik.

vireshk commented 8 months ago

Hi @Ablu, I have addressed comments you left before, please take another look. Also @vireshk and @germag, would be nice to hear your thoughts as well.

Hey, sorry for being late on this. Yeah it is fine for now to disable the feature for Xen builds. That's fine. Thanks.

stefano-garzarella commented 8 months ago

Now that we have tests for the new code, maybe we can enable postcopy for coverage:


diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json
index d201e56..9923e17 100644
--- a/coverage_config_x86_64.json
+++ b/coverage_config_x86_64.json
@@ -1,5 +1,5 @@
 {
   "coverage_score": 75.0,
   "exclude_path": "vhost/src/vhost_kern/",
-  "crate_features": "vhost/vhost-user-frontend,vhost/vhost-user-backend"
+  "crate_features": "vhost/vhost-user-frontend,vhost/vhost-user-backend,vhost-user-backend/postcopy"
 }

I tried it locally and the coverage test passes successfully.

Ablu commented 8 months ago

Thanks!