Re: [dpdk-dev] [PATCH v5 6/6] ethdev: complete closing of port

[Thomas Monjalon <thomas@monjalon.net>]

18/10/2018 10:33, Andrew Rybchenko:
> On 10/18/18 4:24 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 which were done by rte_eth_dev_detach(),
> > are now done at the end of rte_eth_dev_close().
> >
> > Some drivers does not allocate MAC addresses dynamically or separately.
> > In those cases, the pointer is set to NULL, in order to avoid wrongly
> > freeing them in rte_eth_dev_release_port().
> >
> > A closed port will have the state RTE_ETH_DEV_UNUSED which is
> > considered as invalid by rte_eth_dev_is_valid_port().
> > So validity is not checked anymore for closed ports in testpmd.
> >
> > 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>
> 
> I've tested the patch series together with [1].

There are 2 use cases to consider:
	1/ rte_eth_dev_close + rte_dev_remove
	2/ rte_dev_remove without prior close

> As I expected it makes problems and resource leaks if
> rte_eth_dev_close() is used.
> 
> Everything is OK if I do port stop and detach (with net/sfc
> patch which does close from uninit).

This is the case #2.
It requires some PMD updates to avoid the leaks.
It was buggy in previous releases too, because it was not managing
devices with multiple ports.
I think we should continue recommending the use case #1 in 18.11,
and let time for PMDs to be fixed for use case #2.
It PMDs can be fixed in 18.11, it is even better.

> If I do port stop, close and detach, the last one returns
> error since the device already released and net/sfc uninit
> is never called.

This is the case #1.
The bug is assuming that the port is not freed when removing the device.
We should just skip closed ports without any error.
I can fix it in a v6.

> Basically it should be one function which is called in both
> cases: dev_close or pci_device remove. Similar changes
> should be done in many PCI drivers.

Yes, dev_close must be called for all non-closed ports of a PCI device
to remove.
Usually, PMDs have an implementation of dev_close which is different
of the uninit function called with rte_eth_dev_pci_generic_remove()
or rte_eth_dev_destroy(). They have no good reason to be different:
	typedef int (*ethdev_uninit_t)(struct rte_eth_dev *ethdev);
	typedef void (*eth_dev_close_t)(struct rte_eth_dev *dev);

> If I drop the patch, everything seems to be work fine
> from the first sight.
> May be it should be removed from the patchset and
> considered separately.

No, I think I should just fix the use case #1,
and let PMDs fixing the use case #2 when they have a chance.
In my opinion, the use case #2 was never advertised before,
so it is just opening the way for the support of this use case.

> [1] http://patches.dpdk.org/project/dpdk/list/?series=1966