Re: [dpdk-dev] [PATCH v2] ethdev: complete closing of port

[Thomas Monjalon <thomas@monjalon.net>]

10/10/2018 09:50, Andrew Rybchenko:
> On 10/10/18 10:44 AM, Thomas Monjalon wrote:
> > 10/10/2018 08:15, Andrew Rybchenko:
> >> On 10/10/18 1:17 AM, Thomas Monjalon wrote:
> >>> After closing a port, it cannot be restarted.
> >>> So there is no reason to not free all associated resources.
> >>>
> >>> The last step was done with rte_eth_dev_detach() which is deprecated.
> >>> Instead of blindly removing the associated rte_device, the driver should
> >>> check if no more port (ethdev, cryptodev, etc) is open for the device.
> >>>
> >>> The last ethdev freeing (dev_private and final release), which were done
> >>> by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close().
> >>>
> >>> If the driver is trying to free the port again, the function
> >>> rte_eth_dev_release_port() will abort with -ENODEV error.
> >>>
> >>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >>> ---
> >>>    lib/librte_ethdev/rte_ethdev.c | 6 ++++++
> >>>    lib/librte_ethdev/rte_ethdev.h | 3 +--
> >>>    2 files changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> >>> index ed83e5954..3062dc711 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.c
> >>> +++ b/lib/librte_ethdev/rte_ethdev.c
> >>> @@ -506,6 +506,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
> >>>    {
> >>>    	if (eth_dev == NULL)
> >>>    		return -EINVAL;
> >>> +	if (eth_dev->state == RTE_ETH_DEV_UNUSED)
> >>> +		return -ENODEV;
> >>>    
> >>>    	rte_eth_dev_shared_data_prepare();
> >>>    
> >>> @@ -1441,6 +1443,10 @@ rte_eth_dev_close(uint16_t port_id)
> >>>    	dev->data->nb_tx_queues = 0;
> >>>    	rte_free(dev->data->tx_queues);
> >>>    	dev->data->tx_queues = NULL;
> >>> +
> >>> +	rte_free(dev->data->dev_private);
> >> It is used by, for example, PCI device uninit functions.
> >> What does guarantee that uninit is done and we can free the private data.
> > The state of the port is set to UNUSED and the name is NULL.
> > So nobody should try to use it anymore.
> > There are already some checks before calling uninit functions.
> > For instance, in rte_eth_dev_pci_generic_remove(),
> > rte_eth_dev_allocated() will return NULL and won't call uninit function.
> 
> The questions are:
> Is application allowed to call the function? When?
> Who calls uninit in this case? (What does guarantee that uninit is done 
> before close)

So far, everything is allowed:
	- The application can close a port and remove the rte_device later.
	- The application can remove the rte_device and expect the PMD is closing
		associated ports.

In other words, when rte_device is removed, the ports should be closed
by the PMD, except if the application has already closed the ports.
It means ethdev port close is optional, but EAL removal is always required.
The behaviour is not changed.

If we want to go further, we could change the behaviour of the close op,
by asking the PMD to remove the rte_device automatically if all associated
ports are closed. It would allow the application to manage only ports
at ethdev layer without bothering with low-level EAL management.
We can think about it as a planned change for next releases.