From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f48.google.com (mail-wm0-f48.google.com [74.125.82.48]) by dpdk.org (Postfix) with ESMTP id 833AB2C54 for ; Wed, 12 Jul 2017 19:33:52 +0200 (CEST) Received: by mail-wm0-f48.google.com with SMTP id i127so1295037wma.0 for ; Wed, 12 Jul 2017 10:33:52 -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=zxhOW2/7X72Zllk/doSWN8W5sjxGfmrxtfTpf98eP2Y=; b=SQPk84PylgLa8gTGF8qxYsFSOLoMn1FzIhlOpc5tYm5FQCEenYI7qsoLbx7pesyuTI vGMJvw4P7hplwfgI+kmtzzHP1gAUxxBBOYWmy1U9YAWTQ5nKAN+N2i0uQnTrIpzUW6LT aQv6TBJEOtHLyTOc6JXqywZvCOHugXMIguhEBQOgQMX8Z7LtzKjm+v4LFDypai7tU5eX U1zS5pxmSg2ngA+neF1AdzEJK4vwgoF5BLsjD9r8YpLmp4IcersOa1qCAHAJroOxsLmS HuC25dmIqCRDNLzGUU3ucTCn/5+KiE4suiFAOeXKGOSgnQ37xgdBRN/KilEuM/ruKq8r uEdQ== 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=zxhOW2/7X72Zllk/doSWN8W5sjxGfmrxtfTpf98eP2Y=; b=NBOpWTnuUHQP/nMZPgBN2H5scO+fkMb03Qk9LYnBTRalDRICsLmRxm+53L/wuMTFx9 O7t6AATX7Xue3aHCqn3vtk6oUNr0GiGhb16cQiDjisi1LupTKiDeMU9Chjw7Czm9JSM7 1hDLeXks+JROGKUMTxilQEu/bLUDGYiFtkx4gCwixRUV8qcv8mzbi62tnWR/4ECgaKtx WZlCOsWdLSG+E/LL7WMF1wZyp9TS35j3YqfaqXaS0F3Yc3pZPpDm7IU0MXw+H02qF/V8 68JMuo2l2JVoQj02C3yamBPXN79ylKDJBR86ulb8inCDQ+5VhItq05KtDjzUQADkCa3+ RRLQ== X-Gm-Message-State: AIVw112kJWtmZiay79GilOdnjXt1qBK63bhyS5mhDnXiPJUOz2w9eesh Wb5RkPTpd9Depypy X-Received: by 10.28.125.3 with SMTP id y3mr3542310wmc.9.1499880831850; Wed, 12 Jul 2017 10:33:51 -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 e31sm3967164wre.54.2017.07.12.10.33.50 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 12 Jul 2017 10:33:50 -0700 (PDT) Date: Wed, 12 Jul 2017 19:33:42 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Jan Blunck Cc: dev Message-ID: <20170712173342.GF11154@bidouze.vm.6wind.com> References: <1eb5a98019675d97547ec3965738d23b350302ab.1499814957.git.gaetan.rivet@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-dev] [PATCH v3 4/8] eal: fix hotplug add / remove 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, 12 Jul 2017 17:33:53 -0000 On Wed, Jul 12, 2017 at 04:44:28AM -0400, Jan Blunck wrote: > On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet wrote: > > The hotplug API requires a few properties that were not previously > > explicitly enforced: > > > > - Idempotency, two consecutive scans should result in the same state. > > - Upon returning, internal devices are now allocated and available > > through the new `find_device` operator, meaning that they should be > > identifiable. > > > > The current rte_eal_hotplug_add implementation identifies devices by > > their names, as it is readily available and easy to define. > > > > The device name must be passed to the internal rte_device handle in > > order to be available during scan, when it is then assigned to the > > device. The current way of passing down this information from the device > > declaration is through the global rte_devargs list. > > This only true for the virtual bus (vdev). > The way the hotplug API is designed at the moment, the devname passed to it must match the name of the scanned device. Without this match, the hotplug function cannot work, so while this was only true for the vdev bus, this becomes a requirement to support hotplug. The alternative is to be able to transform a "user-centric" name to a canonical name by a bus, which would then allow to search for the canonical name to find the newly scanned device. > > > > Furthermore, the rte_device cannot take a bus-specific generated name, > > as it is then not identifiable by the `find_device` operator. The device > > must take the user-defined name. Ideally, an rte_device name should not > > change during its existence. > > > > This commit generates a new rte_devargs associated with the plugged > > device and inserts it in the global rte_devargs list. It consequently > > releases it upon device removal. > > So unplugging a device which devargs have been passed via the command > line is implicitly removing the command line parameter? This is a > surprising side-effect and it sound wrong to do. What happens if the > device is getting replugged? > If the device is replugged, then the only way to use is to call hotplug_add, meaning that an rte_devargs is recreated with the devname and devargs strings. There is no way for an application currently to be sure that the new device is actually the same as the one that was previously removed. It may share the same PCI address, but nothing tells us that this is actually the same one and that it can take the same arguments. Managing this discrepancy means some involvement anyway, memoizing the previous devargs if necessary is nothing in comparison. > It seems to me that you want to make the hotplug add functionality > behave like a force attach. You can achieve this by calling > rte_eal_devargs_add/parse before calling into hotplug add. > > What do you mean by force attach? Sorry if it's obvious. To me, the way the hotplug is designed, every user would need to use rte_eal_devargs_add/parse beforehand, so it makes sense to put it within the function and spare the code lines. > > Fixes: a3ee360f4440 ("eal: add hotplug add/remove device") > > > > Signed-off-by: Gaetan Rivet > > --- > > lib/librte_eal/common/eal_common_dev.c | 57 ++++++++++++++++++++++++++++++++-- > > 1 file changed, 54 insertions(+), 3 deletions(-) > > > > diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c > > index 32e12b5..f5566a6 100644 > > --- a/lib/librte_eal/common/eal_common_dev.c > > +++ b/lib/librte_eal/common/eal_common_dev.c > > @@ -118,11 +118,32 @@ int rte_eal_dev_detach(struct rte_device *dev) > > return ret; > > } > > > > +static char * > > +full_dev_name(const char *bus, const char *dev, const char *args) > > +{ > > + char *name; > > + size_t len; > > + > > + len = strlen(bus) + 1 + > > + strlen(dev) + 1 + > > + strlen(args) + 1; > > + name = calloc(1, len); > > + if (name == NULL) { > > + RTE_LOG(ERR, EAL, "Could not allocate full device name\n"); > > + return NULL; > > + } > > + snprintf(name, len, "%s:%s,%s", bus, dev, > > + args ? args : ""); > > + return name; > > +} > > + > > int rte_eal_hotplug_add(const char *busname, const char *devname, > > const char *devargs) > > { > > struct rte_bus *bus; > > struct rte_device *dev; > > + struct rte_devargs *da; > > + char *name; > > int ret; > > > > bus = rte_bus_find_by_name(busname); > > @@ -137,21 +158,49 @@ int rte_eal_hotplug_add(const char *busname, const char *devname, > > return -ENOTSUP; > > } > > > > + name = full_dev_name(busname, devname, devargs); > > + if (name == NULL) > > + return -ENOMEM; > > + > > + da = calloc(1, sizeof(*da)); > > + if (da == NULL) { > > + ret = -ENOMEM; > > + goto err_name; > > + } > > + > > + ret = rte_eal_devargs_parse(name, da); > > + if (ret) > > + goto err_devarg; > > + > > + ret = rte_eal_devargs_insert(da); > > + if (ret) > > + goto err_devarg; > > + > > ret = bus->scan(); > > if (ret) > > - return ret; > > + goto err_devarg; > > > > dev = bus->find_device(NULL, cmp_detached_dev_name, devname); > > if (dev == NULL) { > > RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n", > > devname); > > - return -EINVAL; > > + ret = -ENODEV; > > + goto err_devarg; > > } > > > > ret = bus->plug(dev, devargs); > > - if (ret) > > + if (ret) { > > RTE_LOG(ERR, EAL, "Driver cannot attach the device (%s)\n", > > dev->name); > > + goto err_devarg; > > + } > > + free(name); > > + return 0; > > + > > +err_devarg: > > + rte_eal_devargs_remove(busname, devname); > > +err_name: > > + free(name); > > return ret; > > } > > > > @@ -179,6 +228,8 @@ int rte_eal_hotplug_remove(const char *busname, const char *devname) > > return -EINVAL; > > } > > > > + rte_eal_devargs_remove(busname, devname); > > + dev->devargs = NULL; > > ret = bus->unplug(dev); > > if (ret) > > RTE_LOG(ERR, EAL, "Driver cannot detach the device (%s)\n", > > -- > > 2.1.4 > > -- Gaëtan Rivet 6WIND