Michael S. Tsirkin
2022-Dec-26 23:34 UTC
[PATCH 3/4] virtio_ring: introduce a per virtqueue waitqueue
On Mon, Dec 26, 2022 at 03:49:07PM +0800, Jason Wang wrote:> This patch introduces a per virtqueue waitqueue to allow driver to > sleep and wait for more used. Two new helpers are introduced to allow > driver to sleep and wake up. > > Signed-off-by: Jason Wang <jasowang at redhat.com> > --- > Changes since V1: > - check virtqueue_is_broken() as well > - use more_used() instead of virtqueue_get_buf() to allow caller to > get buffers afterwards > --- > drivers/virtio/virtio_ring.c | 29 +++++++++++++++++++++++++++++ > include/linux/virtio.h | 3 +++ > 2 files changed, 32 insertions(+) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 5cfb2fa8abee..9c83eb945493 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -13,6 +13,7 @@ > #include <linux/dma-mapping.h> > #include <linux/kmsan.h> > #include <linux/spinlock.h> > +#include <linux/wait.h> > #include <xen/xen.h> > > #ifdef DEBUG > @@ -60,6 +61,7 @@ > "%s:"fmt, (_vq)->vq.name, ##args); \ > /* Pairs with READ_ONCE() in virtqueue_is_broken(). */ \ > WRITE_ONCE((_vq)->broken, true); \ > + wake_up_interruptible(&(_vq)->wq); \ > } while (0) > #define START_USE(vq) > #define END_USE(vq) > @@ -203,6 +205,9 @@ struct vring_virtqueue { > /* DMA, allocation, and size information */ > bool we_own_ring; > > + /* Wait for buffer to be used */ > + wait_queue_head_t wq; > + > #ifdef DEBUG > /* They're supposed to lock for us. */ > unsigned int in_use; > @@ -2024,6 +2029,8 @@ static struct virtqueue *vring_create_virtqueue_packed( > if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM)) > vq->weak_barriers = false; > > + init_waitqueue_head(&vq->wq); > + > err = vring_alloc_state_extra_packed(&vring_packed); > if (err) > goto err_state_extra; > @@ -2517,6 +2524,8 @@ static struct virtqueue *__vring_new_virtqueue(unsigned int index, > if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM)) > vq->weak_barriers = false; > > + init_waitqueue_head(&vq->wq); > + > err = vring_alloc_state_extra_split(vring_split); > if (err) { > kfree(vq); > @@ -2654,6 +2663,8 @@ static void vring_free(struct virtqueue *_vq) > { > struct vring_virtqueue *vq = to_vvq(_vq); > > + wake_up_interruptible(&vq->wq); > + > if (vq->we_own_ring) { > if (vq->packed_ring) { > vring_free_queue(vq->vq.vdev, > @@ -2863,4 +2874,22 @@ const struct vring *virtqueue_get_vring(struct virtqueue *vq) > } > EXPORT_SYMBOL_GPL(virtqueue_get_vring); > > +int virtqueue_wait_for_used(struct virtqueue *_vq) > +{ > + struct vring_virtqueue *vq = to_vvq(_vq); > + > + /* TODO: Tweak the timeout. */ > + return wait_event_interruptible_timeout(vq->wq, > + virtqueue_is_broken(_vq) || more_used(vq), HZ);There's no good timeout. Let's not even go there, if device goes bad it should set the need reset bit.> +} > +EXPORT_SYMBOL_GPL(virtqueue_wait_for_used); > + > +void virtqueue_wake_up(struct virtqueue *_vq) > +{ > + struct vring_virtqueue *vq = to_vvq(_vq); > + > + wake_up_interruptible(&vq->wq); > +} > +EXPORT_SYMBOL_GPL(virtqueue_wake_up); > + > MODULE_LICENSE("GPL"); > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > index dcab9c7e8784..2eb62c774895 100644 > --- a/include/linux/virtio.h > +++ b/include/linux/virtio.h > @@ -72,6 +72,9 @@ void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len); > void *virtqueue_get_buf_ctx(struct virtqueue *vq, unsigned int *len, > void **ctx); > > +int virtqueue_wait_for_used(struct virtqueue *vq); > +void virtqueue_wake_up(struct virtqueue *vq); > + > void virtqueue_disable_cb(struct virtqueue *vq); > > bool virtqueue_enable_cb(struct virtqueue *vq); > -- > 2.25.1
Jason Wang
2022-Dec-27 03:47 UTC
[PATCH 3/4] virtio_ring: introduce a per virtqueue waitqueue
On Tue, Dec 27, 2022 at 7:34 AM Michael S. Tsirkin <mst at redhat.com> wrote:> > On Mon, Dec 26, 2022 at 03:49:07PM +0800, Jason Wang wrote: > > This patch introduces a per virtqueue waitqueue to allow driver to > > sleep and wait for more used. Two new helpers are introduced to allow > > driver to sleep and wake up. > > > > Signed-off-by: Jason Wang <jasowang at redhat.com> > > --- > > Changes since V1: > > - check virtqueue_is_broken() as well > > - use more_used() instead of virtqueue_get_buf() to allow caller to > > get buffers afterwards > > --- > > drivers/virtio/virtio_ring.c | 29 +++++++++++++++++++++++++++++ > > include/linux/virtio.h | 3 +++ > > 2 files changed, 32 insertions(+) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 5cfb2fa8abee..9c83eb945493 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -13,6 +13,7 @@ > > #include <linux/dma-mapping.h> > > #include <linux/kmsan.h> > > #include <linux/spinlock.h> > > +#include <linux/wait.h> > > #include <xen/xen.h> > > > > #ifdef DEBUG > > @@ -60,6 +61,7 @@ > > "%s:"fmt, (_vq)->vq.name, ##args); \ > > /* Pairs with READ_ONCE() in virtqueue_is_broken(). */ \ > > WRITE_ONCE((_vq)->broken, true); \ > > + wake_up_interruptible(&(_vq)->wq); \ > > } while (0) > > #define START_USE(vq) > > #define END_USE(vq) > > @@ -203,6 +205,9 @@ struct vring_virtqueue { > > /* DMA, allocation, and size information */ > > bool we_own_ring; > > > > + /* Wait for buffer to be used */ > > + wait_queue_head_t wq; > > + > > #ifdef DEBUG > > /* They're supposed to lock for us. */ > > unsigned int in_use; > > @@ -2024,6 +2029,8 @@ static struct virtqueue *vring_create_virtqueue_packed( > > if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM)) > > vq->weak_barriers = false; > > > > + init_waitqueue_head(&vq->wq); > > + > > err = vring_alloc_state_extra_packed(&vring_packed); > > if (err) > > goto err_state_extra; > > @@ -2517,6 +2524,8 @@ static struct virtqueue *__vring_new_virtqueue(unsigned int index, > > if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM)) > > vq->weak_barriers = false; > > > > + init_waitqueue_head(&vq->wq); > > + > > err = vring_alloc_state_extra_split(vring_split); > > if (err) { > > kfree(vq); > > @@ -2654,6 +2663,8 @@ static void vring_free(struct virtqueue *_vq) > > { > > struct vring_virtqueue *vq = to_vvq(_vq); > > > > + wake_up_interruptible(&vq->wq); > > + > > if (vq->we_own_ring) { > > if (vq->packed_ring) { > > vring_free_queue(vq->vq.vdev, > > @@ -2863,4 +2874,22 @@ const struct vring *virtqueue_get_vring(struct virtqueue *vq) > > } > > EXPORT_SYMBOL_GPL(virtqueue_get_vring); > > > > +int virtqueue_wait_for_used(struct virtqueue *_vq) > > +{ > > + struct vring_virtqueue *vq = to_vvq(_vq); > > + > > + /* TODO: Tweak the timeout. */ > > + return wait_event_interruptible_timeout(vq->wq, > > + virtqueue_is_broken(_vq) || more_used(vq), HZ); > > There's no good timeout. Let's not even go there, if device goes > bad it should set the need reset bit.The problem is that we can't depend on the device. If it takes too long for the device to respond to cvq, there's a high possibility that the device is buggy or even malicious. We can have a higher timeout here and it should be still better than waiting forever (the cvq commands need to be serialized so it needs to hold a lock anyway (RTNL) ). Thanks> > > > +} > > +EXPORT_SYMBOL_GPL(virtqueue_wait_for_used); > > + > > +void virtqueue_wake_up(struct virtqueue *_vq) > > +{ > > + struct vring_virtqueue *vq = to_vvq(_vq); > > + > > + wake_up_interruptible(&vq->wq); > > +} > > +EXPORT_SYMBOL_GPL(virtqueue_wake_up); > > + > > MODULE_LICENSE("GPL"); > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > > index dcab9c7e8784..2eb62c774895 100644 > > --- a/include/linux/virtio.h > > +++ b/include/linux/virtio.h > > @@ -72,6 +72,9 @@ void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len); > > void *virtqueue_get_buf_ctx(struct virtqueue *vq, unsigned int *len, > > void **ctx); > > > > +int virtqueue_wait_for_used(struct virtqueue *vq); > > +void virtqueue_wake_up(struct virtqueue *vq); > > + > > void virtqueue_disable_cb(struct virtqueue *vq); > > > > bool virtqueue_enable_cb(struct virtqueue *vq); > > -- > > 2.25.1 >