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 8AA711B70D for ; Wed, 10 Oct 2018 18:43:26 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 0E9C122050; Wed, 10 Oct 2018 12:43:26 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 10 Oct 2018 12:43: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=xtFtWlPocfH705ubpvzOomgL8a9w+SVu1AgIVqN4MCE=; b=PO2yghjDf8Gb C9y87ektye9r2VaDzNbQX2jX01HBz1O1SQ5WVAMlJze0yX/5mz9ullfI7wcSaTI8 TihPimeTic4i+Kfwg/E8cFHMjwWj7X9r3x0SQdM2rZng/NYkVjndAS6G5n2Dpaa8 2rE6SEYpv9HJFe+Zi2B1OMdJ6Zpg0Z4= 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=xtFtWlPocfH705ubpvzOomgL8a9w+SVu1AgIVqN4M CE=; b=ORfthCHLklCnT9E/L+8xKUZs/u9M6hLaHO+eNSFBf9vOQItGumqXHfZmk 4EOoZMlilpHJ/K0KSX5yg5Fwg2HoDlKmoHL3OGmwVF+v6s6LbqNqyeN8lKSf2419 9V5H//NEwVX3iW85E4OEIuyoXwbIKDIEDYpEQLHCN6ID7OidQLZct4TIi+CDHchO Vgk1kejOyF+sfMgIT1f2EoIDKfBj+6h7nqkoT83GzFiX/+47yyKOz5QARfVwg4hI P+b/C4c7yyVLvqlE02CWf5DdIk8FuTCISMBVLkuBj2n2gpti7qov2qyjVAm74l9q CB1NP3lFyfPEgXmo3MSBs8kgMQfMQ== 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 D35F7E4074; Wed, 10 Oct 2018 12:43:23 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko Cc: ferruh.yigit@intel.com, dev@dpdk.org, ophirmu@mellanox.com Date: Wed, 10 Oct 2018 18:43:21 +0200 Message-ID: <2227895.hyEOWiVRGC@xps> In-Reply-To: References: <20180907233929.21950-1-thomas@monjalon.net> <1961538.UcvSjTfz0W@xps> 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 16:43:26 -0000 10/10/2018 17:01, Andrew Rybchenko: > On 10/10/18 11:39 AM, Thomas Monjalon wrote: > > 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. > > If the patch is applied, close frees dev_private which is used by uninit. > So, uninit must be done first. Who does it? > (it looks like I'm missing something obvious, but still can't find it) Yes, you missed my explanation above :) Let me try again: rte_eth_dev_release_port() does 3 things: - RTE_ETH_EVENT_DESTROY notification - state = RTE_ETH_DEV_UNUSED - memset data to 0 Because of state == RTE_ETH_DEV_UNUSED and data->name == NULL, you should not try to use data->dev_private. Before calling uninit function, the dev is retrieved by name: ethdev = rte_eth_dev_allocated(ethdev->data->name); if (!ethdev) return -ENODEV; In our case, it will be -ENODEV, which is a good return when trying to release a closed port. Now I am thinking that PMDs could ignore this -ENODEV error: it is OK to free the rte_device with ethdev port already closed. > > - 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.