Closed vireshk closed 3 weeks ago
stsquad
commented
3 weeks ago The specification says the feature should be offered but it isn't mandatory. The in-kernel vhost-vsock doesn't offer the feature and as far as I can tell the common vsock code in qemu just assumes three queueus (recv/trans/event). Is this really the Xen frontend not knowing how many queues to provision without being able to introspect?
stefano-garzarella
commented
3 weeks ago The specification says the feature should be offered but it isn't mandatory. The in-kernel vhost-vsock doesn't offer the feature and as far as I can tell the common vsock code in qemu just assumes three queueus (recv/trans/event). Is this really the Xen frontend not knowing how many queues to provision without being able to introspect?
@stsquad I agree, but I just saw this from https://qemu-project.gitlab.io/qemu/interop/vhost-user.html :
Back-ends should always implement the VHOST_USER_PROTOCOL_F_MQ protocol feature, even for devices with a fixed number of virtqueues, since it is simple to implement and offers a degree of introspection.
So, I'm fine with this change, but @vireshk what about replacing must with should in the commit description?
Maybe we can also mention that spec statement.
epilys
commented
3 weeks ago So, I'm fine with this change, but @vireshk what about replacing
mustwithshouldin the commit description? Maybe we can also mention that spec statement.
+1
vireshk
commented
3 weeks ago The specification says the feature should be offered but it isn't mandatory. The in-kernel vhost-vsock doesn't offer the feature and as far as I can tell the common vsock code in qemu just assumes three queueus (recv/trans/event). Is this really the Xen frontend not knowing how many queues to provision without being able to introspect?
https://github.com/vireshk/vhost/blob/frontend/stable/crates/vhost-user-frontend/src/generic.rs#L124
This part is wrong then, I should rather fix that instead then.
epilys
commented
3 weeks ago @vireshk if the feature should be offered why not merge this change anyway?
vireshk
commented
3 weeks ago @vireshk if the feature should be offered why not merge this change anyway?
From the link that Stefano shared:
"Some devices do not have a fixed number of virtqueues. Instead the maximum number of virtqueues is chosen by the back-end. The number can depend on host resource availability or back-end implementation details. Such devices are called multiple queue devices."
Though in case of Vsock, the kernel driver and backend all seems to have it fixed to three. They may not work with lesser number of queues and so this feature doesn't fit I guess. Or I misunderstood the whole thing.
stefano-garzarella
commented
2 weeks ago Though in case of Vsock, the kernel driver and backend all seems to have it fixed to three.
Yep, the driver always expects 3 virtqueues. In both vhost-vsock and vhost-user-vsock, 2 of them (TX, RX) are handled by the backend (kernel or external process) and 1 (event) by the VMM.
They may not work with lesser number of queues and so this feature doesn't fit I guess. Or I misunderstood the whole thing.
As suggested by the spec, Back-ends should always implement the VHOST_USER_PROTOCOL_F_MQ protocol feature, even for devices with a fixed number of virtqueues, since it is simple to implement and offers a degree of introspection., so I think should be fine to merge this PR to improve the introspection.
For the frontend/driver point of view, should not change anything.
This feature flag must be set by backends supporting multiple virtqueues. Vsock backend support three virtqueues and hence this must be set.
Summary of the PR
Please summarize here why the changes in this PR are needed.
Requirements
Before submitting your PR, please make sure you addressed the following requirements:
git commit -s), and the commit message has max 60 characters for the summary and max 75 characters for each description line.unsafecode is properly documented.