From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 2489A91 for ; Wed, 31 Oct 2018 19:02:53 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 1690C22038; Wed, 31 Oct 2018 14:02:50 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 31 Oct 2018 14:02:50 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=t0uBcYFUcWvIsJ+F6w/3Ygl/uyWL8HhEEgxGe/Miuck=; b=Vicwhzm8GjBa oe58pJIRPaRcjIoTaHndd1WFwaecbJ/zV55QCBQfd05zuzgx8Hy15cYNtBJMNOAF qm6kJ0F/BCtCHBoIde7rAM0d7pMvFCg2pTkfrFn44HQ81o/4LWQt9tXfQyGx5oFr ni2KH1Y/uOTYZamBDchKPPax0vVhfno= 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-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=t0uBcYFUcWvIsJ+F6w/3Ygl/uyWL8HhEEgxGe/Miu ck=; b=E5vlstBVJFAYH8nw7M3NgZAkKXPyD96u1NZBGXyP8EZYDMgO+t2PxO0cy W52RD9o/rcClbhcjmE1AGU/ovRyOmMwvzyRmxR4HfRS9O/RZ/E0dJeUqQlNyFADF g55qJJF42Jf8p/tZ70S2xnwwYtJs8iaxuT4Jnd1msArcDyHHDu+3bxZtu3IRRlCs kg3djiBB4fYNYgt7n6n1kxRVE7hMgN1nhvwywe+9MykF0ufSRCtGJl/UahMTj7KJ 2bUNpcQpyaS1zCIz0hO6km7DnTwIbdBe7vJ5A0WgVAhenlKZKqUQ4NAh2vbNaDoe IFzd78J2exdg1Y6vqyEhAVbyQSggw== X-ME-Sender: X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id CA54CE427A; Wed, 31 Oct 2018 14:02:46 -0400 (EDT) From: Thomas Monjalon To: Luca Boccassi , "Burakov, Anatoly" Cc: llouis@vmware.com, yongwang@vmware.com, dev@dpdk.org, Chas Williams <3chas3@gmail.com>, Maxime Coquelin , tiwei.bie@intel.com, Bruce Richardson , brussell@vyatta.att-mail.com Date: Wed, 31 Oct 2018 19:02:52 +0100 Message-ID: <4897313.xyAmXeKNU8@xps> In-Reply-To: <1541008002.29722.34.camel@debian.org> References: <20180816135032.28283-1-bluca@debian.org> <16755296.DkulTLluNz@xps> <1541008002.29722.34.camel@debian.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug 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, 31 Oct 2018 18:02:53 -0000 31/10/2018 18:46, Luca Boccassi: > Sorry, been otherwise busy - I can do what you and Chas have asked, but > the problem is that v1 already did that and the VMWare maintainers > asked to change it back. So can I assume that the v1 way is the way to > go? I am expecting an answer from the vmxnet3 maintainer, or other VMware developpers. If they don't reply, we should work without them, and ask for a new maintainer in the community. I assume you can now work on a v3 reproducing what was done in v1. > On Wed, 2018-10-31 at 18:27 +0100, Thomas Monjalon wrote: > > Any update or question for this patch? > > If no update, it will miss 18.11. > > > > > > 27/10/2018 17:09, Thomas Monjalon: > > > 19/09/2018 17:47, Chas Williams: > > > > On Wed, Sep 19, 2018 at 8:58 AM Luca Boccassi > > > > wrote: > > > > > > > > > > The vmxnet3 driver can't call back into dev_close(), and > > > > > possibly > > > > > dev_stop(), in dev_uninit(). When dev_uninit() is called, > > > > > anything > > > > > that those routines would want to clean up has already been > > > > > released. > > > > > Further, for complete cleanup, it is necessary to release any > > > > > of the > > > > > queue resources during dev_close(). > > > > > This allows a vmxnet3 device to be hot-unplugged without > > > > > leaking > > > > > queues. > > > > > > > > > > Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode > > > > > driver implementation") > > > > > Cc: stable@dpdk.org > > > > > > > > > > Signed-off-by: Brian Russell > > > > > Signed-off-by: Luca Boccassi > > > > > --- > > > > > v2: add back extra close() call in uninit() for buggy > > > > > applications as > > > > > requested by the reviewers, and add debug log noting the > > > > > issue > > > > > > > > > > drivers/net/vmxnet3/vmxnet3_ethdev.c | 35 > > > > > +++++++++++++++++++++++----- > > > > > 1 file changed, 29 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > > b/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > > index f1596ab19d..98e5d01890 100644 > > > > > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > > @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev > > > > > *eth_dev) > > > > > if (rte_eal_process_type() != RTE_PROC_PRIMARY) > > > > > return 0; > > > > > > > > This should probably be EPERM as well. Out of scope though. > > > > > > > > > > > > > > - if (hw->adapter_stopped == 0) > > > > > + if (hw->adapter_stopped == 0) { > > > > > + PMD_INIT_LOG(DEBUG, "Device has not been > > > > > closed."); > > > > > vmxnet3_dev_close(eth_dev); > > > > > > > > This just seems wrong. You have called uninit() will the driver > > > > is > > > > still busy. Instead of "fixing" the state of the driver, return > > > > EBUSY > > > > here. > > > > > > I agree. > > > If the port is not stopped, either you stop it or you return EBUSY. > > > > > > Closing the device should be done outside of this check. > > > It is OK to close from uninit if the app did not close it. > > > > > > [...] > > > > > +static void > > > > > +vmxnet3_free_queues(struct rte_eth_dev *dev) > > > > > +{ > > > > > + int i; > > > > > + > > > > > + PMD_INIT_FUNC_TRACE(); > > > > > + > > > > > + for (i = 0; i < dev->data->nb_rx_queues; i++) { > > > > > + void *rxq = dev->data->rx_queues[i]; > > > > > + > > > > > + vmxnet3_dev_rx_queue_release(rxq); > > > > > + } > > > > > + dev->data->nb_rx_queues = 0; > > > > > + > > > > > + for (i = 0; i < dev->data->nb_tx_queues; i++) { > > > > > + void *txq = dev->data->tx_queues[i]; > > > > > + > > > > > + vmxnet3_dev_tx_queue_release(txq); > > > > > + } > > > > > + dev->data->nb_tx_queues = 0; > > > > > } > > > > > > > > > > /* > > > > > @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev) > > > > > static void > > > > > vmxnet3_dev_close(struct rte_eth_dev *dev) > > > > > { > > > > > - struct vmxnet3_hw *hw = dev->data->dev_private; > > > > > - > > > > > PMD_INIT_FUNC_TRACE(); > > > > > > > > > > vmxnet3_dev_stop(dev); > > > > > - hw->adapter_stopped = 1; > > > > > + vmxnet3_free_queues(dev); > > > > > } > > > > > > Good clean-up on dev_close. > > > You probably want to go further and set RTE_ETH_DEV_CLOSE_REMOVE > > > for allowing > > > a real release of the port on close. > > > Note: every PMDs should migrate towards this behaviour. > > > > > > To make things clear (I will write a doc for -rc2): > > > - "stop" should be called by the app but the PMD is allowed to > > > force it. > > > - "close" may be called by the app, and the PMD should enforce > > > it in uninit. > > > With RTE_ETH_DEV_CLOSE_REMOVE flag, it must completely > > > release the port. > > > - "remove" (implemented in PMD as uninit) is responsible of > > > closing > > > ethdev ports if not already done, and release the > > > shared resources > > > which are not specific to a port. It removes the whole > > > EAL rte_device. > > > > > > PS: for any hotplug patch or questions, feel free to Cc me. > > > > > > > > > > > >