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

[Andrew Rybchenko <arybchenko@solarflare.com>]

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.

> Now I am thinking that PMDs could ignore this -ENODEV error:
> it is OK to free the rte_device with ethdev port already closed.

I'll apply all 4 patch series tomorrow, add printout to dev_uninit,
build, run testpmd, do close and check if printout appears.
I hope it will reply on my question. I'll come back when I do and
have results.

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