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 8029C5592 for ; Wed, 24 Oct 2018 16:43:43 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 1338D2200A; Wed, 24 Oct 2018 10:43:43 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Wed, 24 Oct 2018 10:43:43 -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=17hqH4TFec36Dm44BTTPQ05zwinAQT+F1W5OCezo42c=; b=OJu7ln4wX5KV OIwhl629R2gExRH+i0Em5mEMT4tjratBG1icmRqhmgghoh7GLbhVNr1fnbpZuJaI ufY1Z9tkOg9X+QZ4/zaUhMdCs7uijf0olfW7Ni2sbtbs9uJgD3h+Dkj3Lkj80WOV Mp5bMJ6vO+mfws8TyJoqerc5o4PWczw= 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=17hqH4TFec36Dm44BTTPQ05zwinAQT+F1W5OCezo4 2c=; b=mfEfSmNA73OWjDCIYfTZBlVYAtQDwWq/6mK1+TBUlosiHHoNpYeM9pKk3 H8xXExLQD3OuZpCQ7dOLD1w4JddKXjyV3/jMYXSQ1Vbw65rUQ6hkQxDuIbeofUnG C2BbNdHVEs0GAnVCI+RZHVnasV3pwB71nHmS65lfCrlh+19g20xQMRSBkVLqTSv2 jphNadWKudzs+HphzTrJ7+6OKUR3FVjgaHICqIhn/IXe7eqnlg0xOyUElfBeFNxp xSZUF/EXMNI+lenVn/l7z42gnLK0rCgilI7RITxk2TlbcuZ/3p9va3d8RO1kEy9k XAaU6qwoZceYHQ47WIj+s4KYqxxSQ== 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 ABC1F102DE; Wed, 24 Oct 2018 10:43:41 -0400 (EDT) From: Thomas Monjalon To: =?ISO-8859-1?Q?Ga=EBtan?= Rivet Cc: dev@dpdk.org, Qi Zhang Date: Wed, 24 Oct 2018 16:43:45 +0200 Message-ID: <6647495.inI2yHHxz0@xps> In-Reply-To: <20181023223931.kmro2zfyp4c4wbqm@bidouze.vm.6wind.com> References: <20181022054932.39052-1-qi.z.zhang@intel.com> <1576298.HKmtsfqzoT@xps> <20181023223931.kmro2zfyp4c4wbqm@bidouze.vm.6wind.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" Subject: Re: [dpdk-dev] [PATCH] eal: fix floating device argument pointer 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, 24 Oct 2018 14:43:43 -0000 24/10/2018 00:39, Ga=EBtan Rivet: > Hi, >=20 > On Mon, Oct 22, 2018 at 09:25:22AM +0200, Thomas Monjalon wrote: > > 22/10/2018 07:49, Qi Zhang: > > > After we insert a devargs into devargs_list, following bus->scan may > > > destroy it due to another rte_devargs_insert. Its better not to use > > > a devargs pointer after it has been inserted. > >=20 >=20 > A bus scan calls rte_devargs_insert? Mapping devargs to device is the > responsibility of the bus scan, if it calls potentially destructive > functions, it must rebuild the map. >=20 > > I think the problem is in: > >=20 > > rte_devargs_insert(struct rte_devargs *da) > > { > > int ret; > >=20 > > ret =3D rte_devargs_remove(da); = =20 > > if (ret < 0) > > return ret; > > TAILQ_INSERT_TAIL(&devargs_list, da, next); > > return 0; > > } > >=20 > > We insert a structure which is freed! >=20 > Not usually, I hope! >=20 > >=20 > > See http://git.dpdk.org/dpdk/commit/?id=3D55744d83d525 > >=20 > > Gaetan, what can be the fix? >=20 > 1. rte_devargs_insert is misdefined. > It is designed as a function that can never fail. > The function should return void instead. >=20 > 2. rte_devargs_remove(da), will not remove da itself. > It will remove whichever rte_devargs matches da within the internal > list. If da does not match any in the list, it does nothing. > As da is a newly-callocated structure, it is actually safe to > continue using it after having called rte_devargs_remove(), because > it cannot possibly have been inserted in the meantime (so would not > have been freed, even if another devargs matched it). If the devargs pointer passed in parameter is the same as the one in the list, it will be freed. > The actual issue is that the matching rte_devargs within the list > would be referenced by a device after a successful scan, meaning that > this reference is not safe if someone attemps to insert the same > device after the bus->scan(). If my understanding is correct, the above > fix is not necessary, but probing should be guarded against > re-entrancy. We may want to probe again with different parameters. > 3. To fix this bug, one should check that the device one attempts to > hotplug does not already exists as a probed rte_device. > An existing rte_devargs is not sufficient, because a blacklisted > device would have an rte_devargs without having a probed rte_device, > and the current behavior is to supersede the current blacklist and > forcibly insert the new device, as if it was newly whitelisted. > This check can only happen at rte_dev level. >=20 > 4. Your confusion about rte_devargs_remove is understandable, the API is > muddy. The reason for these quirks is because I wanted a user > to be able to remove any devargs, even without having a direct > reference to it: you only had to define the bus and the device id > (name), and it would find it and remove it. It might be preferrable > to force the user to find the rte_device, and from it, use the actual > rte_devargs reference to remove it, but then, it would be impossible > to remove devargs for non-existing devices (spoiler: that's the > blacklisted ones). >=20 > 5. It bears repeating: blacklisted mode is horrible and should be removed. > It is all-around abominable, forces unsightly designs to exist and be > used, makes managers ask questions about "why do you add this quirky > `-w 00:00.0` parameter to your command line and what is your timeline > for not needing it?", makes at least one team integrating OVS ask > themselves "why not --no-pci? but then why can't I hotplug PCI ports?", > and I would not be surprised if it killed puppies as a hobby. >=20 > So far, I was able to collect "but it simplifies testing bot > configuration" as a plus, which I do not agree with. >=20 > And anyone trying to package DPDK on their platform, expecting users > not to know or care about it, would be better off developping a > proper autoconf tool, instead of baking it in the entrails of the > EAL, which are ugly enough as it is. /rant Nice rant :)