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 0EB6E1B108 for ; Thu, 18 Oct 2018 11:32:27 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id AC80E22221; Thu, 18 Oct 2018 05:32:26 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Thu, 18 Oct 2018 05:32:26 -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=9o5i9A3uTtKYHPhZP7eL93vXQcNBLrZwXHx2U6F7sYc=; b=JZiE3xzvGPHG BXFhbrmlisng1CtfZ0m3MLHijai24bVllTHbQeO7NubgoxvhCBQmXQ7BmodGCx5V ZedKQ/mAg+R0unUt/4I6h+Ip3ZUfQXyMnxc2625qNH4F70VocqSgsqBGwuaq46Yn +x4MaCJp/JeJu51ln2ljPX7k/GE1fwk= 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=9o5i9A3uTtKYHPhZP7eL93vXQcNBLrZwXHx2U6F7s Yc=; b=nnZxn1OoO/Myrspr/UNZ3SKxLFWr/rh3O1CLwar5zdvzZ6YiJk2yYeSY1 kjiTFJ3AyxijW3ZjYnzt/CMwkjXYmege0G6Z4/JMVXyvncV2salJb9I2/FVy7YO3 wQExVA21I/EoqaF+phHs1hbtRYO95QMS562Npw6xlQha7UmKScl5rsYrKQ5Racpl vvbEkBigOm4ROS8whGi+ZgupHF/SSgpeRzy23mAKmXxTaK5rLwJv5FRFPcg4K8WA K5zwLs+0AUUbAvCwDEaIxUU13Y/QA9seB4qcevRnd6hNk1l6ladkhmj3dLLxRflR +pVoG0EuR4YbtQG1Sn58o66bHFiQw== 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 43AC7102EA; Thu, 18 Oct 2018 05:32:25 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko Cc: ferruh.yigit@intel.com, dev@dpdk.org, ophirmu@mellanox.com, bernard.iremonger@intel.com, rahul.lakkireddy@chelsio.com Date: Thu, 18 Oct 2018 11:32:29 +0200 Message-ID: <1858661.D0KfzqaYm0@xps> In-Reply-To: <97e9f9fa-067e-57bf-6ade-3745cd65c703@solarflare.com> References: <20180907233929.21950-1-thomas@monjalon.net> <20181018012402.1240-7-thomas@monjalon.net> <97e9f9fa-067e-57bf-6ade-3745cd65c703@solarflare.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v5 6/6] ethdev: complete closing of port 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, 18 Oct 2018 09:32:27 -0000 18/10/2018 10:33, Andrew Rybchenko: > On 10/18/18 4:24 AM, Thomas Monjalon wrote: > > After closing a port, it cannot be restarted. > > So there is no reason to not free all associated resources. > > > > The last step was done with rte_eth_dev_detach() which is deprecated. > > Instead of blindly removing the associated rte_device, the driver should > > check if no more port (ethdev, cryptodev, etc) is open for the device. > > > > The last ethdev freeing which were done by rte_eth_dev_detach(), > > are now done at the end of rte_eth_dev_close(). > > > > Some drivers does not allocate MAC addresses dynamically or separately. > > In those cases, the pointer is set to NULL, in order to avoid wrongly > > freeing them in rte_eth_dev_release_port(). > > > > A closed port will have the state RTE_ETH_DEV_UNUSED which is > > considered as invalid by rte_eth_dev_is_valid_port(). > > So validity is not checked anymore for closed ports in testpmd. > > > > If the driver is trying to free the port again, the function > > rte_eth_dev_release_port() will abort with -ENODEV error. > > > > Signed-off-by: Thomas Monjalon > > I've tested the patch series together with [1]. There are 2 use cases to consider: 1/ rte_eth_dev_close + rte_dev_remove 2/ rte_dev_remove without prior close > As I expected it makes problems and resource leaks if > rte_eth_dev_close() is used. > > Everything is OK if I do port stop and detach (with net/sfc > patch which does close from uninit). This is the case #2. It requires some PMD updates to avoid the leaks. It was buggy in previous releases too, because it was not managing devices with multiple ports. I think we should continue recommending the use case #1 in 18.11, and let time for PMDs to be fixed for use case #2. It PMDs can be fixed in 18.11, it is even better. > If I do port stop, close and detach, the last one returns > error since the device already released and net/sfc uninit > is never called. This is the case #1. The bug is assuming that the port is not freed when removing the device. We should just skip closed ports without any error. I can fix it in a v6. > Basically it should be one function which is called in both > cases: dev_close or pci_device remove. Similar changes > should be done in many PCI drivers. Yes, dev_close must be called for all non-closed ports of a PCI device to remove. Usually, PMDs have an implementation of dev_close which is different of the uninit function called with rte_eth_dev_pci_generic_remove() or rte_eth_dev_destroy(). They have no good reason to be different: typedef int (*ethdev_uninit_t)(struct rte_eth_dev *ethdev); typedef void (*eth_dev_close_t)(struct rte_eth_dev *dev); > If I drop the patch, everything seems to be work fine > from the first sight. > May be it should be removed from the patchset and > considered separately. No, I think I should just fix the use case #1, and let PMDs fixing the use case #2 when they have a chance. In my opinion, the use case #2 was never advertised before, so it is just opening the way for the support of this use case. > [1] http://patches.dpdk.org/project/dpdk/list/?series=1966