[dpdk-dev] [PATCH 0/5] virtio: release port upon close

[dpdk-dev] [PATCH 0/5] virtio: release port upon close

[Tiwei Bie]

Tiwei Bie (5):
  net/virtio: fix queue memory leak on error
  net/virtio: unmap port IO for legacy device
  net/virtio: unmap device on initialization error
  net/virtio: release port upon close
  app/testpmd: drop the workaround for virtio-user

 app/test-pmd/testpmd.c                        | 13 -----
 drivers/net/virtio/virtio_ethdev.c            | 50 ++++++++++++++-----
 drivers/net/virtio/virtio_pci.c               |  1 +
 .../net/virtio/virtio_user/virtio_user_dev.h  |  1 -
 drivers/net/virtio/virtio_user_ethdev.c       | 12 ++---
 5 files changed, 42 insertions(+), 35 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/5] net/virtio: fix queue memory leak on error

[Tiwei Bie]

We should free queues when we failed to initialize the
virtio device.

Fixes: 26b683b4f7d0 ("net/virtio: setup Rx queue interrupts")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index c4570bbf8..df3a218a8 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1752,6 +1752,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 	if (eth_dev->data->dev_conf.intr_conf.rxq) {
 		if (virtio_configure_intr(eth_dev) < 0) {
 			PMD_INIT_LOG(ERR, "failed to configure interrupt");
+			virtio_free_queues(hw);
 			return -1;
 		}
 	}
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/5] net/virtio: fix queue memory leak on error

[Maxime Coquelin]

On 6/5/19 11:43 AM, Tiwei Bie wrote:
> We should free queues when we failed to initialize the
> virtio device.
> 
> Fixes: 26b683b4f7d0 ("net/virtio: setup Rx queue interrupts")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index c4570bbf8..df3a218a8 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1752,6 +1752,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
>   	if (eth_dev->data->dev_conf.intr_conf.rxq) {
>   		if (virtio_configure_intr(eth_dev) < 0) {
>   			PMD_INIT_LOG(ERR, "failed to configure interrupt");
> +			virtio_free_queues(hw);
>   			return -1;
>   		}
>   	}
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-dev] [PATCH 2/5] net/virtio: unmap port IO for legacy device

[Tiwei Bie]

For legacy devices, we should also unmap the port IO
resource on device removal.

Fixes: b8f04520ad71 ("virtio: use PCI ioport API")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index df3a218a8..a2cedcc87 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1876,6 +1876,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 static int
 eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 {
+	struct virtio_hw *hw = eth_dev->data->dev_private;
+
 	PMD_INIT_FUNC_TRACE();
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
@@ -1888,8 +1890,11 @@ eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 	eth_dev->tx_pkt_burst = NULL;
 	eth_dev->rx_pkt_burst = NULL;
 
-	if (eth_dev->device)
+	if (eth_dev->device) {
 		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
+		if (!hw->modern)
+			rte_pci_ioport_unmap(VTPCI_IO(hw));
+	}
 
 	PMD_INIT_LOG(DEBUG, "dev_uninit completed");
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 2/5] net/virtio: unmap port IO for legacy device

[Maxime Coquelin]

On 6/5/19 11:43 AM, Tiwei Bie wrote:
> For legacy devices, we should also unmap the port IO
> resource on device removal.
> 
> Fixes: b8f04520ad71 ("virtio: use PCI ioport API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

[dpdk-dev] [PATCH 3/5] net/virtio: unmap device on initialization error

[Tiwei Bie]

We should unmap the device when we failed to initialize the device.

Fixes: 6ba1f63b5ab0 ("virtio: support specification 1.0")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 12 +++++++++---
 drivers/net/virtio/virtio_pci.c    |  1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index a2cedcc87..46d2e4ac6 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1857,17 +1857,23 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	if (!hw->virtio_user_dev) {
 		ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
 		if (ret)
-			goto out;
+			goto err_vtpci_init;
 	}
 
 	/* reset device and negotiate default features */
 	ret = virtio_init_device(eth_dev, VIRTIO_PMD_DEFAULT_GUEST_FEATURES);
 	if (ret < 0)
-		goto out;
+		goto err_virtio_init;
 
 	return 0;
 
-out:
+err_virtio_init:
+	if (!hw->virtio_user_dev) {
+		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
+		if (!hw->modern)
+			rte_pci_ioport_unmap(VTPCI_IO(hw));
+	}
+err_vtpci_init:
 	rte_free(eth_dev->data->mac_addrs);
 	eth_dev->data->mac_addrs = NULL;
 	return ret;
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index adc02f96a..4468e89cb 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -678,6 +678,7 @@ vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
 
 	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
 	if (rte_pci_ioport_map(dev, 0, VTPCI_IO(hw)) < 0) {
+		rte_pci_unmap_device(dev);
 		if (dev->kdrv == RTE_KDRV_UNKNOWN &&
 		    (!dev->device.devargs ||
 		     dev->device.devargs->bus !=
-- 
2.17.1

Re: [dpdk-dev] [PATCH 3/5] net/virtio: unmap device on initialization error

[Maxime Coquelin]

On 6/5/19 11:43 AM, Tiwei Bie wrote:
> We should unmap the device when we failed to initialize the device.
> 
> Fixes: 6ba1f63b5ab0 ("virtio: support specification 1.0")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 12 +++++++++---
>   drivers/net/virtio/virtio_pci.c    |  1 +
>   2 files changed, 10 insertions(+), 3 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-dev] [PATCH 4/5] net/virtio: release port upon close

[Tiwei Bie]

Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private
resources for the port can be freed by rte_eth_dev_close().

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c            | 42 ++++++++++++-------
 .../net/virtio/virtio_user/virtio_user_dev.h  |  1 -
 drivers/net/virtio/virtio_user_ethdev.c       | 12 ++----
 3 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 46d2e4ac6..afb2ca209 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -35,6 +35,7 @@
 #include "virtio_logs.h"
 #include "virtqueue.h"
 #include "virtio_rxtx.h"
+#include "virtio_user/virtio_user_dev.h"
 
 static int eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev);
 static int  virtio_dev_configure(struct rte_eth_dev *dev);
@@ -733,6 +734,17 @@ virtio_dev_close(struct rte_eth_dev *dev)
 	vtpci_reset(hw);
 	virtio_dev_free_mbufs(dev);
 	virtio_free_queues(hw);
+
+#ifdef RTE_VIRTIO_USER
+	if (hw->virtio_user_dev)
+		virtio_user_dev_uninit(hw->virtio_user_dev);
+	else
+#endif
+	if (dev->device) {
+		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(dev));
+		if (!hw->modern)
+			rte_pci_ioport_unmap(VTPCI_IO(hw));
+	}
 }
 
 static void
@@ -1645,10 +1657,8 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 
 	hw->weak_barriers = !vtpci_with_feature(hw, VIRTIO_F_ORDER_PLATFORM);
 
-	if (!hw->virtio_user_dev) {
+	if (!hw->virtio_user_dev)
 		pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
-		rte_eth_copy_pci_info(eth_dev, pci_dev);
-	}
 
 	/* If host does not support both status and MSI-X then disable LSC */
 	if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS) &&
@@ -1840,6 +1850,12 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 		return 0;
 	}
 
+	/*
+	 * Pass the information to the rte_eth_dev_close() that it should also
+	 * release the private port resources.
+	 */
+	eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
+
 	/* Allocate memory for storing MAC addresses */
 	eth_dev->data->mac_addrs = rte_zmalloc("virtio",
 				VIRTIO_MAX_MAC_ADDRS * RTE_ETHER_ADDR_LEN, 0);
@@ -1865,6 +1881,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	if (ret < 0)
 		goto err_virtio_init;
 
+	hw->opened = true;
+
 	return 0;
 
 err_virtio_init:
@@ -1882,8 +1900,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 static int
 eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 {
-	struct virtio_hw *hw = eth_dev->data->dev_private;
-
 	PMD_INIT_FUNC_TRACE();
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
@@ -1896,12 +1912,6 @@ eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 	eth_dev->tx_pkt_burst = NULL;
 	eth_dev->rx_pkt_burst = NULL;
 
-	if (eth_dev->device) {
-		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
-		if (!hw->modern)
-			rte_pci_ioport_unmap(VTPCI_IO(hw));
-	}
-
 	PMD_INIT_LOG(DEBUG, "dev_uninit completed");
 
 	return 0;
@@ -1963,7 +1973,13 @@ static int eth_virtio_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 
 static int eth_virtio_pci_remove(struct rte_pci_device *pci_dev)
 {
-	return rte_eth_dev_pci_generic_remove(pci_dev, eth_virtio_dev_uninit);
+	int ret;
+
+	ret = rte_eth_dev_pci_generic_remove(pci_dev, eth_virtio_dev_uninit);
+	/* Port has already been released by close. */
+	if (ret == -ENODEV)
+		ret = 0;
+	return ret;
 }
 
 static struct rte_pci_driver rte_virtio_pmd = {
@@ -2123,8 +2139,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 			   DEV_RX_OFFLOAD_VLAN_STRIP))
 		hw->use_simple_rx = 0;
 
-	hw->opened = true;
-
 	return 0;
 }
 
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index db7dc607a..ad8683771 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -9,7 +9,6 @@
 #include <stdbool.h>
 #include "../virtio_pci.h"
 #include "../virtio_ring.h"
-#include "vhost.h"
 
 struct virtio_user_queue {
 	uint16_t used_idx;
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 893f48a5d..0a57db730 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -20,6 +20,7 @@
 #include "virtqueue.h"
 #include "virtio_rxtx.h"
 #include "virtio_user/virtio_user_dev.h"
+#include "virtio_user/vhost.h"
 
 #define virtio_user_get_dev(hw) \
 	((struct virtio_user_dev *)(hw)->virtio_user_dev)
@@ -697,8 +698,6 @@ virtio_user_pmd_remove(struct rte_vdev_device *vdev)
 {
 	const char *name;
 	struct rte_eth_dev *eth_dev;
-	struct virtio_hw *hw;
-	struct virtio_user_dev *dev;
 
 	if (!vdev)
 		return -EINVAL;
@@ -706,8 +705,9 @@ virtio_user_pmd_remove(struct rte_vdev_device *vdev)
 	name = rte_vdev_device_name(vdev);
 	PMD_DRV_LOG(INFO, "Un-Initializing %s", name);
 	eth_dev = rte_eth_dev_allocated(name);
+	/* Port has already been released by close. */
 	if (!eth_dev)
-		return -ENODEV;
+		return 0;
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return rte_eth_dev_release_port(eth_dev);
@@ -715,12 +715,6 @@ virtio_user_pmd_remove(struct rte_vdev_device *vdev)
 	/* make sure the device is stopped, queues freed */
 	rte_eth_dev_close(eth_dev->data->port_id);
 
-	hw = eth_dev->data->dev_private;
-	dev = hw->virtio_user_dev;
-	virtio_user_dev_uninit(dev);
-
-	rte_eth_dev_release_port(eth_dev);
-
 	return 0;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 4/5] net/virtio: release port upon close

[Maxime Coquelin]

On 6/5/19 11:43 AM, Tiwei Bie wrote:
> Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private
> resources for the port can be freed by rte_eth_dev_close().
> 
> Signed-off-by: Tiwei Bie<tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c            | 42 ++++++++++++-------
>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 -
>   drivers/net/virtio/virtio_user_ethdev.c       | 12 ++----
>   3 files changed, 31 insertions(+), 24 deletions(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-dev] [PATCH 5/5] app/testpmd: drop the workaround for virtio-user

[Tiwei Bie]

The RTE_ETH_DEV_CLOSE_REMOVE support has been enabled in
virtio-user, private resources for the port will be freed
by rte_eth_dev_close(), so there is no need to have this
workaround anymore.

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 app/test-pmd/testpmd.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4f2a431e4..281f31c08 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2456,7 +2456,6 @@ detach_port_device(portid_t port_id)
 void
 pmd_test_exit(void)
 {
-	struct rte_device *device;
 	portid_t pt_id;
 	int ret;
 	int i;
@@ -2482,18 +2481,6 @@ pmd_test_exit(void)
 			printf("\nShutting down port %d...\n", pt_id);
 			fflush(stdout);
 			close_port(pt_id);
-
-			/*
-			 * This is a workaround to fix a virtio-user issue that
-			 * requires to call clean-up routine to remove existing
-			 * socket.
-			 * This workaround valid only for testpmd, needs a fix
-			 * valid for all applications.
-			 * TODO: Implement proper resource cleanup
-			 */
-			device = rte_eth_devices[pt_id].device;
-			if (device && !strcmp(device->driver->name, "net_virtio_user"))
-				detach_port_device(pt_id);
 		}
 	}
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 5/5] app/testpmd: drop the workaround for virtio-user

[Maxime Coquelin]

On 6/5/19 11:43 AM, Tiwei Bie wrote:
> The RTE_ETH_DEV_CLOSE_REMOVE support has been enabled in
> virtio-user, private resources for the port will be freed
> by rte_eth_dev_close(), so there is no need to have this
> workaround anymore.
> 
> Signed-off-by: Tiwei Bie<tiwei.bie@intel.com>
> ---
>   app/test-pmd/testpmd.c | 13 -------------
>   1 file changed, 13 deletions(-)

Great!

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-dev] [PATCH 0/5] virtio: release port upon close

[Maxime Coquelin]

On 6/5/19 11:43 AM, Tiwei Bie wrote:
> Tiwei Bie (5):
>    net/virtio: fix queue memory leak on error
>    net/virtio: unmap port IO for legacy device
>    net/virtio: unmap device on initialization error
>    net/virtio: release port upon close
>    app/testpmd: drop the workaround for virtio-user
> 
>   app/test-pmd/testpmd.c                        | 13 -----
>   drivers/net/virtio/virtio_ethdev.c            | 50 ++++++++++++++-----
>   drivers/net/virtio/virtio_pci.c               |  1 +
>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 -
>   drivers/net/virtio/virtio_user_ethdev.c       | 12 ++---
>   5 files changed, 42 insertions(+), 35 deletions(-)
> 


Applied to dpdk-next-virtio/master.

Thanks,
Maxime