Re: [PATCH v7 09/26] virtio_ring: split: implement virtqueue_reset_vring_split()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




在 2022/3/10 下午12:46, Xuan Zhuo 写道:
On Wed, 9 Mar 2022 15:55:44 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
在 2022/3/8 下午8:35, Xuan Zhuo 写道:
virtio ring supports reset.

Queue reset is divided into several stages.

1. notify device queue reset
2. vring release
3. attach new vring
4. notify device queue re-enable

After the first step is completed, the vring reset operation can be
performed. If the newly set vring num does not change, then just reset
the vq related value.

Otherwise, the vring will be released and the vring will be reallocated.
And the vring will be attached to the vq. If this process fails, the
function will exit, and the state of the vq will be the vring release
state. You can call this function again to reallocate the vring.

In addition, vring_align, may_reduce_num are necessary for reallocating
vring, so they are retained when creating vq.

Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
---
   drivers/virtio/virtio_ring.c | 69 ++++++++++++++++++++++++++++++++++++
   1 file changed, 69 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index e0422c04c903..148fb1fd3d5a 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -158,6 +158,12 @@ struct vring_virtqueue {
   			/* DMA address and size information */
   			dma_addr_t queue_dma_addr;
   			size_t queue_size_in_bytes;
+
+			/* The parameters for creating vrings are reserved for
+			 * creating new vrings when enabling reset queue.
+			 */
+			u32 vring_align;
+			bool may_reduce_num;
   		} split;

   		/* Available for packed ring */
@@ -217,6 +223,12 @@ struct vring_virtqueue {
   #endif
   };

+static void vring_free(struct virtqueue *vq);
+static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
+					 struct virtio_device *vdev);
+static int __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
+					  struct virtio_device *vdev,
+					  struct vring vring);

   /*
    * Helpers.
@@ -1012,6 +1024,8 @@ static struct virtqueue *vring_create_virtqueue_split(
   		return NULL;
   	}

+	to_vvq(vq)->split.vring_align = vring_align;
+	to_vvq(vq)->split.may_reduce_num = may_reduce_num;
   	to_vvq(vq)->split.queue_dma_addr = vring.dma_addr;
   	to_vvq(vq)->split.queue_size_in_bytes = vring.queue_size_in_bytes;
   	to_vvq(vq)->we_own_ring = true;
@@ -1019,6 +1033,59 @@ static struct virtqueue *vring_create_virtqueue_split(
   	return vq;
   }

+static int virtqueue_reset_vring_split(struct virtqueue *_vq, u32 num)
+{

So what this function does is to resize the virtqueue actually, I
suggest to rename it as virtqueue_resize_split().
In addition to resize, when num is 0, the function is to reinitialize vq ring
related variables. For example avail_idx_shadow.


We need to move those logic to virtio_reset_vq() (I think we agree to have a better name of it).


So I think 'reset' is more appropriate.


The name is confusing at least to me, since we've already had virtio_reset_vq() and most of the logic is to do the resize.

Thanks



Thanks.


+	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct virtio_device *vdev = _vq->vdev;
+	struct vring_split vring;
+	int err;
+
+	if (num > _vq->num_max)
+		return -E2BIG;
+
+	switch (vq->vq.reset) {
+	case VIRTIO_VQ_RESET_STEP_NONE:
+		return -ENOENT;
+
+	case VIRTIO_VQ_RESET_STEP_VRING_ATTACH:
+	case VIRTIO_VQ_RESET_STEP_DEVICE:
+		if (vq->split.vring.num == num || !num)
+			break;
+
+		vring_free(_vq);
+
+		fallthrough;
+
+	case VIRTIO_VQ_RESET_STEP_VRING_RELEASE:
+		if (!num)
+			num = vq->split.vring.num;
+
+		err = vring_create_vring_split(&vring, vdev,
+					       vq->split.vring_align,
+					       vq->weak_barriers,
+					       vq->split.may_reduce_num, num);
+		if (err)
+			return -ENOMEM;

We'd better need a safe fallback here like:

If we can't allocate new memory, we can keep using the current one.
Otherwise an ethtool -G fail may make the device not usable.

This could be done by not freeing the old vring and virtqueue states
until new is allocated.


+
+		err = __vring_virtqueue_attach_split(vq, vdev, vring.vring);
+		if (err) {
+			vring_free_queue(vdev, vring.queue_size_in_bytes,
+					 vring.queue,
+					 vring.dma_addr);
+			return -ENOMEM;
+		}
+
+		vq->split.queue_dma_addr = vring.dma_addr;
+		vq->split.queue_size_in_bytes = vring.queue_size_in_bytes;
+	}
+
+	__vring_virtqueue_init_split(vq, vdev);
+	vq->we_own_ring = true;

This seems wrong, we have the transport (rproc/mlxtbf) that allocate the
vring by themselves. I think we need to fail the resize for we_own_ring
== false.

Thanks



+	vq->vq.reset = VIRTIO_VQ_RESET_STEP_VRING_ATTACH;
+
+	return 0;
+}
+

   /*
    * Packed ring specific functions - *_packed().
@@ -2317,6 +2384,8 @@ static int __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
   static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
   					 struct virtio_device *vdev)
   {
+	vq->vq.reset = VIRTIO_VQ_RESET_STEP_NONE;
+
   	vq->packed_ring = false;
   	vq->we_own_ring = false;
   	vq->broken = false;




[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Photo Sharing]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux