From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wout2-smtp.messagingengine.com (wout2-smtp.messagingengine.com [64.147.123.25]) by dpdk.org (Postfix) with ESMTP id 9185F201 for ; Mon, 12 Nov 2018 01:50:48 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.west.internal (Postfix) with ESMTP id 6DF4740B; Sun, 11 Nov 2018 19:50:47 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Sun, 11 Nov 2018 19:50:47 -0500 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=2m+d1WRwNqrNX2Bw6fLRkMduKpuZB2wHWSoPVw4FDpo=; b=BFqP3zKO57Qg +WrmREaJeAOYYx+BucfUZUyuCTjGyBbq9x1iV7XpzD+m36pFeE32FtkRzLcFxkZ/ XIhHmH/o9vXOYZJWMXAMkkPcxN6P4Yhes6Df/jAV9Ex9Y2uc+dwvIzbF8xdp0OXk D9P2YDDNL5Kc894IMPnUrovTyxd2bME= 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=2m+d1WRwNqrNX2Bw6fLRkMduKpuZB2wHWSoPVw4FD po=; b=CIogFn6DdTt/Dww8boLzlD8caspA9zDW4I8ymmKAjJ/oU2SdtnL/JbH4+ YGaD604PpqTqMv1++4SvHqrVFugZ3NQkzrOc8qrItN/dlFjGjqaGoxEFhrlsZB5A DJQ84NSxMJa3XSxLkrATvNrzO4p3CVQcw0tqPG4mi16kOXs4O5Oo27u/1H+MCvKU FBabIaMU1u0nTdch8AI/LjHK03wEvRurMjVs3/US05uuPhgOudP1n/WeikEIhtCA pr/85weQ5ZKtuM2vJDtyJRzwSqi8oAgSL+CK0eui52sx1S2Idzr17XNP29OXfAWE DLhI8nPCVRunTKV/L7EmDulbC/nQA== 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 F2B33E476E; Sun, 11 Nov 2018 19:50:45 -0500 (EST) From: Thomas Monjalon To: "Zhang, Qi Z" , =?ISO-8859-1?Q?Ga=EBtan?= Rivet Cc: dev@dpdk.org, "Yigit, Ferruh" Date: Mon, 12 Nov 2018 01:50:44 +0100 Message-ID: <6423178.87hqChtvjm@xps> In-Reply-To: <039ED4275CED7440929022BC67E70611532E0F6F@SHSMSX103.ccr.corp.intel.com> References: <20181106003150.10560-1-qi.z.zhang@intel.com> <20181107171525.urcwrvaqgh7e7amq@bidouze.vm.6wind.com> <039ED4275CED7440929022BC67E70611532E0F6F@SHSMSX103.ccr.corp.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice 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: Mon, 12 Nov 2018 00:50:49 -0000 07/11/2018 18:46, Zhang, Qi Z: > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > > On Wed, Nov 07, 2018 at 04:53:50PM +0000, Zhang, Qi Z wrote: > > > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > > > > On Tue, Nov 06, 2018 at 09:36:22PM +0100, Thomas Monjalon wrote: > > > > > 06/11/2018 16:46, Zhang, Qi Z: > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > 06/11/2018 01:31, Qi Zhang: > > > > > > > > When probe the same device at second time > > > > > > > > > > > > > > Sorry I stop on this first sentence. > > > > > > > How and why do you probe a vdev twice? > > > > > > > > > > > > if we do rte_dev_hotplug_add or rte_dev_proble on a probed devi= ce. > > > > > > (yes, this is not usually what an application want, but it can > > > > > > happen by miss-operation, and this is covered by our test case, > > > > > > it make sense to me that hotplug API should be robust enough to > > > > > > handle that situation.) > > > > > > > > > > Yes I agree we must handle this situation. > > > > > > > > > > > we will failed at the second time as expected, but will not able > > > > > > to detach the device any more, since during the second scan, > > > > > > original > > > > vdev->device.devargs is corrupted. > > > > > > > > > > The root cause is we remove a devargs which was referenced. > > > > > Could we overwrite the first devargs instead of removing it? > > > > > > > > > > > > > > > > > > It's also possible to add a back-reference to an rte_device in [1], > > > > but that can only work if only one rte_device references a devargs. > > > > It seems to be the case now, but it might be good to enforce > > > > explicitly that when a bus scans its devices, it should do a 1-to-1= map to > > devargs. > > > > > > > > If mapping rte_device to rte_devargs needs to respect rules, it > > > > could help bus developpers to have a function that will do the job: > > > > verify that the devargs is not currently used, add the back-referen= ce to > > the rte_device. > > > > > > > > With the proper back-reference, it is possible to clean-up the > > > > device when removing the devargs > > > > > > This may still not work for vdev, since the old reference is used in = vdev_find > > to find a exist device by name during scan. > > > (For PCI device, we have pci_addr, but vdev we use devargs->name to > > > identify device, anyway this can be fixed in vdev, but that required a > > > clone on the device name also break the coupling somehow.) > >=20 > > A bus should keep device identifiers within a device, without relying on > > objects belonging to the EAL. > >=20 > > > I just don't understand "why we must tight the tighten the device -> > > devargs coupling, not loosen it" > > > > >=20 > > My point is that we are seemingly having problems with loose pointers, > > broken mappings, memory leaks. So managing seems already too > > complicated. Adding clones and copies will only make it more difficult = to get > > right. >=20 > Clone is not a problem if they are encapsulated well, what we need here i= s some API like > rte_dev_set_devargs/rte_dev_clear_devargs, and developer just need to rem= ember to use them but not assign devargs directly.=20 >=20 > The point here is remove an item in devargs should not destroy the conten= t in rte_device at the same time (it happens on vdev and I didn't see a fix= base on exist proposal), I have no objection for other way to fix this, bu= t clone is the only way I can figure out right now. >=20 > >=20 > > It seems we have identified in this thread problematic behaviors from > > developpers, instead of giving them more tools to shoot feet we can ins= tead > > give helpers to do what they are trying to do, but properly. > >=20 > > The end-goal is not to have several devargs lying around, copies of each > > other, it is to avoid breaking devargs references. > >=20 > > > (and also to add the rte_devargs_extract() function > > > > that would allow keeping the original devargs and insert it back if > > > > the hotplug fails, then the mapping must be restored). > > > > > > > > > > > [1]: https://mails.dpdk.org/archives/dev/2018-November/118274.html This issue is fixed with a different approach: http://git.dpdk.org/dpdk/commit/?id=3Dc7ad7754 devargs: do not replace already inserted device