[dpdk-dev] [RFC PATCH 0/6] Restructure EAL device model for bus support

Shreyansh Jain shreyansh.jain at nxp.com
Thu Nov 17 14:08:28 CET 2016


On Thursday 17 November 2016 05:25 PM, Jan Blunck wrote:
> On Thu, Nov 17, 2016 at 6:29 AM, Shreyansh Jain <shreyansh.jain at nxp.com> wrote:
>> DPDK has been inherently a PCI inclined framework. Because of this, the
>> design of device tree (or list) within DPDK is also PCI inclined. A non-PCI
>> device doesn't have a way of being expressed without using hooks started from
>> EAL to PMD.
>>
>> With this cover letter, some patches are presented which try to break this
>> strict linkage of EAL with PCI devices. Aim is to generalize the device
>> hierarchy on the lines of how Linux handles it:
>>
>>         device A1
>>           |
>>   +==.===='==============.============+ Bus A.
>>      |                    `--> driver A11     \
>>   device A2                `-> driver A12      \______
>>                                                 |CPU |
>>                                                 /`````
>>         device A1                              /
>>           |                                   /
>>   +==.===='==============.============+ Bus A`
>>      |                    `--> driver B11
>>   device A2                `-> driver B12
>>
>> Simply put:
>>  - a bus is connect to CPU (or core)
>>  - devices are conneted to Bus
>>  - drivers are running instances which manage one or more devices
>>  - bus is responsible for identifying devices (and interrupt propogation)
>>  - driver is responsible for initializing the device
>>
>> (*Reusing text from email [1])
>> In context of DPDK EAL:
>>  - a generic bus (not a driver, not a device). I don't know how to categorize
>>    a bus. It is certainly not a device, and then handler for a bus (physical)
>>    can be considered a 'bus driver'. So, just 'rte_bus'.
>>  - there is a bus for each physical implementation (or virtual). So, a rte_bus
>>    Object for PCI, VDEV, ABC, DEF and so on.
>>  - Buses are registered just like a PMD - RTE_PMD_BUS_REGISTER()
>>  - Each registered bus is part of a doubly list.
>>    -- Each device inherits rte_bus
>>    -- Each driver inherits rte_bus
>>    -- Device and Drivers lists are part of rte_bus
>>  - eth_driver is no more required - it was just a holder for PMDs to register
>>    themselves. It can be replaced with rte_xxx_driver and corresponding init/
>>    uninit moved to rte_driver
>>  - rte_eth_dev modified to disassociate itself from rte_pci_device and connect
>>    to generic rte_device
>>
>> Once again, improvising from [1]:
>>
>>                                   __ rte_bus_list
>>                                  /
>>                      +----------'---+
>>                      |rte_bus       |
>>                      | driver_list------> List of rte_bus specific
>>                      | device_list----    devices
>>                      | scan         | `-> List of rte_bus associated
>>                      | match        |     drivers
>>                      | dump         |
>>                      | ..some refcnt| (#)
>>                      +--|------|----+
>>               _________/        \_________
>>     +--------/----+                     +-\---------------+
>>     |rte_device   |                     |rte_driver       |
>>     | rte_bus     |                     | rte_bus         |
>>     | rte_driver  |(#)                  | init            |
>>     |             |                     | uninit          |
>>     |  devargs    |                     | dev_private_size|
>>     +---||--------+                     | drv_flags       |(#)
>>         ||                              | intr_handle(2*) |(#)
>>         | \                             +----------\\\----+
>>         |  \_____________                           \\\
>>         |                \                          |||
>>  +------|---------+ +----|----------+               |||
>>  |rte_pci_device  | |rte_xxx_device | (4*)          |||
>>  | PCI specific   | | xxx device    |               |||
>>  | info (mem,)    | | specific fns  |              / | \
>>  +----------------+ +---------------+             /  |  \
>>                             _____________________/  /    \
>>                            /                    ___/      \
>>             +-------------'--+    +------------'---+    +--'------------+
>>             |rte_pci_driver  |    |rte_vdev_driver |    |rte_xxx_driver |
>>             | PCI id table,  |    | <probably,     |    | ....          |
>>             | other driver   |    |  nothing>      |    +---------------+
>>             | data           |    +----------------+
>>             +----------------+
>>
>>     (1*) Problem is that probe functions have different arguments. So,
>>          generalizing them might be some rework in the respective device
>>          layers
>>     (2*) Interrupt handling for each driver type might be different. I am not
>>          sure how to generalize that either. This is grey area for me.
>>     (3*) Probably exposing a bitmask for device capabilities. Nothing similar
>>          exists now to relate it. Don't know if that is useful. Allowing
>>          applications to question a device about what it supports and what it
>>          doesn't - making it more flexible at application layer (but more code
>>          as well.)
>>     (4*) Even vdev would be an instantiated as a device. It is not being done
>>          currently.
>>     (#)  Items which are still pending
>>
>> With this cover letter, some patches have been posted. These are _only_ for
>> discussion purpose. They are not complete and neither compilable.
>> All the patches, except 0001, have sufficient context about what the changes
>> are and rationale for same. Obviously, code is best answer.
>>
>> === Patch description: ===
>>
>> Patch 0001: Introduce container_of macro. Originally a patch from Jan.
>>
>> Patch 0002: introduce changes to rte_device/rte_driver for rte_bus, and
>>             rte_bus definition itself.
>>
>> Patch 0003: Add a new layer for 'bus driver' with linuxapp PCI as an example
>>
>> Patch 0004: Changes with respect to rte_bus APIs and impact on eal_common_pci
>
> From my point of view it is beneficial to keep the PCI support in one
> place. Moving the match() and scan()
> to the drivers directory doesn't seem to be technically required but
> it makes the code harder to read and understand.

It is not a technical requirement - just logical placement. These are 
bus specific logic and should exist with the bus driver - where ever 
that is placed.
Though, I am not entirely sure about 'harder to read'. If a person is 
reading through PCI's bus implementation, I am assuming it would be nice 
to have all the hooks (or default hooks) at same place. Isn't it?

Or, maybe you are suggesting move all librte_eal/*/*pci* out to some 
common location. If so, that is something I haven't yet given serious 
thought.

>
>
>> Patch 0005: Change to rte_eal_init (of linuxapp only, for now) for supporting
>>             bus->scan. Probe is still being done old way, but in a new wrapper
>>
>> Patch 0006: eth_driver removal and corresponding changes in ixgbe_ethdev, as
>>             an example. Includes changes to rte_ethdev to remove most possible
>>             PCI references. But, work still remains.
>
> Making rte_ethdev independent from PCI device is not directly related
> to the rest of the patches that add bus support. I think it makes
> sense to handle this separately because of the impact of refactoring
> rte_ethdev.

Once rte_device is available, the change is independent.
Only dependency on it was changes required to rte_ethdev.c file because 
of various PCI naming/variables/functions. And that is indeed a very 
large change - including changes to drivers/* which I haven't yet done.

Are you suggesting breaking out of this series? If so, can be done but 
only when formal patches start coming out.

>
>
>>
>> === Pending Items/Questions: ===
>>
>>  - Interrupt and notification handling. How to allow drivers to be notified
>>    of presence/plugging of a device.
>>  - Placement of bus driver/handling code. librte_bus, bus/, drivers/bus?
>>  -- Also from a pespective of a external library and whether symbols would be
>>     available in that.
>>  -- and secondary processes
>>  - VDEV bus is missing from current set.
>>  - Locking of list for supporting hotplugging. Or, at the least safe add/
>>    remove
>>  - PMDINFOGEN support or lack of it.
>>  - Is there ever a case where rte_eth_dev needs to be resolved from
>>    rte_pci_device? I couldn't find any such use and neither a use-case for it.
>>  - There should be a way for Bus APIs to return a generic list handle so that
>>    EAL doesn't need to bother about bus->driver_list like dereferencing. This
>>    is untidy as well as less portable (in terms of code movement, not arch).
>>  - Are more helper hooks required for a bus?
>>  -- I can think of scan, match, dump, find, plug (device), unplug (device),
>>     associate (driver), disassociate (driver). But, most of the work is
>>     already being done by lower instances (rte_device/driver etc).
>>
>> Further:
>>  - In next few days I will make all necessary changes on the lines mentioned
>>    in the patches. This would include changing the drivers/* and librte_eal/*
>>  - As an when review comments float in and agreement reached, I will keep
>>    changing the model
>>  - There are grey areas like interrupt, notification, locking of bus/list
>>    which require more discussion. I will try and post a rfc for those as well
>>    or if someone can help me on those - great
>
> As already hinted on IRC I'm taking a look at the interrupt usage of ethdev.

Great. Thank you. Do let me know feedback for any changes that you might 
require along the way.

>
>>  - Change would include PCI bus and VDEV bus handling. A new bus (NXP's FSLMC)
>>    would also be layered over this series to verify the model of 'bus
>>    registration'. This is also part of 17.02 roadmap.
>>
>> [1] http://dpdk.org/ml/archives/dev/2016-November/050186.html
>>
>> Jan Viktorin (1):
>>   eal: define container macro
>>
>> Shreyansh Jain (5):
>>   eal: introduce bus-device-driver structure
>>   bus: add bus driver layer
>>   eal/common: handle bus abstraction for device/driver objects
>>   eal: supporting bus model in init process
>>   eal: removing eth_driver
>>
>>  bus/Makefile                               |  36 +++
>>  bus/pci/Makefile                           |  37 +++
>>  bus/pci/linuxapp/pci_bus.c                 | 418 +++++++++++++++++++++++++++++
>>  bus/pci/linuxapp/pci_bus.h                 |  55 ++++
>>  drivers/net/ixgbe/ixgbe_ethdev.c           |  49 ++--
>>  lib/librte_eal/common/eal_common_bus.c     | 188 +++++++++++++
>>  lib/librte_eal/common/eal_common_dev.c     |  31 ++-
>>  lib/librte_eal/common/eal_common_pci.c     | 226 +++++++++-------
>>  lib/librte_eal/common/include/rte_bus.h    | 243 +++++++++++++++++
>>  lib/librte_eal/common/include/rte_common.h |  18 ++
>>  lib/librte_eal/common/include/rte_dev.h    |  36 +--
>>  lib/librte_eal/common/include/rte_pci.h    |  11 +-
>>  lib/librte_eal/linuxapp/eal/Makefile       |   1 +
>>  lib/librte_eal/linuxapp/eal/eal.c          |  51 +++-
>>  lib/librte_eal/linuxapp/eal/eal_pci.c      | 298 --------------------
>>  lib/librte_ether/rte_ethdev.c              |  36 ++-
>>  lib/librte_ether/rte_ethdev.h              |   6 +-
>>  17 files changed, 1262 insertions(+), 478 deletions(-)
>>  create mode 100644 bus/Makefile
>>  create mode 100644 bus/pci/Makefile
>>  create mode 100644 bus/pci/linuxapp/pci_bus.c
>>  create mode 100644 bus/pci/linuxapp/pci_bus.h
>>  create mode 100644 lib/librte_eal/common/eal_common_bus.c
>>  create mode 100644 lib/librte_eal/common/include/rte_bus.h
>>
>> --
>> 2.7.4
>>
>

-
Shreyansh


More information about the dev mailing list