[PATCH v3 0/2] virtio_ring: check desc == NULL when packed and indirect

[PATCH v3 0/2] virtio_ring: check desc == NULL when packed and indirect

[Xuan Zhuo]

In the case of packed, use indirect desc, since desc is allocated by
kmalloc_array(), we should check whether its return value is NULL.

This patch alloc desc inside virtqueue_add_packe(), if desc == NULL,
fall back to not using indirect.

v3:
    Update commit message of the #1 patch.

v2:
    Separate the style fix into a single patch.

Xuan Zhuo (2):
  virtio_ring: fix style of virtqueue_add_indirect_packed
  virtio_ring: check desc == NULL when packed and indirect

 drivers/virtio/virtio_ring.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

--
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 1/2] virtio_ring: fix style of virtqueue_add_indirect_packed

[Xuan Zhuo]

Fix the style problem of virtqueue_add_indirect_packed().
Align arguments to the open ( to make it look prettier.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index dd95dfd85e98..91a46c4da87d 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1050,12 +1050,12 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg,
 }
 
 static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
-				       struct scatterlist *sgs[],
-				       unsigned int total_sg,
-				       unsigned int out_sgs,
-				       unsigned int in_sgs,
-				       void *data,
-				       gfp_t gfp)
+					 struct scatterlist *sgs[],
+					 unsigned int total_sg,
+					 unsigned int out_sgs,
+					 unsigned int in_sgs,
+					 void *data,
+					 gfp_t gfp)
 {
 	struct vring_packed_desc *desc;
 	struct scatterlist *sg;
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 1/2] virtio_ring: fix style of virtqueue_add_indirect_packed

[Michael S. Tsirkin]

On Tue, Oct 19, 2021 at 06:56:56PM +0800, Xuan Zhuo wrote:
> Fix the style problem of virtqueue_add_indirect_packed().
> Align arguments to the open ( to make it look prettier.
> 
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Acked-by: Jason Wang <jasowang@redhat.com>

fix the style problem -> make virtqueue_add_indirect_packed prettier.

same for subject

there's no style problem, certainly not the problem.

> ---
>  drivers/virtio/virtio_ring.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index dd95dfd85e98..91a46c4da87d 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1050,12 +1050,12 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg,
>  }
>  
>  static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> -				       struct scatterlist *sgs[],
> -				       unsigned int total_sg,
> -				       unsigned int out_sgs,
> -				       unsigned int in_sgs,
> -				       void *data,
> -				       gfp_t gfp)
> +					 struct scatterlist *sgs[],
> +					 unsigned int total_sg,
> +					 unsigned int out_sgs,
> +					 unsigned int in_sgs,
> +					 void *data,
> +					 gfp_t gfp)
>  {
>  	struct vring_packed_desc *desc;
>  	struct scatterlist *sg;
> -- 
> 2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 2/2] virtio_ring: check desc == NULL when packed and indirect

[Xuan Zhuo]

In the case of packed, use indirect desc, since desc is allocated by
kmalloc_array(), we should check whether its return value is NULL.

This patch alloc desc inside virtqueue_add_packe(), if desc == NULL,
fall back to not using indirect.

Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 91a46c4da87d..62323c27bfe4 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1051,20 +1051,19 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg,
 
 static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 					 struct scatterlist *sgs[],
+					 struct vring_packed_desc *desc,
 					 unsigned int total_sg,
 					 unsigned int out_sgs,
 					 unsigned int in_sgs,
 					 void *data,
 					 gfp_t gfp)
 {
-	struct vring_packed_desc *desc;
 	struct scatterlist *sg;
 	unsigned int i, n, err_idx;
 	u16 head, id;
 	dma_addr_t addr;
 
 	head = vq->packed.next_avail_idx;
-	desc = alloc_indirect_packed(total_sg, gfp);
 
 	if (unlikely(vq->vq.num_free < 1)) {
 		pr_debug("Can't add buf len 1 - avail = 0\n");
@@ -1191,9 +1190,14 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 
 	BUG_ON(total_sg == 0);
 
-	if (virtqueue_use_indirect(_vq, total_sg))
-		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
-				out_sgs, in_sgs, data, gfp);
+	if (virtqueue_use_indirect(_vq, total_sg)) {
+		desc = alloc_indirect_packed(total_sg, gfp);
+		if (desc)
+			return virtqueue_add_indirect_packed(vq, sgs, desc,
+							     total_sg,
+							     out_sgs, in_sgs,
+							     data, gfp);
+	}
 
 	head = vq->packed.next_avail_idx;
 	avail_used_flags = vq->packed.avail_used_flags;
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 2/2] virtio_ring: check desc == NULL when packed and indirect

[Michael S. Tsirkin]

On Tue, Oct 19, 2021 at 06:56:57PM +0800, Xuan Zhuo wrote:
> In the case of packed, use indirect desc, since desc is allocated by
> kmalloc_array(), we should check whether its return value is NULL.
> 
> This patch alloc desc inside virtqueue_add_packe(), if desc == NULL,

Can we manage without typos in commit log please?

> fall back to not using indirect.


It should say why is the patch created, and how it's fixed,
as opposed to repating what patch does.

E.g.

when using indirect with packed, we don't check for
allocation failures. Check and fall back on direct.



> Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/virtio/virtio_ring.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 91a46c4da87d..62323c27bfe4 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1051,20 +1051,19 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg,
>  
>  static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>  					 struct scatterlist *sgs[],
> +					 struct vring_packed_desc *desc,
>  					 unsigned int total_sg,
>  					 unsigned int out_sgs,
>  					 unsigned int in_sgs,
>  					 void *data,
>  					 gfp_t gfp)

So this gets desc and will free it. I don't much like this.


>  {
> -	struct vring_packed_desc *desc;
>  	struct scatterlist *sg;
>  	unsigned int i, n, err_idx;
>  	u16 head, id;
>  	dma_addr_t addr;
>  
>  	head = vq->packed.next_avail_idx;
> -	desc = alloc_indirect_packed(total_sg, gfp);
>  
>  	if (unlikely(vq->vq.num_free < 1)) {
>  		pr_debug("Can't add buf len 1 - avail = 0\n");
> @@ -1191,9 +1190,14 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>  
>  	BUG_ON(total_sg == 0);
>  
> -	if (virtqueue_use_indirect(_vq, total_sg))
> -		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
> -				out_sgs, in_sgs, data, gfp);
> +	if (virtqueue_use_indirect(_vq, total_sg)) {
> +		desc = alloc_indirect_packed(total_sg, gfp);
> +		if (desc)
> +			return virtqueue_add_indirect_packed(vq, sgs, desc,
> +							     total_sg,
> +							     out_sgs, in_sgs,
> +							     data, gfp);
> +	}
>  
>  	head = vq->packed.next_avail_idx;
>  	avail_used_flags = vq->packed.avail_used_flags;
> -- 
> 2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 2/2] virtio_ring: check desc == NULL when packed and indirect

[Xuan Zhuo]

On Tue, 19 Oct 2021 07:07:58 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Oct 19, 2021 at 06:56:57PM +0800, Xuan Zhuo wrote:
> > In the case of packed, use indirect desc, since desc is allocated by
> > kmalloc_array(), we should check whether its return value is NULL.
> >
> > This patch alloc desc inside virtqueue_add_packe(), if desc == NULL,
>
> Can we manage without typos in commit log please?

I'm sorry. virtqueue_add_packe => virtqueue_add_packed

>
> > fall back to not using indirect.
>
>
> It should say why is the patch created, and how it's fixed,
> as opposed to repating what patch does.
>
> E.g.
>
> when using indirect with packed, we don't check for
> allocation failures. Check and fall back on direct.


It's better.  I know how to do it.


>
>
>
> > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/virtio/virtio_ring.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 91a46c4da87d..62323c27bfe4 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -1051,20 +1051,19 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg,
> >
> >  static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> >  					 struct scatterlist *sgs[],
> > +					 struct vring_packed_desc *desc,
> >  					 unsigned int total_sg,
> >  					 unsigned int out_sgs,
> >  					 unsigned int in_sgs,
> >  					 void *data,
> >  					 gfp_t gfp)
>
> So this gets desc and will free it. I don't much like this.

Under normal circumstances, this function will not release desc. It will
actually release "desc" in detach_buf_packed(). Of course, if you encounter
an error in this function, it will also release desc in this function.

If we call alloc_indirect_packed() in virtqueue_add_indirect_packed() it is also
possible, we need to make virtqueue_add_indirect_packed() return a special
return value.

Thanks.

>
>
> >  {
> > -	struct vring_packed_desc *desc;
> >  	struct scatterlist *sg;
> >  	unsigned int i, n, err_idx;
> >  	u16 head, id;
> >  	dma_addr_t addr;
> >
> >  	head = vq->packed.next_avail_idx;
> > -	desc = alloc_indirect_packed(total_sg, gfp);
> >
> >  	if (unlikely(vq->vq.num_free < 1)) {
> >  		pr_debug("Can't add buf len 1 - avail = 0\n");
> > @@ -1191,9 +1190,14 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> >
> >  	BUG_ON(total_sg == 0);
> >
> > -	if (virtqueue_use_indirect(_vq, total_sg))
> > -		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
> > -				out_sgs, in_sgs, data, gfp);
> > +	if (virtqueue_use_indirect(_vq, total_sg)) {
> > +		desc = alloc_indirect_packed(total_sg, gfp);
> > +		if (desc)
> > +			return virtqueue_add_indirect_packed(vq, sgs, desc,
> > +							     total_sg,
> > +							     out_sgs, in_sgs,
> > +							     data, gfp);
> > +	}
> >
> >  	head = vq->packed.next_avail_idx;
> >  	avail_used_flags = vq->packed.avail_used_flags;
> > --
> > 2.31.0
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 2/2] virtio_ring: check desc == NULL when packed and indirect

[Michael S. Tsirkin]

On Tue, Oct 19, 2021 at 07:12:26PM +0800, Xuan Zhuo wrote:
> On Tue, 19 Oct 2021 07:07:58 -0400, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Oct 19, 2021 at 06:56:57PM +0800, Xuan Zhuo wrote:
> > > In the case of packed, use indirect desc, since desc is allocated by
> > > kmalloc_array(), we should check whether its return value is NULL.
> > >
> > > This patch alloc desc inside virtqueue_add_packe(), if desc == NULL,
> >
> > Can we manage without typos in commit log please?
> 
> I'm sorry. virtqueue_add_packe => virtqueue_add_packed
> 
> >
> > > fall back to not using indirect.
> >
> >
> > It should say why is the patch created, and how it's fixed,
> > as opposed to repating what patch does.
> >
> > E.g.
> >
> > when using indirect with packed, we don't check for
> > allocation failures. Check and fall back on direct.
> 
> 
> It's better.  I know how to do it.
> 
> 
> >
> >
> >
> > > Fixes: 1ce9e6055fa ("virtio_ring: introduce packed ring support")
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  drivers/virtio/virtio_ring.c | 14 +++++++++-----
> > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 91a46c4da87d..62323c27bfe4 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -1051,20 +1051,19 @@ static struct vring_packed_desc *alloc_indirect_packed(unsigned int total_sg,
> > >
> > >  static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> > >  					 struct scatterlist *sgs[],
> > > +					 struct vring_packed_desc *desc,
> > >  					 unsigned int total_sg,
> > >  					 unsigned int out_sgs,
> > >  					 unsigned int in_sgs,
> > >  					 void *data,
> > >  					 gfp_t gfp)
> >
> > So this gets desc and will free it. I don't much like this.
> 
> Under normal circumstances, this function will not release desc. It will
> actually release "desc" in detach_buf_packed(). Of course, if you encounter
> an error in this function, it will also release desc in this function.
> 
> If we call alloc_indirect_packed() in virtqueue_add_indirect_packed() it is also
> possible, we need to make virtqueue_add_indirect_packed() return a special
> return value.
> 
> Thanks.


that seems cleaner

> >
> >
> > >  {
> > > -	struct vring_packed_desc *desc;
> > >  	struct scatterlist *sg;
> > >  	unsigned int i, n, err_idx;
> > >  	u16 head, id;
> > >  	dma_addr_t addr;
> > >
> > >  	head = vq->packed.next_avail_idx;
> > > -	desc = alloc_indirect_packed(total_sg, gfp);
> > >
> > >  	if (unlikely(vq->vq.num_free < 1)) {
> > >  		pr_debug("Can't add buf len 1 - avail = 0\n");
> > > @@ -1191,9 +1190,14 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > >
> > >  	BUG_ON(total_sg == 0);
> > >
> > > -	if (virtqueue_use_indirect(_vq, total_sg))
> > > -		return virtqueue_add_indirect_packed(vq, sgs, total_sg,
> > > -				out_sgs, in_sgs, data, gfp);
> > > +	if (virtqueue_use_indirect(_vq, total_sg)) {
> > > +		desc = alloc_indirect_packed(total_sg, gfp);
> > > +		if (desc)
> > > +			return virtqueue_add_indirect_packed(vq, sgs, desc,
> > > +							     total_sg,
> > > +							     out_sgs, in_sgs,
> > > +							     data, gfp);
> > > +	}
> > >
> > >  	head = vq->packed.next_avail_idx;
> > >  	avail_used_flags = vq->packed.avail_used_flags;
> > > --
> > > 2.31.0
> >

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization