[PATCH] net/vhost: move device stop call in close function

[PATCH] net/vhost: move device stop call in close function

[Sagar Abhang]

Moved the call to "eth_dev_stop" inside "eth_dev_close" because
"rte_eth_dev_close" calls 'close' operation of device, and in existing
code the close was happening without 'stop' operation for vhost device.
Moved code to free rx and tx queues inside "eth_dev_close" because the
"rte_eth_dev_close" function calls the vhost's "eth_dev_close" function
In that case, the memory allocated for the queues is not freed up
before we free the pointer of rx and tx queues causing memory leak.

Signed-off-by: Sagar Abhang <sabhang@brocade.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 7f5cd7e..100d1cf 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -832,11 +832,14 @@ eth_dev_close(struct rte_eth_dev *dev)
 {
 	struct pmd_internal *internal;
 	struct internal_list *list;
+	unsigned int i;
 
 	internal = dev->data->dev_private;
 	if (!internal)
 		return;
 
+	eth_dev_stop(dev);
+
 	rte_vhost_driver_unregister(internal->iface_name);
 
 	list = find_internal_resource(internal->iface_name);
@@ -848,9 +851,17 @@ eth_dev_close(struct rte_eth_dev *dev)
 	pthread_mutex_unlock(&internal_list_lock);
 	rte_free(list);
 
+	for (i = 0; i < dev->data->nb_rx_queues; i++)
+		rte_free(dev->data->rx_queues[i]);
+	for (i = 0; i < dev->data->nb_tx_queues; i++)
+		rte_free(dev->data->tx_queues[i]);
+
+	rte_free(dev->data->mac_addrs);
 	free(internal->dev_name);
 	free(internal->iface_name);
 	rte_free(internal);
+
+	dev->data->dev_private = NULL;
 }
 
 static int
@@ -1259,7 +1270,6 @@ static int
 rte_pmd_vhost_remove(const char *name)
 {
 	struct rte_eth_dev *eth_dev = NULL;
-	unsigned int i;
 
 	RTE_LOG(INFO, PMD, "Un-Initializing pmd_vhost for %s\n", name);
 
@@ -1268,8 +1278,6 @@ rte_pmd_vhost_remove(const char *name)
 	if (eth_dev == NULL)
 		return -ENODEV;
 
-	eth_dev_stop(eth_dev);
-
 	eth_dev_close(eth_dev);
 
 	if (rte_atomic16_sub_return(&nb_started_ports, 1) == 0)
@@ -1278,12 +1286,6 @@ rte_pmd_vhost_remove(const char *name)
 	rte_free(vring_states[eth_dev->data->port_id]);
 	vring_states[eth_dev->data->port_id] = NULL;
 
-	for (i = 0; i < eth_dev->data->nb_rx_queues; i++)
-		rte_free(eth_dev->data->rx_queues[i]);
-	for (i = 0; i < eth_dev->data->nb_tx_queues; i++)
-		rte_free(eth_dev->data->tx_queues[i]);
-
-	rte_free(eth_dev->data->mac_addrs);
 	rte_free(eth_dev->data);
 
 	rte_eth_dev_release_port(eth_dev);
-- 
2.1.4

Re: [PATCH] net/vhost: move device stop call in close function

[Yuanhan Liu]

On Fri, Mar 31, 2017 at 03:47:10PM -0700, Sagar Abhang wrote:
> Moved the call to "eth_dev_stop" inside "eth_dev_close" because
> "rte_eth_dev_close" calls 'close' operation of device, and in existing
> code the close was happening without 'stop' operation for vhost device.
> Moved code to free rx and tx queues inside "eth_dev_close" because the
> "rte_eth_dev_close" function calls the vhost's "eth_dev_close" function
> In that case, the memory allocated for the queues is not freed up
> before we free the pointer of rx and tx queues causing memory leak.

This patch looks Okay to me, expect the log is a bit hard to understand:
you don't have to explain that "rte_eth_dev_xx" calls the vhost "eth_dev_xxx".
Also please use whitespace lines between paragraphs.

Will you try to reword the log a bit, so that I can apply?

Thanks.

	--yliu

[PATCH v2] net/vhost: stop dev in close and address mem leak

[Sagar Abhang]

Move the call to stop the device inside the close routine because close
needs to stop the device if it isn't stopped.

Free the allocated queue buffers in close instead of doing so in remove.
Original code had these clean ups in remove which was causing memory
leak.

Signed-off-by: Sagar Abhang <sabhang@brocade.com>
---

 v2:
 - Modified the log message as requested.

 drivers/net/vhost/rte_eth_vhost.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 41ce5fc..b6fc94a 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -791,11 +791,14 @@ eth_dev_close(struct rte_eth_dev *dev)
 {
 	struct pmd_internal *internal;
 	struct internal_list *list;
+	unsigned int i;
 
 	internal = dev->data->dev_private;
 	if (!internal)
 		return;
 
+	eth_dev_stop(dev);
+
 	rte_vhost_driver_unregister(internal->iface_name);
 
 	list = find_internal_resource(internal->iface_name);
@@ -807,9 +810,17 @@ eth_dev_close(struct rte_eth_dev *dev)
 	pthread_mutex_unlock(&internal_list_lock);
 	rte_free(list);
 
+	for (i = 0; i < dev->data->nb_rx_queues; i++)
+		rte_free(dev->data->rx_queues[i]);
+	for (i = 0; i < dev->data->nb_tx_queues; i++)
+		rte_free(dev->data->tx_queues[i]);
+
+	rte_free(dev->data->mac_addrs);
 	free(internal->dev_name);
 	free(internal->iface_name);
 	rte_free(internal);
+
+	dev->data->dev_private = NULL;
 }
 
 static int
@@ -1198,7 +1209,6 @@ static int
 rte_pmd_vhost_remove(const char *name)
 {
 	struct rte_eth_dev *eth_dev = NULL;
-	unsigned int i;
 
 	RTE_LOG(INFO, PMD, "Un-Initializing pmd_vhost for %s\n", name);
 
@@ -1207,19 +1217,11 @@ rte_pmd_vhost_remove(const char *name)
 	if (eth_dev == NULL)
 		return -ENODEV;
 
-	eth_dev_stop(eth_dev);
-
 	eth_dev_close(eth_dev);
 
 	rte_free(vring_states[eth_dev->data->port_id]);
 	vring_states[eth_dev->data->port_id] = NULL;
 
-	for (i = 0; i < eth_dev->data->nb_rx_queues; i++)
-		rte_free(eth_dev->data->rx_queues[i]);
-	for (i = 0; i < eth_dev->data->nb_tx_queues; i++)
-		rte_free(eth_dev->data->tx_queues[i]);
-
-	rte_free(eth_dev->data->mac_addrs);
 	rte_free(eth_dev->data);
 
 	rte_eth_dev_release_port(eth_dev);
-- 
2.1.4

Re: [PATCH v2] net/vhost: stop dev in close and address mem leak

[Yuanhan Liu]

On Thu, Apr 06, 2017 at 05:26:37PM -0700, Sagar Abhang wrote:
> Move the call to stop the device inside the close routine because close
> needs to stop the device if it isn't stopped.
> 
> Free the allocated queue buffers in close instead of doing so in remove.
> Original code had these clean ups in remove which was causing memory
> leak.
> 
> Signed-off-by: Sagar Abhang <sabhang@brocade.com>

Applied to dpdk-next-virtio.

	--yliu