diff mbox series

[v2] ethdev: complete closing of port

Message ID 20181009221732.17377-1-thomas@monjalon.net (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] ethdev: complete closing of port |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Thomas Monjalon Oct. 9, 2018, 10:17 p.m. UTC 
  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(-)

Comments

Andrew Rybchenko Oct. 10, 2018, 6:15 a.m. UTC | #1 
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.
Thomas Monjalon Oct. 10, 2018, 7:44 a.m. UTC | #2 
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.
Andrew Rybchenko Oct. 10, 2018, 7:50 a.m. UTC | #3 
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)
Thomas Monjalon Oct. 10, 2018, 8:39 a.m. UTC | #4 
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.
Andrew Rybchenko Oct. 10, 2018, 3:01 p.m. UTC | #5 
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)

> 	- 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.
>
>
Thomas Monjalon Oct. 10, 2018, 4:43 p.m. UTC | #6 
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.

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


> > 	- 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.
Andrew Rybchenko Oct. 10, 2018, 6:01 p.m. UTC | #7 
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.
Thomas Monjalon Oct. 10, 2018, 7:03 p.m. UTC | #8 
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.
diff mbox series

Patch

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);
+
+	rte_eth_dev_release_port(dev);
 }
 
 int
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index a8942ff88..378c01afa 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1846,8 +1846,7 @@  int rte_eth_dev_set_link_down(uint16_t port_id);
 
 /**
  * Close a stopped Ethernet device. The device cannot be restarted!
- * The function frees all resources except for needed by the
- * closed state.
+ * The function frees all port resources.
  *
  * @param port_id
  *   The port identifier of the Ethernet device.