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

[Qiu, Michael]

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.

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.

2. leave close() just as the same before, all be done in uninit()
    This will works fine for hotplug, for normal case maybe has
issue(memory not released?).

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.

4. other.

For more details, you could go though this thread.


Thanks,
Michael


On 6/30/2015 9:32 AM, Qiu, Michael wrote:
> On 6/30/2015 12:42 AM, Iremonger, Bernard wrote:
>>> -----Original Message-----
>>> From: Qiu, Michael
>>> Sent: Monday, June 29, 2015 4:22 PM
>>> To: Iremonger, Bernard; dev at dpdk.org
>>> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa at igel.co.jp; Stephen
>>> Hemminger
>>> Subject: Re: [PATCH v2] librte_ether: release memory in uninit function.
>>>
>>> On 2015/6/29 18:20, Iremonger, Bernard wrote:
>>>>> -----Original Message-----
>>>>> From: Qiu, Michael
>>>>> Sent: Monday, June 29, 2015 9:55 AM
>>>>> To: Iremonger, Bernard; dev at dpdk.org
>>>>> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa at igel.co.jp; Stephen
>>>>> Hemminger
>>>>> Subject: Re: [PATCH v2] librte_ether: release memory in uninit function.
>>>>>
>>>>> On 6/26/2015 5:32 PM, Iremonger, Bernard wrote:
>>>>>> Changes in v2:
>>>>>> do not free mac_addrs and hash_mac_addrs here.
>>>>>>
>>>>>> Signed-off-by: Bernard Iremonger <bernard.iremonger at intel.com>
>>>>>> ---
>>>>>>  lib/librte_ether/rte_ethdev.c |    6 +++++-
>>>>>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/librte_ether/rte_ethdev.c
>>>>>> b/lib/librte_ether/rte_ethdev.c index e13fde5..7ae101a 100644
>>>>>> --- a/lib/librte_ether/rte_ethdev.c
>>>>>> +++ b/lib/librte_ether/rte_ethdev.c
>>>>>> @@ -369,8 +369,12 @@ rte_eth_dev_uninit(struct rte_pci_device
>>>>> *pci_dev)
>>>>>>  	/* free ether device */
>>>>>>  	rte_eth_dev_release_port(eth_dev);
>>>>>>
>>>>>> -	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>>>>>> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>>>>>> +		rte_free(eth_dev->data->rx_queues);
>>>>>> +		rte_free(eth_dev->data->tx_queues);
>>>>>>  		rte_free(eth_dev->data->dev_private);
>>>>>> +		memset(eth_dev->data, 0, sizeof(struct
>>>>> rte_eth_dev_data));
>>>>>> +	}
>>>>>>
>>>>>>  	eth_dev->pci_dev = NULL;
>>>>>>  	eth_dev->driver = NULL;
>>>>> Actually, This could be put in rte_eth_dev_close() becasue queues
>>>>> should be released when closed.
>>>>>
>>>>> Also before free dev->data->rx_queues you should make sure
>>>>> dev->data->rx_queues[i] has been freed in PMD close() function, So
>>>>> dev->data->this
>>>>> two should be better done at the same time, ether in
>>>>> rte_eth_dev_close() or in PMD close() function. For hotplug in fm10k,
>>>>> I put it in PMD close() function.
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>> Hi Michael,
>>>>
>>>> The consensus is that the rx_queue and tx_queue memory should not be
>>> released in the PMD as it is not allocated by the PMD. The memory is
>>> allocated in rte_eth_dev_rx_queue_config() and
>>> rte_eth_dev_tx_queue_config(), which are both called from
>>> rte_eth_dev_configure() which is called by the application (for example
>>> test_pmd). So it seems to make sense to free this memory  in
>>> rte_eth_dev_uninit().
>>>
>>> It really make sense to free memory in rte_ether level, but when close a port
>>> with out detached? just as stop --> close() --> quit(), the memory will not be
>>> released :)
>>>
>> In the above scenario lots of memory will not be released.
>>
>> This is why the detach() and the underlying dev_uninit() functions were introduced.
> First detach is only for hotplug, for *users do not use hotplug*, that
> scenario is the right action. So  "lots of memory will not be released"
> is issue need be fixed, actually, in fm10k driver, lots of memory has
> been released.
>
>> The dev_uninit() functions currently call dev_close()  which in turn calls dev_stop() which calls dev_clear_queues(). 
> Users do hotplug then must call stop() --> close() --> dev_uninit(), it
> works fine. But do you think it make sense to release memory when
> close() it?
>  
>> The dev_clear_queues()  function does not release the queue_memory or the queue array memory. The queue memory is now released in the dev_uninit() and the  queue array memory is released in the rte_eth_dev_uninit() function.
> That's your implementation,  make sure not all users will detach a
> device, but the right action must include close(), do you agree?
>
>> If the queue array memory is released in rte_eth_dev_close() then the release of the queue_memory will have to be moved to the dev_close() functions from the dev_uninit() functions. This will impact all the existing  PMD hotplug patches.   It will also change the existing dev_close() functionality.
> Why impact?? Actually it works fine with fm10k driver. What I concern is
> *when user do not use hotplug*, it will lead lots of memory not
> released, that unacceptable, to move release action to
> rte_eth_dev_close()  is just a   suggestion by me, I think *the solution
> should cover both scenario*, am I right?
>
>
>> My preference is to leave the existing dev_close() functions unchanged as far as possible and to do what needs to be done in the dev_uninit() functions.
>>
>> We probably need the view of the maintainers as to whether this should be done in the close() or uninit() functions.  
>>
>> Regards,
>>
>> Bernard.
>>
>>  
>>
>>
>>
>>
>