Re: [dpdk-dev] [PATCH v4 3/4] virtio: free queue memory in virtio_dev_close()

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue,  7 Jul 2015 10:18:06 +0100
Bernard Iremonger <bernard.iremonger@intel.com> wrote:

> Add function virtio_free_queues() and call from virtio_dev_close()
> 
> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index b88f297..ed99f33 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -428,6 +428,24 @@ virtio_dev_cq_queue_setup(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx,
>  }
>  
>  static void
> +virtio_free_queues(struct rte_eth_dev *dev)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +		rte_free(dev->data->rx_queues[i]);
> +		dev->data->rx_queues[i] = NULL;
> +	}
> +	dev->data->nb_rx_queues = 0;
> +
> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +		rte_free(dev->data->tx_queues[i]);
> +		dev->data->tx_queues[i] = NULL;
> +	}
> +	dev->data->nb_tx_queues = 0;
> +}
> +
> +static void
>  virtio_dev_close(struct rte_eth_dev *dev)
>  {
>  	struct virtio_hw *hw = dev->data->dev_private;
> @@ -441,6 +459,7 @@ virtio_dev_close(struct rte_eth_dev *dev)
>  	vtpci_reset(hw);
>  	hw->started = 0;
>  	virtio_dev_free_mbufs(dev);
> +	virtio_free_queues(dev);
>  }
>  
>  static void

I think virtio should implement regular queue free:

Subject: virtio: add proper queue release

When rx or tx queue is released, the virtio driver leaks memory
because it never frees the associated queue structure.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -78,9 +78,6 @@ static int virtio_dev_link_update(struct
 static void virtio_set_hwaddr(struct virtio_hw *hw);
 static void virtio_get_hwaddr(struct virtio_hw *hw);
 
-static void virtio_dev_rx_queue_release(__rte_unused void *rxq);
-static void virtio_dev_tx_queue_release(__rte_unused void *txq);
-
 static void virtio_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats);
 static void virtio_dev_stats_reset(struct rte_eth_dev *dev);
 static void virtio_dev_free_mbufs(struct rte_eth_dev *dev);
@@ -389,6 +386,19 @@ int virtio_dev_queue_setup(struct rte_et
 	return 0;
 }
 
+
+void
+virtio_dev_queue_release(struct virtqueue *vq)
+{
+	struct virtio_hw *hw = vq->hw;
+
+	/* Select and deactivate the queue */
+	VIRTIO_WRITE_REG_2(hw, VIRTIO_PCI_QUEUE_SEL, vq->queue_id);
+	VIRTIO_WRITE_REG_4(hw, VIRTIO_PCI_QUEUE_PFN, 0);
+
+	rte_free(vq);
+}
+
 static int
 virtio_dev_cq_queue_setup(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx,
 		uint32_t socket_id)
@@ -536,10 +546,8 @@ static struct eth_dev_ops virtio_eth_dev
 	.stats_reset             = virtio_dev_stats_reset,
 	.link_update             = virtio_dev_link_update,
 	.rx_queue_setup          = virtio_dev_rx_queue_setup,
-	/* meaningfull only to multiple queue */
 	.rx_queue_release        = virtio_dev_rx_queue_release,
 	.tx_queue_setup          = virtio_dev_tx_queue_setup,
-	/* meaningfull only to multiple queue */
 	.tx_queue_release        = virtio_dev_tx_queue_release,
 	/* collect stats per queue */
 	.queue_stats_mapping_set = virtio_dev_queue_stats_mapping_set,
@@ -1278,19 +1286,6 @@ rte_virtio_pmd_init(const char *name __r
 }
 
 /*
- * Only 1 queue is supported, no queue release related operation
- */
-static void
-virtio_dev_rx_queue_release(__rte_unused void *rxq)
-{
-}
-
-static void
-virtio_dev_tx_queue_release(__rte_unused void *txq)
-{
-}
-
-/*
  * Configure virtio device
  * It returns 0 on success.
  */
--- a/lib/librte_pmd_virtio/virtio_ethdev.h
+++ b/lib/librte_pmd_virtio/virtio_ethdev.h
@@ -84,15 +84,21 @@ int virtio_dev_queue_setup(struct rte_et
 			unsigned int socket_id,
 			struct virtqueue **pvq);
 
+void virtio_dev_queue_release(struct virtqueue *vq);
+
 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,
 		const struct rte_eth_rxconf *rx_conf,
 		struct rte_mempool *mb_pool);
 
+void virtio_dev_rx_queue_release(void *rxq);
+
 int  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
 		uint16_t nb_tx_desc, unsigned int socket_id,
 		const struct rte_eth_txconf *tx_conf);
 
+void virtio_dev_tx_queue_release(void *txq);
+
 uint16_t virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		uint16_t nb_pkts);
 
--- a/lib/librte_pmd_virtio/virtio_rxtx.c
+++ b/lib/librte_pmd_virtio/virtio_rxtx.c
@@ -401,6 +401,13 @@ virtio_dev_rx_queue_setup(struct rte_eth
 	return 0;
 }
 
+void
+virtio_dev_rx_queue_release(void *rxq)
+{
+	if (rxq)
+		virtio_dev_queue_release(rxq);
+}
+
 /*
  * struct rte_eth_dev *dev: Used to update dev
  * uint16_t nb_desc: Defaults to values read from config space
@@ -455,6 +462,13 @@ virtio_dev_tx_queue_setup(struct rte_eth
 	return 0;
 }
 
+void
+virtio_dev_tx_queue_release(void *txq)
+{
+	if (txq)
+		virtio_dev_queue_release(txq);
+}
+
 static void
 virtio_discard_rxbuf(struct virtqueue *vq, struct rte_mbuf *m)
 {