diff mbox series

[v6,06/26] virtio_ring: packed: extrace the logic of creating vring

Message ID 20220224081102.80224-7-xuanzhuo@linux.alibaba.com (mailing list archive)
State Superseded
Headers show
Series virtio pci support VIRTIO_F_RING_RESET | expand

Commit Message

Xuan Zhuo Feb. 24, 2022, 8:10 a.m. UTC
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@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 121 ++++++++++++++++++++++++++---------
 1 file changed, 92 insertions(+), 29 deletions(-)

Comments

Michael S. Tsirkin March 7, 2022, 10:17 p.m. UTC | #1
On Thu, Feb 24, 2022 at 04:10:42PM +0800, Xuan Zhuo wrote:
> 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@linux.alibaba.com>

Subject has a typo.
Besides:

> ---
>  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 dc6313b79305..41864c5e665f 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -92,6 +92,18 @@ struct vring_split {
>  	struct vring vring;
>  };
>  
> +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;
> +};
> +
>  struct vring_virtqueue {
>  	struct virtqueue vq;
>  
> @@ -1683,45 +1695,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(struct vring_virtqueue *v
>  	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_dma_addr         = 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);

ring_size_in_bytes is uninitialized here.

Which begs the question how was this tested patchset generally and
this patch in particular.
Please add note on tested configurations and tests run to the patchset.

> +}
> +
> +static int vring_create_vring_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_create_vring_packed(&vring, vdev, num))
> +		goto err_vq;
>  
>  	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
>  	if (!vq)
> @@ -1753,17 +1821,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;
> @@ -1804,12 +1872,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;
>  }
>  
> -- 
> 2.31.0
Xuan Zhuo March 8, 2022, 7:01 a.m. UTC | #2
On Mon, 7 Mar 2022 17:17:51 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Feb 24, 2022 at 04:10:42PM +0800, Xuan Zhuo wrote:
> > 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@linux.alibaba.com>
>
> Subject has a typo.

I will fix it.

> Besides:
>
> > ---
> >  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 dc6313b79305..41864c5e665f 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -92,6 +92,18 @@ struct vring_split {
> >  	struct vring vring;
> >  };
> >
> > +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;
> > +};
> > +
> >  struct vring_virtqueue {
> >  	struct virtqueue vq;
> >
> > @@ -1683,45 +1695,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(struct vring_virtqueue *v
> >  	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_dma_addr         = 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);
>
> ring_size_in_bytes is uninitialized here.
>
> Which begs the question how was this tested patchset generally and
> this patch in particular.
> Please add note on tested configurations and tests run to the patchset.

Sorry, my environment is running in split mode. I did not retest the packed mode
before sending patches. Because my dpdk vhost-user is not easy to use, I
need to change the kernel of the host.

I would like to ask if there are other lightweight environments that can be used
to test packed mode.


Thanks.


>
> > +}
> > +
> > +static int vring_create_vring_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_create_vring_packed(&vring, vdev, num))
> > +		goto err_vq;
> >
> >  	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
> >  	if (!vq)
> > @@ -1753,17 +1821,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;
> > @@ -1804,12 +1872,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;
> >  }
> >
> > --
> > 2.31.0
>
Jason Wang March 8, 2022, 7:28 a.m. UTC | #3
在 2022/3/8 下午3:01, Xuan Zhuo 写道:
> On Mon, 7 Mar 2022 17:17:51 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> On Thu, Feb 24, 2022 at 04:10:42PM +0800, Xuan Zhuo wrote:
>>> 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@linux.alibaba.com>
>> Subject has a typo.
> I will fix it.
>
>> Besides:
>>
>>> ---
>>>   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 dc6313b79305..41864c5e665f 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -92,6 +92,18 @@ struct vring_split {
>>>   	struct vring vring;
>>>   };
>>>
>>> +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;
>>> +};
>>> +
>>>   struct vring_virtqueue {
>>>   	struct virtqueue vq;
>>>
>>> @@ -1683,45 +1695,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(struct vring_virtqueue *v
>>>   	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_dma_addr         = 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);
>> ring_size_in_bytes is uninitialized here.
>>
>> Which begs the question how was this tested patchset generally and
>> this patch in particular.
>> Please add note on tested configurations and tests run to the patchset.
> Sorry, my environment is running in split mode. I did not retest the packed mode
> before sending patches. Because my dpdk vhost-user is not easy to use, I
> need to change the kernel of the host.
>
> I would like to ask if there are other lightweight environments that can be used
> to test packed mode.


You can use Qemu's dataplane. It has support for packed virtqueue.

Thanks


>
>
> Thanks.
>
>
>>> +}
>>> +
>>> +static int vring_create_vring_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_create_vring_packed(&vring, vdev, num))
>>> +		goto err_vq;
>>>
>>>   	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
>>>   	if (!vq)
>>> @@ -1753,17 +1821,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;
>>> @@ -1804,12 +1872,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;
>>>   }
>>>
>>> --
>>> 2.31.0
Xuan Zhuo March 8, 2022, 8:01 a.m. UTC | #4
On Tue, 8 Mar 2022 15:28:22 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/3/8 下午3:01, Xuan Zhuo 写道:
> > On Mon, 7 Mar 2022 17:17:51 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> On Thu, Feb 24, 2022 at 04:10:42PM +0800, Xuan Zhuo wrote:
> >>> 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@linux.alibaba.com>
> >> Subject has a typo.
> > I will fix it.
> >
> >> Besides:
> >>
> >>> ---
> >>>   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 dc6313b79305..41864c5e665f 100644
> >>> --- a/drivers/virtio/virtio_ring.c
> >>> +++ b/drivers/virtio/virtio_ring.c
> >>> @@ -92,6 +92,18 @@ struct vring_split {
> >>>   	struct vring vring;
> >>>   };
> >>>
> >>> +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;
> >>> +};
> >>> +
> >>>   struct vring_virtqueue {
> >>>   	struct virtqueue vq;
> >>>
> >>> @@ -1683,45 +1695,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(struct vring_virtqueue *v
> >>>   	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_dma_addr         = 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);
> >> ring_size_in_bytes is uninitialized here.
> >>
> >> Which begs the question how was this tested patchset generally and
> >> this patch in particular.
> >> Please add note on tested configurations and tests run to the patchset.
> > Sorry, my environment is running in split mode. I did not retest the packed mode
> > before sending patches. Because my dpdk vhost-user is not easy to use, I
> > need to change the kernel of the host.
> >
> > I would like to ask if there are other lightweight environments that can be used
> > to test packed mode.
>
>
> You can use Qemu's dataplane. It has support for packed virtqueue.


I thought about it, I feel that the current Qemu's virtio-net seems to have no
problem if it adds a PACKED feature, so I tried it. I manually added the PACKED
feature to Qemu's virtio-net. After I start the vm, run OK. PACKED also
negotiated successfully. After the test, it is also OK.

I think virtio-net in Qemu just does not open PACKED, but the implementation of
PACKED is in virtio core, so as long as virtio-net opens PACKED, it will be
fine. If there is any problem, I hope someone will approve it.

If my idea is correct, then virtio-net can add a parameter to open PACKED, which
will be very convenient when testing the packed mode of virtio.

Thanks.

>
> Thanks
>
>
> >
> >
> > Thanks.
> >
> >
> >>> +}
> >>> +
> >>> +static int vring_create_vring_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_create_vring_packed(&vring, vdev, num))
> >>> +		goto err_vq;
> >>>
> >>>   	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
> >>>   	if (!vq)
> >>> @@ -1753,17 +1821,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;
> >>> @@ -1804,12 +1872,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;
> >>>   }
> >>>
> >>> --
> >>> 2.31.0
>
diff mbox series

Patch

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index dc6313b79305..41864c5e665f 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -92,6 +92,18 @@  struct vring_split {
 	struct vring vring;
 };
 
+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;
+};
+
 struct vring_virtqueue {
 	struct virtqueue vq;
 
@@ -1683,45 +1695,101 @@  static struct vring_desc_extra *vring_alloc_desc_extra(struct vring_virtqueue *v
 	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_dma_addr         = 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_create_vring_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_create_vring_packed(&vring, vdev, num))
+		goto err_vq;
 
 	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
 	if (!vq)
@@ -1753,17 +1821,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;
@@ -1804,12 +1872,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;
 }