On Thu, Nov 17, 2016 at 2:08 PM, Shreyansh Jain [off-list ref] wrote:

On Thursday 17 November 2016 05:25 PM, Jan Blunck wrote:

quoted

On Thu, Nov 17, 2016 at 6:29 AM, Shreyansh Jain [off-list ref]
wrote:

quoted

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.

Yes, either have everything PCI related stuff in drivers or in
librte_eal (or librte_eal_pci). I don't believe it is required to have
two different locations for the rte_pci_bus and the rte_pci_*
functions that are now in librte_eal.

quoted

quoted

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.

Yes, that is fine too.

quoted

quoted

=== 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.

quoted

quoted

 - 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