Re: [dpdk-dev] [PATCH v3 1/2] ethdev: free all common data when releasing port

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/15/18 2:20 AM, Thomas Monjalon wrote:
> This is a clean-up of common ethdev data freeing.
> All data freeing are moved to rte_eth_dev_release_port()
> and done only in case of primary process.
>
> It is probably fixing some memory leaks for PMDs which were
> not freeing all data.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

<...>

> diff --git a/drivers/net/cxgbe/cxgbe_main.c b/drivers/net/cxgbe/cxgbe_main.c
> index a135df9c7..88dc851f8 100644
> --- a/drivers/net/cxgbe/cxgbe_main.c
> +++ b/drivers/net/cxgbe/cxgbe_main.c
> @@ -1710,12 +1710,7 @@ void cxgbe_close(struct adapter *adapter)
>   			if (pi->viid != 0)
>   				t4_free_vi(adapter, adapter->mbox,
>   					   adapter->pf, 0, pi->viid);
> -			rte_free(pi->eth_dev->data->mac_addrs);
> -			/* Skip first port since it'll be freed by DPDK stack */
> -			if (i) {
> -				rte_free(pi->eth_dev->data->dev_private);
> -				rte_eth_dev_release_port(pi->eth_dev);
> -			}
> +			rte_eth_dev_release_port(pi->eth_dev);
>   		}
>   		adapter->flags &= ~FULL_INIT_DONE;
>   	}
> @@ -1918,14 +1913,7 @@ int cxgbe_probe(struct adapter *adapter)
>   		if (pi->viid != 0)
>   			t4_free_vi(adapter, adapter->mbox, adapter->pf,
>   				   0, pi->viid);
> -		/* Skip first port since it'll be de-allocated by DPDK */
> -		if (i == 0)
> -			continue;
> -		if (pi->eth_dev) {
> -			if (pi->eth_dev->data->dev_private)
> -				rte_free(pi->eth_dev->data->dev_private);
> -			rte_eth_dev_release_port(pi->eth_dev);
> -		}
> +		rte_eth_dev_release_port(pi->eth_dev);
>   	}
>   
>   	if (adapter->flags & FW_OK)
> diff --git a/drivers/net/cxgbe/cxgbevf_main.c b/drivers/net/cxgbe/cxgbevf_main.c
> index 4214d0312..6223e1250 100644
> --- a/drivers/net/cxgbe/cxgbevf_main.c
> +++ b/drivers/net/cxgbe/cxgbevf_main.c
> @@ -282,14 +282,7 @@ int cxgbevf_probe(struct adapter *adapter)
>   		if (pi->viid != 0)
>   			t4_free_vi(adapter, adapter->mbox, adapter->pf,
>   				   0, pi->viid);
> -		/* Skip first port since it'll be de-allocated by DPDK */
> -		if (i == 0)
> -			continue;
> -		if (pi->eth_dev) {
> -			if (pi->eth_dev->data->dev_private)
> -				rte_free(pi->eth_dev->data->dev_private);
> -			rte_eth_dev_release_port(pi->eth_dev);
> -		}
> +		rte_eth_dev_release_port(pi->eth_dev);
>   	}
>   	return -err;
>   }

Above changes looks frightening. I've add cxgbe maintainer in CC.


> diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c
> index 0fd264e25..f01ed36b6 100644
> --- a/drivers/net/softnic/rte_eth_softnic.c
> +++ b/drivers/net/softnic/rte_eth_softnic.c
> @@ -556,7 +556,6 @@ static int
>   pmd_remove(struct rte_vdev_device *vdev)
>   {
>   	struct rte_eth_dev *dev = NULL;
> -	struct pmd_internals *p;
>   
>   	if (!vdev)
>   		return -EINVAL;
> @@ -567,12 +566,11 @@ pmd_remove(struct rte_vdev_device *vdev)
>   	dev = rte_eth_dev_allocated(rte_vdev_device_name(vdev));
>   	if (dev == NULL)
>   		return -ENODEV;
> -	p = dev->data->dev_private;
>   
>   	/* Free device data structures*/
> -	rte_free(dev->data);
> +	pmd_free(dev->data->dev_private);
> +	dev->data->dev_private = NULL;
>   	rte_eth_dev_release_port(dev);
> -	pmd_free(p);
>   
>   	return 0;
>   }

The above basically violates approach used everywhere else. It is OK to 
go, but should be reconsidered.

> diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
> index f652596f4..23257e986 100644
> --- a/lib/librte_ethdev/rte_ethdev_pci.h
> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> @@ -135,17 +135,6 @@ rte_eth_dev_pci_allocate(struct rte_pci_device *dev, size_t private_data_size)
>   static inline void
>   rte_eth_dev_pci_release(struct rte_eth_dev *eth_dev)
>   {
> -	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> -		rte_free(eth_dev->data->dev_private);
> -
> -	eth_dev->data->dev_private = NULL;
> -
> -	/*
> -	 * Secondary process will check the name to attach.
> -	 * Clear this field to avoid attaching a released ports.
> -	 */
> -	eth_dev->data->name[0] = '\0';
> -
>   	eth_dev->device = NULL;
>   	eth_dev->intr_handle = NULL;

Not directly related to the patch, but I don't understand why does
rte_eth_dev_pci_release () exist? It does nothing PCI specific.
May be just for symmetry to rte_eth_dev_pci_allocate().
Why is intr_handle set to NULL above? Is it PCI specific?
It does not look so, since failsafe uses it.

In general it looks good, really big work, but ideally it should be
acked by cxgbe maintainer as well.

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>