-
Notifications
You must be signed in to change notification settings - Fork 70
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
Fix return value of GET_VRING_BASE
message
#154
Fix return value of GET_VRING_BASE
message
#154
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I wonder whether we should now call reset() for example in set_owner() or reset_owner()
When the frontend sends the `GET_VRING_BASE` message, we should return the vring's last available index and stop the vring. To return the correct value we should not reset the queue before getting its value, otherwise we will always return 0. Signed-off-by: German Maglione <gmaglione@redhat.com>
2ffaac5
to
7e38924
Compare
According to the spec
Not sure about doing in |
I see, thanks for checking!
Okay, the current version leaves Anyway, I'd wait @slp since that change was part of rust-vmm/vhost-user-backend#18 |
(Sorry, I forgot to comment the diff between v1 and v2) v2:
|
@jiangliu are you okay with resetting the vring? As pointed out by German, neither qemu's libvhost-user nor dpdk reset the vring when receiving GET_VRING_BASE. |
It would be safer to keep the same behavior as qemu/libvhost-user:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current version LGTM.
It's not clear to me though why we have to reset the whole VQ, would it be better just to stop it? |
So, let's merge this version that should have less impact, then we can try to figure out if we can avoid resetting the VQ and if we need to move that call in some other place |
I'm honestly not sure, but I'd prefer to cut a release without altering that behavior, and then consider a different approach (along with things like #155) in the next one. Sounds reasonable? |
Yep, same conclusion #154 (comment) :-) |
Summary of the PR
When the frontend sends the
GET_VRING_BASE
message, we should return the vring's last available index and stop the vring. Commit 1713135 reset the vring upon receivingGET_VRING_BASE
, But to return the correct index we should not reset the queue before getting its value, otherwise we will always return 0.Note:
Neither qemu[0] nor dpdk[1] reset the vring when receiving
GET_VRING_BASE
[0] https://github.com/qemu/qemu/blob/792f77f376adef944f9a03e601f6ad90c2f891b2/subprojects/libvhost-user/libvhost-user.c#L1175
[1] https://github.com/DPDK/dpdk/blob/d03446724972d2a1bb645ce7f3e64f5ef0203d61/lib/vhost/vhost_user.c#L2133