[dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function.

[Iremonger, Bernard]

> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp]
> Sent: Wednesday, July 8, 2015 4:48 AM
> To: Iremonger, Bernard; dev at dpdk.org
> Cc: Qiu, Michael; thomas.monjalon at 6wind.com; Ananyev, Konstantin;
> Stephen Hemminger; Zhang, Helin; Chen, Jing D; Neil Horman
> Subject: Re: [dpdk-dev] [PATCH v2] librte_ether: release memory in uninit
> function.
> 
> On 2015/07/07 19:53, Iremonger, Bernard wrote:
> >> -----Original Message-----
> >> From: Tetsuya Mukawa [mailto:mukawa at igel.co.jp]
> >> Sent: Tuesday, July 7, 2015 4:38 AM
> >> To: dev at dpdk.org
> >> Cc: Qiu, Michael; Iremonger, Bernard; thomas.monjalon at 6wind.com;
> >> Ananyev, Konstantin; Stephen Hemminger; Zhang, Helin; Chen, Jing D;
> >> Neil Horman
> >> Subject: Re: [dpdk-dev] [PATCH v2] librte_ether: release memory in
> >> uninit function.
> >>
> >> On 2015/07/06 20:35, Qiu, Michael wrote:
> >>> Hi, all
> >>>
> >>> As we has gap on the memory release action to be done in which step,
> >>> I appreciate all your comments on this patch.
> >>>
> >>> Currently, the correct quit sequence for testpmd is stop() --->
> >>> port_stop() --> port_close() --> quit(). This will lead lots of
> >>> memory not released by default, like queues.
> > There is also the scenario (outside of testpmd) where an application can call
> rte_eth_dev_close() or rte_eth_dev_detach() which calls
> rte_eth_dev_uninit() directly from an application.
> > In these cases the memory allocated in the EAL layer should be released in
> both rte_eth_dev_close() and rte_eth_dev_detach().
> 
> Hi Bernard,
> 
> All DPDK applications that uses hotpluggingmust call rte_eth_dev_stop() and
> rte_eth_dev_close()APIs before detaching ports.
> (This is described in DPDK documentation) Could we ignore applications that
> only calls rte_eth_dev_detach()?
> 
> > This patch is intended to address the rte_eth_dev_detach() case only
> (hotplug case).
> > Perhaps there should be a separate patch to address the
> rte_eth_dev_close() case.
> 
> We all needs to have consensus about how to implement finalization code in
> close() and uninit().
> Probably one of options will be implementing finalization code in
> close() as much as possible.
> 
> Tetsuya,

Hi Tetsuya,
Testpmd enforces the dev_stop(), dev_close() and dev_detach() sequence.
There is no reason why an application cannot call dev_detach() directly. 

During internal review of PMD dev_uninit()  functions it was decided to ensure that this sequence was adhered to by calling dev_close() which calls dev_stop() from the dev_uninit() function.
In the PMD hotplug patches the following calling sequence is implemented:
dev_uninit() calls dev_close() calls dev_stop().
At present most of the finalization code is implemented in dev_close() and dev_stop().
The dev_uninit() functions implement what is left of the finalization code.

Regards,

Bernard.

> 
> > Regards,
> >
> > Bernard.
> >
> >>> We have 3 potential proposals(normal case means without hotplug):
> >>>
> >>> 1. Those memory release in close()  other left in uninit()
> >>>     This will work fine for both hotplug case and normal case.
> >> +1
> >>
> >> I assume that once close() is called, we cannot start the port again
> >> without hotplugging.
> >> This is my premise.
> >>
> >> It might be good that we move finalization code to close() as much as
> >> possible, because of followings.
> >> 1. Anyway, once close() is called, we cannot start the port again. So
> >> it should be ok to free resources in close().
> >> 2. Non-hotplugging applications can release resources if we implement
> >> finalization code to close().
> >>
> >> Also I guess Stephen's suggestion is important to keep code clean.
> >> It may be good that 'dev->data->rx/tx_queues[queue_id]' are freed in
> >> rx/tx_queue_release() of each PMD, and rx/tx_queue_release() will be
> >> called by rte_eth_dev_close(). Also 'dev->data->rx/tx_queues[]'
> >> should be freed in ethdev.c.
> >>
> >>> 3. Combine close() and uninit(), only one will be left.
> >>>     This will work fine for both hotplug case and normal case. But
> >>> it may change a lot, such as logic.
> >> I guess this will be second option.  But I agree we need to change a
> >> lot. Also after enabling hotplug by default, when someone only wants
> >> to close the port, it will be impossible.
> >>
> >> Regards,
> >> Tetsuya