Re: [PATCH v9 00/25] Introducing rte_driver/rte_device generalization

[Declan Doherty <declan.doherty@intel.com>]

On 07/09/16 15:07, Shreyansh Jain wrote:
> Based on master (e22856313)
>
> Background:
> ===========
>
> It includes two different patch-sets floated on ML earlier:
>  * Original patch series is from David Marchand [1], [2].
>   `- This focused mainly on PCI (PDEV) part
>   `- v7 of this was posted by me [8] in August/2016
>  * Patch series [4] from Jan Viktorin
>   `- This focused on VDEV and rte_device integration
>
> Introduction:
> =============
>
> This patch series introduces a generic device model, moving away from PCI
> centric code layout. Key change is to introduce rte_driver/rte_device
> structures at the top level which are inherited by
> rte_XXX_driver/rte_XXX_device - where XXX belongs to {pci, vdev, soc (in
> future),...}.
>
> Key motivation for this series is to move away from PCI centric design of
> EAL to a more hierarchical device model - pivoted around a generic driver
> and device. Each specific driver and device can inherit the common
> properties of the generic set and build upon it through driver/device
> specific functions.
>
> Earlier, the EAL device initialization model was:
> (Refer: [3])
>
> --
>  Constructor:
>   |- PMD_DRIVER_REGISTER(rte_driver)
>      `-  insert into dev_driver_list, rte_driver object
>
>  rte_eal_init():
>   |- rte_eal_pci_init()
>   |  `- scan and fill pci_device_list from sysfs
>   |
>   |- rte_eal_dev_init()
>   |  `- For each rte_driver in dev_driver_list
>   |     `- call the rte_driver->init() function
>   |        |- PMDs designed to call rte_eth_driver_register(eth_driver)
>   |        |- eth_driver have rte_pci_driver embedded in them
>   |        `- rte_eth_driver_register installs the
>   |           rte_pci_driver->devinit/devuninit callbacks.
>   |
>   |- rte_eal_pci_probe()
>   |  |- For each device detected, dev_driver_list is parsed and matching is
>   |  |  done.
>   |  |- For each matching device, the rte_pci_driver->devinit() is called.
>   |  |- Default map is to rte_eth_dev_init() which in turn creates a
>   |  |  new ethernet device (eth_dev)
>   |  |  `- eth_drv->eth_dev_init() is called which is implemented by
>   `--|    individual PMD drivers.
>
> --
>
> The structure of driver looks something like:
>
>  +------------+     ._____.
>  | rte_driver <-----| PMD |___
>  |  .init     |     `-----`   \
>  +----.-------+      |         \
>       `-.            |         What PMD actually is
>          \           |          |
>           +----------v----+     |
>           | eth_driver    |     |
>           | .eth_dev_init |     |
>           +----.----------+     |
>                `-.              |
>                   \             |
>                    +------------v---+
>                    | rte_pci_driver |
>                    | .pci_devinit   |
>                    +----------------+
>
>   and all devices are part of a following linked lists:
>     - dev_driver_list for all rte_drivers
>     - pci_device_list for all devices, whether PCI or VDEV
>
>
> From the above:
>  * a PMD initializes a rte_driver, eth_driver even though actually it is a
>    pci_driver
>  * initialization routines are passed from rte_driver->pci_driver->eth_driver
>    even though they should ideally be rte_eal_init()->rte_pci_driver()
>  * For a single driver/device type model, this is not necessarily a
>    functional issue - but more of a design language.
>  * But, when number of driver/device type increase, this would create problem
>    in how driver<=>device links are represented.
>
> Proposed Architecture:
> ======================
>
> A nice representation has already been created by David in [3]. Copying that
> here:
>
>                 +------------------+ +-------------------------------+
>                 |                  | |                               |
>                 | rte_pci_device   | | rte_pci_driver                |
>                 |                  | |                               |
> +-------------+ | +--------------+ | | +---------------------------+ |
> |             | | |              | | | |                           | |
> | rte_eth_dev +---> rte_device   +-----> rte_driver                | |
> |             | | |  char name[] | | | |  char name[]              | |
> +-------------+ | |              | | | |  int init(rte_device *)   | |
>                 | +--------------+ | | |  int uninit(rte_device *) | |
>                 |                  | | |                           | |
>                 +------------------+ | +---------------------------+ |
>                                      |                               |
>                                      +-------------------------------+
>
> - for ethdev on top of vdev devices
>
>                 +------------------+ +-------------------------------+
>                 |                  | |                               |
>                 | drv specific     | | rte_vdev_driver               |
>                 |                  | |                               |
> +-------------+ | +--------------+ | | +---------------------------+ |
> |             | | |              | | | |                           | |
> | rte_eth_dev +---> rte_device   +-----> rte_driver                | |
> |             | | |  char name[] | | | |  char name[]              | |
> +-------------+ | |              | | | |  int init(rte_device *)   | |
>                 | +--------------+ | | |  int uninit(rte_device *) | |
>                 |                  | | |                           | |
>                 +------------------+ | +---------------------------+ |
>                                      |                               |
>                                      |   int priv_size               |
>                                      |                               |
>                                      +-------------------------------+
>
> Representing from above, it would be:
>
> +--------------+
> | rte_driver   |
> |  name        |
> |  <Future>    |
> +------^-------+      pci_driver_list
>        |                   /                vdev_driver_list
>        `---. <<Inherits>> /                  /
>            |\____________/_______           /
>            |            /        \         /
>            +-----------/-----+   +--------/---------+
>            | rte_pci_driver  |   | rte_vdev_driver  |
>            |  pci_devinit()  |   |  vdev_devinit()  |
>            |  pci_devuninit()|   |  vdev_devuninit()|
>            |  <more>         |   |  <more>          |
>            +-----------------+   +------------------+
>
>
> +--------------+
> | rte_device   |
> |  name        |
> |  <Future>    |
> +------^-------+        pci_device_list
>        |                   /               xxx_device_list
>        `---. <<Inherits>> /                  /
>            |\____________/________          /
>            |            /         \        /
>            +-----------/-----+   +--------/---------+
>            | rte_pci_device  |   | rte_xxx_device   |
>            |  <dev data>     |   |  <dev data>      |
>            |  <flags/intr>   |   |  <flags/intr>    |
>            |  <more>         |   |  <more>          |
>            +-----------------+   +------------------+
>
>  * Each driver type has its own structure which derives from the generic
>    rte_driver structure.
>    \- Each driver type maintains its own list, at the same time, rte_driver
>       list also exists - so that *all* drivers can be looped on, if required.
>  * Each device, associated with one or more drivers, has its own type
>    derived from rte_device
>    \- Each device _may_ maintain its own list (for example, in current
>       implementation, vdev is not maintaining it).
>
> ==Introducing a new device/driver type implies==
>   - creating their own rte_<xxx>.h file which contains the device/driver
>     definitions.
>   - defining the DRIVER_REGISTER_XXX helpers
>
> ==Hotplugging Support==
>   - devices should be able to support attach/detach operations.
>   - Earlier these functions were part of ethdev. They have been moved to eal
>     to be more generic.
>
>  This patch is part of larger aim to:
>
>  --------------------+ <is type of>
>  eth_driver (PMD)    |-------------> rte_driver
>  crypto_driver (PMD) |               ^
>  <more in future>    |               |
>  --------------------+               | <inherits>
>                                     /
>            +-----------------------/+
>            | rte_pci_driver         |
>            | rte_vdev_driver        |
>            | rte_soc_driver         |
>            | rte_xxx_driver         |
>
>  Where PMD devices (rte_eth_dev, rte_cryptodev_dev) are connected to their
>  drivers rather than explicitly inheriting type specific driver (PCI, etc).
>
>

....

>
> Notes:
> ======
>
> * Some sign-off were already provided by Jan on the original v5; But, as a
>   large number of merges have been made, I am leaving those out just in case
>   it is not sync with initial understanding.
>
> * This might not be in-sync with pmdinfogen as PMD_REGISTER_DRIVER is
>   removed [7].
>
> Future Work/Pending:
> ===================
>  - Presently eth_driver, rte_eth_dev are not aligned to the rte_driver/
>    rte_device model. eth_driver still is a PCI specific entity. This
>    has been highlighted by comments from Ferruh in [9].
>  - cryptodev driver too still remains to be normalized over the rte_driver
>    model
>  - Some variables, like drv_name (as highlighted by Ferruh), are getting
>    duplicated across rte_xxx_driver/device and rte_driver/device.
>

  ....

>

Hey Shreyansh,

I got some time over the last couple of days to got through this 
patchset and your soc changes, and it's really a great clean up of the 
driver and device infrastructure code.

I guess I'm coming to this party a bit late but I have some thoughts on 
the the object model that you've outlined it improves things a huge 
amount but I wonder does it go far enough.

I think that currently the coupling of the rte_driver to instances of 
rte_pci_driver, rte_vdev_driver etc still really clouds the abstraction. 
I think if we introduced a rte_bus and rte_bus_data abstractions with 
all rte_drivers having a rte_bus instance then we could further simplify 
the driver and device management, I think this would greatly reduce the 
number of driver/device APIs in the EAL as I think most bus 
functionality could be abstracted into a simpler API. This would also 
make it possible to create new driver and device types and allowing them 
to be plug-able components into DPDK and which I don't think would work 
currently even  with the changes included here.

http://imgur.com/a/3b7XM

The link above is to a png of a visio which outlines the object model 
that I'm thinking of. The main extension from your current patchset 
would be that rte_eth_driver and rte_crypto_driver would now always be 
inheriting from rte_driver and not rte_soc_driver/rte_pci_driver, etv 
but the rte_driver object would have rte_bus instance which could be of 
type rte_bus_pci/ rte_bus_virtual/ rte_bus_soc etc.  This I think would 
allow the EAL to be written in such a way that it only every operates on 
the base object types rte_device/rte_driver directly never the 
derivative objects, like the rte_vdev_driver, rte_pci_driver. Also I 
prefer the abstraction that a driver has a bus rather than it is the 
bus. Also I think would be the devices layer to written driver/bus 
agnostic, so the eth_dev layer shouldn't know if and underlying PMD is 
PCI/memory based or virtual. Only the poll mode driver itself, and the 
part of the EAL which manages driver initialization should have 
knowledge of the drivers bus type.

Anyway have a look, and let me know what you think.

Thanks
Declan