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

  Changes in v2:
do not free mac_addrs and hash_mac_addrs here.

Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 lib/librte_ether/rte_ethdev.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
  

> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Friday, June 26, 2015 10:33 AM
> To: dev@dpdk.org
> Cc: Zhang, Helin; Ananyev, Konstantin; Qiu, Michael; mukawa@igel.co.jp; Iremonger, Bernard
> Subject: [PATCH v2] librte_ether: release memory in uninit function.
> 
> Changes in v2:
> do not free mac_addrs and hash_mac_addrs here.
> 
> Signed-off-by: Bernard Iremonger <bernard.iremonger@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;
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 1.7.4.1
  

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

> -----Original Message-----
> From: Qiu, Michael
> Sent: Monday, June 29, 2015 9:55 AM
> To: Iremonger, Bernard; dev@dpdk.org
> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa@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@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 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().

Regards,

Bernard.
  

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@dpdk.org
>> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa@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@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 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 :)

Thanks,
Michael
 
>
> Regards,
>
> Bernard.
>
>
>
>
>
  

> -----Original Message-----
> From: Qiu, Michael
> Sent: Monday, June 29, 2015 4:22 PM
> To: Iremonger, Bernard; dev@dpdk.org
> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa@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@dpdk.org
> >> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa@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@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.
The dev_uninit() functions currently call dev_close()  which in turn calls dev_stop() which calls dev_clear_queues(). 
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.

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.

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.
  

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@dpdk.org
>> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa@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@dpdk.org
>>>> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa@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@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.
>
>  
>
>
>
>
  

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@dpdk.org
>>> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa@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@dpdk.org
>>>>> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa@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@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.
>>
>>  
>>
>>
>>
>>
>
  

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

> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Tuesday, July 7, 2015 4:38 AM
> To: dev@dpdk.org
> Cc: Qiu, Michael; Iremonger, Bernard; thomas.monjalon@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().
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.

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
  

On 2015/07/07 19:53, Iremonger, Bernard wrote:
>> -----Original Message-----
>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>> Sent: Tuesday, July 7, 2015 4:38 AM
>> To: dev@dpdk.org
>> Cc: Qiu, Michael; Iremonger, Bernard; thomas.monjalon@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,

> 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
  

> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Wednesday, July 8, 2015 4:48 AM
> To: Iremonger, Bernard; dev@dpdk.org
> Cc: Qiu, Michael; thomas.monjalon@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@igel.co.jp]
> >> Sent: Tuesday, July 7, 2015 4:38 AM
> >> To: dev@dpdk.org
> >> Cc: Qiu, Michael; Iremonger, Bernard; thomas.monjalon@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
  

2015-07-08 09:49, Iremonger, Bernard:
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> > On 2015/07/07 19:53, Iremonger, Bernard wrote:
> > > From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> > >> 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.

It appears that the API is not defined.
None of these assumptions is written in rte_ethdev.h.
Please continue the discussion by submitting a patch fixing the doxygen
comments of these functions.
How an application developper is supposed to use these functions without
having a clear explanation of their roles in doxygen?
  

On 2015/07/08 18:59, Thomas Monjalon wrote:
> 2015-07-08 09:49, Iremonger, Bernard:
>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>>> On 2015/07/07 19:53, Iremonger, Bernard wrote:
>>>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>>>>> 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.
> It appears that the API is not defined.
> None of these assumptions is written in rte_ethdev.h.
> Please continue the discussion by submitting a patch fixing the doxygen
> comments of these functions.
> How an application developper is supposed to use these functions without
> having a clear explanation of their roles in doxygen?

Yes, this assumption is only written in DPDK user's guide.
So we should add description to doxygen. I will take care of it.

Anyway, I will add description to doxygen like user's guide.
 (A) stop() and close() must be called before detach().
 (B) close() releases all most all resources.
Probably Bernard and Michael patches are fit for above restrictions.

It may be good to add below in DPDK-2.2, though supporting below feature
will be more user friendly.
 (C) Even if stop() and close() are not called, detach() will take care
of it.

Probably, it's not so much time left before releasing DPDK-2.1.
So how about keeping current restriction written in user's guide, then
add a new feature in DPDK-2.2.

Regards,
Tetsuya
  

On 7/9/2015 11:32 AM, Tetsuya Mukawa wrote:
> On 2015/07/08 18:59, Thomas Monjalon wrote:
>> 2015-07-08 09:49, Iremonger, Bernard:
>>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>>>> On 2015/07/07 19:53, Iremonger, Bernard wrote:
>>>>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>>>>>> 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.

[.../...]

>> It appears that the API is not defined.
>> None of these assumptions is written in rte_ethdev.h.
>> Please continue the discussion by submitting a patch fixing the doxygen
>> comments of these functions.
>> How an application developper is supposed to use these functions without
>> having a clear explanation of their roles in doxygen?
> Yes, this assumption is only written in DPDK user's guide.
> So we should add description to doxygen. I will take care of it.
>
> Anyway, I will add description to doxygen like user's guide.
>  (A) stop() and close() must be called before detach().
>  (B) close() releases all most all resources.
> Probably Bernard and Michael patches are fit for above restrictions.
>
> It may be good to add below in DPDK-2.2, though supporting below feature
> will be more user friendly.
>  (C) Even if stop() and close() are not called, detach() will take care
> of it.
>
> Probably, it's not so much time left before releasing DPDK-2.1.
> So how about keeping current restriction written in user's guide, then
> add a new feature in DPDK-2.2.

Yes, agree.

Thanks,
Michael
> Regards,
> Tetsuya
>