From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <thomas@monjalon.net>
Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com
[66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 6E9961B10A
for <dev@dpdk.org>; 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: <xms:F02-WzbFLM-eeOKiF3LZr8hB2Qs7mtLNughQ8wIl_WM2pd9vUFVbvA>
X-ME-Proxy: <xmx:F02-WymwtEGLxeoOMKy3VpAo8yncj8cGugnIFz1uA1TLGvnfdnUGDQ>
<xmx:F02-W2-dRH70YFneJSSgF4YpZCxj7-EaDbA8HO1LUDEzSGM1E_VhNw>
<xmx:F02-W90ronWwhlLbvbTmInrLCqlx_qwr5xgvTRIgl0Kmgkrcu3ZfGQ>
<xmx:F02-W2u7263lPZpLYX0GhWmleaSgpuzIS1EEIWw7lqD_i6XsILFYlA>
<xmx:F02-Wz-wJ99k-iqfOkWCVu7P1m0_rINAbasif81S_ZT_gGD0aeXKQw>
<xmx:F02-W1kmoUzgPAjTy7wpiFzNeVtvwm_nw2wVSI37VLetDfXvgWAR8A>
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 <thomas@monjalon.net>
To: Andrew Rybchenko <arybchenko@solarflare.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <thomas@monjalon.net>
> >>>>>>> ---
> >>>>>>> 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.