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 7A2B31B3DA for ; Wed, 10 Oct 2018 10:39:23 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 107032207E; Wed, 10 Oct 2018 04:39:21 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Wed, 10 Oct 2018 04:39: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=fmpwI4TJ02pVjGuMRQ8SfJvChzJ912dPuRNa/4EvDN0=; b=jRivPKs8LUoU Ks2SgWzYK6VD4M/HO227iOxc1nFTfz1wuQ5K4uNVX8HDTKNCqm2/edqSiB49sKUN aqq9wHQbn/3cCzm/LJFVk+rjTr03804rB5yPN6BFhISxrqoweQyzmTULU5GvTpsx ycZYZW36wvfZFJ7GqouGGleMikWmeMc= 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=fmpwI4TJ02pVjGuMRQ8SfJvChzJ912dPuRNa/4EvD N0=; b=D0cxP2ruR3ymmod++7WTo3Vv+QahKIhe1g8hQZhHk8rP6wMmbS/wR7W3v wZCir8t60vrTV/8ORjdJMEDENPZn67YPhviTR0wnIuoNfu90FhaGqxqK9D82pHpw XrwBRASq2O+ron0E+e63G6E3ZxzVTgxNxgEVBzQtIJtf0cBB+6B5C0wUTU55jHop MCWB37e/+88ld13SIAZo+OtySdTRWbejs3auIoBkRepazOPNwouz0IfHyRCYMDSe 9PgpTHyAI+Jnn3742DrxE+xpzzBmfHGvNGSGl7D6sUy3VY3tfNn9FUn320jj93jB mt5hyrXMK3y9gBZ4leeVu60Ri+i/A== X-ME-Sender: X-ME-Proxy: Received: from xps.localnet (239.43.136.77.rev.sfr.net [77.136.43.239]) by mail.messagingengine.com (Postfix) with ESMTPA id 5AEAB102DE; Wed, 10 Oct 2018 04:39:19 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko Cc: ferruh.yigit@intel.com, dev@dpdk.org, ophirmu@mellanox.com Date: Wed, 10 Oct 2018 10:39:14 +0200 Message-ID: <1961538.UcvSjTfz0W@xps> In-Reply-To: <5f013fa6-762c-0d7e-a057-a8225bed7634@solarflare.com> References: <20180907233929.21950-1-thomas@monjalon.net> <7172106.4TAESsr7Yr@xps> <5f013fa6-762c-0d7e-a057-a8225bed7634@solarflare.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v2] 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: Wed, 10 Oct 2018 08:39:23 -0000 10/10/2018 09:50, Andrew Rybchenko: > On 10/10/18 10:44 AM, Thomas Monjalon wrote: > > 10/10/2018 08:15, Andrew Rybchenko: > >> On 10/10/18 1:17 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 (dev_private and final release), which were done > >>> by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close(). > >>> > >>> 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 > >>> --- > >>> lib/librte_ethdev/rte_ethdev.c | 6 ++++++ > >>> lib/librte_ethdev/rte_ethdev.h | 3 +-- > >>> 2 files changed, 7 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > >>> index ed83e5954..3062dc711 100644 > >>> --- a/lib/librte_ethdev/rte_ethdev.c > >>> +++ b/lib/librte_ethdev/rte_ethdev.c > >>> @@ -506,6 +506,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev) > >>> { > >>> if (eth_dev == NULL) > >>> return -EINVAL; > >>> + if (eth_dev->state == RTE_ETH_DEV_UNUSED) > >>> + return -ENODEV; > >>> > >>> rte_eth_dev_shared_data_prepare(); > >>> > >>> @@ -1441,6 +1443,10 @@ rte_eth_dev_close(uint16_t port_id) > >>> dev->data->nb_tx_queues = 0; > >>> rte_free(dev->data->tx_queues); > >>> dev->data->tx_queues = NULL; > >>> + > >>> + rte_free(dev->data->dev_private); > >> It is used by, for example, PCI device uninit functions. > >> What does guarantee that uninit is done and we can free the private data. > > The state of the port is set to UNUSED and the name is NULL. > > So nobody should try to use it anymore. > > There are already some checks before calling uninit functions. > > For instance, in rte_eth_dev_pci_generic_remove(), > > rte_eth_dev_allocated() will return NULL and won't call uninit function. > > The questions are: > Is application allowed to call the function? When? > Who calls uninit in this case? (What does guarantee that uninit is done > before close) So far, everything is allowed: - The application can close a port and remove the rte_device later. - The application can remove the rte_device and expect the PMD is closing associated ports. In other words, when rte_device is removed, the ports should be closed by the PMD, except if the application has already closed the ports. It means ethdev port close is optional, but EAL removal is always required. The behaviour is not changed. If we want to go further, we could change the behaviour of the close op, by asking the PMD to remove the rte_device automatically if all associated ports are closed. It would allow the application to manage only ports at ethdev layer without bothering with low-level EAL management. We can think about it as a planned change for next releases.