On Wed, Mar 30, 2022 at 2:15 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
>
> 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.
Yes, I think so. This is because the vring is not allocated by us,
even if we resize there's no way for the user to know about that.
Thanks
>
> Thanks.
>
> >
> > Thanks
> >
> >
> > >
> > > Thanks.
> > >
> > >
> > >> Thanks
> > >>
> > >>
> > >>> + return err;
> > >>> +}
> > >>> +
> > >>>
> > >>> /*
> > >>> * Packed ring specific functions - *_packed().
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
