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

[Nithin Dabilpuram <nithind1988@gmail.com>]

On Mon, Apr 29, 2019 at 10:30:00PM +0200, Thomas Monjalon wrote:
> 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.
I have a related query. Even after rte_eth_dev_close(), we have to call
rte_eal_hotplug_remove() to free up common resource if any 
and remove the device from application ?
And rte_eal_hotplug_remove() would still call drivers .remove function for this.
> 
> > 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.
> 
>