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 B74A02A58 for ; Mon, 6 Jul 2015 13:35:44 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 06 Jul 2015 04:35:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,414,1432623600"; d="scan'208";a="756802679" Received: from pgsmsx104.gar.corp.intel.com ([10.221.44.91]) by fmsmga002.fm.intel.com with ESMTP; 06 Jul 2015 04:35:20 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by PGSMSX104.gar.corp.intel.com (10.221.44.91) with Microsoft SMTP Server (TLS) id 14.3.224.2; Mon, 6 Jul 2015 19:35:18 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.246]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.146]) with mapi id 14.03.0224.002; Mon, 6 Jul 2015 19:35:17 +0800 From: "Qiu, Michael" To: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function. Thread-Index: AQHQt9/U2AZ/ModqekWE2dzyoQhvbA== Date: Mon, 6 Jul 2015 11:35:16 +0000 Message-ID: <533710CFB86FA344BFBF2D6802E60286046AE60D@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> <533710CFB86FA344BFBF2D6802E60286046A75A4@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 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: Mon, 06 Jul 2015 11:35:48 -0000 Hi, all=0A= =0A= As we has gap on the memory release action to be done in which step, I=0A= appreciate all your comments on this patch.=0A= =0A= Currently, the correct quit sequence for testpmd is stop() --->=0A= port_stop() --> port_close() --> quit(). This will lead lots of memory=0A= not released by default, like queues.=0A= =0A= We have 3 potential proposals(normal case means without hotplug):=0A= =0A= 1. Those memory release in close() other left in uninit()=0A= This will work fine for both hotplug case and normal case.=0A= =0A= 2. leave close() just as the same before, all be done in uninit()=0A= This will works fine for hotplug, for normal case maybe has=0A= issue(memory not released?).=0A= =0A= 3. Combine close() and uninit(), only one will be left.=0A= This will work fine for both hotplug case and normal case. But it=0A= may change a lot, such as logic.=0A= =0A= 4. other.=0A= =0A= For more details, you could go though this thread.=0A= =0A= =0A= Thanks,=0A= Michael=0A= =0A= =0A= On 6/30/2015 9:32 AM, Qiu, Michael wrote:=0A= > On 6/30/2015 12:42 AM, Iremonger, Bernard wrote:=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 functi= on.=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 = introduced.=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 cal= ls dev_stop() which calls dev_clear_queues(). =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 th= e 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() functio= n.=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= >> If the queue array memory is released in rte_eth_dev_close() then the re= lease of the queue_memory will have to be moved to the dev_close() function= s from the dev_uninit() functions. This will impact all the existing PMD h= otplug patches. It will also change the existing dev_close() functionalit= y.=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= >> My preference is to leave the existing dev_close() functions unchanged a= s far as possible and to do what needs to be done in the dev_uninit() funct= ions.=0A= >>=0A= >> We probably need the view of the maintainers as to whether this should b= e done in the close() or uninit() functions. =0A= >>=0A= >> Regards,=0A= >>=0A= >> Bernard.=0A= >>=0A= >> =0A= >>=0A= >>=0A= >>=0A= >>=0A= >=0A= =0A=