[dpdk-dev,1/2,v4] fm10k: Free queues when close port

  When close a port, lots of memory should be released,
such as software rings, queues, etc.

Signed-off-by: Michael Qiu <michael.qiu@intel.com>
---
 drivers/net/fm10k/fm10k_ethdev.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)
  

> -----Original Message-----
> From: Qiu, Michael
> Sent: Friday, June 26, 2015 9:30 AM
> To: dev@dpdk.org
> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael
> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port
> 
> When close a port, lots of memory should be released, such as software
> rings, queues, etc.
> 
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
Hi Michael,

There are 2 comments inline 

>  drivers/net/fm10k/fm10k_ethdev.c | 37
> +++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
> b/drivers/net/fm10k/fm10k_ethdev.c
> index 406c350..eba7228 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> @@ -65,6 +65,8 @@ static void
>  fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool add);
> static void  fm10k_MACVLAN_remove_all(struct rte_eth_dev *dev);
> +static void fm10k_tx_queue_release(void *queue); static void
> +fm10k_rx_queue_release(void *queue);
> 
>  static void
>  fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@
> fm10k_dev_stop(struct rte_eth_dev *dev)
> 
>  	PMD_INIT_FUNC_TRACE();
> 
> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
> -		fm10k_dev_tx_queue_stop(dev, i);
> +	if (dev->data->tx_queues)
> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> +			fm10k_dev_tx_queue_stop(dev, i);
> 
> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
> -		fm10k_dev_rx_queue_stop(dev, i);
> +	if (dev->data->rx_queues)
> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> +			fm10k_dev_rx_queue_stop(dev, i);
> +}
> +
> +static void
> +fm10k_dev_queue_release(struct rte_eth_dev *dev) {
> +	int i;
> +
> +	PMD_INIT_FUNC_TRACE();
> +
> +	if (dev->data->tx_queues) {
> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> +			fm10k_tx_queue_release(dev->data-
> >tx_queues[i]);
> +		rte_free(dev->data->tx_queues);
> +		dev->data->tx_queues = NULL;

The memory for dev->data->tx_queues  is not allocated in the fm10k PMD,
so it should probably not be released here.
I have submitted a patch today for rte_eth_dev.c  to do this.  
/dev/patchwork/patch/5829/

> +		dev->data->nb_tx_queues = 0;
> +	}
> +
> +	if (dev->data->rx_queues) {
> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> +			fm10k_rx_queue_release(dev->data-
> >rx_queues[i]);
> +		rte_free(dev->data->rx_queues);
> +		dev->data->rx_queues = NULL;

The memory for dev->data->rx_queues  is not allocated in the fm10k PMD,
so it should probably not be released here.
I have submitted a patch today for rte_eth_dev.c  to do this.  
/dev/patchwork/patch/5829/

Regards,

Bernard.


> +		dev->data->nb_rx_queues = 0;
> +	}
>  }
> 
>  static void
> @@ -828,6 +856,7 @@ fm10k_dev_close(struct rte_eth_dev *dev)
>  	/* Stop mailbox service first */
>  	fm10k_close_mbx_service(hw);
>  	fm10k_dev_stop(dev);
> +	fm10k_dev_queue_release(dev);
>  	fm10k_stop_hw(hw);
>  }
> 
> --
> 1.9.3
  

On 6/26/2015 7:02 PM, Iremonger, Bernard wrote:
>> -----Original Message-----
>> From: Qiu, Michael
>> Sent: Friday, June 26, 2015 9:30 AM
>> To: dev@dpdk.org
>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael
>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port
>>
>> When close a port, lots of memory should be released, such as software
>> rings, queues, etc.
>>
>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>> ---
> Hi Michael,
>
> There are 2 comments inline 
>
>>  drivers/net/fm10k/fm10k_ethdev.c | 37
>> +++++++++++++++++++++++++++++++++----
>>  1 file changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
>> b/drivers/net/fm10k/fm10k_ethdev.c
>> index 406c350..eba7228 100644
>> --- a/drivers/net/fm10k/fm10k_ethdev.c
>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
>> @@ -65,6 +65,8 @@ static void
>>  fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool add);
>> static void  fm10k_MACVLAN_remove_all(struct rte_eth_dev *dev);
>> +static void fm10k_tx_queue_release(void *queue); static void
>> +fm10k_rx_queue_release(void *queue);
>>
>>  static void
>>  fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@
>> fm10k_dev_stop(struct rte_eth_dev *dev)
>>
>>  	PMD_INIT_FUNC_TRACE();
>>
>> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
>> -		fm10k_dev_tx_queue_stop(dev, i);
>> +	if (dev->data->tx_queues)
>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
>> +			fm10k_dev_tx_queue_stop(dev, i);
>>
>> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
>> -		fm10k_dev_rx_queue_stop(dev, i);
>> +	if (dev->data->rx_queues)
>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
>> +			fm10k_dev_rx_queue_stop(dev, i);
>> +}
>> +
>> +static void
>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) {
>> +	int i;
>> +
>> +	PMD_INIT_FUNC_TRACE();
>> +
>> +	if (dev->data->tx_queues) {
>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
>> +			fm10k_tx_queue_release(dev->data-
>>> tx_queues[i]);
>> +		rte_free(dev->data->tx_queues);
>> +		dev->data->tx_queues = NULL;
> The memory for dev->data->tx_queues  is not allocated in the fm10k PMD,
> so it should probably not be released here.
> I have submitted a patch today for rte_eth_dev.c  to do this.  
> /dev/patchwork/patch/5829/
>
>> +		dev->data->nb_tx_queues = 0;
>> +	}
>> +
>> +	if (dev->data->rx_queues) {
>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
>> +			fm10k_rx_queue_release(dev->data-
>>> rx_queues[i]);
>> +		rte_free(dev->data->rx_queues);
>> +		dev->data->rx_queues = NULL;
> The memory for dev->data->rx_queues  is not allocated in the fm10k PMD,
> so it should probably not be released here.
> I have submitted a patch today for rte_eth_dev.c  to do this.  
> /dev/patchwork/patch/5829/

Is it a good idea?  What about to close the port for twice at a time?
I think it is better to do it in rte_eth_dev_close(), I will give the
comments to you.

Thanks,
Michael

> Regards,
>
> Bernard.
>
>
>> +		dev->data->nb_rx_queues = 0;
>> +	}
>>  }
>>
>>  static void
>> @@ -828,6 +856,7 @@ fm10k_dev_close(struct rte_eth_dev *dev)
>>  	/* Stop mailbox service first */
>>  	fm10k_close_mbx_service(hw);
>>  	fm10k_dev_stop(dev);
>> +	fm10k_dev_queue_release(dev);
>>  	fm10k_stop_hw(hw);
>>  }
>>
>> --
>> 1.9.3
>
  

> -----Original Message-----
> From: Qiu, Michael
> Sent: Monday, June 29, 2015 9:17 AM
> To: Iremonger, Bernard; dev@dpdk.org
> Cc: Chen, Jing D; He, Shaopeng
> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port
> 
> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote:
> >> -----Original Message-----
> >> From: Qiu, Michael
> >> Sent: Friday, June 26, 2015 9:30 AM
> >> To: dev@dpdk.org
> >> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael
> >> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port
> >>
> >> When close a port, lots of memory should be released, such as
> >> software rings, queues, etc.
> >>
> >> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> >> ---
> > Hi Michael,
> >
> > There are 2 comments inline
> >
> >>  drivers/net/fm10k/fm10k_ethdev.c | 37
> >> +++++++++++++++++++++++++++++++++----
> >>  1 file changed, 33 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
> >> b/drivers/net/fm10k/fm10k_ethdev.c
> >> index 406c350..eba7228 100644
> >> --- a/drivers/net/fm10k/fm10k_ethdev.c
> >> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> >> @@ -65,6 +65,8 @@ static void
> >>  fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool
> >> add); static void  fm10k_MACVLAN_remove_all(struct rte_eth_dev
> *dev);
> >> +static void fm10k_tx_queue_release(void *queue); static void
> >> +fm10k_rx_queue_release(void *queue);
> >>
> >>  static void
> >>  fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@
> >> fm10k_dev_stop(struct rte_eth_dev *dev)
> >>
> >>  	PMD_INIT_FUNC_TRACE();
> >>
> >> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
> >> -		fm10k_dev_tx_queue_stop(dev, i);
> >> +	if (dev->data->tx_queues)
> >> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> >> +			fm10k_dev_tx_queue_stop(dev, i);
> >>
> >> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
> >> -		fm10k_dev_rx_queue_stop(dev, i);
> >> +	if (dev->data->rx_queues)
> >> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> >> +			fm10k_dev_rx_queue_stop(dev, i);
> >> +}
> >> +
> >> +static void
> >> +fm10k_dev_queue_release(struct rte_eth_dev *dev) {
> >> +	int i;
> >> +
> >> +	PMD_INIT_FUNC_TRACE();
> >> +
> >> +	if (dev->data->tx_queues) {
> >> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> >> +			fm10k_tx_queue_release(dev->data-
> >>> tx_queues[i]);
> >> +		rte_free(dev->data->tx_queues);
> >> +		dev->data->tx_queues = NULL;
> > The memory for dev->data->tx_queues  is not allocated in the fm10k
> > PMD, so it should probably not be released here.
> > I have submitted a patch today for rte_eth_dev.c  to do this.
> > /dev/patchwork/patch/5829/
> >
> >> +		dev->data->nb_tx_queues = 0;
> >> +	}
> >> +
> >> +	if (dev->data->rx_queues) {
> >> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> >> +			fm10k_rx_queue_release(dev->data-
> >>> rx_queues[i]);
> >> +		rte_free(dev->data->rx_queues);
> >> +		dev->data->rx_queues = NULL;
> > The memory for dev->data->rx_queues  is not allocated in the fm10k
> > PMD, so it should probably not be released here.
> > I have submitted a patch today for rte_eth_dev.c  to do this.
> > /dev/patchwork/patch/5829/
> 
> Is it a good idea?  What about to close the port for twice at a time?
> I think it is better to do it in rte_eth_dev_close(), I will give the comments to
> you.
> 
> Thanks,
> Michael

Hi Michael,
Could you take a look at the comments on http://dpdk.org/dev/patchwork/patch/5829/
The consensus is that memory should be freed in the component that allocated it.
In my pmd hotplug patches I have used a flag to ensure that dev_close is not called twice.
In the e1000 patch I have added a stopped flag to struct e1000_adapter.   
http://dpdk.org/dev/patchwork/patch/5655/

Regards,

Bernard.

<snip>
  

On 6/29/2015 4:57 PM, Iremonger, Bernard wrote:
>> -----Original Message-----
>> From: Qiu, Michael
>> Sent: Monday, June 29, 2015 9:17 AM
>> To: Iremonger, Bernard; dev@dpdk.org
>> Cc: Chen, Jing D; He, Shaopeng
>> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port
>>
>> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote:
>>>> -----Original Message-----
>>>> From: Qiu, Michael
>>>> Sent: Friday, June 26, 2015 9:30 AM
>>>> To: dev@dpdk.org
>>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael
>>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port
>>>>
>>>> When close a port, lots of memory should be released, such as
>>>> software rings, queues, etc.
>>>>
>>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>>>> ---
>>> Hi Michael,
>>>
>>> There are 2 comments inline
>>>
>>>>  drivers/net/fm10k/fm10k_ethdev.c | 37
>>>> +++++++++++++++++++++++++++++++++----
>>>>  1 file changed, 33 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
>>>> b/drivers/net/fm10k/fm10k_ethdev.c
>>>> index 406c350..eba7228 100644
>>>> --- a/drivers/net/fm10k/fm10k_ethdev.c
>>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
>>>> @@ -65,6 +65,8 @@ static void
>>>>  fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool
>>>> add); static void  fm10k_MACVLAN_remove_all(struct rte_eth_dev
>> *dev);
>>>> +static void fm10k_tx_queue_release(void *queue); static void
>>>> +fm10k_rx_queue_release(void *queue);
>>>>
>>>>  static void
>>>>  fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@
>>>> fm10k_dev_stop(struct rte_eth_dev *dev)
>>>>
>>>>  	PMD_INIT_FUNC_TRACE();
>>>>
>>>> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>> -		fm10k_dev_tx_queue_stop(dev, i);
>>>> +	if (dev->data->tx_queues)
>>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>> +			fm10k_dev_tx_queue_stop(dev, i);
>>>>
>>>> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>> -		fm10k_dev_rx_queue_stop(dev, i);
>>>> +	if (dev->data->rx_queues)
>>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>> +			fm10k_dev_rx_queue_stop(dev, i);
>>>> +}
>>>> +
>>>> +static void
>>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) {
>>>> +	int i;
>>>> +
>>>> +	PMD_INIT_FUNC_TRACE();
>>>> +
>>>> +	if (dev->data->tx_queues) {
>>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>> +			fm10k_tx_queue_release(dev->data-
>>>>> tx_queues[i]);
>>>> +		rte_free(dev->data->tx_queues);
>>>> +		dev->data->tx_queues = NULL;
>>> The memory for dev->data->tx_queues  is not allocated in the fm10k
>>> PMD, so it should probably not be released here.
>>> I have submitted a patch today for rte_eth_dev.c  to do this.
>>> /dev/patchwork/patch/5829/
>>>
>>>> +		dev->data->nb_tx_queues = 0;
>>>> +	}
>>>> +
>>>> +	if (dev->data->rx_queues) {
>>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>> +			fm10k_rx_queue_release(dev->data-
>>>>> rx_queues[i]);
>>>> +		rte_free(dev->data->rx_queues);
>>>> +		dev->data->rx_queues = NULL;
>>> The memory for dev->data->rx_queues  is not allocated in the fm10k
>>> PMD, so it should probably not be released here.
>>> I have submitted a patch today for rte_eth_dev.c  to do this.
>>> /dev/patchwork/patch/5829/
>> Is it a good idea?  What about to close the port for twice at a time?
>> I think it is better to do it in rte_eth_dev_close(), I will give the comments to
>> you.
>>
>> Thanks,
>> Michael
> Hi Michael,
> Could you take a look at the comments on http://dpdk.org/dev/patchwork/patch/5829/

Hi, Bernard

I have give comments on it.

> The consensus is that memory should be freed in the component that allocated it.
> In my pmd hotplug patches I have used a flag to ensure that dev_close is not called twice.
> In the e1000 patch I have added a stopped flag to struct e1000_adapter.   
> http://dpdk.org/dev/patchwork/patch/5655/



I reviewed your patch about ixgbe and fvl before. But forget e1000.

In my logic, when dev->data->rx_queues is NULL, that means this device
has been closed before. What else, we even do not care about whether it
has been closed or not, when close() function be called, all memory
should be freed if exist am I right?

So, check  dev->data->rx_queues whether it is NULL will be recommend in
close function, only this could avoid unsafe situations for pointer.

Thanks,
Michael
> Regards,
>
> Bernard.
>
> <snip>
>
>
>
  

> -----Original Message-----
> From: Qiu, Michael
> Sent: Monday, June 29, 2015 10:21 AM
> To: Iremonger, Bernard; dev@dpdk.org
> Cc: Chen, Jing D; He, Shaopeng
> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port
> 
> On 6/29/2015 4:57 PM, Iremonger, Bernard wrote:
> >> -----Original Message-----
> >> From: Qiu, Michael
> >> Sent: Monday, June 29, 2015 9:17 AM
> >> To: Iremonger, Bernard; dev@dpdk.org
> >> Cc: Chen, Jing D; He, Shaopeng
> >> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port
> >>
> >> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote:
> >>>> -----Original Message-----
> >>>> From: Qiu, Michael
> >>>> Sent: Friday, June 26, 2015 9:30 AM
> >>>> To: dev@dpdk.org
> >>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael
> >>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port
> >>>>
> >>>> When close a port, lots of memory should be released, such as
> >>>> software rings, queues, etc.
> >>>>
> >>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> >>>> ---
> >>> Hi Michael,
> >>>
> >>> There are 2 comments inline
> >>>
> >>>>  drivers/net/fm10k/fm10k_ethdev.c | 37
> >>>> +++++++++++++++++++++++++++++++++----
> >>>>  1 file changed, 33 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
> >>>> b/drivers/net/fm10k/fm10k_ethdev.c
> >>>> index 406c350..eba7228 100644
> >>>> --- a/drivers/net/fm10k/fm10k_ethdev.c
> >>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> >>>> @@ -65,6 +65,8 @@ static void
> >>>>  fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool
> >>>> add); static void  fm10k_MACVLAN_remove_all(struct rte_eth_dev
> >> *dev);
> >>>> +static void fm10k_tx_queue_release(void *queue); static void
> >>>> +fm10k_rx_queue_release(void *queue);
> >>>>
> >>>>  static void
> >>>>  fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@
> >>>> fm10k_dev_stop(struct rte_eth_dev *dev)
> >>>>
> >>>>  	PMD_INIT_FUNC_TRACE();
> >>>>
> >>>> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
> >>>> -		fm10k_dev_tx_queue_stop(dev, i);
> >>>> +	if (dev->data->tx_queues)
> >>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> >>>> +			fm10k_dev_tx_queue_stop(dev, i);
> >>>>
> >>>> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
> >>>> -		fm10k_dev_rx_queue_stop(dev, i);
> >>>> +	if (dev->data->rx_queues)
> >>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> >>>> +			fm10k_dev_rx_queue_stop(dev, i); }
> >>>> +
> >>>> +static void
> >>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) {
> >>>> +	int i;
> >>>> +
> >>>> +	PMD_INIT_FUNC_TRACE();
> >>>> +
> >>>> +	if (dev->data->tx_queues) {
> >>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> >>>> +			fm10k_tx_queue_release(dev->data-
> >>>>> tx_queues[i]);
> >>>> +		rte_free(dev->data->tx_queues);
> >>>> +		dev->data->tx_queues = NULL;
> >>> The memory for dev->data->tx_queues  is not allocated in the fm10k
> >>> PMD, so it should probably not be released here.
> >>> I have submitted a patch today for rte_eth_dev.c  to do this.
> >>> /dev/patchwork/patch/5829/
> >>>
> >>>> +		dev->data->nb_tx_queues = 0;
> >>>> +	}
> >>>> +
> >>>> +	if (dev->data->rx_queues) {
> >>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> >>>> +			fm10k_rx_queue_release(dev->data-
> >>>>> rx_queues[i]);
> >>>> +		rte_free(dev->data->rx_queues);
> >>>> +		dev->data->rx_queues = NULL;
> >>> The memory for dev->data->rx_queues  is not allocated in the fm10k
> >>> PMD, so it should probably not be released here.
> >>> I have submitted a patch today for rte_eth_dev.c  to do this.
> >>> /dev/patchwork/patch/5829/
> >> Is it a good idea?  What about to close the port for twice at a time?
> >> I think it is better to do it in rte_eth_dev_close(), I will give the
> >> comments to you.
> >>
> >> Thanks,
> >> Michael
> > Hi Michael,
> > Could you take a look at the comments on
> > http://dpdk.org/dev/patchwork/patch/5829/
> 
> Hi, Bernard
> 
> I have give comments on it.
> 
> > The consensus is that memory should be freed in the component that
> allocated it.
> > In my pmd hotplug patches I have used a flag to ensure that dev_close is
> not called twice.
> > In the e1000 patch I have added a stopped flag to struct e1000_adapter.
> > http://dpdk.org/dev/patchwork/patch/5655/
> 
> 
> 
> I reviewed your patch about ixgbe and fvl before. But forget e1000.
> 
> In my logic, when dev->data->rx_queues is NULL, that means this device has
> been closed before. What else, we even do not care about whether it has
> been closed or not, when close() function be called, all memory should be
> freed if exist am I right?
> 
> So, check  dev->data->rx_queues whether it is NULL will be recommend in
> close function, only this could avoid unsafe situations for pointer.
> 
> Thanks,
> Michael

Hi Michael,

In most of the pmd's memory is allocated in the dev_init()functions and released in the dev_uninit() functions. The dev_uninit() functions call dev_close(), so either way the memory is released.
The point I am trying to make is that the rx_queue and tx_queue memory is not allocated by the pmd and so it should not be freed by the pmd (please see comments on 
http://dpdk.org/dev/patchwork/patch/5790/)
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.
  

Hi Michael,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael
> Sent: Monday, June 29, 2015 10:21 AM
> To: Iremonger, Bernard; dev@dpdk.org
> Cc: He, Shaopeng
> Subject: Re: [dpdk-dev] [PATCH 1/2 v4] fm10k: Free queues when close port
> 
> On 6/29/2015 4:57 PM, Iremonger, Bernard wrote:
> >> -----Original Message-----
> >> From: Qiu, Michael
> >> Sent: Monday, June 29, 2015 9:17 AM
> >> To: Iremonger, Bernard; dev@dpdk.org
> >> Cc: Chen, Jing D; He, Shaopeng
> >> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port
> >>
> >> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote:
> >>>> -----Original Message-----
> >>>> From: Qiu, Michael
> >>>> Sent: Friday, June 26, 2015 9:30 AM
> >>>> To: dev@dpdk.org
> >>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael
> >>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port
> >>>>
> >>>> When close a port, lots of memory should be released, such as
> >>>> software rings, queues, etc.
> >>>>
> >>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> >>>> ---
> >>> Hi Michael,
> >>>
> >>> There are 2 comments inline
> >>>
> >>>>  drivers/net/fm10k/fm10k_ethdev.c | 37
> >>>> +++++++++++++++++++++++++++++++++----
> >>>>  1 file changed, 33 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
> >>>> b/drivers/net/fm10k/fm10k_ethdev.c
> >>>> index 406c350..eba7228 100644
> >>>> --- a/drivers/net/fm10k/fm10k_ethdev.c
> >>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> >>>> @@ -65,6 +65,8 @@ static void
> >>>>  fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool
> >>>> add); static void  fm10k_MACVLAN_remove_all(struct rte_eth_dev
> >> *dev);
> >>>> +static void fm10k_tx_queue_release(void *queue); static void
> >>>> +fm10k_rx_queue_release(void *queue);
> >>>>
> >>>>  static void
> >>>>  fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@
> >>>> fm10k_dev_stop(struct rte_eth_dev *dev)
> >>>>
> >>>>  	PMD_INIT_FUNC_TRACE();
> >>>>
> >>>> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
> >>>> -		fm10k_dev_tx_queue_stop(dev, i);
> >>>> +	if (dev->data->tx_queues)
> >>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> >>>> +			fm10k_dev_tx_queue_stop(dev, i);
> >>>>
> >>>> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
> >>>> -		fm10k_dev_rx_queue_stop(dev, i);
> >>>> +	if (dev->data->rx_queues)
> >>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> >>>> +			fm10k_dev_rx_queue_stop(dev, i);
> >>>> +}
> >>>> +
> >>>> +static void
> >>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) {
> >>>> +	int i;
> >>>> +
> >>>> +	PMD_INIT_FUNC_TRACE();
> >>>> +
> >>>> +	if (dev->data->tx_queues) {
> >>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
> >>>> +			fm10k_tx_queue_release(dev->data-
> >>>>> tx_queues[i]);
> >>>> +		rte_free(dev->data->tx_queues);
> >>>> +		dev->data->tx_queues = NULL;
> >>> The memory for dev->data->tx_queues  is not allocated in the fm10k
> >>> PMD, so it should probably not be released here.
> >>> I have submitted a patch today for rte_eth_dev.c  to do this.
> >>> /dev/patchwork/patch/5829/
> >>>
> >>>> +		dev->data->nb_tx_queues = 0;
> >>>> +	}
> >>>> +
> >>>> +	if (dev->data->rx_queues) {
> >>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
> >>>> +			fm10k_rx_queue_release(dev->data-
> >>>>> rx_queues[i]);
> >>>> +		rte_free(dev->data->rx_queues);
> >>>> +		dev->data->rx_queues = NULL;
> >>> The memory for dev->data->rx_queues  is not allocated in the fm10k
> >>> PMD, so it should probably not be released here.
> >>> I have submitted a patch today for rte_eth_dev.c  to do this.
> >>> /dev/patchwork/patch/5829/
> >> Is it a good idea?  What about to close the port for twice at a time?
> >> I think it is better to do it in rte_eth_dev_close(), I will give the comments to
> >> you.
> >>
> >> Thanks,
> >> Michael
> > Hi Michael,
> > Could you take a look at the comments on http://dpdk.org/dev/patchwork/patch/5829/
> 
> Hi, Bernard
> 
> I have give comments on it.
> 
> > The consensus is that memory should be freed in the component that allocated it.
> > In my pmd hotplug patches I have used a flag to ensure that dev_close is not called twice.
> > In the e1000 patch I have added a stopped flag to struct e1000_adapter.
> > http://dpdk.org/dev/patchwork/patch/5655/
> 
> 
> 
> I reviewed your patch about ixgbe and fvl before. But forget e1000.
> 
> In my logic, when dev->data->rx_queues is NULL, that means this device
> has been closed before. What else, we even do not care about whether it
> has been closed or not, when close() function be called, all memory
> should be freed if exist am I right?
> 
> So, check  dev->data->rx_queues whether it is NULL will be recommend in
> close function, only this could avoid unsafe situations for pointer.


It seems you are mixing 2 things there:
Contents of  dev->data->rx_queues[] is mamanged by each PMD, and yes should be alloced and freed by each PMD.
dev->data->rx_queues[] itself is allocated/reallocated and should be freed by rte_ethdev layer.
PMD, in theory, simly doesn't know how it was allocated.
Konstantin


> 
> Thanks,
> Michael
> > Regards,
> >
> > Bernard.
> >
> > <snip>
> >
> >
> >
  

On 2015/6/29 17:54, Iremonger, Bernard wrote:
>> -----Original Message-----
>> From: Qiu, Michael
>> Sent: Monday, June 29, 2015 10:21 AM
>> To: Iremonger, Bernard; dev@dpdk.org
>> Cc: Chen, Jing D; He, Shaopeng
>> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port
>>
>> On 6/29/2015 4:57 PM, Iremonger, Bernard wrote:
>>>> -----Original Message-----
>>>> From: Qiu, Michael
>>>> Sent: Monday, June 29, 2015 9:17 AM
>>>> To: Iremonger, Bernard; dev@dpdk.org
>>>> Cc: Chen, Jing D; He, Shaopeng
>>>> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port
>>>>
>>>> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote:
>>>>>> -----Original Message-----
>>>>>> From: Qiu, Michael
>>>>>> Sent: Friday, June 26, 2015 9:30 AM
>>>>>> To: dev@dpdk.org
>>>>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael
>>>>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port
>>>>>>
>>>>>> When close a port, lots of memory should be released, such as
>>>>>> software rings, queues, etc.
>>>>>>
>>>>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>>>>>> ---
>>>>> Hi Michael,
>>>>>
>>>>> There are 2 comments inline
>>>>>
>>>>>>  drivers/net/fm10k/fm10k_ethdev.c | 37
>>>>>> +++++++++++++++++++++++++++++++++----
>>>>>>  1 file changed, 33 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
>>>>>> b/drivers/net/fm10k/fm10k_ethdev.c
>>>>>> index 406c350..eba7228 100644
>>>>>> --- a/drivers/net/fm10k/fm10k_ethdev.c
>>>>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
>>>>>> @@ -65,6 +65,8 @@ static void
>>>>>>  fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool
>>>>>> add); static void  fm10k_MACVLAN_remove_all(struct rte_eth_dev
>>>> *dev);
>>>>>> +static void fm10k_tx_queue_release(void *queue); static void
>>>>>> +fm10k_rx_queue_release(void *queue);
>>>>>>
>>>>>>  static void
>>>>>>  fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@
>>>>>> fm10k_dev_stop(struct rte_eth_dev *dev)
>>>>>>
>>>>>>  	PMD_INIT_FUNC_TRACE();
>>>>>>
>>>>>> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>>>> -		fm10k_dev_tx_queue_stop(dev, i);
>>>>>> +	if (dev->data->tx_queues)
>>>>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>>>> +			fm10k_dev_tx_queue_stop(dev, i);
>>>>>>
>>>>>> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>>>> -		fm10k_dev_rx_queue_stop(dev, i);
>>>>>> +	if (dev->data->rx_queues)
>>>>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>>>> +			fm10k_dev_rx_queue_stop(dev, i); }
>>>>>> +
>>>>>> +static void
>>>>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) {
>>>>>> +	int i;
>>>>>> +
>>>>>> +	PMD_INIT_FUNC_TRACE();
>>>>>> +
>>>>>> +	if (dev->data->tx_queues) {
>>>>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>>>> +			fm10k_tx_queue_release(dev->data-
>>>>>>> tx_queues[i]);
>>>>>> +		rte_free(dev->data->tx_queues);
>>>>>> +		dev->data->tx_queues = NULL;
>>>>> The memory for dev->data->tx_queues  is not allocated in the fm10k
>>>>> PMD, so it should probably not be released here.
>>>>> I have submitted a patch today for rte_eth_dev.c  to do this.
>>>>> /dev/patchwork/patch/5829/
>>>>>
>>>>>> +		dev->data->nb_tx_queues = 0;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (dev->data->rx_queues) {
>>>>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>>>> +			fm10k_rx_queue_release(dev->data-
>>>>>>> rx_queues[i]);
>>>>>> +		rte_free(dev->data->rx_queues);
>>>>>> +		dev->data->rx_queues = NULL;
>>>>> The memory for dev->data->rx_queues  is not allocated in the fm10k
>>>>> PMD, so it should probably not be released here.
>>>>> I have submitted a patch today for rte_eth_dev.c  to do this.
>>>>> /dev/patchwork/patch/5829/
>>>> Is it a good idea?  What about to close the port for twice at a time?
>>>> I think it is better to do it in rte_eth_dev_close(), I will give the
>>>> comments to you.
>>>>
>>>> Thanks,
>>>> Michael
>>> Hi Michael,
>>> Could you take a look at the comments on
>>> http://dpdk.org/dev/patchwork/patch/5829/
>> Hi, Bernard
>>
>> I have give comments on it.
>>
>>> The consensus is that memory should be freed in the component that
>> allocated it.
>>> In my pmd hotplug patches I have used a flag to ensure that dev_close is
>> not called twice.
>>> In the e1000 patch I have added a stopped flag to struct e1000_adapter.
>>> http://dpdk.org/dev/patchwork/patch/5655/
>>
>>
>> I reviewed your patch about ixgbe and fvl before. But forget e1000.
>>
>> In my logic, when dev->data->rx_queues is NULL, that means this device has
>> been closed before. What else, we even do not care about whether it has
>> been closed or not, when close() function be called, all memory should be
>> freed if exist am I right?
>>
>> So, check  dev->data->rx_queues whether it is NULL will be recommend in
>> close function, only this could avoid unsafe situations for pointer.
>>
>> Thanks,
>> Michael
> Hi Michael,
>
> In most of the pmd's memory is allocated in the dev_init()functions and released in the dev_uninit() functions. The dev_uninit() functions call dev_close(), so either way the memory is released.

Yes, but consider that, without hotplug, users just stop a port and
close it. then what happens? the memory does not released!
So that's why I recommend to release the memory on close() function, it
could be in EAL level like rte_eth_dev_close().

Maybe my understand is wrong, please point me out :)

Thanks,
Michael

> The point I am trying to make is that the rx_queue and tx_queue memory is not allocated by the pmd and so it should not be freed by the pmd (please see comments on 
> http://dpdk.org/dev/patchwork/patch/5790/)
> 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 19:08, Ananyev, Konstantin wrote:
> Hi Michael,
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Qiu, Michael
>> Sent: Monday, June 29, 2015 10:21 AM
>> To: Iremonger, Bernard; dev@dpdk.org
>> Cc: He, Shaopeng
>> Subject: Re: [dpdk-dev] [PATCH 1/2 v4] fm10k: Free queues when close port
>>
>> On 6/29/2015 4:57 PM, Iremonger, Bernard wrote:
>>>> -----Original Message-----
>>>> From: Qiu, Michael
>>>> Sent: Monday, June 29, 2015 9:17 AM
>>>> To: Iremonger, Bernard; dev@dpdk.org
>>>> Cc: Chen, Jing D; He, Shaopeng
>>>> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port
>>>>
>>>> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote:
>>>>>> -----Original Message-----
>>>>>> From: Qiu, Michael
>>>>>> Sent: Friday, June 26, 2015 9:30 AM
>>>>>> To: dev@dpdk.org
>>>>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael
>>>>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port
>>>>>>
>>>>>> When close a port, lots of memory should be released, such as
>>>>>> software rings, queues, etc.
>>>>>>
>>>>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>>>>>> ---
>>>>> Hi Michael,
>>>>>
>>>>> There are 2 comments inline
>>>>>
>>>>>>  drivers/net/fm10k/fm10k_ethdev.c | 37
>>>>>> +++++++++++++++++++++++++++++++++----
>>>>>>  1 file changed, 33 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
>>>>>> b/drivers/net/fm10k/fm10k_ethdev.c
>>>>>> index 406c350..eba7228 100644
>>>>>> --- a/drivers/net/fm10k/fm10k_ethdev.c
>>>>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
>>>>>> @@ -65,6 +65,8 @@ static void
>>>>>>  fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool
>>>>>> add); static void  fm10k_MACVLAN_remove_all(struct rte_eth_dev
>>>> *dev);
>>>>>> +static void fm10k_tx_queue_release(void *queue); static void
>>>>>> +fm10k_rx_queue_release(void *queue);
>>>>>>
>>>>>>  static void
>>>>>>  fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@
>>>>>> fm10k_dev_stop(struct rte_eth_dev *dev)
>>>>>>
>>>>>>  	PMD_INIT_FUNC_TRACE();
>>>>>>
>>>>>> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>>>> -		fm10k_dev_tx_queue_stop(dev, i);
>>>>>> +	if (dev->data->tx_queues)
>>>>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>>>> +			fm10k_dev_tx_queue_stop(dev, i);
>>>>>>
>>>>>> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>>>> -		fm10k_dev_rx_queue_stop(dev, i);
>>>>>> +	if (dev->data->rx_queues)
>>>>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>>>> +			fm10k_dev_rx_queue_stop(dev, i);
>>>>>> +}
>>>>>> +
>>>>>> +static void
>>>>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) {
>>>>>> +	int i;
>>>>>> +
>>>>>> +	PMD_INIT_FUNC_TRACE();
>>>>>> +
>>>>>> +	if (dev->data->tx_queues) {
>>>>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>>>> +			fm10k_tx_queue_release(dev->data-
>>>>>>> tx_queues[i]);
>>>>>> +		rte_free(dev->data->tx_queues);
>>>>>> +		dev->data->tx_queues = NULL;
>>>>> The memory for dev->data->tx_queues  is not allocated in the fm10k
>>>>> PMD, so it should probably not be released here.
>>>>> I have submitted a patch today for rte_eth_dev.c  to do this.
>>>>> /dev/patchwork/patch/5829/
>>>>>
>>>>>> +		dev->data->nb_tx_queues = 0;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (dev->data->rx_queues) {
>>>>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>>>> +			fm10k_rx_queue_release(dev->data-
>>>>>>> rx_queues[i]);
>>>>>> +		rte_free(dev->data->rx_queues);
>>>>>> +		dev->data->rx_queues = NULL;
>>>>> The memory for dev->data->rx_queues  is not allocated in the fm10k
>>>>> PMD, so it should probably not be released here.
>>>>> I have submitted a patch today for rte_eth_dev.c  to do this.
>>>>> /dev/patchwork/patch/5829/
>>>> Is it a good idea?  What about to close the port for twice at a time?
>>>> I think it is better to do it in rte_eth_dev_close(), I will give the comments to
>>>> you.
>>>>
>>>> Thanks,
>>>> Michael
>>> Hi Michael,
>>> Could you take a look at the comments on http://dpdk.org/dev/patchwork/patch/5829/
>> Hi, Bernard
>>
>> I have give comments on it.
>>
>>> The consensus is that memory should be freed in the component that allocated it.
>>> In my pmd hotplug patches I have used a flag to ensure that dev_close is not called twice.
>>> In the e1000 patch I have added a stopped flag to struct e1000_adapter.
>>> http://dpdk.org/dev/patchwork/patch/5655/
>>
>>
>> I reviewed your patch about ixgbe and fvl before. But forget e1000.
>>
>> In my logic, when dev->data->rx_queues is NULL, that means this device
>> has been closed before. What else, we even do not care about whether it
>> has been closed or not, when close() function be called, all memory
>> should be freed if exist am I right?
>>
>> So, check  dev->data->rx_queues whether it is NULL will be recommend in
>> close function, only this could avoid unsafe situations for pointer.
>
> It seems you are mixing 2 things there:
> Contents of  dev->data->rx_queues[] is mamanged by each PMD, and yes should be alloced and freed by each PMD.
> dev->data->rx_queues[] itself is allocated/reallocated and should be freed by rte_ethdev layer.
> PMD, in theory, simly doesn't know how it was allocated.

I really do not mix these, what I mean is when you try to release the

dev->data->rx_queues you must confirm two things:
1.  dev->data->rx_queues is not NULL
2.  dev->data->rx_queues[i] is already freed

Also I agree dev->data->rx_queues is better freed in rte_ethdev layer.

And I will remove free dev->data->rx_queues memory in PMD, and based on bernard's new patch.
 
Also in PMD, to check dev->data->rx_queues to see if could release dev->data->rx_queues[i] when close()

Thanks,
Michael

> Konstantin
>
>
>> Thanks,
>> Michael
>>> Regards,
>>>
>>> Bernard.
>>>
>>> <snip>
>>>
>>>
>>>
>
  

On 2015/6/29 22:58, Qiu, Michael wrote:
> On 2015/6/29 17:54, Iremonger, Bernard wrote:
>>> -----Original Message-----
>>> From: Qiu, Michael
>>> Sent: Monday, June 29, 2015 10:21 AM
>>> To: Iremonger, Bernard; dev@dpdk.org
>>> Cc: Chen, Jing D; He, Shaopeng
>>> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port
>>>
>>> On 6/29/2015 4:57 PM, Iremonger, Bernard wrote:
>>>>> -----Original Message-----
>>>>> From: Qiu, Michael
>>>>> Sent: Monday, June 29, 2015 9:17 AM
>>>>> To: Iremonger, Bernard; dev@dpdk.org
>>>>> Cc: Chen, Jing D; He, Shaopeng
>>>>> Subject: Re: [PATCH 1/2 v4] fm10k: Free queues when close port
>>>>>
>>>>> On 6/26/2015 7:02 PM, Iremonger, Bernard wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Qiu, Michael
>>>>>>> Sent: Friday, June 26, 2015 9:30 AM
>>>>>>> To: dev@dpdk.org
>>>>>>> Cc: Chen, Jing D; He, Shaopeng; Iremonger, Bernard; Qiu, Michael
>>>>>>> Subject: [PATCH 1/2 v4] fm10k: Free queues when close port
>>>>>>>
>>>>>>> When close a port, lots of memory should be released, such as
>>>>>>> software rings, queues, etc.
>>>>>>>
>>>>>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>>>>>>> ---
>>>>>> Hi Michael,
>>>>>>
>>>>>> There are 2 comments inline
>>>>>>
>>>>>>>  drivers/net/fm10k/fm10k_ethdev.c | 37
>>>>>>> +++++++++++++++++++++++++++++++++----
>>>>>>>  1 file changed, 33 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
>>>>>>> b/drivers/net/fm10k/fm10k_ethdev.c
>>>>>>> index 406c350..eba7228 100644
>>>>>>> --- a/drivers/net/fm10k/fm10k_ethdev.c
>>>>>>> +++ b/drivers/net/fm10k/fm10k_ethdev.c
>>>>>>> @@ -65,6 +65,8 @@ static void
>>>>>>>  fm10k_MAC_filter_set(struct rte_eth_dev *dev, const u8 *mac, bool
>>>>>>> add); static void  fm10k_MACVLAN_remove_all(struct rte_eth_dev
>>>>> *dev);
>>>>>>> +static void fm10k_tx_queue_release(void *queue); static void
>>>>>>> +fm10k_rx_queue_release(void *queue);
>>>>>>>
>>>>>>>  static void
>>>>>>>  fm10k_mbx_initlock(struct fm10k_hw *hw) @@ -809,11 +811,37 @@
>>>>>>> fm10k_dev_stop(struct rte_eth_dev *dev)
>>>>>>>
>>>>>>>  	PMD_INIT_FUNC_TRACE();
>>>>>>>
>>>>>>> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>>>>> -		fm10k_dev_tx_queue_stop(dev, i);
>>>>>>> +	if (dev->data->tx_queues)
>>>>>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>>>>> +			fm10k_dev_tx_queue_stop(dev, i);
>>>>>>>
>>>>>>> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>>>>> -		fm10k_dev_rx_queue_stop(dev, i);
>>>>>>> +	if (dev->data->rx_queues)
>>>>>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>>>>> +			fm10k_dev_rx_queue_stop(dev, i); }
>>>>>>> +
>>>>>>> +static void
>>>>>>> +fm10k_dev_queue_release(struct rte_eth_dev *dev) {
>>>>>>> +	int i;
>>>>>>> +
>>>>>>> +	PMD_INIT_FUNC_TRACE();
>>>>>>> +
>>>>>>> +	if (dev->data->tx_queues) {
>>>>>>> +		for (i = 0; i < dev->data->nb_tx_queues; i++)
>>>>>>> +			fm10k_tx_queue_release(dev->data-
>>>>>>>> tx_queues[i]);
>>>>>>> +		rte_free(dev->data->tx_queues);
>>>>>>> +		dev->data->tx_queues = NULL;
>>>>>> The memory for dev->data->tx_queues  is not allocated in the fm10k
>>>>>> PMD, so it should probably not be released here.
>>>>>> I have submitted a patch today for rte_eth_dev.c  to do this.
>>>>>> /dev/patchwork/patch/5829/
>>>>>>
>>>>>>> +		dev->data->nb_tx_queues = 0;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (dev->data->rx_queues) {
>>>>>>> +		for (i = 0; i < dev->data->nb_rx_queues; i++)
>>>>>>> +			fm10k_rx_queue_release(dev->data-
>>>>>>>> rx_queues[i]);
>>>>>>> +		rte_free(dev->data->rx_queues);
>>>>>>> +		dev->data->rx_queues = NULL;
>>>>>> The memory for dev->data->rx_queues  is not allocated in the fm10k
>>>>>> PMD, so it should probably not be released here.
>>>>>> I have submitted a patch today for rte_eth_dev.c  to do this.
>>>>>> /dev/patchwork/patch/5829/
>>>>> Is it a good idea?  What about to close the port for twice at a time?
>>>>> I think it is better to do it in rte_eth_dev_close(), I will give the
>>>>> comments to you.
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>> Hi Michael,
>>>> Could you take a look at the comments on
>>>> http://dpdk.org/dev/patchwork/patch/5829/
>>> Hi, Bernard
>>>
>>> I have give comments on it.
>>>
>>>> The consensus is that memory should be freed in the component that
>>> allocated it.
>>>> In my pmd hotplug patches I have used a flag to ensure that dev_close is
>>> not called twice.
>>>> In the e1000 patch I have added a stopped flag to struct e1000_adapter.
>>>> http://dpdk.org/dev/patchwork/patch/5655/
>>>
>>> I reviewed your patch about ixgbe and fvl before. But forget e1000.
>>>
>>> In my logic, when dev->data->rx_queues is NULL, that means this device has
>>> been closed before. What else, we even do not care about whether it has
>>> been closed or not, when close() function be called, all memory should be
>>> freed if exist am I right?
>>>
>>> So, check  dev->data->rx_queues whether it is NULL will be recommend in
>>> close function, only this could avoid unsafe situations for pointer.
>>>
>>> Thanks,
>>> Michael
>> Hi Michael,
>>
>> In most of the pmd's memory is allocated in the dev_init()functions and released in the dev_uninit() functions. The dev_uninit() functions call dev_close(), so either way the memory is released.
> Yes, but consider that, without hotplug, users just stop a port and
> close it. then what happens? the memory does not released!
> So that's why I recommend to release the memory on close() function, it
> could be in EAL level like rte_eth_dev_close().

Sorry, here EAL should be rte_ether

>
> Maybe my understand is wrong, please point me out :)
>
> Thanks,
> Michael
>
>> The point I am trying to make is that the rx_queue and tx_queue memory is not allocated by the pmd and so it should not be freed by the pmd (please see comments on 
>> http://dpdk.org/dev/patchwork/patch/5790/)
>> 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.
>>
>>
>>
>