Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update vhost-user-backend to 0.11 series #461

Merged
merged 2 commits into from
Nov 14, 2023
Merged

Conversation

Ablu
Copy link
Contributor

@Ablu Ablu commented Sep 29, 2023

  • features were renamed from slave -> backend
  • generics got simplified

Draft until the version is actually released

Requirements

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

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

Cargo.lock Outdated Show resolved Hide resolved
@Ablu
Copy link
Contributor Author

Ablu commented Nov 7, 2023

Hm... We need another virtio-vsock update as well...

@Ablu
Copy link
Contributor Author

Ablu commented Nov 13, 2023

Hm... vsock_conn.rs tests could probably benefit from a WriteVolatile impl on Cursor. But I am not going through another round of major updates for that. Will post the patches to add it in vm-memory but then do an additional copy for the test code here. Once the vm-memory update then goes through we can drop it.

@stefano-garzarella
Copy link
Member

Hm... vsock_conn.rs tests could probably benefit from a WriteVolatile impl on Cursor. But I am not going through another round of major updates for that. Will post the patches to add it in vm-memory but then do an additional copy for the test code here. Once the vm-memory update then goes through we can drop it.

Yep, I agree!

@Ablu
Copy link
Contributor Author

Ablu commented Nov 13, 2023

This is ready now! The vsock tests gave me some headache and went back and forth a couple of times until I came up with this. Since the VecDequeue impls cannot be done very nicely, I am not sure how much value there is in trying to upstream them to vm-memory. The tricks that we did to make it fast for Vec do not work here...

These Read/Write implementations always read/wrote to the start of the
slice. This is a bit awkward and does not become easier by the test
relying on this behaviour.
Fix this by using a pair of VecDequeue, one for the read and one for the
write buffer. This better mimics a pair of network sockets. This allows
mocking the read/writes in a bit more natural ways.

VecDeque also implements Read/Write, making our impls for those a simple
forward.

Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
@Ablu
Copy link
Contributor Author

Ablu commented Nov 13, 2023

This is ready now!

Except that it needs a rebase now...

@Ablu
Copy link
Contributor Author

Ablu commented Nov 13, 2023

Rebased.

@Ablu Ablu marked this pull request as ready for review November 13, 2023 15:27
@Ablu
Copy link
Contributor Author

Ablu commented Nov 13, 2023

Ah. Forgot about staging...

Copy link
Member

@stefano-garzarella stefano-garzarella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for taking care of vsock tests!

I left some minor comments.

@stefano-garzarella
Copy link
Member

@Ablu
Copy link
Contributor Author

Ablu commented Nov 13, 2023

@Ablu staging crates are still failing: https://buildkite.com/rust-vmm/vhost-device-ci/builds/1960#018bc957-95ab-48d2-bea7-00fea0939a0a
Is it related?

Yes, still fixing it. forgot to bump backend, but something else is missing too. Will sort it out.

@Ablu
Copy link
Contributor Author

Ablu commented Nov 13, 2023

Hm. Is this crash known for staging?

Caused by:
  process didn't exit successfully: `/home/ablu/projects/rust-vmm/vhost-device/staging/target/debug/deps/vhost_device_sound-45dd1b9dd31276f6` (signal: 11, SIGSEGV: invalid memory reference)

@Ablu
Copy link
Contributor Author

Ablu commented Nov 13, 2023

Apart from the crash I also finished the update in staging.

@stefano-garzarella
Copy link
Member

Hm. Is this crash known for staging?

Caused by:
  process didn't exit successfully: `/home/ablu/projects/rust-vmm/vhost-device/staging/target/debug/deps/vhost_device_sound-45dd1b9dd31276f6` (signal: 11, SIGSEGV: invalid memory reference)

@dorindabassey @MatiasVara @epilys any idea of this?

@MatiasVara
Copy link
Contributor

Hm. Is this crash known for staging?

Caused by:
  process didn't exit successfully: `/home/ablu/projects/rust-vmm/vhost-device/staging/target/debug/deps/vhost_device_sound-45dd1b9dd31276f6` (signal: 11, SIGSEGV: invalid memory reference)

@dorindabassey @MatiasVara @epilys any idea of this?

Those logs are about virtio-device-video if I am not wrong.

@stefano-garzarella
Copy link
Member

Hm. Is this crash known for staging?

Caused by:
  process didn't exit successfully: `/home/ablu/projects/rust-vmm/vhost-device/staging/target/debug/deps/vhost_device_sound-45dd1b9dd31276f6` (signal: 11, SIGSEGV: invalid memory reference)

@dorindabassey @MatiasVara @epilys any idea of this?

Those logs are about virtio-device-video if I am not wrong.

The executable looks like the sound one: vhost_device_sound-45dd1b9dd31276f6

- Features were renamed from slave -> backend
- Generics got simplified
- Some write and read functions on Volatile slice got turned into
  standalone traits: ReadVolatile, WriteVolatile
- handle_event no longer returns a bool

Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
@Ablu
Copy link
Contributor Author

Ablu commented Nov 13, 2023

I can no longer reproduce the crash during tests... So I guess that can be handled separately from this. I will try to reproduce it with address sanitizers and open an issue if I can reproduce it again.

@epilys
Copy link
Member

epilys commented Nov 13, 2023

Is it okay with everyone to delay merging till we test some of the devices by launching them with VMs? Just to make sure everything is ok to go.

@Ablu
Copy link
Contributor Author

Ablu commented Nov 14, 2023

Is it okay with everyone to delay merging till we test some of the devices by launching them with VMs? Just to make sure everything is ok to go.

Sure! Though we probably have a testing gap somewhere if stuff is failing there but not in our tests.

@stefano-garzarella
Copy link
Member

I reproduced the vhost-device-sound issue also on main, so I don't think it is related to this PR. For me, we can go head merging this.

@Ablu
Copy link
Contributor Author

Ablu commented Nov 14, 2023

Thanks for reviews!

@epilys: Got any specific tests in mind? Feel free to hit merge when you are happy.

I am having better integration tests somewhere on my TODO list. Though I am not yet sure about the final form... We will probably develop integration tests for TRS, but those would only be able to run post-merge.

Maybe it might make sense to collect ideas on a ticket? Overall, I think this is not more or less dangerous compared to any other PR. While this includes some major bumps, the actual changes are fairly "simple".

@epilys epilys merged commit 31154ea into rust-vmm:main Nov 14, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants