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(). >
Jason Wang
2022-Mar-30 03:48 UTC
[PATCH v8 10/16] virtio_ring: split: implement virtqueue_resize_split()
? 2022/3/24 ??4:44, Xuan Zhuo ??:> 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() .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.> >> >>> + 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() Thanks> > Thanks. > > >> Thanks >> >> >>> + return err; >>> +} >>> + >>> >>> /* >>> * Packed ring specific functions - *_packed().