From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 0672814EC for ; Wed, 7 Nov 2018 18:46:13 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Nov 2018 09:46:13 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,476,1534834800"; d="scan'208";a="104258818" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga004.fm.intel.com with ESMTP; 07 Nov 2018 09:46:13 -0800 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 7 Nov 2018 09:46:12 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 7 Nov 2018 09:46:12 -0800 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.161]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.214]) with mapi id 14.03.0415.000; Thu, 8 Nov 2018 01:46:10 +0800 From: "Zhang, Qi Z" To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: Thomas Monjalon , "dev@dpdk.org" , "Yigit, Ferruh" Thread-Topic: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice Thread-Index: AQHUdWgBdNm8YH3IMkueZLyDXXxNA6VB7W2AgADwpZD//9HDAIAAMZgAgAGnR5D//4FRgIAAihIg Date: Wed, 7 Nov 2018 17:46:09 +0000 Message-ID: <039ED4275CED7440929022BC67E70611532E0F6F@SHSMSX103.ccr.corp.intel.com> References: <20181106003150.10560-1-qi.z.zhang@intel.com> <11443385.dze8hbQCXQ@xps> <039ED4275CED7440929022BC67E70611532E0279@SHSMSX103.ccr.corp.intel.com> <2180900.HKzicAuZ6Y@xps> <20181106233352.qhe7kuhgiexpvpih@bidouze.vm.6wind.com> <039ED4275CED7440929022BC67E70611532E0AC3@SHSMSX103.ccr.corp.intel.com> <20181107171525.urcwrvaqgh7e7amq@bidouze.vm.6wind.com> In-Reply-To: <20181107171525.urcwrvaqgh7e7amq@bidouze.vm.6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiY2Q5MDA5NTEtYmM4Yi00ZTFhLWE5NmItMjc3MGY5MmQ1MGUzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoidkg4QjFUdmVJSG9GVnA1bDdnTk8zSnJWd1VRZFh5bG5JNDRVS2pVdmVGYXdjZFwvNzlvRFZXRDBaZXpSbTF1TTEifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Wed, 07 Nov 2018 17:46:14 -0000 > -----Original Message----- > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > Sent: Wednesday, November 7, 2018 10:15 AM > To: Zhang, Qi Z > Cc: Thomas Monjalon ; dev@dpdk.org; Yigit, Ferruh > > Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice >=20 > On Wed, Nov 07, 2018 at 04:53:50PM +0000, Zhang, Qi Z wrote: > > > > > > > -----Original Message----- > > > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > > > Sent: Tuesday, November 6, 2018 4:34 PM > > > To: Thomas Monjalon > > > Cc: Zhang, Qi Z ; dev@dpdk.org; Yigit, Ferruh > > > > > > Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device > > > twice > > > > > > 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 device= . > > > > > (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 m= ap 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-reference= 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 vd= ev_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. Clone is not a problem if they are encapsulated well, what we need here is = some API like rte_dev_set_devargs/rte_dev_clear_devargs, and developer just need to remem= ber to use them but not assign devargs directly.=20 The point here is remove an item in devargs should not destroy the content = in rte_device at the same time (it happens on vdev and I didn't see a fix b= ase on exist proposal), I have no objection for other way to fix this, but = clone is the only way I can figure out right now. >=20 > It seems we have identified in this thread problematic behaviors from > developpers, instead of giving them more tools to shoot feet we can inste= ad > 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 > > > > > > -- > > > Ga=EBtan Rivet > > > 6WIND >=20 > -- > Ga=EBtan Rivet > 6WIND