From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id A31EE5A95 for ; Tue, 30 Jun 2015 03:31:44 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 29 Jun 2015 18:31:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,374,1432623600"; d="scan'208";a="755504557" Received: from pgsmsx108.gar.corp.intel.com ([10.221.44.103]) by orsmga002.jf.intel.com with ESMTP; 29 Jun 2015 18:31:42 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by PGSMSX108.gar.corp.intel.com (10.221.44.103) with Microsoft SMTP Server (TLS) id 14.3.224.2; Tue, 30 Jun 2015 09:31:40 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.246]) by shsmsx102.ccr.corp.intel.com ([169.254.2.165]) with mapi id 14.03.0224.002; Tue, 30 Jun 2015 09:31:39 +0800 From: "Qiu, Michael" To: "Iremonger, Bernard" , "dev@dpdk.org" Thread-Topic: [PATCH v2] librte_ether: release memory in uninit function. Thread-Index: AQHQr/MWrMnfGSFKL02MpXHseHw6cg== Date: Tue, 30 Jun 2015 01:31:39 +0000 Message-ID: <533710CFB86FA344BFBF2D6802E60286046A75A4@SHSMSX101.ccr.corp.intel.com> References: <1435311160-8679-1-git-send-email-bernard.iremonger@intel.com> <533710CFB86FA344BFBF2D6802E60286046A625A@SHSMSX101.ccr.corp.intel.com> <8CEF83825BEC744B83065625E567D7C204A42C88@IRSMSX108.ger.corp.intel.com> <533710CFB86FA344BFBF2D6802E60286046A6ED8@SHSMSX101.ccr.corp.intel.com> <8CEF83825BEC744B83065625E567D7C204A42F5F@IRSMSX108.ger.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 Subject: Re: [dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Jun 2015 01:31:45 -0000 On 6/30/2015 12:42 AM, Iremonger, Bernard wrote:=0A= >=0A= >> -----Original Message-----=0A= >> From: Qiu, Michael=0A= >> Sent: Monday, June 29, 2015 4:22 PM=0A= >> To: Iremonger, Bernard; dev@dpdk.org=0A= >> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa@igel.co.jp; Stephen=0A= >> Hemminger=0A= >> Subject: Re: [PATCH v2] librte_ether: release memory in uninit function.= =0A= >>=0A= >> On 2015/6/29 18:20, Iremonger, Bernard wrote:=0A= >>>> -----Original Message-----=0A= >>>> From: Qiu, Michael=0A= >>>> Sent: Monday, June 29, 2015 9:55 AM=0A= >>>> To: Iremonger, Bernard; dev@dpdk.org=0A= >>>> Cc: Zhang, Helin; Ananyev, Konstantin; mukawa@igel.co.jp; Stephen=0A= >>>> Hemminger=0A= >>>> Subject: Re: [PATCH v2] librte_ether: release memory in uninit functio= n.=0A= >>>>=0A= >>>> On 6/26/2015 5:32 PM, Iremonger, Bernard wrote:=0A= >>>>> Changes in v2:=0A= >>>>> do not free mac_addrs and hash_mac_addrs here.=0A= >>>>>=0A= >>>>> Signed-off-by: Bernard Iremonger =0A= >>>>> ---=0A= >>>>> lib/librte_ether/rte_ethdev.c | 6 +++++-=0A= >>>>> 1 files changed, 5 insertions(+), 1 deletions(-)=0A= >>>>>=0A= >>>>> diff --git a/lib/librte_ether/rte_ethdev.c=0A= >>>>> b/lib/librte_ether/rte_ethdev.c index e13fde5..7ae101a 100644=0A= >>>>> --- a/lib/librte_ether/rte_ethdev.c=0A= >>>>> +++ b/lib/librte_ether/rte_ethdev.c=0A= >>>>> @@ -369,8 +369,12 @@ rte_eth_dev_uninit(struct rte_pci_device=0A= >>>> *pci_dev)=0A= >>>>> /* free ether device */=0A= >>>>> rte_eth_dev_release_port(eth_dev);=0A= >>>>>=0A= >>>>> - if (rte_eal_process_type() =3D=3D RTE_PROC_PRIMARY)=0A= >>>>> + if (rte_eal_process_type() =3D=3D RTE_PROC_PRIMARY) {=0A= >>>>> + rte_free(eth_dev->data->rx_queues);=0A= >>>>> + rte_free(eth_dev->data->tx_queues);=0A= >>>>> rte_free(eth_dev->data->dev_private);=0A= >>>>> + memset(eth_dev->data, 0, sizeof(struct=0A= >>>> rte_eth_dev_data));=0A= >>>>> + }=0A= >>>>>=0A= >>>>> eth_dev->pci_dev =3D NULL;=0A= >>>>> eth_dev->driver =3D NULL;=0A= >>>> Actually, This could be put in rte_eth_dev_close() becasue queues=0A= >>>> should be released when closed.=0A= >>>>=0A= >>>> Also before free dev->data->rx_queues you should make sure=0A= >>>> dev->data->rx_queues[i] has been freed in PMD close() function, So=0A= >>>> dev->data->this=0A= >>>> two should be better done at the same time, ether in=0A= >>>> rte_eth_dev_close() or in PMD close() function. For hotplug in fm10k,= =0A= >>>> I put it in PMD close() function.=0A= >>>>=0A= >>>> Thanks,=0A= >>>> Michael=0A= >>> Hi Michael,=0A= >>>=0A= >>> The consensus is that the rx_queue and tx_queue memory should not be=0A= >> released in the PMD as it is not allocated by the PMD. The memory is=0A= >> allocated in rte_eth_dev_rx_queue_config() and=0A= >> rte_eth_dev_tx_queue_config(), which are both called from=0A= >> rte_eth_dev_configure() which is called by the application (for example= =0A= >> test_pmd). So it seems to make sense to free this memory in=0A= >> rte_eth_dev_uninit().=0A= >>=0A= >> It really make sense to free memory in rte_ether level, but when close a= port=0A= >> with out detached? just as stop --> close() --> quit(), the memory will = not be=0A= >> released :)=0A= >>=0A= > In the above scenario lots of memory will not be released.=0A= >=0A= > This is why the detach() and the underlying dev_uninit() functions were i= ntroduced.=0A= =0A= First detach is only for hotplug, for *users do not use hotplug*, that=0A= scenario is the right action. So "lots of memory will not be released"=0A= is issue need be fixed, actually, in fm10k driver, lots of memory has=0A= been released.=0A= =0A= > The dev_uninit() functions currently call dev_close() which in turn call= s dev_stop() which calls dev_clear_queues(). =0A= =0A= Users do hotplug then must call stop() --> close() --> dev_uninit(), it=0A= works fine. But do you think it make sense to release memory when=0A= close() it?=0A= =0A= > 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() a= nd the queue array memory is released in the rte_eth_dev_uninit() function= .=0A= =0A= That's your implementation, make sure not all users will detach a=0A= device, but the right action must include close(), do you agree?=0A= =0A= >=0A= > If the queue array memory is released in rte_eth_dev_close() then the rel= ease 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 ho= tplug patches. It will also change the existing dev_close() functionality= .=0A= =0A= Why impact?? Actually it works fine with fm10k driver. What I concern is=0A= *when user do not use hotplug*, it will lead lots of memory not=0A= released, that unacceptable, to move release action to=0A= rte_eth_dev_close() is just a suggestion by me, I think *the solution=0A= should cover both scenario*, am I right?=0A= =0A= =0A= >=0A= > 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() functi= ons.=0A= >=0A= > We probably need the view of the maintainers as to whether this should be= done in the close() or uninit() functions. =0A= >=0A= > Regards,=0A= >=0A= > Bernard.=0A= >=0A= > =0A= >=0A= >=0A= >=0A= >=0A= =0A=