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 936E92674 for ; Sat, 27 Oct 2018 17:09:35 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 1D63121F03; Sat, 27 Oct 2018 11:09:35 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Sat, 27 Oct 2018 11:09:35 -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=hBaam2eva9ySKl9xtA9+3rVPNFzOQuVFX2Z0cV+xO9Q=; b=B2UCAGuv7y8I W8W4xCU3wlb526L2iymwWSuraeY6eS8rKcyKkHNFa6aRfgGUy98AvksAiGzMYZx8 6Wz1LhH6MCbABThdIsCk/Cc+VjI9LN+qv/5KJS2sORUyRXeKMar8FPb5I9nx/IQx 30PCLupCAWR/xJIYhFx2uczCB1R1oZo= 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=hBaam2eva9ySKl9xtA9+3rVPNFzOQuVFX2Z0cV+xO 9Q=; b=gr/hRhi7ECjjmcHjL6jbxVJAY9C5E+x9wnkIcWBw5LSjfMl9rLqd3ySar AbLCMjMYYA4Rt2XazjdEp+w6Q6nSD5iGSWd5L8sx28/zFBZVtDg2uc19ox7xMf/g BAwNjfvKglrEEUzXCXl04LG7wXk5aUKZ2Vri7YMrddlOQNqLxxJDITjn8tbSBUKs Ip6pFdGZHEDJQCRs3mjfAqWxPlumz4RNDfvPYHcwAhMj49+bnZEr/q2ud4GWVl05 B6c/rEQ19KhNQeDpOIQ1J1HGRCMxPpu4fFux18zxoTWxgEYx5ERRnAVo8pKRL88x BC3a3nxMme2VsU9JOzj2wuqgupTGQ== 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 BAEDDE4A3F; Sat, 27 Oct 2018 11:09:32 -0400 (EDT) From: Thomas Monjalon To: Chas Williams <3chas3@gmail.com>, bluca@debian.org, llouis@vmware.com Cc: dev@dpdk.org, Maxime Coquelin , tiwei.bie@intel.com, yongwang@vmware.com, Bruce Richardson , Jianfeng Tan , "Burakov, Anatoly" , brussell@vyatta.att-mail.com Date: Sat, 27 Oct 2018 17:09:39 +0200 Message-ID: <2858283.f32l3dFHSD@xps> In-Reply-To: References: <20180816135032.28283-1-bluca@debian.org> <20180919125757.17938-2-bluca@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: Sat, 27 Oct 2018 15:09:35 -0000 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.