Re: [dpdk-dev] [PATCH v1 00/18] Device querying

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

On Thu, Mar 15, 2018 at 06:49:30PM +0100, Gaetan Rivet wrote:
> This patchset introduces a new EAL API for querying devices,
> filtered by arbitrary properties.
> 
> The following elements are introduced to this end:
> 
>  * A new object, "rte_class", is used to describe
>    the device class abstraction layer (eth, crypto, ...).
> 
>  * Both rte_bus and rte_class now offer a way to
>    list their devices and filter the result
>    using locally defined properties.
> 
>  * The rte_dev API now has an rte_dev_iterator, which
>    is the way for the user to define the device filter
>    and iterate upon the resulting set.
> 
> As an example, the "eth" device class is implemented.
> 
> Additionally, the device filters for
> 
>   + rte_bus_pci
>   + rte_bus_vdev
>   + rte_class_eth
> 
> are implemented and can be used with some
> properties each, to show how to extend those.
> 
> Some example of filters:
> 
>   "bus=pci/class=eth"
>   "bus=pci"
>   "class=eth"
>   "class=eth,name=net_ring0"
>   "bus=pci,id=00:00.0"
>   "bus=vdev,driver=net_ring"
> 
> Gaetan Rivet (18):

To ease reviewing and have a state of this work, here is a summary of
what to expect / my opinion of what's done:

>   eal: introduce dtor macros

  * Not sure this is necessary, but as long as the libc has a dlclose,
    it makes sense to have a way to cleanly unload objects.

>   eal: introduce device class abstraction

  * The class abstraction might seem controversial introduced there,
    but I do not think there is any other solution than adding it.
    The EAL needs a way to converse in a generic way with each types of
    devices, even if the technical solution I proposed afterward is not
    accepted.

>   eal/class: register destructor

  * If the dtor patch above is accepted, this patch could be squashed
    with the previous one. Otherwise, it can be dropped.

>   eal: add lightweight kvarg parsing utility
>   eal/dev: add device iterator interface

  * An important question regarding device matching is how to deal
    when the query is ambiguous and several devices could match.
    I have taken the technical bias of leaving the management of this
    case to the user.
    This has the beneficial side-effect of allowing to list all devices
    matching specific properties. I think this is useful.
    However, much of the complexity of this solution comes from this
    added functionality.

  * The iterator only allows filtering on the two abstractions:
    + bus
    + class
    This limitation is hard-coded, simplifying the implementation.

>   eal/dev: implement device iteration initialization

  * An important element of this initialization was to make it entirely
    static. To help the user correctly use this API, this seemed important.
    No dynamic allocation was to be performed here.

    This means that the device description string must live for the time
    of the iteration. An iterator must be iterated in the same scope it
    has been initialized.

    A big problem with this approach, is that the matching strings
    cannot be duplicated to be properly reformated, helping the
    subsequent parsing. Thus, the iteration operation implemented both
    in rte_bus and rte_class are more complex, having to take care of
    both cases where an ending '/' and '\0' are present, and this, for
    all implementations. I profoundly dislike this complexity being
    replicated thusly, but I think the advantage of having a lean
    iterator init is worth it.

>   eal/class: add device iteration
>   eal/bus: add device iteration
>   eal/dev: implement device iteration

  * Iteration has been choosen over implementing only a match function,
    that would be used in conjunction with the existing
    bus->find_device

    The reasoning for this is two-fold:

      - Classes are not streamlined. They usually link rte_devices
        instead of extending the rte_device object with their own
        meta-data, like rte_buses are doing.
        So the iteration itself had to be as flexible as possible,
        to allow rte_classes to do anything they might need to
        speed up / simplify the iteration.

     - The filtering strings could have been made once for all
       filters, having only to call *_find_device with the proper
       matching function.

       However, instead of replicating the string handling, each
       matching would thus have to parse the kvargs itself or have the
       kvargs parsed prior to being called. The latter would mean having
       the EAL dependent on librte_kvargs. The former meaning that
       kvargs would be repeated again and again, with dynamic
       allocations with it (meaning that they could not just be kept
       away until the next iteration as they would be already parsed, as
       the user could always break the iteration, loosing the
       reference).

    I think the choice of iteration vs. match function is central to the
    design of this solution and should be carefully reviewed. I tried to
    make the right call, can be wrong.

vvvvvvvvv <-- the rest only builds upon the foundations layed out by the
              previous patches, so choices here are less important.

              Last point however: I havent yet implemented the
              rte_eth_dev finder taking a device description as input
              and giving the corresponding port_id. It should be simple
              to implement with this new API so I will do it shortly,
              once I have some time for it.

>   ethdev: register ether layer as a class
>   ethdev: add device matching field name
>   bus/pci: fix find device implementation
>   bus/pci: implement device iteration and comparison
>   bus/pci: add device matching field id
>   bus/vdev: fix find device implementation
>   bus/vdev: implement device iteration
>   bus/vdev: add device matching field driver
>   app/testpmd: add show device command
> 
>  app/test-pmd/cmdline.c                     |  52 +++++++++
>  drivers/bus/pci/Makefile                   |   2 +-
>  drivers/bus/pci/pci_common.c               |  97 ++++++++++++++--
>  drivers/bus/pci/rte_bus_pci.h              |   3 +
>  drivers/bus/vdev/Makefile                  |   2 +-
>  drivers/bus/vdev/rte_bus_vdev.h            |   3 +
>  drivers/bus/vdev/vdev.c                    |  86 +++++++++++++-
>  lib/Makefile                               |   2 +-
>  lib/librte_eal/bsdapp/eal/Makefile         |   1 +
>  lib/librte_eal/common/Makefile             |   2 +-
>  lib/librte_eal/common/eal_common_class.c   |  62 ++++++++++
>  lib/librte_eal/common/eal_common_dev.c     | 177 +++++++++++++++++++++++++++++
>  lib/librte_eal/common/eal_private.h        |  34 ++++++
>  lib/librte_eal/common/include/rte_bus.h    |   1 +
>  lib/librte_eal/common/include/rte_class.h  | 127 +++++++++++++++++++++
>  lib/librte_eal/common/include/rte_common.h |  23 ++++
>  lib/librte_eal/common/include/rte_dev.h    |  84 ++++++++++++++
>  lib/librte_eal/linuxapp/eal/Makefile       |   1 +
>  lib/librte_eal/rte_eal_version.map         |   4 +
>  lib/librte_ether/Makefile                  |   3 +-
>  lib/librte_ether/rte_class_eth.c           | 100 ++++++++++++++++
>  21 files changed, 847 insertions(+), 19 deletions(-)
>  create mode 100644 lib/librte_eal/common/eal_common_class.c
>  create mode 100644 lib/librte_eal/common/include/rte_class.h
>  create mode 100644 lib/librte_ether/rte_class_eth.c
> 
> -- 
> 2.11.0
> 

Regards,
-- 
Gaëtan Rivet
6WIND