From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 0BFD41BEE5 for ; Thu, 5 Jul 2018 03:38:58 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Jul 2018 18:38:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,309,1526367600"; d="scan'208";a="64505036" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga003.jf.intel.com with ESMTP; 04 Jul 2018 18:38:53 -0700 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 4 Jul 2018 18:38:53 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 4 Jul 2018 18:38:52 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.100]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.17]) with mapi id 14.03.0319.002; Thu, 5 Jul 2018 09:38:49 +0800 From: "Zhang, Qi Z" To: Thomas Monjalon CC: "dev@dpdk.org" , "Burakov, Anatoly" , "Ananyev, Konstantin" , "Richardson, Bruce" , "Yigit, Ferruh" , "Shelton, Benjamin H" , "Vangati, Narender" , "arybchenko@solarflare.com" Thread-Topic: [dpdk-dev] [PATCH v8 04/19] ethdev: introduce device lock Thread-Index: AQHUEcfFbxDt7G1dj06vIuuMXtd6DqR8vnAAgAC+i3CAAA+dAIAAtvlg///jkACAALHwkIAAPO4AgAC+AZA= Date: Thu, 5 Jul 2018 01:38:49 +0000 Message-ID: <039ED4275CED7440929022BC67E7061153250867@SHSMSX103.ccr.corp.intel.com> References: <20180607123849.14439-1-qi.z.zhang@intel.com> <4367091.6QlV0rL9d1@xps> <039ED4275CED7440929022BC67E706115324B015@shsmsx102.ccr.corp.intel.com> <6105228.vaBHSlUH0d@xps> In-Reply-To: <6105228.vaBHSlUH0d@xps> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjlmOTYxODgtNWQ3YS00MTBlLTllM2EtZTY0NjRkYmVkZmU3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoidlwvYXRNbmt4MXg5dGs2bEFpSWJrcXQyMEpIVGhKelwvc21zWnJWWUlYZVBZVm1kV1Z2K2t3TmswUUtVZkc5QktyIn0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action 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 v8 04/19] ethdev: introduce device lock X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Jul 2018 01:38:59 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Thursday, July 5, 2018 5:42 AM > To: Zhang, Qi Z > Cc: dev@dpdk.org; Burakov, Anatoly ; Ananyev, > Konstantin ; Richardson, Bruce > ; Yigit, Ferruh ; She= lton, > Benjamin H ; Vangati, Narender > ; arybchenko@solarflare.com > Subject: Re: [dpdk-dev] [PATCH v8 04/19] ethdev: introduce device lock >=20 > 04/07/2018 12:49, Zhang, Qi Z: > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > 04/07/2018 03:47, Zhang, Qi Z: > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > > > 03/07/2018 17:08, Zhang, Qi Z: > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > > > > > 02/07/2018 07:44, Qi Zhang: > > > > > > > > Introduce API rte_eth_dev_lock and rte_eth_dev_unlock to > > > > > > > > let application lock or unlock on specific ethdev, a > > > > > > > > locked device can't be detached, this help applicaiton to > > > > > > > > prevent unexpected device detaching, especially in multi-pr= ocess > envrionment. > > > > > > > > > > > > > > Trying to understand: a process of an application could try > > > > > > > to detach a port while another process is against this decisi= on. > > > > > > > Why an application needs to be protected against itself? > > > > > > > > > > > > I think we can regard this as a help function, it help > > > > > > application to simplified > > > > > the situation when one process want to detach a device while > > > > > another one is still using it. > > > > > > Application can register a callback which can do to necessary > > > > > > clean up (like > > > > > stop traffic, release memory ...) before device be detached. > > > > > > > > > > Yes I agree such hook can be a good idea. > [...] > > > > > After all, it is just a pre-detach hook. > > > > > > > > > Wait, how is it different of RTE_ETH_EVENT_DESTROY callback? > > > > > Perhaps we just need to improve the handling of the DESTROY event= ? > > > > > > > > I have thought about this before. > > > > Not like RTE_ETH_EVENT_DESTROY and other event hook, the hook here > > > need to give feedback, pass or fail will impact the following > > > behavior, this make it special, so I separate it from all exist > > > rte_eth_event_type handle mechanism. > > > > > > Look at _rte_eth_dev_callback_process, there is a "ret_param". > > > > OK, that should work. > > > > > > > The alternative solution is > > > > we just introduce a new event type like RTE_ETH_EVENT_PRE_DETACH > > > > and reuse all exist API > > > rte_eth_dev_callback_register/rte_eth_dev_callback_unregister. > > > > > > I don't think we need a new event. > > > Let's try to use RTE_ETH_EVENT_DESTROY. > > > > The problem is RTE_ETH_EVENT_DESTROY is used in > rte_eth_dev_release_port already. > > And in PMD, rte_eth_dev_release_port is called after dev_uninit, that > > mean its too late to reject a detach >=20 > You're right. >=20 > It's a real mess currently. > The right order should be to remove ethdev ports before removing the > underlying EAL device. But it's strangely not the case. >=20 > We need to separate things. > The function rte_eth_dev_close can be used to remove an ethdev port if we > add a call to rte_eth_dev_release_port. > So we could call rte_eth_dev_close in PMD remove functions. > Is "close" a good time to ask confirmation to the application? > Or should we ask confirmation a step before, on "stop"? I think the confirmation should before any cleanup stage, it should at the = beginning of driver->remove. Also we should not put it into rte_eth_dev_stop, because, rte_eth_dev_stop = can invoked by application directly, in that case, we don't what any callba= ck be invoked. >=20 > > So , do you mean we can remove > > _rte_eth_dev_callback_process(RTE_ETH_EVENT_DESTROPY) in > > rte_eth_dev_release_port >=20 > I would say we need RTE_ETH_EVENT_DESTROY to notify that the port is real= ly > destroyed. > Maybe the right thing to do is to add a new event > RTE_ETH_EVENT_CLOSE_REQUEST or something else. > Note that we already have 2 removal events in ethdev: > - RTE_ETH_EVENT_INTR_RMV when the port cannot be used anymore > - RTE_ETH_EVENT_DESTROY when the port is going to be deleted >=20 > > And where is right place to call > _rte_eth_dev_callback_process(RTE_ETH_EVENT_DESTROY)? > > If can't be called in rte_eth_dev_detach, because if device is removed = by > rte_eal_hotplug_remove, it will be skipped. >=20 > No, rte_eth_dev_detach and rte_eal_hotplug_remove are 2 different things. > One is a mix of ethdev and EAL (and should be deprecated), the other one = is > for the underlying device at EAL level. >=20 > > probably we need to call this at the beginning of each PMD driver->remo= ve?, > that means, we need to change all PMD drivers? >=20 > Yes, we can call rte_eth_dev_stop and rte_eth_dev_close at the beginning = of > PMD remove. > Note that there is already a helper rte_eth_dev_destroy called in some PM= D to > achieve the removal, but curiously, it doesn't call stop and close functi= ons. Currently PMD implement driver->remove with different way, rte_eth_dev_stop= / rte_eth_dev_close / rte_eth_dev_destroy is not always be invoked. So Before we standardize what ethdev API and what sequence should be called= in driver->remove (I think this is a separate task) I will suggest=20 1. Create another help function like _rte_eth_dev_allow_to_remove,=20 2. the help function will call _rte_eth_dev_callback_process(RTE_ETH_EVENT_= PRE_REMOVE) and update ret_param which contain a reject count. 3. the help function should to invoked at beginning at driver->remove and d= river->remove will abort if the help function failed. But once we standardized that , we can do cleanup to merge it into another = rte_eth_xxx API in next step. What do you think? >=20 >=20