From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f182.google.com (mail-wr0-f182.google.com [209.85.128.182]) by dpdk.org (Postfix) with ESMTP id 782785905 for ; Wed, 16 Aug 2017 17:26:18 +0200 (CEST) Received: by mail-wr0-f182.google.com with SMTP id y96so15858521wrc.1 for ; Wed, 16 Aug 2017 08:26:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=te+QbhwT99DbyjsjC3nnuTzSn8fDYZEgxlF8RFtLg+o=; b=HwKYP9ZGBsw8UzwUMkTCwQEMraieYlmxH3ueNHsYU42bROf2mMP1zan04XCSSC48XO SjY0UlRXL7RBDaxtNAk+8Oe6K7df/WfUoHJ5GS4PcXmTrLDi8RiT7i+JMsE2E+3fUYgf 5nGAmFUwdhWknjtDOdQwypawJizL1NDSZT7x8Iz3rbPvmxyOFspQKwVo1nOsRkladAK5 biwhxzD85f5/oBwnkE/H2jZju0iBE35kbNeO/fhZv9pjT2cFv+DNn5RBgqMcPyTsMQA9 YD8csazniGLSn5Gbb8Xq5KR9sQgPrmdOypWWNxILCJRw6OiVpPRhj2iqYCbcc5/YqUx1 5pxw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=te+QbhwT99DbyjsjC3nnuTzSn8fDYZEgxlF8RFtLg+o=; b=QYbVHW5EOmJc3+nOXk/g1O6aSsw1rtA3wxuD987cHvsA/r1No/blGzyrp9q2cyCPst 73iJi2T9HXgftrxVl8+3MZwVtFUkJz8aFYtjWj+E3iowk19BVY8Q/ooSJ/AIlCtyhcav L52/sKW9mbxsuNk8bjAgBS7QxrUJ+8vyt65wVKV4/YG8EypYGRU+qh7j30b5OiUdt/Yq Ak9fP3TJQ2O22y2JvFddJ7xZmYnyLbwjjnnd9vQXSMTMSRJDNP7+kzN0B8VF6S6Wmzmt YLc5Ho9G2rgk7st2iw7WyQ5RmH9vGyXH9JTsqLrFnvh8TcUgzp29rDOGXtXkFjicOOwG N8JQ== X-Gm-Message-State: AHYfb5jlUzTqLE78bNhFVL06ooL4yKsYSRQuTlVoaB6fUJLQHfOdUm+r l3S1Z54M/BQot0ij7j0= X-Received: by 10.223.128.47 with SMTP id 44mr1528347wrk.175.1502897177933; Wed, 16 Aug 2017 08:26:17 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id q141sm2180099wmb.11.2017.08.16.08.26.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 16 Aug 2017 08:26:16 -0700 (PDT) Date: Wed, 16 Aug 2017 17:26:07 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Shahaf Shuler Cc: Thomas Monjalon , "dev@dpdk.org" , "stable@dpdk.org" Message-ID: <20170816152607.GO8124@bidouze.vm.6wind.com> References: <20170816114308.165850-1-shahafs@mellanox.com> <20170816124130.GL8124@bidouze.vm.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-stable] [PATCH] ethdev: fix device state on close X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Aug 2017 15:26:18 -0000 On Wed, Aug 16, 2017 at 02:17:16PM +0000, Shahaf Shuler wrote: > Wednesday, August 16, 2017 3:42 PM, Gaëtan Rivet: > > On Wed, Aug 16, 2017 at 02:43:08PM +0300, Shahaf Shuler wrote: > > > Currently device state moves between ATTACHED when device was > > > successfully probed to UNUSED when device is detached or released. > > > > > > The device state following rte_eth_dev_close() operation is inconsist, > > > The device is still in ATTACHED state, however it cannot be used in > > > any way till it will be probed again. > > > > > > Fixing it by changing the state to UNUSED. > > > > > > > You are right that simply closing the device leaves it in a unusable state. > > > > However it seems to be by design. > > Most drivers call `rte_eth_dev_release_port` when being removed, which > > sets the state to RTE_ETH_DEV_UNUSED. > > > > If I'm not mistaken, the API of rte_eth_dev_close is that the only available > > action should then be to detach the driver. At least PCI and vdev buses > > expects a `remove` callback from their driver, which can be called by the user > > (previously using specific API like `rte_eal_vdev_uninit` for example, now > > using `rte_eal_hotplug_remove` or `rte_eth_dev_detach` from the ether > > layer). > > > > So, it seems that this burden lies with the driver which should call the proper > > API when removing their device. > > Even though it is reasonable for driver to call the rte_eth_dev_port_release, I still think the ethdev layer should protect from such bad behavior from the application side. > It is more robust than counting on the different PMD to implement such release. > The ethdev layer does so in the rte_eth_dev_detach function, which is currently the only way to detach a device from the ethdev level. `rte_eth_dev_detach` setting the device state seems to me to be a crutch against badly implemented drivers. If I was nitpicky I would actually remove it and ideally everyone should enforce the call of rte_eth_dev_release_port from device removal functions when reviewing PMD implementations. The hotplug API is available to applications and the only way to have consistent devices states after calling rte_eal_hotplug_remove is to have drivers using a hook in the ethdev layer allowing to clean-up resources upon release. While it is trivial in its current state, such entry-point in the ethdev layer will be useful and should be kept and enforced IMO. I'm thinking about the 16-bit portid scheduled for v17.11, which implies an arbitrary number of port available. This would imply a dynamic allocation of rte_eth_devices, which would *need* such release hook to be available. Well the API should not be designed from conjectures or speculations of course, but I think it should be useful and there is no reason to remove it. Going further, I find it dangerous to have two points where the port is semantically released (device state set to UNUSED). If the API of the port release changes, we now have two points where we need to enforce the changes. While trivial concerning an enum, it could become more complex / dangerous if this veered toward memory management. > > > > Maybe Thomas will have a better insight about the scope of the > > `rte_eth_dev_close` function. But IMO the API is respected. > > After all, until the proper `dev_detach` function is called, the device is still > > attached, even if closed. > > > > If you disagree, there might possibly be an argument to make about either > > adding finer-grained device states or streamlining the API. This is however a > > discussion about API design and not about its implementation anymore. > > Well my first thought when I created this patch was to add fine-grained device states. However then I read the commit log which changed the device states, specifically : > > " "DEV_DETACHED" is renamed "RTE_ETH_DEV_UNUSED" to better reflect that > the emptiness of a slot is not necessarily the result of detaching a > device." > > Which convince me to reuse the UNUSED state to reflect that this device cannot longer be used (even though it is still attached). > When the device is closed, it could still be in the `RTE_ETH_DEV_DEFERRED` state. This state means that the device is managed by a third party (application or whatever). It is forbidden for the ethdev layer to change the state of the device unless said third-party releases it. The only place where the device state could unequivocally be set to UNUSED is upon the proper release of the device. As far as the ethdev layer is concerned, this is currently within rte_eth_dev_detach. > > > > > Fixes: d52268a8b24b ("ethdev: expose device states") > > > Cc: gaetan.rivet@6wind.com > > > Cc: stable@dpdk.org > > > > > > Signed-off-by: Shahaf Shuler > > > --- > > > lib/librte_ether/rte_ethdev.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/lib/librte_ether/rte_ethdev.c > > > b/lib/librte_ether/rte_ethdev.c index 0597641ee..98d9e929c 100644 > > > --- a/lib/librte_ether/rte_ethdev.c > > > +++ b/lib/librte_ether/rte_ethdev.c > > > @@ -992,6 +992,8 @@ rte_eth_dev_close(uint8_t port_id) > > > dev->data->nb_tx_queues = 0; > > > rte_free(dev->data->tx_queues); > > > dev->data->tx_queues = NULL; > > > + > > > + dev->state = RTE_ETH_DEV_UNUSED; > > > } > > > > > > int > > > -- > > > 2.12.0 > > > > > > > -- > > Gaëtan Rivet > > 6WIND -- Gaëtan Rivet 6WIND