From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id ACED51B8D0 for ; Wed, 4 Jul 2018 09:27:05 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 5365321BA9; Wed, 4 Jul 2018 03:27:05 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 04 Jul 2018 03:27:05 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc; s=mesmtp; bh=aoa+4A/kdckZxlvRWFZBoFi2Ir Q2FxdtfIIE7X3B3Vs=; b=hymXkAXza6LJtEVdmwlDhaxt50pzAhxZ75EAPXoZYO fiQVRvfsmilEvu+OZLGuJlXZunoQG/WBD5RzRdHeDwpxFKq1u5mvsp9ZGDxu6+5x CG73taCknrVQ/wFjBoM8ADX5MCJ9kTi4KyYOsrbRkY+fTvo/lJ/PHtlpEmYo67XS k= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=aoa+4A /kdckZxlvRWFZBoFi2IrQ2FxdtfIIE7X3B3Vs=; b=hvGU5LU2dFREPrNDe704Kh 1uM3tc7OQ3rGuBxPvXi8HQBURoCsLIpeZbmvaNy0IdUB/BPd8k995RGEyC8qmHIa sq77iDDIB1w5WSwBc+j0KJrpbM7yy+D43lWOe4lu4/tldUO1KeqD8HSjENsbw29n CtWsikUyADV4evA/2k/NqxPMLLL+AxjQWy36unaUGRZbPeX//15D9fnLYJ3nTDnt HeBBzMhFLfo937xIIeShbZe+HHmP/gJ1N+wR0D2a8l5ewimsYdJvZWrd1AdYCtBz BBW+AleVFgkOw9lggGs29MjstVr4vgLFGssYCbI23hiH4JZDxqBQVxvm/vG/S8pw == X-ME-Proxy: X-ME-Sender: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id BC874E4439; Wed, 4 Jul 2018 03:27:03 -0400 (EDT) From: Thomas Monjalon To: "Zhang, Qi Z" Cc: "dev@dpdk.org" , "Burakov, Anatoly" , "Ananyev, Konstantin" , "Richardson, Bruce" , "Yigit, Ferruh" , "Shelton, Benjamin H" , "Vangati, Narender" Date: Wed, 04 Jul 2018 09:27:02 +0200 Message-ID: <4367091.6QlV0rL9d1@xps> In-Reply-To: <039ED4275CED7440929022BC67E7061153249CCA@shsmsx102.ccr.corp.intel.com> References: <20180607123849.14439-1-qi.z.zhang@intel.com> <1712900.Rv84bhh2vl@xps> <039ED4275CED7440929022BC67E7061153249CCA@shsmsx102.ccr.corp.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 07:27:06 -0000 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 ethdev 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, 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". > 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. > But in _rte_eth_dev_callback_process we need to add a code branch for PRE_DETACH handle. > > If (event = RTE_ETH_EVENT_PRE_DETACH) > <...>. > else { > <....> > } > > And we may also need to keep rte_eth_dev_lock/unlock which will register a default callback for PRE_DETACH. The default callback can be registered by the application. > What do you think about?