From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <michael.qiu@intel.com>
Received: from mga09.intel.com (mga09.intel.com [134.134.136.24])
 by dpdk.org (Postfix) with ESMTP id ABA76C450
 for <dev@dpdk.org>; Mon, 29 Jun 2015 17:16:08 +0200 (CEST)
Received: from fmsmga002.fm.intel.com ([10.253.24.26])
 by orsmga102.jf.intel.com with ESMTP; 29 Jun 2015 08:16:01 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.13,699,1427785200"; d="scan'208";a="752563903"
Received: from pgsmsx102.gar.corp.intel.com ([10.221.44.80])
 by fmsmga002.fm.intel.com with ESMTP; 29 Jun 2015 08:16:00 -0700
Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by
 PGSMSX102.gar.corp.intel.com (10.221.44.80) with Microsoft SMTP Server (TLS)
 id 14.3.224.2; Mon, 29 Jun 2015 23:15:59 +0800
Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.246]) by
 SHSMSX104.ccr.corp.intel.com ([169.254.5.129]) with mapi id 14.03.0224.002;
 Mon, 29 Jun 2015 23:15:58 +0800
From: "Qiu, Michael" <michael.qiu@intel.com>
To: "Iremonger, Bernard" <bernard.iremonger@intel.com>, "dev@dpdk.org"
 <dev@dpdk.org>
Thread-Topic: [dpdk-dev] [PATCH 1/2 v4] fm10k: Free queues when close port
Thread-Index: AQHQr+pPvWjWc3reIUWJMy4rOvx5Tg==
Date: Mon, 29 Jun 2015 15:15:58 +0000
Message-ID: <533710CFB86FA344BFBF2D6802E60286046A6E98@SHSMSX101.ccr.corp.intel.com>
References: <1434702717-11877-1-git-send-email-michael.qiu@intel.com>
 <1435307390-20140-1-git-send-email-michael.qiu@intel.com>
 <1435307390-20140-2-git-send-email-michael.qiu@intel.com>
 <8CEF83825BEC744B83065625E567D7C204A422AD@IRSMSX108.ger.corp.intel.com>
 <533710CFB86FA344BFBF2D6802E60286046A5FD7@SHSMSX101.ccr.corp.intel.com>
 <8CEF83825BEC744B83065625E567D7C204A42BB8@IRSMSX108.ger.corp.intel.com>
 <533710CFB86FA344BFBF2D6802E60286046A6343@SHSMSX101.ccr.corp.intel.com>
 <8CEF83825BEC744B83065625E567D7C204A42C4A@IRSMSX108.ger.corp.intel.com>
 <533710CFB86FA344BFBF2D6802E60286046A6D19@SHSMSX101.ccr.corp.intel.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-originating-ip: [10.239.127.40]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Cc: "He, Shaopeng" <shaopeng.he@intel.com>
Subject: Re: [dpdk-dev] [PATCH 1/2 v4] fm10k: Free queues when close port
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 29 Jun 2015 15:16:09 -0000

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