On Thu, 28 Jul 2022 17:04:36 +0800, Jason Wang [off-list ref] wrote:

On Thu, Jul 28, 2022 at 4:18 PM Xuan Zhuo [off-list ref] wrote:

quoted

On Thu, 28 Jul 2022 15:42:50 +0800, Jason Wang [off-list ref] wrote:

quoted

On Thu, Jul 28, 2022 at 3:24 PM Xuan Zhuo [off-list ref] wrote:

quoted

On Thu, 28 Jul 2022 10:38:51 +0800, Jason Wang [off-list ref] wrote:

quoted

On Wed, Jul 27, 2022 at 3:44 PM Xuan Zhuo [off-list ref] wrote:

quoted

On Wed, 27 Jul 2022 11:12:19 +0800, Jason Wang [off-list ref] wrote:

quoted

在 2022/7/26 15:21, Xuan Zhuo 写道:

quoted

virtio ring split supports resize.

Only after the new vring is successfully allocated based on the new num,
we will release the old vring. In any case, an error is returned,
indicating that the vring still points to the old vring.

In the case of an error, re-initialize(virtqueue_reinit_split()) the
virtqueue to ensure that the vring can be used.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
  drivers/virtio/virtio_ring.c | 34 ++++++++++++++++++++++++++++++++++
  1 file changed, 34 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b6fda91c8059..58355e1ac7d7 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c

@@ -220,6 +220,7 @@ static struct virtqueue *__vring_new_virtqueue(unsigned int index,
                                           void (*callback)(struct virtqueue *),
                                           const char *name);
  static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num);
+static void vring_free(struct virtqueue *_vq);

  /*
   * Helpers.

@@ -1117,6 +1118,39 @@ static struct virtqueue *vring_create_virtqueue_split(
    return vq;
  }

+static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
+{
+   struct vring_virtqueue_split vring_split = {};
+   struct vring_virtqueue *vq = to_vvq(_vq);
+   struct virtio_device *vdev = _vq->vdev;
+   int err;
+
+   err = vring_alloc_queue_split(&vring_split, vdev, num,
+                                 vq->split.vring_align,
+                                 vq->split.may_reduce_num);
+   if (err)
+           goto err;

I think we don't need to do anything here?

Am I missing something?

I meant it looks to me most of the virtqueue_reinit() is unnecessary.
We probably only need to reinit avail/used idx there.

In this function, we can indeed remove some code.

quoted

      static void virtqueue_reinit_split(struct vring_virtqueue *vq)
      {
              int size, i;

              memset(vq->split.vring.desc, 0, vq->split.queue_size_in_bytes);

              size = sizeof(struct vring_desc_state_split) * vq->split.vring.num;
              memset(vq->split.desc_state, 0, size);

              size = sizeof(struct vring_desc_extra) * vq->split.vring.num;
              memset(vq->split.desc_extra, 0, size);

These memsets can be removed, and theoretically it will not cause any
exceptions.

Yes, otherwise we have bugs in detach_buf().

quoted

quoted

              for (i = 0; i < vq->split.vring.num - 1; i++)
                      vq->split.desc_extra[i].next = i + 1;

This can also be removed, but we need to record free_head that will been update
inside virtqueue_init().

We can simply keep free_head unchanged? Otherwise it's a bug somewhere I guess.

quoted

quoted

              virtqueue_init(vq, vq->split.vring.num);

There are some operations in this, which can also be skipped, such as setting
use_dma_api. But I think calling this function directly will be more convenient
for maintenance.

I don't see anything that is necessary here.

These three are currently inside virtqueue_init()

vq->last_used_idx = 0;
vq->event_triggered = false;
vq->num_added = 0;

Right. Let's keep it there.

(Though it's kind of strange that the last_used_idx is not initialized
at the same place with avail_idx/flags_shadow, we can optimize it on
top).

I put free_head = 0 in the attach function, it is only necessary to set
free_head = 0 when a new state/extra is attached.

In this way, when we call virtqueue_init(), we don't have to worry about
free_head being modified.

Rethinking this problem, I think virtqueue_init() can be rewritten and some
variables that will not change are removed from it. (use_dma_api, event,
weak_barriers)

+static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
+{
+       vq->vq.num_free = num;
+
+       if (vq->packed_ring)
+               vq->last_used_idx = 0 | (1 << VRING_PACKED_EVENT_F_WRAP_CTR);
+       else
+               vq->last_used_idx = 0;
+
+       vq->event_triggered = false;
+       vq->num_added = 0;
+
+#ifdef DEBUG
+       vq->in_use = false;
+       vq->last_add_time_valid = false;
+#endif
+}
+

Thanks.

Thanks

quoted

Thanks.

quoted

quoted

quoted

              virtqueue_vring_init_split(&vq->split, vq);

virtqueue_vring_init_split() is necessary.

Right.

quoted

quoted

      }

Another method, we can take out all the variables to be reinitialized
separately, and repackage them into a new function. I don’t think it’s worth
it, because this path will only be reached if the memory allocation fails, which
is a rare occurrence. In this case, doing so will increase the cost of
maintenance. If you think so also, I will remove the above memset in the next
version.

I agree.

Thanks

quoted

Thanks.

quoted

Thanks

quoted

quoted

quoted

+
+   err = vring_alloc_state_extra_split(&vring_split);
+   if (err) {
+           vring_free_split(&vring_split, vdev);
+           goto err;

I suggest to move vring_free_split() into a dedicated error label.

Will change.

Thanks.

quoted

Thanks

quoted

+   }
+
+   vring_free(&vq->vq);
+
+   virtqueue_vring_init_split(&vring_split, vq);
+
+   virtqueue_init(vq, vring_split.vring.num);
+   virtqueue_vring_attach_split(vq, &vring_split);
+
+   return 0;
+
+err:
+   virtqueue_reinit_split(vq);
+   return -ENOMEM;
+}
+

  /*
   * Packed ring specific functions - *_packed().