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 6E9961B10A for ; Wed, 10 Oct 2018 21:03:52 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 0844821F4F; Wed, 10 Oct 2018 15:03:52 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 10 Oct 2018 15:03:52 -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=nwl/OXu+5N/mNYxQV3oMEpHG/HPiBHbSoSJTZomNNrc=; b=QKthT4ZQ0TyQ uEClNtWdpS8gUEkVNp5nG99qElgMdp51U5Z/yPOmhBGib8RQtKMjF2D8lYXbBFH/ eQXmLP3KUHUA11rIkdOtRc07AU9eFwqvKHqBEiTVMEOSQp1bRCi9fbsYN/wdiOea knZhUPYlBXW/MIDRAUCtYoXZ6170wVk= 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=nwl/OXu+5N/mNYxQV3oMEpHG/HPiBHbSoSJTZomNN rc=; b=x0hiJyA+mDhtKkLMaA9Ed90u+txb2exH0+PM64PiHEURva5xFeHyrQBzL ok99Pz7o0YqIfe24bi5U9YFB1XeCGYXeJ7SmOQ4xb8z80Eja/pHv7qJO3445ueKi rJR/oudowJ/COk38p04mEO7u4bz9yaJhbynyiLI4nrctHRGXyC8pXUSbWDl4RIbf 1sEZjLTmZwXICLcykMcZTco9a2PqeEkx8qGvU+iwJZCTHERc09Rbv+Yb2bbH7EV8 +RQYnloqK2ffWSr+j37Y9HMbRGwIkK7foQ5KbiM0OlL1c256VS1/EHkeacQoev1C Y1R7+s9PFkDVZ0J+gVwKVq2eaHvtA== 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 D2100E4428; Wed, 10 Oct 2018 15:03:50 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko Cc: ferruh.yigit@intel.com, dev@dpdk.org, ophirmu@mellanox.com Date: Wed, 10 Oct 2018 21:03:49 +0200 Message-ID: <1920542.7PG1eB4q5u@xps> In-Reply-To: <32cc4a78-44e7-f9e8-c2b8-3735e00f5322@solarflare.com> References: <20180907233929.21950-1-thomas@monjalon.net> <2227895.hyEOWiVRGC@xps> <32cc4a78-44e7-f9e8-c2b8-3735e00f5322@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 19:03:52 -0000 10/10/2018 20:01, Andrew Rybchenko: > On 10.10.2018 19:43, Thomas Monjalon wrote: > > 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. > > Yes, it replies on the question why dev_uninit is not called > upon device removal after close. But it does not reply on > the question what does call dev_uninit before/during dev_close. Maybe I don't understand your question correctly. dev_uninit is not called during dev_close. However, as suggested by Ferruh, it would be cleaner to call dev_close instead of dev_uninit which should do the same thing. Currently, most of the PMDs expect both dev_close and dev_uninit to be called in order to completely free a port. The call tree to reach dev_uninit is: [rte_eal_hotplug_remove] rte_dev_remove bus.unplug driver.remove for port not closed dev_uninit free rte_device resources In a next step, I would suggest to drop dev_uninit, and call dev_close instead.