From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id C038C1BDFF for ; Wed, 4 Jul 2018 12:49:52 +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 fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Jul 2018 03:49:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,306,1526367600"; d="scan'208";a="64364543" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga003.jf.intel.com with ESMTP; 04 Jul 2018 03:49:51 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 4 Jul 2018 03:49:51 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 4 Jul 2018 03:49:50 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.100]) with mapi id 14.03.0319.002; Wed, 4 Jul 2018 18:49:48 +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" Thread-Topic: [dpdk-dev] [PATCH v8 04/19] ethdev: introduce device lock Thread-Index: AQHUEcfFbxDt7G1dj06vIuuMXtd6DqR8vnAAgAC+i3CAAA+dAIAAtvlg///jkACAALHwkA== Date: Wed, 4 Jul 2018 10:49:47 +0000 Message-ID: <039ED4275CED7440929022BC67E706115324B015@shsmsx102.ccr.corp.intel.com> References: <20180607123849.14439-1-qi.z.zhang@intel.com> <1712900.Rv84bhh2vl@xps> <039ED4275CED7440929022BC67E7061153249CCA@shsmsx102.ccr.corp.intel.com> <4367091.6QlV0rL9d1@xps> In-Reply-To: <4367091.6QlV0rL9d1@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: Wed, 04 Jul 2018 10:49:53 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Wednesday, July 4, 2018 3:27 PM > To: Zhang, Qi Z > Cc: dev@dpdk.org; Burakov, Anatoly ; Ananyev, > Konstantin ; Richardson, Bruce > ; Yigit, Ferruh ; She= lton, > Benjamin H ; Vangati, Narender > > Subject: Re: [dpdk-dev] [PATCH v8 04/19] ethdev: introduce device lock >=20 > 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 detached. > > > > > > Yes I agree such hook can be a good idea. > > > > > > > > > > > I guess it is only an application inter-process management. > > > > > If we really want to provide such helper in DPDK, it should not > > > > > be limited to ethdev. > > > > > > > > Once we move to eal layer, we will have > > > > rte_eal_dev_lock/unlock(devname, > > > busname). > > > > But its also better we keep rte_eth_dev_lock/unlock to make ethdev > > > > API more completed (any port be locked means underline rte_device > > > > also be > > > locked?) and this is same for other device family. > > > > > > No. Again, a port is not exactly a device. > > > There can be several ports per device. > > > > Yes, I know that. > > what I mean is, we should assume lock any port of that rte_device will > prevent the device be detached. > > > > > > > > I think the right place for this hook is in port-level API (ethdev, > > > crypto, etc). And as we improve only ethdev currently, without any > > > common genericity for other device classes, it is probably fine in et= hdev for > now. > > > > > > > > > (for info, see class implementation: > > > > > https://patches.dpdk.org/patch/41605/) > > > > > > > > > > What about hardware unplug? > > > > > Can we detach the locked ports associated to the unplugged device= ? > > > > > > > > NO, we can't. > > > > But do you think, we need to introduce a "force detach" version, > > > > which will > > > ignore all locks. > > > > > > No, I don't think so. > > > I am just trying to show that you cannot really "lock" a port. > > > Maybe you should just rename those functions. > > > 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, t= his > make it special, so I separate it from all exist rte_eth_event_type handl= e > mechanism. >=20 > Look at _rte_eth_dev_callback_process, there is a "ret_param". OK, that should work. >=20 > > 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. >=20 > 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 al= ready. And in PMD, rte_eth_dev_release_port is called after dev_uninit, that mean = its too late to reject a detach So , do you mean we can remove _rte_eth_dev_callback_process(RTE_ETH_EVENT_= DESTROPY) in rte_eth_dev_release_port=20 And where is right place to call _rte_eth_dev_callback_process(RTE_ETH_EVEN= T_DESTROY)? If can't be called in rte_eth_dev_detach, because if device is removed by r= te_eal_hotplug_remove, it will be skipped. probably we need to call this at the beginning of each PMD driver->remove?,= that means, we need to change all PMD drivers? >=20 > > But in _rte_eth_dev_callback_process we need to add a code branch for > PRE_DETACH handle. > > > > If (event =3D RTE_ETH_EVENT_PRE_DETACH) > > <...>. > > else { > > <....> > > } > > > > And we may also need to keep rte_eth_dev_lock/unlock which will registe= r a > default callback for PRE_DETACH. >=20 > The default callback can be registered by the application. OK. >=20 > > What do you think about? >=20 >=20