[dpdk-dev] [PATCH v3 4/8] eal: fix hotplug add / remove

Gaëtan Rivet gaetan.rivet at 6wind.com
Wed Jul 12 19:33:42 CEST 2017


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 at 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 at 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


More information about the dev mailing list