diff mbox series

[v7,08/26] virtio_ring: extract the logic of freeing vring

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

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count fail Series longer than 15 patches (and no cover letter)
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 21 this patch: 22
netdev/cc_maintainers warning 5 maintainers not CCed: andrii@kernel.org kpsingh@kernel.org kafai@fb.com songliubraving@fb.com yhs@fb.com
netdev/build_clang fail Errors and warnings before: 23 this patch: 25
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 27 this patch: 28
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 60 lines checked
netdev/kdoc success Errors and warnings before: 3 this patch: 3
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Xuan Zhuo March 8, 2022, 12:35 p.m. UTC 
Introduce vring_free() to free the vring of vq.

Prevent double free by setting vq->reset.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 25 ++++++++++++++++++++-----
 include/linux/virtio.h       |  8 ++++++++
 2 files changed, 28 insertions(+), 5 deletions(-)

Comments

Jason Wang March 9, 2022, 7:51 a.m. UTC | #1 
在 2022/3/8 下午8:35, Xuan Zhuo 写道:
> Introduce vring_free() to free the vring of vq.
>
> Prevent double free by setting vq->reset.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   drivers/virtio/virtio_ring.c | 25 ++++++++++++++++++++-----
>   include/linux/virtio.h       |  8 ++++++++
>   2 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index b5a9bf4f45b3..e0422c04c903 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2442,14 +2442,10 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
>   }
>   EXPORT_SYMBOL_GPL(vring_new_virtqueue);
>   
> -void vring_del_virtqueue(struct virtqueue *_vq)
> +static void __vring_free(struct virtqueue *_vq)
>   {
>   	struct vring_virtqueue *vq = to_vvq(_vq);
>   
> -	spin_lock(&vq->vq.vdev->vqs_list_lock);
> -	list_del(&_vq->list);
> -	spin_unlock(&vq->vq.vdev->vqs_list_lock);
> -
>   	if (vq->we_own_ring) {
>   		if (vq->packed_ring) {
>   			vring_free_queue(vq->vq.vdev,
> @@ -2480,6 +2476,25 @@ void vring_del_virtqueue(struct virtqueue *_vq)
>   		kfree(vq->split.desc_state);
>   		kfree(vq->split.desc_extra);
>   	}
> +}
> +
> +static void vring_free(struct virtqueue *vq)
> +{
> +	__vring_free(vq);
> +	vq->reset = VIRTIO_VQ_RESET_STEP_VRING_RELEASE;
> +}
> +
> +void vring_del_virtqueue(struct virtqueue *_vq)
> +{
> +	struct vring_virtqueue *vq = to_vvq(_vq);
> +
> +	spin_lock(&vq->vq.vdev->vqs_list_lock);
> +	list_del(&_vq->list);
> +	spin_unlock(&vq->vq.vdev->vqs_list_lock);
> +
> +	if (_vq->reset != VIRTIO_VQ_RESET_STEP_VRING_RELEASE)
> +		__vring_free(_vq);
> +
>   	kfree(vq);
>   }
>   EXPORT_SYMBOL_GPL(vring_del_virtqueue);
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index d59adc4be068..e3714e6db330 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -10,6 +10,13 @@
>   #include <linux/mod_devicetable.h>
>   #include <linux/gfp.h>
>   
> +enum virtio_vq_reset_step {
> +	VIRTIO_VQ_RESET_STEP_NONE,
> +	VIRTIO_VQ_RESET_STEP_DEVICE,
> +	VIRTIO_VQ_RESET_STEP_VRING_RELEASE,
> +	VIRTIO_VQ_RESET_STEP_VRING_ATTACH,
> +};


This part looks not related to the subject.

And it needs detail documentation on this.

But I wonder how useful it is, anyway we can check the reset status via 
transport specific way and in the future we may want to do more than 
just resizing (e.g PASID).

Thanks


> +
>   /**
>    * virtqueue - a queue to register buffers for sending or receiving.
>    * @list: the chain of virtqueues for this device
> @@ -33,6 +40,7 @@ struct virtqueue {
>   	unsigned int num_free;
>   	unsigned int num_max;
>   	void *priv;
> +	enum virtio_vq_reset_step reset;
>   };
>   
>   int virtqueue_add_outbuf(struct virtqueue *vq,
Xuan Zhuo March 9, 2022, 9:22 a.m. UTC | #2 
On Wed, 9 Mar 2022 15:51:13 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/3/8 下午8:35, Xuan Zhuo 写道:
> > Introduce vring_free() to free the vring of vq.
> >
> > Prevent double free by setting vq->reset.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >   drivers/virtio/virtio_ring.c | 25 ++++++++++++++++++++-----
> >   include/linux/virtio.h       |  8 ++++++++
> >   2 files changed, 28 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index b5a9bf4f45b3..e0422c04c903 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -2442,14 +2442,10 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
> >   }
> >   EXPORT_SYMBOL_GPL(vring_new_virtqueue);
> >
> > -void vring_del_virtqueue(struct virtqueue *_vq)
> > +static void __vring_free(struct virtqueue *_vq)
> >   {
> >   	struct vring_virtqueue *vq = to_vvq(_vq);
> >
> > -	spin_lock(&vq->vq.vdev->vqs_list_lock);
> > -	list_del(&_vq->list);
> > -	spin_unlock(&vq->vq.vdev->vqs_list_lock);
> > -
> >   	if (vq->we_own_ring) {
> >   		if (vq->packed_ring) {
> >   			vring_free_queue(vq->vq.vdev,
> > @@ -2480,6 +2476,25 @@ void vring_del_virtqueue(struct virtqueue *_vq)
> >   		kfree(vq->split.desc_state);
> >   		kfree(vq->split.desc_extra);
> >   	}
> > +}
> > +
> > +static void vring_free(struct virtqueue *vq)
> > +{
> > +	__vring_free(vq);
> > +	vq->reset = VIRTIO_VQ_RESET_STEP_VRING_RELEASE;
> > +}
> > +
> > +void vring_del_virtqueue(struct virtqueue *_vq)
> > +{
> > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > +
> > +	spin_lock(&vq->vq.vdev->vqs_list_lock);
> > +	list_del(&_vq->list);
> > +	spin_unlock(&vq->vq.vdev->vqs_list_lock);
> > +
> > +	if (_vq->reset != VIRTIO_VQ_RESET_STEP_VRING_RELEASE)
> > +		__vring_free(_vq);
> > +
> >   	kfree(vq);
> >   }
> >   EXPORT_SYMBOL_GPL(vring_del_virtqueue);
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index d59adc4be068..e3714e6db330 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -10,6 +10,13 @@
> >   #include <linux/mod_devicetable.h>
> >   #include <linux/gfp.h>
> >
> > +enum virtio_vq_reset_step {
> > +	VIRTIO_VQ_RESET_STEP_NONE,
> > +	VIRTIO_VQ_RESET_STEP_DEVICE,
> > +	VIRTIO_VQ_RESET_STEP_VRING_RELEASE,
> > +	VIRTIO_VQ_RESET_STEP_VRING_ATTACH,
> > +};
>
>
> This part looks not related to the subject.
>
> And it needs detail documentation on this.
>
> But I wonder how useful it is, anyway we can check the reset status via
> transport specific way and in the future we may want to do more than
> just resizing (e.g PASID).


I will try and remove it from here.

Thanks.

>
> Thanks
>
>
> > +
> >   /**
> >    * virtqueue - a queue to register buffers for sending or receiving.
> >    * @list: the chain of virtqueues for this device
> > @@ -33,6 +40,7 @@ struct virtqueue {
> >   	unsigned int num_free;
> >   	unsigned int num_max;
> >   	void *priv;
> > +	enum virtio_vq_reset_step reset;
> >   };
> >
> >   int virtqueue_add_outbuf(struct virtqueue *vq,
>
diff mbox series

Patch

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b5a9bf4f45b3..e0422c04c903 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2442,14 +2442,10 @@  struct virtqueue *vring_new_virtqueue(unsigned int index,
 }
 EXPORT_SYMBOL_GPL(vring_new_virtqueue);
 
-void vring_del_virtqueue(struct virtqueue *_vq)
+static void __vring_free(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
-	spin_lock(&vq->vq.vdev->vqs_list_lock);
-	list_del(&_vq->list);
-	spin_unlock(&vq->vq.vdev->vqs_list_lock);
-
 	if (vq->we_own_ring) {
 		if (vq->packed_ring) {
 			vring_free_queue(vq->vq.vdev,
@@ -2480,6 +2476,25 @@  void vring_del_virtqueue(struct virtqueue *_vq)
 		kfree(vq->split.desc_state);
 		kfree(vq->split.desc_extra);
 	}
+}
+
+static void vring_free(struct virtqueue *vq)
+{
+	__vring_free(vq);
+	vq->reset = VIRTIO_VQ_RESET_STEP_VRING_RELEASE;
+}
+
+void vring_del_virtqueue(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+
+	spin_lock(&vq->vq.vdev->vqs_list_lock);
+	list_del(&_vq->list);
+	spin_unlock(&vq->vq.vdev->vqs_list_lock);
+
+	if (_vq->reset != VIRTIO_VQ_RESET_STEP_VRING_RELEASE)
+		__vring_free(_vq);
+
 	kfree(vq);
 }
 EXPORT_SYMBOL_GPL(vring_del_virtqueue);
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index d59adc4be068..e3714e6db330 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -10,6 +10,13 @@ 
 #include <linux/mod_devicetable.h>
 #include <linux/gfp.h>
 
+enum virtio_vq_reset_step {
+	VIRTIO_VQ_RESET_STEP_NONE,
+	VIRTIO_VQ_RESET_STEP_DEVICE,
+	VIRTIO_VQ_RESET_STEP_VRING_RELEASE,
+	VIRTIO_VQ_RESET_STEP_VRING_ATTACH,
+};
+
 /**
  * virtqueue - a queue to register buffers for sending or receiving.
  * @list: the chain of virtqueues for this device
@@ -33,6 +40,7 @@  struct virtqueue {
 	unsigned int num_free;
 	unsigned int num_max;
 	void *priv;
+	enum virtio_vq_reset_step reset;
 };
 
 int virtqueue_add_outbuf(struct virtqueue *vq,