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 51DB710A3 for ; Wed, 31 Oct 2018 18:27:22 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id DCF18220E3; Wed, 31 Oct 2018 13:27:21 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 31 Oct 2018 13:27:21 -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=AcVLpa+sTGEAIQF6X02UY0kVBDCAO7hlVi3wTVcoBMg=; b=a5h1dD4mhWMl dPVP1hZc+ZfaD7DLCwwiMkDzdKwHZK8a2berRJ6fXfMAiIZUQDA/OOQjR5WnM37r GfWQR40lU7nODKvxP9/+TfaZDtJzWOXR99juFyhoDv6UA6UITONpcrxX6Sd2a/Hb hXras49Q8l/C432hkUlcCDtQypFUQ3U= 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=AcVLpa+sTGEAIQF6X02UY0kVBDCAO7hlVi3wTVcoB Mg=; b=sopZv1/zO3mj+l90GoFovSREatXbamU/ahYqrDls5gXOCl0weYuD2Dp+x CH+fLFZWMET/TP4wjedwnz8joxfIoptMG5MZFxpV/DeKKvVLtAX/1NP958w8XH+Q cjnTeSMi4jDoth3QFnWfBgezWEs2BAC+MxREPfyBnxHhoCILoaziGDkC8V4Qk732 NWrEEgYEUEoPzllhfyG0dXeaukPgp6V+EBpx/HGm7Z+XAw9Yr9lI6gtOG8pkF/8m pHsHyKue+IdnOW5gPNhuOkRLR6joY9HOKVrLA+bXw28IvXRT1cDPHwUHD0F6hLDv ytw1Kn4nLxOWTtydm2bO/g0TAR6QQ== 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 C15BBE43A6; Wed, 31 Oct 2018 13:27:19 -0400 (EDT) From: Thomas Monjalon To: bluca@debian.org, llouis@vmware.com, yongwang@vmware.com Cc: dev@dpdk.org, Chas Williams <3chas3@gmail.com>, Maxime Coquelin , tiwei.bie@intel.com, Bruce Richardson , Jianfeng Tan , "Burakov, Anatoly" , brussell@vyatta.att-mail.com Date: Wed, 31 Oct 2018 18:27:25 +0100 Message-ID: <16755296.DkulTLluNz@xps> In-Reply-To: <2858283.f32l3dFHSD@xps> References: <20180816135032.28283-1-bluca@debian.org> <2858283.f32l3dFHSD@xps> 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 17:27:22 -0000 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.