Re: [dpdk-dev] CALL to eth PMD maintainers: complete closing of port

[Thomas Monjalon <thomas@monjalon.net>]

29/04/2019 18:51, Ferruh Yigit:
> On 4/18/2019 11:59 AM, Thomas Monjalon wrote:
> > Hi all,
> > 
> > Since DPDK 18.11, the behaviour of the close operation is changed
> > if RTE_ETH_DEV_CLOSE_REMOVE is enabled in the driver:
> > port is released (i.e. totally freed and data erased) on close.
> > This new behaviour is enabled per driver for a migration period.
> > 
> > Looking at the code, you can see these comments:
> > /* old behaviour: only free queue arrays */
> > RTE_ETHDEV_LOG(DEBUG, "Port closing is using an old behaviour.\n"
> > 	"The driver %s should migrate to the new behaviour.\n",
> > /* new behaviour: send event + reset state + free all data */
> > 
> > You can find an advice in the commit:
> > 	http://git.dpdk.org/dpdk/commit/?id=23ea57a2a
> > "
> > When enabling RTE_ETH_DEV_CLOSE_REMOVE,
> > the PMD must free all its private resources for the port,
> > in its dev_close function.
> > It is advised to call the dev_close function in the remove function
> > in order to support removing a device without closing its ports.
> > "
> > 
> > It would be great to complete this migration for the next LTS
> > version, which will be 19.11.
> > It means the work should be ideally finished during the summer.
> 
> I would like to detail a little more what needs to be done, mainly for the sake
> of the discussion, please comment if something missing/wrong.
> 
> There are two exit paths for driver:
> 1) Hotplug remove the device
>    rte_dev_remove() OR rte_eal_hotplug_remove()
> 
> 2) Application exit:
>    rte_eth_dev_close()
> 
> 
> (1) can be called without ethdev stop and close functions called, so the path
> should cover those functions.
> And in the PMD entry point in this path is .remove() function. In this .remove()
> function PMD should:
> - stop forwarding
> - clean PMD private resources (dev_close() ? )

yes

> - clean ethdev generic resources (rte_eth_dev_release_port() ? )

already done in dev_close thanks to RTE_ETH_DEV_CLOSE_REMOVE

> - remove device resources, which already done by remove APIs

There can be some specific PMD private resources,
not cleaned when closing a port, to clean in "remove".

> (2) ethdev won't be usable after "rte_eth_dev_close()" and it should clear PMD
> private and ethdev generic resources.
> With RTE_ETH_DEV_CLOSE_REMOVE flag 'rte_eth_dev_close()' will free generic
> ethdev resources (rte_eth_dev_release_port()) so PMD specific '.dev_close()' should:
> - stop forwarding
> - clean PMD private resources

yes, but only port-related resources,
not those common with other ports or features of the device.

> If agreed on above, the task is:
> - '.dev_close()'
>   - stop forwarding
>   - clean PMD private resources
> 
> - '.remove()'
>   - call '.dev_close()'

Yes, looks right.

> A question is if application should call 'rte_eth_dev_stop()' explicitly before
> close or should it be part of close? For (2) it makes sense app call the stop
> explicitly, but for device remove it is not clear who to stop the forwarding,
> for this case 'dev_close()' stopping forwarding makes things easier.

"stop" is a prerequisite for "close" anyway.
In most cases, the app should manage "stop" itself to properly free
related resources.
The question is to know whether we return an error on "close"
if the port is not stopped, or we stop it implicitly.
The API says:
	 * Close a stopped Ethernet device. The device cannot be restarted!
We may explicit the behaviour if the port is not stopped.