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

Qiu, Michael michael.qiu at intel.com
Tue Jun 30 03:31:39 CEST 2015


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.
>
>  
>
>
>
>



More information about the dev mailing list