Virtualization - Mar 2022 - [PATCH v8 11/16] virtio_ring: packed: extract the logic of alloc queue

On Tue, 22 Mar 2022 14:38:12 +0800, Jason Wang <jasowang at redhat.com>
wrote:
>
> ? 2022/3/14 ??5:34, Xuan Zhuo ??:
> > Separate the logic of packed to create vring queue.
> >
> > For the convenience of passing parameters, add a structure
> > vring_packed.
> >
> > This feature is required for subsequent virtuqueue reset vring.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com>
> > ---
> >   drivers/virtio/virtio_ring.c | 121
++++++++++++++++++++++++++---------
> >   1 file changed, 92 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c
b/drivers/virtio/virtio_ring.c
> > index a15869514146..96ff2dabda14 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -85,6 +85,18 @@ struct vring_desc_extra {
> >   	u16 next;			/* The next desc state in a list. */
> >   };
> >
> > +struct vring_packed {
> > +	u32 num;
> > +	struct vring_packed_desc *ring;
> > +	struct vring_packed_desc_event *driver;
> > +	struct vring_packed_desc_event *device;
> > +	dma_addr_t ring_dma_addr;
> > +	dma_addr_t driver_event_dma_addr;
> > +	dma_addr_t device_event_dma_addr;
> > +	size_t ring_size_in_bytes;
> > +	size_t event_size_in_bytes;
> > +};
> > +
>
>
> Interesting,? a nitpick here is that if it is only used by one helper,
> it's probably not worth to bother.

Indeed only one helper will use it. Because vring_alloc_queue_packed will pass
too many variables. If we use function parameters, I think this function is too
ugly.

>
> And if we want to do that, we need
>
> 1) use a separated patch
If we split this patch again, we can only separate vring_free_vring_packed()
out.
>
> 2) do it for split virtqueue as well
Earlier versions, I did create a struct for split too, but you replied that it
wasn't worth it, so I removed the separate struct used by split.

Thanks.
>
> Thanks
>
>
> >   struct vring_virtqueue {
> >   	struct virtqueue vq;
> >
> > @@ -1735,45 +1747,101 @@ static struct vring_desc_extra
*vring_alloc_desc_extra(unsigned int num)
> >   	return desc_extra;
> >   }
> >
> > -static struct virtqueue *vring_create_virtqueue_packed(
> > -	unsigned int index,
> > -	unsigned int num,
> > -	unsigned int vring_align,
> > -	struct virtio_device *vdev,
> > -	bool weak_barriers,
> > -	bool may_reduce_num,
> > -	bool context,
> > -	bool (*notify)(struct virtqueue *),
> > -	void (*callback)(struct virtqueue *),
> > -	const char *name)
> > +static void vring_free_vring_packed(struct vring_packed *vring,
> > +				    struct virtio_device *vdev)
> > +{
> > +	dma_addr_t ring_dma_addr, driver_event_dma_addr,
device_event_dma_addr;
> > +	struct vring_packed_desc_event *driver, *device;
> > +	size_t ring_size_in_bytes, event_size_in_bytes;
> > +	struct vring_packed_desc *ring;
> > +
> > +	ring                  = vring->ring;
> > +	driver                = vring->driver;
> > +	device                = vring->device;
> > +	ring_size_in_bytes    = vring->ring_size_in_bytes;
> > +	event_size_in_bytes   = vring->event_size_in_bytes;
> > +	ring_dma_addr         = vring->ring_dma_addr;
> > +	driver_event_dma_addr = vring->driver_event_dma_addr;
> > +	device_event_dma_addr = vring->device_event_dma_addr;
> > +
> > +	if (device)
> > +		vring_free_queue(vdev, event_size_in_bytes, device,
device_event_dma_addr);
> > +
> > +	if (driver)
> > +		vring_free_queue(vdev, event_size_in_bytes, driver,
driver_event_dma_addr);
> > +
> > +	if (ring)
> > +		vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
> > +}
> > +
> > +static int vring_alloc_queue_packed(struct vring_packed *vring,
> > +				    struct virtio_device *vdev,
> > +				    u32 num)
> >   {
> > -	struct vring_virtqueue *vq;
> >   	struct vring_packed_desc *ring;
> >   	struct vring_packed_desc_event *driver, *device;
> >   	dma_addr_t ring_dma_addr, driver_event_dma_addr,
device_event_dma_addr;
> >   	size_t ring_size_in_bytes, event_size_in_bytes;
> >
> > +	memset(vring, 0, sizeof(*vring));
> > +
> >   	ring_size_in_bytes = num * sizeof(struct vring_packed_desc);
> >
> >   	ring = vring_alloc_queue(vdev, ring_size_in_bytes,
> >   				 &ring_dma_addr,
> >   				 GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> >   	if (!ring)
> > -		goto err_ring;
> > +		goto err;
> > +
> > +	vring->num = num;
> > +	vring->ring = ring;
> > +	vring->ring_size_in_bytes = ring_size_in_bytes;
> > +	vring->ring_dma_addr = ring_dma_addr;
> >
> >   	event_size_in_bytes = sizeof(struct vring_packed_desc_event);
> > +	vring->event_size_in_bytes = event_size_in_bytes;
> >
> >   	driver = vring_alloc_queue(vdev, event_size_in_bytes,
> >   				   &driver_event_dma_addr,
> >   				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> >   	if (!driver)
> > -		goto err_driver;
> > +		goto err;
> > +
> > +	vring->driver = driver;
> > +	vring->driver_event_dma_addr = driver_event_dma_addr;
> >
> >   	device = vring_alloc_queue(vdev, event_size_in_bytes,
> >   				   &device_event_dma_addr,
> >   				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> >   	if (!device)
> > -		goto err_device;
> > +		goto err;
> > +
> > +	vring->device = device;
> > +	vring->device_event_dma_addr = device_event_dma_addr;
> > +	return 0;
> > +
> > +err:
> > +	vring_free_vring_packed(vring, vdev);
> > +	return -ENOMEM;
> > +}
> > +
> > +static struct virtqueue *vring_create_virtqueue_packed(
> > +	unsigned int index,
> > +	unsigned int num,
> > +	unsigned int vring_align,
> > +	struct virtio_device *vdev,
> > +	bool weak_barriers,
> > +	bool may_reduce_num,
> > +	bool context,
> > +	bool (*notify)(struct virtqueue *),
> > +	void (*callback)(struct virtqueue *),
> > +	const char *name)
> > +{
> > +	struct vring_virtqueue *vq;
> > +	struct vring_packed vring;
> > +
> > +	if (vring_alloc_queue_packed(&vring, vdev, num))
> > +		goto err_vq;
> >
> >   	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
> >   	if (!vq)
> > @@ -1805,17 +1873,17 @@ static struct virtqueue
*vring_create_virtqueue_packed(
> >   	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> >   		vq->weak_barriers = false;
> >
> > -	vq->packed.ring_dma_addr = ring_dma_addr;
> > -	vq->packed.driver_event_dma_addr = driver_event_dma_addr;
> > -	vq->packed.device_event_dma_addr = device_event_dma_addr;
> > +	vq->packed.ring_dma_addr = vring.ring_dma_addr;
> > +	vq->packed.driver_event_dma_addr = vring.driver_event_dma_addr;
> > +	vq->packed.device_event_dma_addr = vring.device_event_dma_addr;
> >
> > -	vq->packed.ring_size_in_bytes = ring_size_in_bytes;
> > -	vq->packed.event_size_in_bytes = event_size_in_bytes;
> > +	vq->packed.ring_size_in_bytes = vring.ring_size_in_bytes;
> > +	vq->packed.event_size_in_bytes = vring.event_size_in_bytes;
> >
> >   	vq->packed.vring.num = num;
> > -	vq->packed.vring.desc = ring;
> > -	vq->packed.vring.driver = driver;
> > -	vq->packed.vring.device = device;
> > +	vq->packed.vring.desc = vring.ring;
> > +	vq->packed.vring.driver = vring.driver;
> > +	vq->packed.vring.device = vring.device;
> >
> >   	vq->packed.next_avail_idx = 0;
> >   	vq->packed.avail_wrap_counter = 1;
> > @@ -1856,12 +1924,7 @@ static struct virtqueue
*vring_create_virtqueue_packed(
> >   err_desc_state:
> >   	kfree(vq);
> >   err_vq:
> > -	vring_free_queue(vdev, event_size_in_bytes, device,
device_event_dma_addr);
> > -err_device:
> > -	vring_free_queue(vdev, event_size_in_bytes, driver,
driver_event_dma_addr);
> > -err_driver:
> > -	vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
> > -err_ring:
> > +	vring_free_vring_packed(&vring, vdev);
> >   	return NULL;
> >   }
> >
>

? 2022/3/24 ??4:28, Xuan Zhuo ??:
> On Tue, 22 Mar 2022 14:38:12 +0800, Jason Wang <jasowang at
redhat.com> wrote:
>> ? 2022/3/14 ??5:34, Xuan Zhuo ??:
>>> Separate the logic of packed to create vring queue.
>>>
>>> For the convenience of passing parameters, add a structure
>>> vring_packed.
>>>
>>> This feature is required for subsequent virtuqueue reset vring.
>>>
>>> Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com>
>>> ---
>>>    drivers/virtio/virtio_ring.c | 121
++++++++++++++++++++++++++---------
>>>    1 file changed, 92 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/virtio/virtio_ring.c
b/drivers/virtio/virtio_ring.c
>>> index a15869514146..96ff2dabda14 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -85,6 +85,18 @@ struct vring_desc_extra {
>>>    	u16 next;			/* The next desc state in a list. */
>>>    };
>>>
>>> +struct vring_packed {
>>> +	u32 num;
>>> +	struct vring_packed_desc *ring;
>>> +	struct vring_packed_desc_event *driver;
>>> +	struct vring_packed_desc_event *device;
>>> +	dma_addr_t ring_dma_addr;
>>> +	dma_addr_t driver_event_dma_addr;
>>> +	dma_addr_t device_event_dma_addr;
>>> +	size_t ring_size_in_bytes;
>>> +	size_t event_size_in_bytes;
>>> +};
>>> +
>>
>> Interesting,? a nitpick here is that if it is only used by one helper,
>> it's probably not worth to bother.
>
> Indeed only one helper will use it. Because vring_alloc_queue_packed will
pass
> too many variables. If we use function parameters, I think this function is
too
> ugly.
>
>
>> And if we want to do that, we need
>>
>> 1) use a separated patch
> If we split this patch again, we can only separate
vring_free_vring_packed()
> out.
>
>> 2) do it for split virtqueue as well
> Earlier versions, I did create a struct for split too, but you replied that
it
> wasn't worth it, so I removed the separate struct used by split.

So I think let's just stick to the current style by passing individual 
parameters, and we can refactor it on top of this series.

This will let us to focus on the core logic.

Thanks

>
> Thanks.
>
>> Thanks
>>
>>
>>>    struct vring_virtqueue {
>>>    	struct virtqueue vq;
>>>
>>> @@ -1735,45 +1747,101 @@ static struct vring_desc_extra
*vring_alloc_desc_extra(unsigned int num)
>>>    	return desc_extra;
>>>    }
>>>
>>> -static struct virtqueue *vring_create_virtqueue_packed(
>>> -	unsigned int index,
>>> -	unsigned int num,
>>> -	unsigned int vring_align,
>>> -	struct virtio_device *vdev,
>>> -	bool weak_barriers,
>>> -	bool may_reduce_num,
>>> -	bool context,
>>> -	bool (*notify)(struct virtqueue *),
>>> -	void (*callback)(struct virtqueue *),
>>> -	const char *name)
>>> +static void vring_free_vring_packed(struct vring_packed *vring,
>>> +				    struct virtio_device *vdev)
>>> +{
>>> +	dma_addr_t ring_dma_addr, driver_event_dma_addr,
device_event_dma_addr;
>>> +	struct vring_packed_desc_event *driver, *device;
>>> +	size_t ring_size_in_bytes, event_size_in_bytes;
>>> +	struct vring_packed_desc *ring;
>>> +
>>> +	ring                  = vring->ring;
>>> +	driver                = vring->driver;
>>> +	device                = vring->device;
>>> +	ring_size_in_bytes    = vring->ring_size_in_bytes;
>>> +	event_size_in_bytes   = vring->event_size_in_bytes;
>>> +	ring_dma_addr         = vring->ring_dma_addr;
>>> +	driver_event_dma_addr = vring->driver_event_dma_addr;
>>> +	device_event_dma_addr = vring->device_event_dma_addr;
>>> +
>>> +	if (device)
>>> +		vring_free_queue(vdev, event_size_in_bytes, device,
device_event_dma_addr);
>>> +
>>> +	if (driver)
>>> +		vring_free_queue(vdev, event_size_in_bytes, driver,
driver_event_dma_addr);
>>> +
>>> +	if (ring)
>>> +		vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
>>> +}
>>> +
>>> +static int vring_alloc_queue_packed(struct vring_packed *vring,
>>> +				    struct virtio_device *vdev,
>>> +				    u32 num)
>>>    {
>>> -	struct vring_virtqueue *vq;
>>>    	struct vring_packed_desc *ring;
>>>    	struct vring_packed_desc_event *driver, *device;
>>>    	dma_addr_t ring_dma_addr, driver_event_dma_addr,
device_event_dma_addr;
>>>    	size_t ring_size_in_bytes, event_size_in_bytes;
>>>
>>> +	memset(vring, 0, sizeof(*vring));
>>> +
>>>    	ring_size_in_bytes = num * sizeof(struct vring_packed_desc);
>>>
>>>    	ring = vring_alloc_queue(vdev, ring_size_in_bytes,
>>>    				 &ring_dma_addr,
>>>    				 GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>>>    	if (!ring)
>>> -		goto err_ring;
>>> +		goto err;
>>> +
>>> +	vring->num = num;
>>> +	vring->ring = ring;
>>> +	vring->ring_size_in_bytes = ring_size_in_bytes;
>>> +	vring->ring_dma_addr = ring_dma_addr;
>>>
>>>    	event_size_in_bytes = sizeof(struct vring_packed_desc_event);
>>> +	vring->event_size_in_bytes = event_size_in_bytes;
>>>
>>>    	driver = vring_alloc_queue(vdev, event_size_in_bytes,
>>>    				   &driver_event_dma_addr,
>>>    				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>>>    	if (!driver)
>>> -		goto err_driver;
>>> +		goto err;
>>> +
>>> +	vring->driver = driver;
>>> +	vring->driver_event_dma_addr = driver_event_dma_addr;
>>>
>>>    	device = vring_alloc_queue(vdev, event_size_in_bytes,
>>>    				   &device_event_dma_addr,
>>>    				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>>>    	if (!device)
>>> -		goto err_device;
>>> +		goto err;
>>> +
>>> +	vring->device = device;
>>> +	vring->device_event_dma_addr = device_event_dma_addr;
>>> +	return 0;
>>> +
>>> +err:
>>> +	vring_free_vring_packed(vring, vdev);
>>> +	return -ENOMEM;
>>> +}
>>> +
>>> +static struct virtqueue *vring_create_virtqueue_packed(
>>> +	unsigned int index,
>>> +	unsigned int num,
>>> +	unsigned int vring_align,
>>> +	struct virtio_device *vdev,
>>> +	bool weak_barriers,
>>> +	bool may_reduce_num,
>>> +	bool context,
>>> +	bool (*notify)(struct virtqueue *),
>>> +	void (*callback)(struct virtqueue *),
>>> +	const char *name)
>>> +{
>>> +	struct vring_virtqueue *vq;
>>> +	struct vring_packed vring;
>>> +
>>> +	if (vring_alloc_queue_packed(&vring, vdev, num))
>>> +		goto err_vq;
>>>
>>>    	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
>>>    	if (!vq)
>>> @@ -1805,17 +1873,17 @@ static struct virtqueue
*vring_create_virtqueue_packed(
>>>    	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
>>>    		vq->weak_barriers = false;
>>>
>>> -	vq->packed.ring_dma_addr = ring_dma_addr;
>>> -	vq->packed.driver_event_dma_addr = driver_event_dma_addr;
>>> -	vq->packed.device_event_dma_addr = device_event_dma_addr;
>>> +	vq->packed.ring_dma_addr = vring.ring_dma_addr;
>>> +	vq->packed.driver_event_dma_addr =
vring.driver_event_dma_addr;
>>> +	vq->packed.device_event_dma_addr =
vring.device_event_dma_addr;
>>>
>>> -	vq->packed.ring_size_in_bytes = ring_size_in_bytes;
>>> -	vq->packed.event_size_in_bytes = event_size_in_bytes;
>>> +	vq->packed.ring_size_in_bytes = vring.ring_size_in_bytes;
>>> +	vq->packed.event_size_in_bytes = vring.event_size_in_bytes;
>>>
>>>    	vq->packed.vring.num = num;
>>> -	vq->packed.vring.desc = ring;
>>> -	vq->packed.vring.driver = driver;
>>> -	vq->packed.vring.device = device;
>>> +	vq->packed.vring.desc = vring.ring;
>>> +	vq->packed.vring.driver = vring.driver;
>>> +	vq->packed.vring.device = vring.device;
>>>
>>>    	vq->packed.next_avail_idx = 0;
>>>    	vq->packed.avail_wrap_counter = 1;
>>> @@ -1856,12 +1924,7 @@ static struct virtqueue
*vring_create_virtqueue_packed(
>>>    err_desc_state:
>>>    	kfree(vq);
>>>    err_vq:
>>> -	vring_free_queue(vdev, event_size_in_bytes, device,
device_event_dma_addr);
>>> -err_device:
>>> -	vring_free_queue(vdev, event_size_in_bytes, driver,
driver_event_dma_addr);
>>> -err_driver:
>>> -	vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
>>> -err_ring:
>>> +	vring_free_vring_packed(&vring, vdev);
>>>    	return NULL;
>>>    }
>>>