From: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
To: Jan Blunck <jblunck@infradead.org>
Cc: dev <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3 4/8] eal: fix hotplug add / remove
Date: Wed, 12 Jul 2017 19:33:42 +0200 [thread overview]
Message-ID: <20170712173342.GF11154@bidouze.vm.6wind.com> (raw)
In-Reply-To: <CALe+Z02jheQ8QxJp1cYAedOJ3MGYkJoZJcZWGNCAxxmPy8RG2A@mail.gmail.com>
On Wed, Jul 12, 2017 at 04:44:28AM -0400, Jan Blunck wrote:
> On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet <gaetan.rivet@6wind.com> 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 <gaetan.rivet@6wind.com>
> > ---
> > 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
next prev parent reply other threads:[~2017-07-12 17:33 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-09 1:45 [dpdk-dev] [PATCH 0/9] fix hotplug API Gaetan Rivet
2017-07-09 1:45 ` [dpdk-dev] [PATCH 1/9] eal: return device handle upon plugin Gaetan Rivet
2017-07-09 1:45 ` [dpdk-dev] [PATCH 2/9] eal: fix hotplug add Gaetan Rivet
2017-07-09 1:45 ` [dpdk-dev] [PATCH 3/9] devargs: introduce removal function Gaetan Rivet
2017-07-09 1:45 ` [dpdk-dev] [PATCH 4/9] eal: release devargs on device removal Gaetan Rivet
2017-07-09 1:45 ` [dpdk-dev] [PATCH 5/9] pci: use given name as generic name Gaetan Rivet
2017-07-09 1:45 ` [dpdk-dev] [PATCH 6/9] pci: fix generic driver pointer on probe error Gaetan Rivet
2017-07-09 1:45 ` [dpdk-dev] [PATCH 7/9] pci: fix hotplug operations Gaetan Rivet
2017-07-09 1:45 ` [dpdk-dev] [PATCH 8/9] vdev: implement plug operation Gaetan Rivet
2017-07-09 1:45 ` [dpdk-dev] [PATCH 9/9] bus: remove useless plug parameter Gaetan Rivet
2017-07-09 10:38 ` [dpdk-dev] [PATCH 0/9] fix hotplug API Jan Blunck
2017-07-09 12:19 ` Gaëtan Rivet
2017-07-09 14:19 ` Thomas Monjalon
[not found] ` <20170711160211.GW11154@bidouze.vm.6wind.com>
[not found] ` <CALe+Z00Rc_6cn_ckWrK1cD2RsdWENM3s+ytOpxNymTmaqh8e0g@mail.gmail.com>
2017-07-11 19:21 ` Gaëtan Rivet
2017-07-10 23:18 ` [dpdk-dev] [PATCH v2 0/8] " Gaetan Rivet
2017-07-10 23:19 ` [dpdk-dev] [PATCH v2 1/8] vdev: implement plug operation Gaetan Rivet
2017-07-10 23:19 ` [dpdk-dev] [PATCH v2 2/8] devargs: introduce removal function Gaetan Rivet
2017-07-10 23:19 ` [dpdk-dev] [PATCH v2 3/8] devargs: introduce insert function Gaetan Rivet
2017-07-11 22:13 ` Thomas Monjalon
2017-07-10 23:19 ` [dpdk-dev] [PATCH v2 4/8] eal: fix hotplug add / remove Gaetan Rivet
2017-07-11 22:18 ` Thomas Monjalon
2017-07-10 23:19 ` [dpdk-dev] [PATCH v2 5/8] pci: use given name as generic name Gaetan Rivet
2017-07-11 22:05 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2017-07-10 23:19 ` [dpdk-dev] [PATCH v2 6/8] pci: fix generic driver pointer on probe error Gaetan Rivet
2017-07-10 23:19 ` [dpdk-dev] [PATCH v2 7/8] pci: fix hotplug operations Gaetan Rivet
2017-07-10 23:19 ` [dpdk-dev] [PATCH v2 8/8] bus: remove useless plug parameter Gaetan Rivet
2017-07-11 23:25 ` [dpdk-dev] [PATCH v3 0/8] fix hotplug API Gaetan Rivet
2017-07-11 23:25 ` [dpdk-dev] [PATCH v3 1/8] vdev: implement plug operation Gaetan Rivet
2017-07-11 23:25 ` [dpdk-dev] [PATCH v3 2/8] devargs: introduce removal function Gaetan Rivet
2017-07-12 8:27 ` Jan Blunck
2017-07-12 17:22 ` Gaëtan Rivet
2017-07-11 23:25 ` [dpdk-dev] [PATCH v3 3/8] devargs: introduce insert function Gaetan Rivet
2017-07-12 8:20 ` Jan Blunck
2017-07-12 17:20 ` Gaëtan Rivet
2017-07-13 19:44 ` Jan Blunck
2017-07-11 23:25 ` [dpdk-dev] [PATCH v3 4/8] eal: fix hotplug add / remove Gaetan Rivet
2017-07-12 8:44 ` Jan Blunck
2017-07-12 17:33 ` Gaëtan Rivet [this message]
2017-07-11 23:25 ` [dpdk-dev] [PATCH v3 5/8] pci: use given name as generic name Gaetan Rivet
2017-07-11 23:25 ` [dpdk-dev] [PATCH v3 6/8] pci: fix generic driver pointer on probe error Gaetan Rivet
2017-07-11 23:25 ` [dpdk-dev] [PATCH v3 7/8] pci: fix hotplug operations Gaetan Rivet
2017-07-11 23:25 ` [dpdk-dev] [PATCH v3 8/8] bus: remove useless plug parameter Gaetan Rivet
2017-07-15 17:56 ` [dpdk-dev] [PATCH v4 0/8] fix hotplug API Gaetan Rivet
2017-07-15 17:56 ` [dpdk-dev] [PATCH v4 1/8] vdev: implement plug operation Gaetan Rivet
2017-07-15 17:56 ` [dpdk-dev] [PATCH v4 2/8] devargs: introduce removal function Gaetan Rivet
2017-07-15 17:56 ` [dpdk-dev] [PATCH v4 3/8] devargs: introduce insert function Gaetan Rivet
2017-07-15 17:56 ` [dpdk-dev] [PATCH v4 4/8] eal: fix hotplug add / remove Gaetan Rivet
2017-07-15 17:56 ` [dpdk-dev] [PATCH v4 5/8] pci: use given name as generic name Gaetan Rivet
2017-07-15 17:56 ` [dpdk-dev] [PATCH v4 6/8] pci: fix generic driver pointer on probe error Gaetan Rivet
2017-07-15 17:56 ` [dpdk-dev] [PATCH v4 7/8] pci: fix hotplug operations Gaetan Rivet
2017-07-15 17:56 ` [dpdk-dev] [PATCH v4 8/8] bus: remove useless plug parameter Gaetan Rivet
2017-07-19 21:27 ` [dpdk-dev] [PATCH v4 0/8] fix hotplug API Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170712173342.GF11154@bidouze.vm.6wind.com \
--to=gaetan.rivet@6wind.com \
--cc=dev@dpdk.org \
--cc=jblunck@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).