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

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

[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 ff909fe21f.

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 | 69 ++++++++++++++++++++------------------
 drivers/net/virtio/virtio_ethdev.h |  2 +-
 drivers/net/virtio/virtio_rxtx.c   |  4 +--
 3 files changed, 40 insertions(+), 35 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 63a368a..54eacf6 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -261,12 +261,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, int io_related)
+{
 	struct virtio_hw *hw;
 
 	if (vq) {
 		hw = vq->hw;
-		hw->vtpci_ops->del_queue(hw, vq);
+		if (io_related)
+			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);
@@ -286,6 +292,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 	unsigned int vq_size, size;
 	struct virtio_hw *hw = dev->data->dev_private;
 	struct virtqueue *vq = NULL;
+	const char *queue_names[] = {"rvq", "txq", "cvq"};
 
 	PMD_INIT_LOG(DEBUG, "setting up queue: %u", vtpci_queue_idx);
 
@@ -305,34 +312,34 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 		return -EINVAL;
 	}
 
-	if (queue_type == VTNET_RQ) {
-		snprintf(vq_name, sizeof(vq_name), "port%d_rvq%d",
-			dev->data->port_id, queue_idx);
-		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
-			vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE);
-		vq->sw_ring = rte_zmalloc_socket("rxq->sw_ring",
-			(RTE_PMD_VIRTIO_RX_MAX_BURST + vq_size) *
-			sizeof(vq->sw_ring[0]), RTE_CACHE_LINE_SIZE, socket_id);
-	} else if (queue_type == VTNET_TQ) {
-		snprintf(vq_name, sizeof(vq_name), "port%d_tvq%d",
-			dev->data->port_id, queue_idx);
-		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
-			vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE);
-	} else if (queue_type == VTNET_CQ) {
-		snprintf(vq_name, sizeof(vq_name), "port%d_cvq",
-			dev->data->port_id);
-		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
-			vq_size * sizeof(struct vq_desc_extra),
-			RTE_CACHE_LINE_SIZE);
+	if (queue_type < VTNET_RQ || queue_type > VTNET_RQ) {
+		PMD_INIT_LOG(ERR, "invalid queue type: %d", queue_type);
+		return -EINVAL;
 	}
+
+	snprintf(vq_name, sizeof(vq_name), "port%d_%s%d",
+		 dev->data->port_id, queue_names[queue_type], queue_idx);
+	vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
+			 vq_size * sizeof(struct vq_desc_extra),
+			 RTE_CACHE_LINE_SIZE);
 	if (vq == NULL) {
 		PMD_INIT_LOG(ERR, "Can not allocate virtqueue");
 		return -ENOMEM;
 	}
-	if (queue_type == VTNET_RQ && vq->sw_ring == NULL) {
-		PMD_INIT_LOG(ERR, "Can not allocate RX soft ring");
-		rte_free(vq);
-		return -ENOMEM;
+
+	if (queue_type == VTNET_RQ) {
+		size_t sz_sw;
+
+		sz_sw = (RTE_PMD_VIRTIO_RX_MAX_BURST + vq_size) *
+			sizeof(vq->sw_ring[0]);
+		vq->sw_ring = rte_zmalloc_socket("rxq->sw_ring", sz_sw,
+						 RTE_CACHE_LINE_SIZE,
+						 socket_id);
+		if (!vq->sw_ring) {
+			PMD_INIT_LOG(ERR, "Can not allocate RX soft ring");
+			virtio_dev_queue_release(vq, 0);
+			return -ENOMEM;
+		}
 	}
 
 	vq->hw = hw;
@@ -358,7 +365,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, 0);
 			return -ENOMEM;
 		}
 	}
@@ -370,7 +377,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, 0);
 		return -ENOMEM;
 	}
 
@@ -380,8 +387,6 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 	vq->vq_ring_virt_mem = mz->addr;
 	PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:      0x%"PRIx64, (uint64_t)mz->phys_addr);
 	PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64, (uint64_t)(uintptr_t)mz->addr);
-	vq->virtio_net_hdr_mz  = NULL;
-	vq->virtio_net_hdr_mem = 0;
 
 	if (queue_type == VTNET_TQ) {
 		const struct rte_memzone *hdr_mz;
@@ -402,7 +407,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, 0);
 				return -ENOMEM;
 			}
 		}
@@ -436,7 +441,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, 0);
 				return -ENOMEM;
 			}
 		}
@@ -1184,7 +1189,7 @@ eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 	eth_dev->tx_pkt_burst = NULL;
 	eth_dev->rx_pkt_burst = NULL;
 
-	virtio_dev_queue_release(hw->cvq);
+	virtio_dev_queue_release(hw->cvq, 1);
 
 	rte_free(eth_dev->data->mac_addrs);
 	eth_dev->data->mac_addrs = NULL;
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index 66423a0..070345a 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -83,7 +83,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 			unsigned int socket_id,
 			struct virtqueue **pvq);
 
-void virtio_dev_queue_release(struct virtqueue *vq);
+void virtio_dev_queue_release(struct virtqueue *vq, int io_related);
 
 int  virtio_dev_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id,
 		uint16_t nb_rx_desc, unsigned int socket_id,
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index ef21d8e..f8cbb9c 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -464,7 +464,7 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
 void
 virtio_dev_rx_queue_release(void *rxq)
 {
-	virtio_dev_queue_release(rxq);
+	virtio_dev_queue_release(rxq, 1);
 }
 
 /*
@@ -539,7 +539,7 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 void
 virtio_dev_tx_queue_release(void *txq)
 {
-	virtio_dev_queue_release(txq);
+	virtio_dev_queue_release(txq, 1);
 }
 
 static void
-- 
2.1.4

Re: [dpdk-dev] [PATCH] virtio: fix memory leak of virtqueue memzones

[Yuanhan Liu]

On Tue, Apr 26, 2016 at 12:32:12PM +0000, Jianfeng Tan wrote:
> 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 ff909fe21f.

The more proper way to reference a commit is

	commit_id ("$commit_subject")

Like what the fixline does.

> 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 | 69 ++++++++++++++++++++------------------
>  drivers/net/virtio/virtio_ethdev.h |  2 +-
>  drivers/net/virtio/virtio_rxtx.c   |  4 +--
>  3 files changed, 40 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 63a368a..54eacf6 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -261,12 +261,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, int io_related)
> +{
>  	struct virtio_hw *hw;
>  
>  	if (vq) {
>  		hw = vq->hw;
> -		hw->vtpci_ops->del_queue(hw, vq);
> +		if (io_related)
> +			hw->vtpci_ops->del_queue(hw, vq);

What is "io_related" supposed to mean here, queue has been started/set
up? If so, "started" might be better. And remember to put it into the vq
struct: we don't need an extra parameter for that.

> +
> +		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);
> @@ -286,6 +292,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
>  	unsigned int vq_size, size;
>  	struct virtio_hw *hw = dev->data->dev_private;
>  	struct virtqueue *vq = NULL;
> +	const char *queue_names[] = {"rvq", "txq", "cvq"};
>  
>  	PMD_INIT_LOG(DEBUG, "setting up queue: %u", vtpci_queue_idx);
>  
> @@ -305,34 +312,34 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
>  		return -EINVAL;
>  	}
>  
> -	if (queue_type == VTNET_RQ) {
> -		snprintf(vq_name, sizeof(vq_name), "port%d_rvq%d",
> -			dev->data->port_id, queue_idx);
> -		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
> -			vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE);
> -		vq->sw_ring = rte_zmalloc_socket("rxq->sw_ring",
> -			(RTE_PMD_VIRTIO_RX_MAX_BURST + vq_size) *
> -			sizeof(vq->sw_ring[0]), RTE_CACHE_LINE_SIZE, socket_id);
> -	} else if (queue_type == VTNET_TQ) {
> -		snprintf(vq_name, sizeof(vq_name), "port%d_tvq%d",
> -			dev->data->port_id, queue_idx);
> -		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
> -			vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE);
> -	} else if (queue_type == VTNET_CQ) {
> -		snprintf(vq_name, sizeof(vq_name), "port%d_cvq",
> -			dev->data->port_id);
> -		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
> -			vq_size * sizeof(struct vq_desc_extra),
> -			RTE_CACHE_LINE_SIZE);
> +	if (queue_type < VTNET_RQ || queue_type > VTNET_RQ) {
> +		PMD_INIT_LOG(ERR, "invalid queue type: %d", queue_type);
> +		return -EINVAL;
>  	}
> +
> +	snprintf(vq_name, sizeof(vq_name), "port%d_%s%d",
> +		 dev->data->port_id, queue_names[queue_type], queue_idx);
> +	vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
> +			 vq_size * sizeof(struct vq_desc_extra),
> +			 RTE_CACHE_LINE_SIZE);

This is a cleanup, a good cleanup. So, make a patch for that, and do
NOT mix cleanup and fix in one single patch, which is something I
have told you quite few times, right?


	--yliu

Re: [dpdk-dev] [PATCH] virtio: fix memory leak of virtqueue memzones

[Tan, Jianfeng]

Hi Yuanhan,


On 4/28/2016 6:37 AM, Yuanhan Liu wrote:
> On Tue, Apr 26, 2016 at 12:32:12PM +0000, Jianfeng Tan wrote:
>> 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 ff909fe21f.
> The more proper way to reference a commit is
>
> 	commit_id ("$commit_subject")
>
> Like what the fixline does.

OK.

>
>> 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 | 69 ++++++++++++++++++++------------------
>>   drivers/net/virtio/virtio_ethdev.h |  2 +-
>>   drivers/net/virtio/virtio_rxtx.c   |  4 +--
>>   3 files changed, 40 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index 63a368a..54eacf6 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -261,12 +261,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, int io_related)
>> +{
>>   	struct virtio_hw *hw;
>>   
>>   	if (vq) {
>>   		hw = vq->hw;
>> -		hw->vtpci_ops->del_queue(hw, vq);
>> +		if (io_related)
>> +			hw->vtpci_ops->del_queue(hw, vq);
> What is "io_related" supposed to mean here, queue has been started/set
> up? If so, "started" might be better. And remember to put it into the vq
> struct: we don't need an extra parameter for that.

Good suggestion.

>
>> +
>> +		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);
>> @@ -286,6 +292,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
>>   	unsigned int vq_size, size;
>>   	struct virtio_hw *hw = dev->data->dev_private;
>>   	struct virtqueue *vq = NULL;
>> +	const char *queue_names[] = {"rvq", "txq", "cvq"};
>>   
>>   	PMD_INIT_LOG(DEBUG, "setting up queue: %u", vtpci_queue_idx);
>>   
>> @@ -305,34 +312,34 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
>>   		return -EINVAL;
>>   	}
>>   
>> -	if (queue_type == VTNET_RQ) {
>> -		snprintf(vq_name, sizeof(vq_name), "port%d_rvq%d",
>> -			dev->data->port_id, queue_idx);
>> -		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
>> -			vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE);
>> -		vq->sw_ring = rte_zmalloc_socket("rxq->sw_ring",
>> -			(RTE_PMD_VIRTIO_RX_MAX_BURST + vq_size) *
>> -			sizeof(vq->sw_ring[0]), RTE_CACHE_LINE_SIZE, socket_id);
>> -	} else if (queue_type == VTNET_TQ) {
>> -		snprintf(vq_name, sizeof(vq_name), "port%d_tvq%d",
>> -			dev->data->port_id, queue_idx);
>> -		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
>> -			vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE);
>> -	} else if (queue_type == VTNET_CQ) {
>> -		snprintf(vq_name, sizeof(vq_name), "port%d_cvq",
>> -			dev->data->port_id);
>> -		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
>> -			vq_size * sizeof(struct vq_desc_extra),
>> -			RTE_CACHE_LINE_SIZE);
>> +	if (queue_type < VTNET_RQ || queue_type > VTNET_RQ) {
>> +		PMD_INIT_LOG(ERR, "invalid queue type: %d", queue_type);
>> +		return -EINVAL;
>>   	}
>> +
>> +	snprintf(vq_name, sizeof(vq_name), "port%d_%s%d",
>> +		 dev->data->port_id, queue_names[queue_type], queue_idx);
>> +	vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
>> +			 vq_size * sizeof(struct vq_desc_extra),
>> +			 RTE_CACHE_LINE_SIZE);
> This is a cleanup, a good cleanup. So, make a patch for that, and do
> NOT mix cleanup and fix in one single patch, which is something I
> have told you quite few times, right?

You mean submit a specific patch for cleanup or just another commit 
inside this patch set?

Thanks,
Jianfeng

>
>
> 	--yliu

Re: [dpdk-dev] [PATCH] virtio: fix memory leak of virtqueue memzones

[Yuanhan Liu]

On Thu, Apr 28, 2016 at 10:01:06AM +0800, Tan, Jianfeng wrote:
> >>+
> >>+	snprintf(vq_name, sizeof(vq_name), "port%d_%s%d",
> >>+		 dev->data->port_id, queue_names[queue_type], queue_idx);
> >>+	vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
> >>+			 vq_size * sizeof(struct vq_desc_extra),
> >>+			 RTE_CACHE_LINE_SIZE);
> >This is a cleanup, a good cleanup. So, make a patch for that, and do
> >NOT mix cleanup and fix in one single patch, which is something I
> >have told you quite few times, right?
> 
> You mean submit a specific patch for cleanup or just another commit inside
> this patch set?

Do it as a patchset, as the two are connected: the fix should be based
on the cleanup patch.

	--yliu

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

[Jianfeng Tan]

Commit 1: Do some cleanup in virtio_dev_queue_setup();
Commit 2: Fix the memory leak bug.

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Jianfeng Tan (2):
  virtio: cleanup virtio_dev_queue_setup()
  virtio: fix memory leak of virtqueue memzones

 drivers/net/virtio/virtio_ethdev.c | 66 +++++++++++++++++++++-----------------
 drivers/net/virtio/virtqueue.h     |  2 ++
 2 files changed, 39 insertions(+), 29 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH v2 1/2] virtio: cleanup virtio_dev_queue_setup()

[Jianfeng Tan]

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 47 +++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 1fe90ae..0553b67 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -285,6 +285,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 	unsigned int vq_size, size;
 	struct virtio_hw *hw = dev->data->dev_private;
 	struct virtqueue *vq = NULL;
+	const char *queue_names[] = {"rvq", "txq", "cvq"};
 
 	PMD_INIT_LOG(DEBUG, "setting up queue: %u", vtpci_queue_idx);
 
@@ -304,34 +305,34 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 		return -EINVAL;
 	}
 
-	if (queue_type == VTNET_RQ) {
-		snprintf(vq_name, sizeof(vq_name), "port%d_rvq%d",
-			dev->data->port_id, queue_idx);
-		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
-			vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE);
-		vq->sw_ring = rte_zmalloc_socket("rxq->sw_ring",
-			(RTE_PMD_VIRTIO_RX_MAX_BURST + vq_size) *
-			sizeof(vq->sw_ring[0]), RTE_CACHE_LINE_SIZE, socket_id);
-	} else if (queue_type == VTNET_TQ) {
-		snprintf(vq_name, sizeof(vq_name), "port%d_tvq%d",
-			dev->data->port_id, queue_idx);
-		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
-			vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE);
-	} else if (queue_type == VTNET_CQ) {
-		snprintf(vq_name, sizeof(vq_name), "port%d_cvq",
-			dev->data->port_id);
-		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
-			vq_size * sizeof(struct vq_desc_extra),
-			RTE_CACHE_LINE_SIZE);
+	if (queue_type < VTNET_RQ || queue_type > VTNET_RQ) {
+		PMD_INIT_LOG(ERR, "invalid queue type: %d", queue_type);
+		return -EINVAL;
 	}
+
+	snprintf(vq_name, sizeof(vq_name), "port%d_%s%d",
+		 dev->data->port_id, queue_names[queue_type], queue_idx);
+	vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
+			 vq_size * sizeof(struct vq_desc_extra),
+			 RTE_CACHE_LINE_SIZE);
 	if (vq == NULL) {
 		PMD_INIT_LOG(ERR, "Can not allocate virtqueue");
 		return -ENOMEM;
 	}
-	if (queue_type == VTNET_RQ && vq->sw_ring == NULL) {
-		PMD_INIT_LOG(ERR, "Can not allocate RX soft ring");
-		rte_free(vq);
-		return -ENOMEM;
+
+	if (queue_type == VTNET_RQ) {
+		size_t sz_sw;
+
+		sz_sw = (RTE_PMD_VIRTIO_RX_MAX_BURST + vq_size) *
+			sizeof(vq->sw_ring[0]);
+		vq->sw_ring = rte_zmalloc_socket("rxq->sw_ring", sz_sw,
+						 RTE_CACHE_LINE_SIZE,
+						 socket_id);
+		if (!vq->sw_ring) {
+			PMD_INIT_LOG(ERR, "Can not allocate RX soft ring");
+			rte_free(vq);
+			return -ENOMEM;
+		}
 	}
 
 	vq->hw = hw;
-- 
2.1.4

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

[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 <jianfeng.tan@intel.com>
---
 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 0553b67..a14cc88 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

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

[Tan, Jianfeng]

Self NACK.

Missed to reset vq->started.

> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Friday, April 29, 2016 3:04 AM
> To: dev@dpdk.org
> Cc: Xie, Huawei; yuanhan.liu@linux.intel.com; Tan, Jianfeng
> Subject: [PATCH v2 0/2] virtio: fix memory leak of virtqueue memzones
> 
> Commit 1: Do some cleanup in virtio_dev_queue_setup();
> Commit 2: Fix the memory leak bug.
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Jianfeng Tan (2):
>   virtio: cleanup virtio_dev_queue_setup()
>   virtio: fix memory leak of virtqueue memzones
> 
>  drivers/net/virtio/virtio_ethdev.c | 66 +++++++++++++++++++++------------
> -----
>  drivers/net/virtio/virtqueue.h     |  2 ++
>  2 files changed, 39 insertions(+), 29 deletions(-)
> 
> --
> 2.1.4

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

[Tan, Jianfeng]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tan, Jianfeng
> Sent: Friday, April 29, 2016 3:05 AM
> To: dev@dpdk.org
> Cc: Xie, Huawei; yuanhan.liu@linux.intel.com
> Subject: Re: [dpdk-dev] [PATCH v2 0/2] virtio: fix memory leak of virtqueue
> memzones
> 
> Self NACK.
> 
> Missed to reset vq->started.

Given a second thought, no need to do that. Please go with this version.

Thanks,
Jianfeng

> 
> > -----Original Message-----
> > From: Tan, Jianfeng
> > Sent: Friday, April 29, 2016 3:04 AM
> > To: dev@dpdk.org
> > Cc: Xie, Huawei; yuanhan.liu@linux.intel.com; Tan, Jianfeng
> > Subject: [PATCH v2 0/2] virtio: fix memory leak of virtqueue memzones
> >
> > Commit 1: Do some cleanup in virtio_dev_queue_setup();
> > Commit 2: Fix the memory leak bug.
> >
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> >
> > Jianfeng Tan (2):
> >   virtio: cleanup virtio_dev_queue_setup()
> >   virtio: fix memory leak of virtqueue memzones
> >
> >  drivers/net/virtio/virtio_ethdev.c | 66 +++++++++++++++++++++----------
> --
> > -----
> >  drivers/net/virtio/virtqueue.h     |  2 ++
> >  2 files changed, 39 insertions(+), 29 deletions(-)
> >
> > --
> > 2.1.4

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

[Jianfeng Tan]

Patch 1: Do some cleanup in virtio_dev_queue_setup();
Patch 2: Fix the memory leak bug.

Jianfeng Tan (2):
  v3: Fix a typo in the queue_type check.
  v2: split cleanup and fix into two patches.

  virtio: cleanup virtio_dev_queue_setup()
  virtio: fix memory leak of virtqueue memzones

 drivers/net/virtio/virtio_ethdev.c | 66 +++++++++++++++++++++-----------------
 drivers/net/virtio/virtqueue.h     |  2 ++
 2 files changed, 39 insertions(+), 29 deletions(-)

-- 
2.1.4

[dpdk-dev] [PATCH v3 1/2] virtio: cleanup virtio_dev_queue_setup()

[Jianfeng Tan]

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 47 +++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 1fe90ae..b3f4158 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -285,6 +285,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 	unsigned int vq_size, size;
 	struct virtio_hw *hw = dev->data->dev_private;
 	struct virtqueue *vq = NULL;
+	const char *queue_names[] = {"rvq", "txq", "cvq"};
 
 	PMD_INIT_LOG(DEBUG, "setting up queue: %u", vtpci_queue_idx);
 
@@ -304,34 +305,34 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 		return -EINVAL;
 	}
 
-	if (queue_type == VTNET_RQ) {
-		snprintf(vq_name, sizeof(vq_name), "port%d_rvq%d",
-			dev->data->port_id, queue_idx);
-		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
-			vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE);
-		vq->sw_ring = rte_zmalloc_socket("rxq->sw_ring",
-			(RTE_PMD_VIRTIO_RX_MAX_BURST + vq_size) *
-			sizeof(vq->sw_ring[0]), RTE_CACHE_LINE_SIZE, socket_id);
-	} else if (queue_type == VTNET_TQ) {
-		snprintf(vq_name, sizeof(vq_name), "port%d_tvq%d",
-			dev->data->port_id, queue_idx);
-		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
-			vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE);
-	} else if (queue_type == VTNET_CQ) {
-		snprintf(vq_name, sizeof(vq_name), "port%d_cvq",
-			dev->data->port_id);
-		vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
-			vq_size * sizeof(struct vq_desc_extra),
-			RTE_CACHE_LINE_SIZE);
+	if (queue_type < VTNET_RQ || queue_type > VTNET_CQ) {
+		PMD_INIT_LOG(ERR, "invalid queue type: %d", queue_type);
+		return -EINVAL;
 	}
+
+	snprintf(vq_name, sizeof(vq_name), "port%d_%s%d",
+		 dev->data->port_id, queue_names[queue_type], queue_idx);
+	vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
+			 vq_size * sizeof(struct vq_desc_extra),
+			 RTE_CACHE_LINE_SIZE);
 	if (vq == NULL) {
 		PMD_INIT_LOG(ERR, "Can not allocate virtqueue");
 		return -ENOMEM;
 	}
-	if (queue_type == VTNET_RQ && vq->sw_ring == NULL) {
-		PMD_INIT_LOG(ERR, "Can not allocate RX soft ring");
-		rte_free(vq);
-		return -ENOMEM;
+
+	if (queue_type == VTNET_RQ) {
+		size_t sz_sw;
+
+		sz_sw = (RTE_PMD_VIRTIO_RX_MAX_BURST + vq_size) *
+			sizeof(vq->sw_ring[0]);
+		vq->sw_ring = rte_zmalloc_socket("rxq->sw_ring", sz_sw,
+						 RTE_CACHE_LINE_SIZE,
+						 socket_id);
+		if (!vq->sw_ring) {
+			PMD_INIT_LOG(ERR, "Can not allocate RX soft ring");
+			rte_free(vq);
+			return -ENOMEM;
+		}
 	}
 
 	vq->hw = hw;
-- 
2.1.4

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

[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 <jianfeng.tan@intel.com>
---
 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

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

[Yuanhan Liu]

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

Re: [dpdk-dev] [PATCH v3 1/2] virtio: cleanup virtio_dev_queue_setup()

[Yuanhan Liu]

On Fri, Apr 29, 2016 at 12:48:45AM +0000, Jianfeng Tan wrote:
> +	if (queue_type < VTNET_RQ || queue_type > VTNET_CQ) {
> +		PMD_INIT_LOG(ERR, "invalid queue type: %d", queue_type);
> +		return -EINVAL;
>  	}

I'm thinking this check is not necessary. We can make sure it's a valid
queue type.

	--yliu

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

[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 +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

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

[Tan, Jianfeng]

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

Re: [dpdk-dev] [PATCH v3 1/2] virtio: cleanup virtio_dev_queue_setup()

[Tan, Jianfeng]

Hi Yuanhan,

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Thursday, May 5, 2016 11:20 AM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; Xie, Huawei
> Subject: Re: [PATCH v3 1/2] virtio: cleanup virtio_dev_queue_setup()
> 
> On Fri, Apr 29, 2016 at 12:48:45AM +0000, Jianfeng Tan wrote:
> > +	if (queue_type < VTNET_RQ || queue_type > VTNET_CQ) {
> > +		PMD_INIT_LOG(ERR, "invalid queue type: %d", queue_type);
> > +		return -EINVAL;
> >  	}
> 
> I'm thinking this check is not necessary. We can make sure it's a valid
> queue type.

Yes, this is not necessary, and I was also entangled with whether to keep it or not. And ok, I'll send a new version with this check removed.

Thanks,
Jianfeng

> 
> 	--yliu

Re: [dpdk-dev] [PATCH v3 1/2] virtio: cleanup virtio_dev_queue_setup()

[Yuanhan Liu]

On Mon, May 09, 2016 at 07:58:26AM +0000, Tan, Jianfeng wrote:
> Hi Yuanhan,
> 
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Thursday, May 5, 2016 11:20 AM
> > To: Tan, Jianfeng
> > Cc: dev@dpdk.org; Xie, Huawei
> > Subject: Re: [PATCH v3 1/2] virtio: cleanup virtio_dev_queue_setup()
> > 
> > On Fri, Apr 29, 2016 at 12:48:45AM +0000, Jianfeng Tan wrote:
> > > +	if (queue_type < VTNET_RQ || queue_type > VTNET_CQ) {
> > > +		PMD_INIT_LOG(ERR, "invalid queue type: %d", queue_type);
> > > +		return -EINVAL;
> > >  	}
> > 
> > I'm thinking this check is not necessary. We can make sure it's a valid
> > queue type.
> 
> Yes, this is not necessary, and I was also entangled with whether to keep it or not. And ok, I'll send a new version with this check removed.

Not necessary; it's trivial for me to fix it while applying it. So,
this series is applied to dpdk-next-virtio, with following changes:

	--yliu

---
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 6b291f5..70ee12a 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -266,7 +266,7 @@ virtio_dev_queue_release(struct virtqueue *vq)
 
 	if (vq) {
 		hw = vq->hw;
-		if (vq->started)
+		if (vq->configured)
 			hw->vtpci_ops->del_queue(hw, vq);
 
 		rte_memzone_free(vq->mz);
@@ -311,11 +311,6 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 		return -EINVAL;
 	}
 
-	if (queue_type < VTNET_RQ || queue_type > VTNET_CQ) {
-		PMD_INIT_LOG(ERR, "invalid queue type: %d", queue_type);
-		return -EINVAL;
-	}
-
 	snprintf(vq_name, sizeof(vq_name), "port%d_%s%d",
 		 dev->data->port_id, queue_names[queue_type], queue_idx);
 	vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) +
@@ -453,7 +448,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 
 	hw->vtpci_ops->setup_queue(hw, vq);
 
-	vq->started = 1;
+	vq->configured = 1;
 	*pvq = vq;
 	return 0;
 }
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 0006a29..4e543d2 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -201,7 +201,7 @@ struct virtqueue {
 
 	uint16_t	*notify_addr;
 
-	int		started;
+	int		configured;
 
 	struct vq_desc_extra {
 		void              *cookie;