vhost: support VIRTIO_F_RING_RESET for vhost-user
v3 cover vhost: support VIRTIO_F_RING_RESET for vhost-user
Add VIRTIO_F_RING_RESET, which indicates that the driver can reset a virtqueue individually.
VIRTIO_F_RING_RESET feature is added to virtio-spec 1.2. The relevant information is in
https://github.com/oasis-tcs/virtio-spec/issues/124 https://github.com/oasis-tcs/virtio-spec/issues/139
The implementation only adds the feature bit in supported features. We reuse the existing vhost protocol and make small modifications to the VHOST_USER_GET_VRING_BASE implementation(do not destroy the device unless the vq is the last active one in the device).
Virtqueue reset process can be concluded as two parts:
1. The driver can reset a virtqueue. It will send VHOST_USER_GET_VRING_BASE to DPDK. After received the message, DPDK will stop the virtqueue.
2. After the virtqueue is disabled, the driver may optionally re-enable it. To avoid confusion with VHOST_USER_SET_VRING_ENABLE, we call this part as "restart". The virtqueue's information may be changed when restarting it. Thus, the information of the reset virtqueue should be updated. This part is basically similar to when the virtqueue is started for the first time, except that the restart process does not need to set features and set mem table since they do not change. QEMU will send messages containing size, base, addr, kickfd and callfd of the virtqueue in order. Specifically, the DPDK will receive these messages in order:
a. VHOST_USER_SET_VRING_NUM
b. VHOST_USER_SET_VRING_BASE
c. VHOST_USER_SET_VRING_ADDR
d. VHOST_USER_SET_VRING_KICK
e. VHOST_USER_SET_VRING_CALL
f. VHOST_USER_SET_VRING_ENABLE
The last VHOST_USER_SET_VRING_ENABLE message with "payload.state.num" set to 1, will be sent to enable the virtqueue and the restart process is finished.
Test environment:
Host: 5.4.189
Qemu: QEMU emulator version 7.1.50 (With vq reset support)
Guest: 5.19.0-rc3 (With vq reset support)
DPDK: 22.11-rc0
Test Cmd: ethtool -g eth1; ethtool -G eth1 rx $1 tx $2;
ethtool -g eth1;
The driver can resize the virtio queue, then virtio queue reset
function should be triggered.Guest Kernel Patch:
https://lore.kernel.org/bpf/20220801063902.129329-1-xuanzhuo@
linux.alibaba.com/
QEMU Patch:
https://lore.kernel.org/qemu-devel/cover.1662949366.git.kangjie.xu@ linux.alibaba.com/ https://lore.kernel.org/qemu-devel/cover.1662916759.git.kangjie.xu@
linux.alibaba.com/
Looking forward to your review and comments.
Kangjie Xu (2):
vhost: destroy device when all vqs are inactive
vhost: support VIRTIO_F_RING_RESET for vhost-user
lib/vhost/vhost.h | 8 +++++++- lib/vhost/vhost_user.c | 10 ++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-)
Signature · 1 line
2.32.0
PATCH v3 1/2 vhost: destroy device when all vqs are inactive
We change the behavior of vhost_user_get_vring_base(). Previosly, destroying a virtqueue will cause the whole device to be destroyed. The behavior is not specified in the vhost-user protocol.
Thus, we refactor this part. The device will be destroyed only when all virtqueues in the device are going to be destroyed.
This helps us to simplify the implementation when resetting a virtqueue.
--- lib/vhost/vhost_user.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
lib/vhost/vhost_user.c +8 -2
@@ -2088,10 +2088,16 @@ vhost_user_get_vring_base(struct virtio_net **pdev, {struct virtio_net *dev = *pdev;struct vhost_virtqueue *vq = dev->virtqueue[ctx->msg.payload.state.index]; +uint32_t i, num_live_vring =0 ;uint64_t val; -/* We have to stop the queue (virtio) if it is running. */ -vhost_destroy_device_notify (dev); +/* Stop the device when vq is the last active queue */ +for (i =0 ; i < dev->nr_vring; i++) +if (dev->virtqueue[i]->access_ok) + num_live_vring++; + +if (num_live_vring ==1 && vq->access_ok) +vhost_destroy_device_notify (dev); dev->flags &= ~VIRTIO_DEV_READY ; dev->flags &= ~VIRTIO_DEV_VDPA_CONFIGURED ;
Signature · 1 line
2.32.0
Re: [PATCH v3 1/2] vhost: destroy device when all vqs are inactive
Quoted 28 lines · Kangjie Xu
We change the behavior of
vhost_user_get_vring_base(). Previosly, destroying a virtqueue will cause the whole device to be destroyed. The behavior is not specified in the vhost-user protocol.Thus, we refactor this part. The device will be destroyed only when all virtqueues in the device are going to be destroyed.
This helps us to simplify the implementation when resetting a virtqueue.
Signed-off-by Kangjie Xu <[email protected]>Signed-off-by Xuan Zhuo <[email protected]>--- lib/vhost/vhost_user.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)lib/vhost/vhost_user.c +1
-2@@ -2088,10 +2088,16 @@ vhost_user_get_vring_base(struct virtio_net **pdev, {struct virtio_net *dev = *pdev;struct vhost_virtqueue *vq = dev->virtqueue[ctx->msg.payload.state.index]; +uint32_t i, num_live_vring =0 ;uint64_t val; -/* We have to stop the queue (virtio) if it is running. */ -vhost_destroy_device_notify (dev);
lib/vhost/vhost_user.c vhost_user_get_vring_base(struct virtio_net **pdev, · line 2103
Quoted 10 lines
lib/vhost/vhost_user.c +7
-0+/* Stop the device when vq is the last active queue */ +for (i =0 ; i < dev->nr_vring; i++) +if (dev->virtqueue[i]->access_ok) + num_live_vring++; + +if (num_live_vring ==1 && vq->access_ok) +vhost_destroy_device_notify (dev); dev->flags &= ~VIRTIO_DEV_READY ; dev->flags &= ~VIRTIO_DEV_VDPA_CONFIGURED ;
I think we are missing something here.
We used to send the device destroy notification before getting the ring indexes, in order to ensure that the application has stopped processing the rings.
With this patch, the application may still be polling the ring while we get the ring indexes (e.g. a thread in the application may be in the middle of rte_vhost_dequeue_burst() on that ring). So at best the ring indexes returned to the Vhost-user master will be outdated. At worst, it will crash the application because we call vring_invalidate() without the vq's lock being taken.
I think you should protect all the VQ indexes fetching and VQ deinit using its access_lock.
Maxime
Re: [PATCH v3 1/2] vhost: destroy device when all vqs are inactive
Quoted 53 lines · On Tue, 11 Oct 2022 18:44:28 +0200 Maxime Coquelin…
Quoted 28 lines · Kangjie Xu
We change the behavior of
vhost_user_get_vring_base(). Previosly, destroying a virtqueue will cause the whole device to be destroyed. The behavior is not specified in the vhost-user protocol.Thus, we refactor this part. The device will be destroyed only when all virtqueues in the device are going to be destroyed.
This helps us to simplify the implementation when resetting a virtqueue.
Signed-off-by Kangjie Xu <[email protected]>Signed-off-by Xuan Zhuo <[email protected]>--- lib/vhost/vhost_user.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)lib/vhost/vhost_user.c +1
-2@@ -2088,10 +2088,16 @@ vhost_user_get_vring_base(struct virtio_net **pdev, {struct virtio_net *dev = *pdev;struct vhost_virtqueue *vq = dev->virtqueue[ctx->msg.payload.state.index]; +uint32_t i, num_live_vring =0 ;uint64_t val; -/* We have to stop the queue (virtio) if it is running. */ -vhost_destroy_device_notify (dev);lib/vhost/vhost_user.c vhost_user_get_vring_base(struct virtio_net **pdev, · line 2103
Quoted 10 lines
lib/vhost/vhost_user.c +7
-0+/* Stop the device when vq is the last active queue */ +for (i =0 ; i < dev->nr_vring; i++) +if (dev->virtqueue[i]->access_ok) + num_live_vring++; + +if (num_live_vring ==1 && vq->access_ok) +vhost_destroy_device_notify (dev); dev->flags &= ~VIRTIO_DEV_READY ; dev->flags &= ~VIRTIO_DEV_VDPA_CONFIGURED ;I think we are missing something here.
We used to send the device destroy notification before getting the ring indexes, in order to ensure that the application has stopped processing the rings.
With this patch, the application may still be polling the ring while we get the ring indexes (e.g. a thread in the application may be in the middle of
rte_vhost_dequeue_burst()on that ring). So at best the ring indexes returned to the Vhost-user master will be outdated. At worst, it will crash the application because we callvring_invalidate()without the vq's lock being taken.
Quoted 5 lines
I think you should protect all the VQ indexes fetching and VQ deinit using its
access_lock.Maxime
Please address Maxime's feedback.
Re: [PATCH v3 1/2] vhost: destroy device when all vqs are inactive
Quoted 56 lines · Stephen Hemminger
Quoted 53 lines · On Tue, 11 Oct 2022 18:44:28 +0200 Maxime Coquelin…
Quoted 28 lines · Kangjie Xu
We change the behavior of
vhost_user_get_vring_base(). Previosly, destroying a virtqueue will cause the whole device to be destroyed. The behavior is not specified in the vhost-user protocol.Thus, we refactor this part. The device will be destroyed only when all virtqueues in the device are going to be destroyed.
This helps us to simplify the implementation when resetting a virtqueue.
Signed-off-by Kangjie Xu <[email protected]>Signed-off-by Xuan Zhuo <[email protected]>--- lib/vhost/vhost_user.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)lib/vhost/vhost_user.c +1
-2@@ -2088,10 +2088,16 @@ vhost_user_get_vring_base(struct virtio_net **pdev, {struct virtio_net *dev = *pdev;struct vhost_virtqueue *vq = dev->virtqueue[ctx->msg.payload.state.index]; +uint32_t i, num_live_vring =0 ;uint64_t val; -/* We have to stop the queue (virtio) if it is running. */ -vhost_destroy_device_notify (dev);lib/vhost/vhost_user.c vhost_user_get_vring_base(struct virtio_net **pdev, · line 2103
Quoted 10 lines
lib/vhost/vhost_user.c +7
-0+/* Stop the device when vq is the last active queue */ +for (i =0 ; i < dev->nr_vring; i++) +if (dev->virtqueue[i]->access_ok) + num_live_vring++; + +if (num_live_vring ==1 && vq->access_ok) +vhost_destroy_device_notify (dev); dev->flags &= ~VIRTIO_DEV_READY ; dev->flags &= ~VIRTIO_DEV_VDPA_CONFIGURED ;I think we are missing something here.
We used to send the device destroy notification before getting the ring indexes, in order to ensure that the application has stopped processing the rings.
With this patch, the application may still be polling the ring while we get the ring indexes (e.g. a thread in the application may be in the middle of
rte_vhost_dequeue_burst()on that ring). So at best the ring indexes returned to the Vhost-user master will be outdated. At worst, it will crash the application because we callvring_invalidate()without the vq's lock being taken.
Quoted 7 lines
Quoted 5 lines
I think you should protect all the VQ indexes fetching and VQ deinit using its
access_lock.Maxime
Please address Maxime's feedback.
Kangjie has already resigned.
Sorry, we don't have anyone in charge of this.
If you need it, you can propose a new patch to solve this problem.
Thanks.
PATCH v3 2/2 vhost: support VIRTIO_F_RING_RESET for vhost-user
In reply to Kangjie XuAdd VIRTIO_F_RING_RESET, which indicates that the driver can reset a queue individually.
The feature is added to virtio-spec 1.2. The relevant information is in https://github.com/oasis-tcs/virtio-spec/issues/124
--- lib/vhost/vhost.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
lib/vhost/vhost.h +7 -1
@@ -376,6 +376,11 @@ struct vhost_msg {#define VIRTIO_F_VERSION_1 32#endif +/* This feature indicates that the driver can reset a queue individually. */ +#ifndef VIRTIO_F_RING_RESET +#define VIRTIO_F_RING_RESET 40 +#endif +/* Declare packed ring related bits for older kernels */ #ifndef VIRTIO_F_RING_PACKED
@@ -438,7 +443,8 @@ struct vring_packed_desc_event { (1ULL << VIRTIO_NET_F_MTU) | \ (1ULL << VIRTIO_F_IN_ORDER) | \ (1ULL << VIRTIO_F_IOMMU_PLATFORM) | \ - (1ULL << VIRTIO_F_RING_PACKED)) + (1ULL << VIRTIO_F_RING_PACKED) | \ + (1ULL << VIRTIO_F_RING_RESET)) struct guest_page {
Signature · 1 line
2.32.0