diff mbox series

[v9,12/32] virtio_ring: packed: extract the logic of alloc queue

Message ID 20220406034346.74409-13-xuanzhuo@linux.alibaba.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series virtio pci support VIRTIO_F_RING_RESET (refactor vring) | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Xuan Zhuo April 6, 2022, 3:43 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 | 70 ++++++++++++++++++++++++++++--------
 1 file changed, 56 insertions(+), 14 deletions(-)

Comments

Jason Wang April 12, 2022, 6:28 a.m. UTC | #1
在 2022/4/6 上午11:43, 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@linux.alibaba.com>
> ---
>   drivers/virtio/virtio_ring.c | 70 ++++++++++++++++++++++++++++--------
>   1 file changed, 56 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 33864134a744..ea451ae2aaef 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1817,19 +1817,17 @@ 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 int vring_alloc_queue_packed(struct virtio_device *vdev,
> +				    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 *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;
> @@ -1857,6 +1855,52 @@ static struct virtqueue *vring_create_virtqueue_packed(
>   	if (!device)
>   		goto err_device;
>   
> +	*_ring                   = ring;
> +	*_driver                 = driver;
> +	*_device                 = device;
> +	*_ring_dma_addr          = ring_dma_addr;
> +	*_driver_event_dma_addr  = driver_event_dma_addr;
> +	*_device_event_dma_addr  = device_event_dma_addr;
> +	*_ring_size_in_bytes     = ring_size_in_bytes;
> +	*_event_size_in_bytes    = event_size_in_bytes;


I wonder if we can simply factor out split and packed from struct 
vring_virtqueue:

struct vring_virtqueue {
     union {
         struct {} split;
         struct {} packed;
     };
};

to

struct vring_virtqueue_split {};
struct vring_virtqueue_packed {};

Then we can do things like:

vring_create_virtqueue_packed(struct virtio_device *vdev, u32 num, 
struct vring_virtqueue_packed *packed);

and

vring_vritqueue_attach_packed(struct vring_virtqueue *vq, struct 
vring_virtqueue_packed packed);

Thanks


> +
> +	return 0;
> +
> +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:
> +	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)
> +{
> +	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;
> +	struct vring_virtqueue *vq;
> +
> +	if (vring_alloc_queue_packed(vdev, num, &ring, &driver, &device,
> +				     &ring_dma_addr, &driver_event_dma_addr,
> +				     &device_event_dma_addr,
> +				     &ring_size_in_bytes,
> +				     &event_size_in_bytes))
> +		goto err_ring;
> +
>   	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
>   	if (!vq)
>   		goto err_vq;
> @@ -1939,9 +1983,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
>   	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:
>   	return NULL;
Xuan Zhuo April 13, 2022, 3:23 a.m. UTC | #2
On Tue, 12 Apr 2022 14:28:24 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/4/6 上午11:43, 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@linux.alibaba.com>
> > ---
> >   drivers/virtio/virtio_ring.c | 70 ++++++++++++++++++++++++++++--------
> >   1 file changed, 56 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 33864134a744..ea451ae2aaef 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -1817,19 +1817,17 @@ 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 int vring_alloc_queue_packed(struct virtio_device *vdev,
> > +				    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 *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;
> > @@ -1857,6 +1855,52 @@ static struct virtqueue *vring_create_virtqueue_packed(
> >   	if (!device)
> >   		goto err_device;
> >
> > +	*_ring                   = ring;
> > +	*_driver                 = driver;
> > +	*_device                 = device;
> > +	*_ring_dma_addr          = ring_dma_addr;
> > +	*_driver_event_dma_addr  = driver_event_dma_addr;
> > +	*_device_event_dma_addr  = device_event_dma_addr;
> > +	*_ring_size_in_bytes     = ring_size_in_bytes;
> > +	*_event_size_in_bytes    = event_size_in_bytes;
>
>
> I wonder if we can simply factor out split and packed from struct
> vring_virtqueue:
>
> struct vring_virtqueue {
>      union {
>          struct {} split;
>          struct {} packed;
>      };
> };
>
> to
>
> struct vring_virtqueue_split {};
> struct vring_virtqueue_packed {};
>
> Then we can do things like:
>
> vring_create_virtqueue_packed(struct virtio_device *vdev, u32 num,
> struct vring_virtqueue_packed *packed);
>
> and
>
> vring_vritqueue_attach_packed(struct vring_virtqueue *vq, struct
> vring_virtqueue_packed packed);

This idea is very similar to my previous idea, just without introducing a new
structure.

I'd be more than happy to revise this.

Thanks.


>
> Thanks
>
>
> > +
> > +	return 0;
> > +
> > +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:
> > +	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)
> > +{
> > +	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;
> > +	struct vring_virtqueue *vq;
> > +
> > +	if (vring_alloc_queue_packed(vdev, num, &ring, &driver, &device,
> > +				     &ring_dma_addr, &driver_event_dma_addr,
> > +				     &device_event_dma_addr,
> > +				     &ring_size_in_bytes,
> > +				     &event_size_in_bytes))
> > +		goto err_ring;
> > +
> >   	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
> >   	if (!vq)
> >   		goto err_vq;
> > @@ -1939,9 +1983,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
> >   	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:
> >   	return NULL;
>
Jason Wang April 14, 2022, 9:18 a.m. UTC | #3
On Wed, Apr 13, 2022 at 11:26 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 12 Apr 2022 14:28:24 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >
> > 在 2022/4/6 上午11:43, 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@linux.alibaba.com>
> > > ---
> > >   drivers/virtio/virtio_ring.c | 70 ++++++++++++++++++++++++++++--------
> > >   1 file changed, 56 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 33864134a744..ea451ae2aaef 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -1817,19 +1817,17 @@ 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 int vring_alloc_queue_packed(struct virtio_device *vdev,
> > > +                               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 *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;
> > > @@ -1857,6 +1855,52 @@ static struct virtqueue *vring_create_virtqueue_packed(
> > >     if (!device)
> > >             goto err_device;
> > >
> > > +   *_ring                   = ring;
> > > +   *_driver                 = driver;
> > > +   *_device                 = device;
> > > +   *_ring_dma_addr          = ring_dma_addr;
> > > +   *_driver_event_dma_addr  = driver_event_dma_addr;
> > > +   *_device_event_dma_addr  = device_event_dma_addr;
> > > +   *_ring_size_in_bytes     = ring_size_in_bytes;
> > > +   *_event_size_in_bytes    = event_size_in_bytes;
> >
> >
> > I wonder if we can simply factor out split and packed from struct
> > vring_virtqueue:
> >
> > struct vring_virtqueue {
> >      union {
> >          struct {} split;
> >          struct {} packed;
> >      };
> > };
> >
> > to
> >
> > struct vring_virtqueue_split {};
> > struct vring_virtqueue_packed {};
> >
> > Then we can do things like:
> >
> > vring_create_virtqueue_packed(struct virtio_device *vdev, u32 num,
> > struct vring_virtqueue_packed *packed);
> >
> > and
> >
> > vring_vritqueue_attach_packed(struct vring_virtqueue *vq, struct
> > vring_virtqueue_packed packed);
>
> This idea is very similar to my previous idea, just without introducing a new
> structure.

Yes, it's better to not introduce new structures if it's possible.

>
> I'd be more than happy to revise this.

Good to know this.

Thanks

>
> Thanks.
>
>
> >
> > Thanks
> >
> >
> > > +
> > > +   return 0;
> > > +
> > > +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:
> > > +   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)
> > > +{
> > > +   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;
> > > +   struct vring_virtqueue *vq;
> > > +
> > > +   if (vring_alloc_queue_packed(vdev, num, &ring, &driver, &device,
> > > +                                &ring_dma_addr, &driver_event_dma_addr,
> > > +                                &device_event_dma_addr,
> > > +                                &ring_size_in_bytes,
> > > +                                &event_size_in_bytes))
> > > +           goto err_ring;
> > > +
> > >     vq = kmalloc(sizeof(*vq), GFP_KERNEL);
> > >     if (!vq)
> > >             goto err_vq;
> > > @@ -1939,9 +1983,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
> > >     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:
> > >     return NULL;
> >
>
diff mbox series

Patch

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 33864134a744..ea451ae2aaef 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1817,19 +1817,17 @@  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 int vring_alloc_queue_packed(struct virtio_device *vdev,
+				    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 *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;
@@ -1857,6 +1855,52 @@  static struct virtqueue *vring_create_virtqueue_packed(
 	if (!device)
 		goto err_device;
 
+	*_ring                   = ring;
+	*_driver                 = driver;
+	*_device                 = device;
+	*_ring_dma_addr          = ring_dma_addr;
+	*_driver_event_dma_addr  = driver_event_dma_addr;
+	*_device_event_dma_addr  = device_event_dma_addr;
+	*_ring_size_in_bytes     = ring_size_in_bytes;
+	*_event_size_in_bytes    = event_size_in_bytes;
+
+	return 0;
+
+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:
+	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)
+{
+	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;
+	struct vring_virtqueue *vq;
+
+	if (vring_alloc_queue_packed(vdev, num, &ring, &driver, &device,
+				     &ring_dma_addr, &driver_event_dma_addr,
+				     &device_event_dma_addr,
+				     &ring_size_in_bytes,
+				     &event_size_in_bytes))
+		goto err_ring;
+
 	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
 	if (!vq)
 		goto err_vq;
@@ -1939,9 +1983,7 @@  static struct virtqueue *vring_create_virtqueue_packed(
 	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:
 	return NULL;