Re: [PATCH 3/4] virtio_ring: introduce a per virtqueue waitqueue
From: Jason Wang
Date: Tue Dec 27 2022 - 04:10:34 EST
在 2022/12/27 15:19, Michael S. Tsirkin 写道:
On Tue, Dec 27, 2022 at 11:47:34AM +0800, Jason Wang wrote:
On Tue, Dec 27, 2022 at 7:34 AM Michael S. Tsirkin <mst@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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
With a TODO item like this I'd expect this to be an RFC.
Here's why:
Making driver more robust from device failures is a laudable goal but it's really
hard to be 100% foolproof here. E.g. device can just block pci reads and
it would be very hard to recover.
Yes.
So I'm going to only merge patches
like this if they at least theoretically have very little chance
of breaking existing users.
AFAIK, this is not theoretical, consider:
1) DPU may implement virtio-net CVQ with codes running in CPU
2) VDUSE may want to support CVQ in the future
And note that in most setups, CVQ is only used at startup and then left mostly alone.
Finally, note that lots of guests need virtio to do anything useful at all.
So just failing commands is not enough to recover - you need to try
harder maybe by attempting to reset device.
This requires upper layer support which seems not existed in the
networking subsystem.
Could be a question of
policy - might need to make this guest configurable.
Yes.
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