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

["Qiu, Michael" <michael.qiu@intel.com>]

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