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 E204E1C138 for ; Thu, 5 Jul 2018 12:54:54 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 6ABE921D28; Thu, 5 Jul 2018 06:54:54 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Thu, 05 Jul 2018 06:54:54 -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=QWBBR3e7rWNknbMFKNejZvKf6u hbtC/dI8HUIAZ7T6s=; b=MJzpRlvLz7EDmFCnexNiJGKEEqipMpdy5Au76kp9r4 3rWL/XeMmDDieuWiOsw+jLFJN95Z6t/ie1N+CTplmq94nTykhJbGcxygvyB6ICIv zQBR3+UQ8CFnP5l2M/koEIE8IyP8T+S7oUieyVPcCJdGQWugofxye25weM6C3jX9 M= 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=QWBBR3 e7rWNknbMFKNejZvKf6uhbtC/dI8HUIAZ7T6s=; b=E8/YwzmSM7Pv9mog25apmP 5Zz79KCWFbPQKOYNTWCqH7Zda8UPM59k7rsECs3Ep9OQqZXU4gh1wAI5h8SDLqNw WfwxHrvpezTSbXGulkIPYtSwA7JzlFdcY0Mf39l4L9u18ruupCFZJeRug5rK67TM FJD7pwguFRwX0/3brmqKb5L2dTUkOzYtN6Qd6mwGUCR/11OO4Ztbssr2vV8a7XQI K6w9M7RDXXlKvrckWqSrlxMVlmUct/tsTeaMLGrhqBjELL1nNK5ff6zP1Hc8ST/i wItWIOAgLP74M07051UCv4CxJC2IEJ9bKGTUEWgUx0amQRgmqvzSf8I9NORJeerQ == 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 C2A46102B3; Thu, 5 Jul 2018 06:54:52 -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" , "arybchenko@solarflare.com" Date: Thu, 05 Jul 2018 12:54:51 +0200 Message-ID: <7184076.3FrzpcIOyp@xps> In-Reply-To: <039ED4275CED7440929022BC67E7061153254B26@SHSMSX103.ccr.corp.intel.com> References: <20180607123849.14439-1-qi.z.zhang@intel.com> <4080531.8J5Q0D2ZWA@xps> <039ED4275CED7440929022BC67E7061153254B26@SHSMSX103.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: Thu, 05 Jul 2018 10:54:55 -0000 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 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 > > > > > > > > > > > > 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 the 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 it? > > > > > > 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 again. > > > 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? An option is to let the application manages itself its process synchronization and authorization for detaching devices.