From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 1CF635A44 for ; Wed, 8 Jul 2015 11:49:42 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP; 08 Jul 2015 02:49:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,431,1432623600"; d="scan'208";a="520719788" Received: from irsmsx104.ger.corp.intel.com ([163.33.3.159]) by FMSMGA003.fm.intel.com with ESMTP; 08 Jul 2015 02:49:40 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.201]) by IRSMSX104.ger.corp.intel.com ([169.254.5.171]) with mapi id 14.03.0224.002; Wed, 8 Jul 2015 10:49:39 +0100 From: "Iremonger, Bernard" To: Tetsuya Mukawa , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2] librte_ether: release memory in uninit function. Thread-Index: AQHQr/MWrMnfGSFKL02MpXHseHw6cp3DptjAgAvF98qAAHP60IABEDGAgABx9UA= Date: Wed, 8 Jul 2015 09:49:38 +0000 Message-ID: <8CEF83825BEC744B83065625E567D7C204A46F17@IRSMSX108.ger.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> <533710CFB86FA344BFBF2D6802E60286046AE60D@SHSMSX101.ccr.corp.intel.com> <559B49A0.1060904@igel.co.jp> <8CEF83825BEC744B83065625E567D7C204A467FF@IRSMSX108.ger.corp.intel.com> <559C9D55.6000109@igel.co.jp> In-Reply-To: <559C9D55.6000109@igel.co.jp> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] 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: Wed, 08 Jul 2015 09:49:43 -0000 > -----Original Message----- > From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp] > Sent: Wednesday, July 8, 2015 4:48 AM > To: Iremonger, Bernard; dev@dpdk.org > Cc: Qiu, Michael; thomas.monjalon@6wind.com; Ananyev, Konstantin; > Stephen Hemminger; Zhang, Helin; Chen, Jing D; Neil Horman > Subject: Re: [dpdk-dev] [PATCH v2] librte_ether: release memory in uninit > function. >=20 > On 2015/07/07 19:53, Iremonger, Bernard wrote: > >> -----Original Message----- > >> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp] > >> Sent: Tuesday, July 7, 2015 4:38 AM > >> To: dev@dpdk.org > >> Cc: Qiu, Michael; Iremonger, Bernard; thomas.monjalon@6wind.com; > >> Ananyev, Konstantin; Stephen Hemminger; Zhang, Helin; Chen, Jing D; > >> Neil Horman > >> Subject: Re: [dpdk-dev] [PATCH v2] librte_ether: release memory in > >> uninit function. > >> > >> On 2015/07/06 20:35, Qiu, Michael wrote: > >>> Hi, all > >>> > >>> As we has gap on the memory release action to be done in which step, > >>> I appreciate all your comments on this patch. > >>> > >>> Currently, the correct quit sequence for testpmd is stop() ---> > >>> port_stop() --> port_close() --> quit(). This will lead lots of > >>> memory not released by default, like queues. > > There is also the scenario (outside of testpmd) where an application ca= n call > rte_eth_dev_close() or rte_eth_dev_detach() which calls > rte_eth_dev_uninit() directly from an application. > > In these cases the memory allocated in the EAL layer should be released= in > both rte_eth_dev_close() and rte_eth_dev_detach(). >=20 > Hi Bernard, >=20 > All DPDK applications that uses hotpluggingmust call rte_eth_dev_stop() a= nd > rte_eth_dev_close()APIs before detaching ports. > (This is described in DPDK documentation) Could we ignore applications th= at > only calls rte_eth_dev_detach()? >=20 > > This patch is intended to address the rte_eth_dev_detach() case only > (hotplug case). > > Perhaps there should be a separate patch to address the > rte_eth_dev_close() case. >=20 > We all needs to have consensus about how to implement finalization code i= n > close() and uninit(). > Probably one of options will be implementing finalization code in > close() as much as possible. >=20 > Tetsuya, Hi Tetsuya, Testpmd enforces the dev_stop(), dev_close() and dev_detach() sequence. There is no reason why an application cannot call dev_detach() directly.=20 During internal review of PMD dev_uninit() functions it was decided to ens= ure that this sequence was adhered to by calling dev_close() which calls de= v_stop() from the dev_uninit() function. In the PMD hotplug patches the following calling sequence is implemented: dev_uninit() calls dev_close() calls dev_stop(). At present most of the finalization code is implemented in dev_close() and = dev_stop(). The dev_uninit() functions implement what is left of the finalization code. Regards, Bernard. >=20 > > Regards, > > > > Bernard. > > > >>> We have 3 potential proposals(normal case means without hotplug): > >>> > >>> 1. Those memory release in close() other left in uninit() > >>> This will work fine for both hotplug case and normal case. > >> +1 > >> > >> I assume that once close() is called, we cannot start the port again > >> without hotplugging. > >> This is my premise. > >> > >> It might be good that we move finalization code to close() as much as > >> possible, because of followings. > >> 1. Anyway, once close() is called, we cannot start the port again. So > >> it should be ok to free resources in close(). > >> 2. Non-hotplugging applications can release resources if we implement > >> finalization code to close(). > >> > >> Also I guess Stephen's suggestion is important to keep code clean. > >> It may be good that 'dev->data->rx/tx_queues[queue_id]' are freed in > >> rx/tx_queue_release() of each PMD, and rx/tx_queue_release() will be > >> called by rte_eth_dev_close(). Also 'dev->data->rx/tx_queues[]' > >> should be freed in ethdev.c. > >> > >>> 3. Combine close() and uninit(), only one will be left. > >>> This will work fine for both hotplug case and normal case. But > >>> it may change a lot, such as logic. > >> I guess this will be second option. But I agree we need to change a > >> lot. Also after enabling hotplug by default, when someone only wants > >> to close the port, it will be impossible. > >> > >> Regards, > >> Tetsuya