On Wed, 30 Mar 2022 11:48:29 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
>
> 在 2022/3/24 下午4:44, Xuan Zhuo 写道:
> > 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() .
>
>
> So from what I'm understanding the code, the key point for attaching is:
>
> vq->split.vring = vring;
>
> But this is not what __vring_free() did, it just free the resources.
OK.
>
>
> >
> >>
> >>> + 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?
>
>
> Nope, I meant there are some transports that allocate the vring by
> themselves, we can't resize those vring.
>
> See the callers of vring_new_virtqueue()
So, you mean, do not implement resize for we_own_ring is false.
Thanks.
>
> Thanks
>
>
> >
> > Thanks.
> >
> >
> >> Thanks
> >>
> >>
> >>> + return err;
> >>> +}
> >>> +
> >>>
> >>> /*
> >>> * Packed ring specific functions - *_packed().
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
