On Tue, 22 Mar 2022 14:30:29 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
>
> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> > 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, we will re-initialize the state of the vring to ensure that
> > the vring can be used.
> >
> > In addition, vring_align, may_reduce_num are necessary for reallocating
> > vring, so they are retained for creating vq.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > ---
> > drivers/virtio/virtio_ring.c | 62 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 62 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 81bbfd65411e..a15869514146 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -139,6 +139,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 */
> > @@ -198,6 +204,16 @@ 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 void __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
> > + struct vring vring,
> > + struct vring_desc_state_split *desc_state,
> > + struct vring_desc_extra *desc_extra);
> > +static int __vring_alloc_state_extra_split(u32 num,
> > + struct vring_desc_state_split **desc_state,
> > + struct vring_desc_extra **desc_extra);
> >
> > /*
> > * Helpers.
> > @@ -991,6 +1007,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 = dma_addr;
> > to_vvq(vq)->split.queue_size_in_bytes = queue_size_in_bytes;
> > to_vvq(vq)->we_own_ring = true;
> > @@ -998,6 +1016,50 @@ static struct virtqueue *vring_create_virtqueue_split(
> > return vq;
> > }
> >
> > +static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
> > +{
> > + struct vring_virtqueue *vq = to_vvq(_vq);
> > + struct virtio_device *vdev = _vq->vdev;
> > + struct vring_desc_state_split *state;
> > + struct vring_desc_extra *extra;
> > + size_t queue_size_in_bytes;
> > + dma_addr_t dma_addr;
> > + struct vring vring;
> > + int err = -ENOMEM;
> > + void *queue;
> > +
> > + BUG_ON(!vq->we_own_ring);
>
>
> I don't see any checks in virtqueue_resize(). So I think it's better to
> either
>
> 1) return -EINVAL here
>
> or
>
> 2) add a check in virtqueue_resize and fail there
>
>
> > +
> > + queue = vring_alloc_queue_split(vdev, &dma_addr, &num,
> > + vq->split.vring_align,
> > + vq->weak_barriers,
> > + vq->split.may_reduce_num);
> > + if (!queue)
> > + goto init;
> > +
> > + queue_size_in_bytes = vring_size(num, vq->split.vring_align);
> > +
> > + err = __vring_alloc_state_extra_split(num, &state, &extra);
> > + if (err) {
> > + vring_free_queue(vdev, queue_size_in_bytes, queue, dma_addr);
> > + goto init;
> > + }
> > +
> > + __vring_free(&vq->vq);
> > +
> > + vring_init(&vring, num, queue, vq->split.vring_align);
> > + __vring_virtqueue_attach_split(vq, vring, state, extra);
>
>
> I wonder if we need a symmetric virtqueue_resize_detach() internal helper.
I think __vring_free() is somewhat similar to what you said about
virtqueue_resize_detach() .
>
>
> > + vq->split.queue_dma_addr = dma_addr;
> > + vq->split.queue_size_in_bytes = queue_size_in_bytes;
> > +
> > + err = 0;
> > +
> > +init:
> > + __vring_virtqueue_init_split(vq, vdev);
> > + vq->we_own_ring = true;
>
>
> Then we can leave this unchanged.
I think you mean "vq->we_own_ring = true";
The reason for modifying we_own_ring alone is that in the general process of
creating a ring, __vring_virtqueue_init_split is called in
__vring_new_virtqueue. At this time, we_own_ring is false.
vring_create_virtqueue_split will change it to true. So after calling
__vring_virtqueue_init_split alone, we_own_ring is false.
I think it's possible to wrap __vring_virtqueue_init_split() again
static void vring_virtqueue_init_split(struct vring_virtqueue *vq,
struct virtio_device *vdev)
{
__vring_virtqueue_init_split(vq, vdev);
vq->we_own_ring = true;
}
Is this what you want?
Thanks.
>
> Thanks
>
>
> > + return err;
> > +}
> > +
> >
> > /*
> > * Packed ring specific functions - *_packed().
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
