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