Re: [dpdk-dev] [PATCH v3 3/8] devargs: introduce insert function

[Jan Blunck <jblunck@infradead.org>]

On Wed, Jul 12, 2017 at 1:20 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> On Wed, Jul 12, 2017 at 04:20:48AM -0400, Jan Blunck wrote:
>> On Tue, Jul 11, 2017 at 7:25 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
>> > Some buses will operate either in whitelist or blacklist mode.
>> > This mode is currently passed down by the rte_eal_devargs_add function
>> > with the devtype argument.
>> >
>> > When inserting devices using the hotplug API, the implicit assumption is
>> > that this device is being whitelisted, meaning that it is explicitly
>> > requested by the application to be used. This can conflict with the
>> > initial bus configuration.
>>
>> Actually I don't think that this is correct. If I blacklist a device
>> via devargs I don't want this to be probed in case my udev helper is
>> calling hotplug add.
>>
>
> This would be good for a udev handler yes. But should we expect this
> behavior from all potential hotplug initiators?
>
> To put it another way: should this rule be enforced at the EAL level or
> from those using it? And is it impossible to imagine a system that would
> actually need to be able to update a device, changing it from
> blacklisted to whitelisted on some specific condition?
>
> The fail-safe at least makes use of this ability. This is at the moment
> the only hotplug user.
>

In tree .... I have hotplug functionality available for quite some time already.


>> Maybe it is better to just update the args field in case the devargs
>> instance is already found.
>>
>> >
>> > While the rte_eal_devargs_add API is being deprecated soon, it cannot
>> > be modified at the moment to accomodate this situation.
>> > As such, this new experimental API offers a bare interface for inserting
>> > rte_devargs without directly manipulating the global rte_devargs list.
>> >
>> > This new function expects a fully-formed rte_devargs, previously parsed
>> > and allocated.
>> >
>> > It does not check whether the new rte_devargs is compatible with current
>> > bus configuration, but will replace any eventual existing one for the same
>> > device, allowing the hotplug operation to proceed. i.e. a previously
>> > blacklisted device can be redefined as being whitelisted.
>> >
>> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>> > ---
>> >  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
>> >  lib/librte_eal/common/eal_common_devargs.c      | 12 ++++++++++++
>> >  lib/librte_eal/common/include/rte_devargs.h     | 13 +++++++++++++
>> >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
>> >  4 files changed, 27 insertions(+)
>> >
>> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> > index 40cd523..8b24309 100644
>> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> > @@ -206,6 +206,7 @@ DPDK_17.08 {
>> >  EXPERIMENTAL {
>> >         global:
>> >
>> > +       rte_eal_devargs_insert;
>> >         rte_eal_devargs_parse;
>> >         rte_eal_devargs_remove;
>> >         rte_eal_hotplug_add;
>> > diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
>> > index bcdee13..ff6c2a8 100644
>> > --- a/lib/librte_eal/common/eal_common_devargs.c
>> > +++ b/lib/librte_eal/common/eal_common_devargs.c
>> > @@ -138,6 +138,18 @@ rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
>> >         return 0;
>> >  }
>> >
>> > +int
>> > +rte_eal_devargs_insert(struct rte_devargs *da)
>> > +{
>> > +       int ret;
>> > +
>> > +       ret = rte_eal_devargs_remove(da->bus->name, da->name);
>> > +       if (ret < 0)
>> > +               return ret;
>> > +       TAILQ_INSERT_TAIL(&devargs_list, da, next);
>> > +       return 0;
>> > +}
>> > +
>> >  /* store a whitelist parameter for later parsing */
>> >  int
>> >  rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
>> > diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
>> > index 36453b6..7b63fa3 100644
>> > --- a/lib/librte_eal/common/include/rte_devargs.h
>> > +++ b/lib/librte_eal/common/include/rte_devargs.h
>> > @@ -139,6 +139,19 @@ rte_eal_devargs_parse(const char *dev,
>> >                       struct rte_devargs *da);
>> >
>> >  /**
>> > + * Insert an rte_devargs in the global list.
>> > + *
>> > + * @param da
>> > + *  The devargs structure to insert.
>> > + *
>> > + * @return
>> > + *   - 0 on success
>> > + *   - Negative on error.
>> > + */
>> > +int
>> > +rte_eal_devargs_insert(struct rte_devargs *da);
>> > +
>> > +/**
>> >   * Add a device to the user device list
>> >   *
>> >   * For PCI devices, the format of arguments string is "PCI_ADDR" or
>> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> > index a8ee349..81f6af3 100644
>> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> > @@ -211,6 +211,7 @@ DPDK_17.08 {
>> >  EXPERIMENTAL {
>> >         global:
>> >
>> > +       rte_eal_devargs_insert;
>> >         rte_eal_devargs_parse;
>> >         rte_eal_devargs_remove;
>> >         rte_eal_hotplug_add;
>> > --
>> > 2.1.4
>> >
>
> --
> Gaėtan Rivet
> 6WIND