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