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 45D771BDDE for ; Thu, 5 Jul 2018 14:17:09 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Jul 2018 05:17:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,312,1526367600"; d="scan'208";a="243187181" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga006.fm.intel.com with ESMTP; 05 Jul 2018 05:16:43 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 5 Jul 2018 05:16:42 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.100]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.81]) with mapi id 14.03.0319.002; Thu, 5 Jul 2018 20:16:40 +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+AZD//4jKgIAAm+1g//+/loAAE9lJMP//nG+A//9j/+A= Date: Thu, 5 Jul 2018 12:16:40 +0000 Message-ID: <039ED4275CED7440929022BC67E7061153254C7B@SHSMSX103.ccr.corp.intel.com> References: <20180607123849.14439-1-qi.z.zhang@intel.com> <4080531.8J5Q0D2ZWA@xps> <039ED4275CED7440929022BC67E7061153254B26@SHSMSX103.ccr.corp.intel.com> <7184076.3FrzpcIOyp@xps> In-Reply-To: <7184076.3FrzpcIOyp@xps> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTk5OWQ2YzctZjAzNC00NzhiLWE2OWItNTVjZTRhNTg0YWIzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiVURObjNjWnNtS1wvd0JKZUpoVEY2c0dcL3E0cHhFVDFFRHJZMGE4dm4xK25teVU3ZXBrd0NjSWhqSTRjSFR5OW43In0= 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 12:17:10 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Thursday, July 5, 2018 6:55 PM > 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 > 05/07/2018 11:54, Zhang, Qi Z: > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > 05/07/2018 05:37, Zhang, Qi Z: > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > > > 05/07/2018 03:38, Zhang, Qi Z: > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > > > > > 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-process > > > > > > > envrionment. > > > > > > > > > > > > > > > > > > > > > > > > > > Trying to understand: a process of an > > > > > > > > > > > > > application could try to detach a port while > > > > > > > > > > > > > another process is against this > > > > > decision. > > > > > > > > > > > > > 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 de= tached. > > > > > > > > > > > > > > > > > > > > > > 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_pa= ram". > > > > > > > > > > > > > > > > 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_unregi= ster. > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > You're right. > > > > > > > > > > > > > > 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 th= e case. > > > > > > > > > > > > > > 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. > > > > > > > > > > So you stop a port, even if the app policy is against detaching i= t? > > > > > > > > My understanding is, stop and detach is different, we may stop a > > > > device and > > > reconfigure it then restart it. > > > > but for detach, properly we will not use it, unless it be probed ag= ain. > > > > For dev_close , it should be called after dev_stop. > > > > so we have to like below. > > > > > > > > If (dev->started) { > > > > dev_stop /* but still problem here, if traffic is ongoing */ > > > > if (dev_close()) { > > > > dev_start() > > > > return -EBUSY. > > > > } > > > > } else { > > > > If (dev_close()) > > > > Return _EBUSY > > > > } > > > > > > > > So for me, neither rte_eth_dev_stop and rte_eth_dev_close is the > > > > right place > > > to check this. > > > > But rte_eth_dev_destroy looks like a good one. We can put all the > > > > ethdev general logic into it, and PMD specific dev_unit will be > > > > called at last > > > > > > If you want to detach a port, you need to stop it. > > > If one process try to detach a port, but another process decides > > > (via callback) that the port should not be detached, you will have > > > stopped a port for no good reason. > > > To me it is a real design issue. > > > > Yes, so I think we still need two iterates for detach. > > First iterate to get agreement on all processes. > > Secondary iterate to do the detach. > > > > But how? >=20 > An option is to let the application manages itself its process synchroniz= ation > and authorization for detaching devices. Yes, I agree Before we figure out a comprehensive solution, leave this to application's = handle so far. I will remove this one from patchset. >=20