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

[Thomas Monjalon <thomas@monjalon.net>]

10/10/2018 20:01, Andrew Rybchenko:
> On 10.10.2018 19:43, Thomas Monjalon wrote:
> > 10/10/2018 17:01, Andrew Rybchenko:
> >> On 10/10/18 11:39 AM, Thomas Monjalon wrote:
> >>> 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.
> >> If the patch is applied, close frees dev_private which is used by uninit.
> >> So, uninit must be done first. Who does it?
> >> (it looks like I'm missing something obvious, but still can't find it)
> > Yes, you missed my explanation above :)
> > Let me try again:
> >
> > rte_eth_dev_release_port() does 3 things:
> > 	- RTE_ETH_EVENT_DESTROY notification
> > 	- state = RTE_ETH_DEV_UNUSED
> > 	- memset data to 0
> >
> > Because of state == RTE_ETH_DEV_UNUSED and data->name == NULL,
> > you should not try to use data->dev_private.
> > Before calling uninit function, the dev is retrieved by name:
> >
> >      ethdev = rte_eth_dev_allocated(ethdev->data->name);
> >      if (!ethdev)
> >          return -ENODEV;
> >
> > In our case, it will be -ENODEV, which is a good return when trying
> > to release a closed port.
> 
> Yes, it replies on the question why dev_uninit is not called
> upon device removal after close. But it does not reply on
> the question what does call dev_uninit before/during dev_close.

Maybe I don't understand your question correctly.
dev_uninit is not called during dev_close.
However, as suggested by Ferruh, it would be cleaner to call dev_close
instead of dev_uninit which should do the same thing.
Currently, most of the PMDs expect both dev_close and dev_uninit to be
called in order to completely free a port.

The call tree to reach dev_uninit is:
	[rte_eal_hotplug_remove]
		rte_dev_remove
			bus.unplug
				driver.remove
					for port not closed
						dev_uninit
					free rte_device resources

In a next step, I would suggest to drop dev_uninit,
and call dev_close instead.