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