diff mbox

[dpdk-dev,v3,2/2] virtio: fix memory leak of virtqueue memzones

Message ID 1461890926-16727-3-git-send-email-jianfeng.tan@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Yuanhan Liu
Headers

Commit Message

Jianfeng Tan April 29, 2016, 12:48 a.m. UTC 
  Issue: When virtio was proposed in DPDK, there is no API to free memzones.
But this has changed since rte_memzone_free() has been implemented by
commit ff909fe21f0a ("mem: introduce memzone freeing").

This patch is to make sure memzones in struct virtqueue, like mz and
virtio_net_hdr_mz, are freed when queue is released or setup fails.

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 21 ++++++++++++++-------
 drivers/net/virtio/virtqueue.h     |  2 ++
 2 files changed, 16 insertions(+), 7 deletions(-)

Comments

Yuanhan Liu April 29, 2016, 5:33 a.m. UTC | #1 
On Fri, Apr 29, 2016 at 12:48:46AM +0000, Jianfeng Tan wrote:
> @@ -447,6 +453,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
>  
>  	hw->vtpci_ops->setup_queue(hw, vq);
>  
> +	vq->started = 1;

Judging that this is in the "_queue_setup" stage, and we have another
stage called "_dev_start", naming it to "started" seems confusing then.

So, how about naming it to something like "configured"? Besides that,
this patch set looks good to me. If you agree the name change, or have
better idea, I could fix it while applying it.

	--yliu
Yuanhan Liu May 5, 2016, 3:27 a.m. UTC | #2 
ping...

On Thu, Apr 28, 2016 at 10:33:08PM -0700, Yuanhan Liu wrote:
> On Fri, Apr 29, 2016 at 12:48:46AM +0000, Jianfeng Tan wrote:
> > @@ -447,6 +453,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
> >  
> >  	hw->vtpci_ops->setup_queue(hw, vq);
> >  
> > +	vq->started = 1;
> 
> Judging that this is in the "_queue_setup" stage, and we have another
> stage called "_dev_start", naming it to "started" seems confusing then.
> 
> So, how about naming it to something like "configured"? Besides that,
> this patch set looks good to me. If you agree the name change, or have
> better idea, I could fix it while applying it.
> 
> 	--yliu
Jianfeng Tan May 5, 2016, 4:51 a.m. UTC | #3 
Hi Yuanhan,

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Thursday, May 5, 2016 11:28 AM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; Xie, Huawei
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] virtio: fix memory leak of virtqueue
> memzones
> 
> ping...
> 
> On Thu, Apr 28, 2016 at 10:33:08PM -0700, Yuanhan Liu wrote:
> > On Fri, Apr 29, 2016 at 12:48:46AM +0000, Jianfeng Tan wrote:
> > > @@ -447,6 +453,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev
> *dev,
> > >
> > >  	hw->vtpci_ops->setup_queue(hw, vq);
> > >
> > > +	vq->started = 1;
> >
> > Judging that this is in the "_queue_setup" stage, and we have another
> > stage called "_dev_start", naming it to "started" seems confusing then.
> >
> > So, how about naming it to something like "configured"? Besides that,
> > this patch set looks good to me. If you agree the name change, or have
> > better idea, I could fix it while applying it.

Yes, I agree _configured_ would be better.

Thanks,
Jianfeng 

> >
> > 	--yliu
diff mbox

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index b3f4158..bd990ff 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -260,12 +260,18 @@  virtio_set_multiple_queues(struct rte_eth_dev *dev, uint16_t nb_queues)
 }
 
 void
-virtio_dev_queue_release(struct virtqueue *vq) {
+virtio_dev_queue_release(struct virtqueue *vq)
+{
 	struct virtio_hw *hw;
 
 	if (vq) {
 		hw = vq->hw;
-		hw->vtpci_ops->del_queue(hw, vq);
+		if (vq->started)
+			hw->vtpci_ops->del_queue(hw, vq);
+
+		rte_memzone_free(vq->mz);
+		if (vq->virtio_net_hdr_mz)
+			rte_memzone_free(vq->virtio_net_hdr_mz);
 
 		rte_free(vq->sw_ring);
 		rte_free(vq);
@@ -330,7 +336,7 @@  int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 						 socket_id);
 		if (!vq->sw_ring) {
 			PMD_INIT_LOG(ERR, "Can not allocate RX soft ring");
-			rte_free(vq);
+			virtio_dev_queue_release(vq);
 			return -ENOMEM;
 		}
 	}
@@ -358,7 +364,7 @@  int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 		if (rte_errno == EEXIST)
 			mz = rte_memzone_lookup(vq_name);
 		if (mz == NULL) {
-			rte_free(vq);
+			virtio_dev_queue_release(vq);
 			return -ENOMEM;
 		}
 	}
@@ -370,7 +376,7 @@  int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 	 */
 	if ((mz->phys_addr + vq->vq_ring_size - 1) >> (VIRTIO_PCI_QUEUE_ADDR_SHIFT + 32)) {
 		PMD_INIT_LOG(ERR, "vring address shouldn't be above 16TB!");
-		rte_free(vq);
+		virtio_dev_queue_release(vq);
 		return -ENOMEM;
 	}
 
@@ -402,7 +408,7 @@  int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 			if (rte_errno == EEXIST)
 				hdr_mz = rte_memzone_lookup(vq_name);
 			if (hdr_mz == NULL) {
-				rte_free(vq);
+				virtio_dev_queue_release(vq);
 				return -ENOMEM;
 			}
 		}
@@ -436,7 +442,7 @@  int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 				vq->virtio_net_hdr_mz =
 					rte_memzone_lookup(vq_name);
 			if (vq->virtio_net_hdr_mz == NULL) {
-				rte_free(vq);
+				virtio_dev_queue_release(vq);
 				return -ENOMEM;
 			}
 		}
@@ -447,6 +453,7 @@  int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 
 	hw->vtpci_ops->setup_queue(hw, vq);
 
+	vq->started = 1;
 	*pvq = vq;
 	return 0;
 }
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 4e9239e..83d89ca 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -201,6 +201,8 @@  struct virtqueue {
 
 	uint16_t	*notify_addr;
 
+	int		started;
+
 	struct vq_desc_extra {
 		void              *cookie;
 		uint16_t          ndescs;