Jason Wang
2022-Mar-22 06:30 UTC
[PATCH v8 10/16] virtio_ring: split: implement virtqueue_resize_split()
? 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 at 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.> + 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. Thanks> + return err; > +} > + > > /* > * Packed ring specific functions - *_packed().
Xuan Zhuo
2022-Mar-24 08:44 UTC
[PATCH v8 10/16] virtio_ring: split: implement virtqueue_resize_split()
On Tue, 22 Mar 2022 14:30:29 +0800, Jason Wang <jasowang at 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 <xuanzhuo at 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() .> > > > + 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(). >