On Wed, Mar 30, 2022 at 2:15 PM Xuan Zhuo <xuanz...@linux.alibaba.com> wrote: > > On Wed, 30 Mar 2022 11:48:29 +0800, Jason Wang <jasow...@redhat.com> wrote: > > > > 在 2022/3/24 下午4:44, Xuan Zhuo 写道: > > > On Tue, 22 Mar 2022 14:30:29 +0800, Jason Wang <jasow...@redhat.com> > > > 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 <xuanz...@linux.alibaba.com> > > >>> --- > > >>> 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@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization