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

2016-05-05 Thread Tan, Jianfeng
Hi Yuanhan,

> -Original Message-
> From: Yuanhan Liu [mailto:yuanhan.liu at linux.intel.com]
> Sent: Thursday, May 5, 2016 11:28 AM
> To: Tan, Jianfeng
> Cc: dev at 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 +, 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


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

2016-05-04 Thread Yuanhan Liu
ping...

On Thu, Apr 28, 2016 at 10:33:08PM -0700, Yuanhan Liu wrote:
> On Fri, Apr 29, 2016 at 12:48:46AM +, 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


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

2016-04-29 Thread Jianfeng Tan
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 
---
 drivers/net/virtio/virtio_ethdev.c | 21 ++---
 drivers/net/virtio/virtqueue.h |  2 ++
 2 files changed, 16 insertions(+), 7 deletions(-)

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;
-- 
2.1.4



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

2016-04-28 Thread Yuanhan Liu
On Fri, Apr 29, 2016 at 12:48:46AM +, 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