[dpdk-dev] Clarification for eth_driver changes

[Shreyansh Jain]

Hello Ferruh,

(Please ignore if line wrappings are not correct. Using a possibly
unconfigured mail client).

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit at intel.com]
> Sent: Saturday, November 12, 2016 12:46 AM
> To: Shreyansh Jain <shreyansh.jain at nxp.com>; David Marchand
> <david.marchand at 6wind.com>
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] Clarification for eth_driver changes
> 
> On 11/10/2016 11:05 AM, Shreyansh Jain wrote:
> > Hello David,
> >
> > On Thursday 10 November 2016 01:46 PM, David Marchand wrote:
> >> Hello Shreyansh,
> >>
> >> On Thu, Nov 10, 2016 at 8:26 AM, Shreyansh Jain <shreyansh.jain at nxp.com>
> wrote:
> >>> I need some help and clarification regarding some changes I am doing to
> >>> cleanup the EAL code.
> >>>
> >>> There are some changes which should be done for eth_driver/rte_eth_device
> >>> structures:
> >>>
> >>> 1. most obvious, eth_driver should be renamed to rte_eth_driver.
> >>> 2. eth_driver currently has rte_pci_driver embedded in it
> >>>  - there can be ethernet devices which are _not_ PCI
> >>>  - in which case, this structure should be removed.
> >>
> >> Do we really need to keep a eth_driver ?
> >
> > No. As you have rightly mentioned below (as well as in your Jan'16
> > post), it is a mere convenience.
> 
> Isn't it good to separate the logic related which bus device connected
> and what functionality it provides. Because these two can be flexible:

Indeed. The very idea of a Bus model is to make a hierarchy which allows
for pluggability/flexibility in terms of devices being used. But, until now I
have only considered placement hierarchy and not functional hierarchy. (more
below)

> 
> device -> virtual_bus -> ethernet_functionality
> device -> pci_bus     -> crypto_functionality
> device -> x_bus       -> y_function
>

Ok.

> 
> what about:
> 
> create generic bus driver/device and all eal level deal with generic
> bus. different buses inherit from generic bus logic
 
From what I had in mind: (and very much similar to what you have already 
                          mentioned in this email)
 - 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()
 -- There is a problem of making sure the constructor for bus registration is
    executed before drivers. Probably by putting the bus code within lib*
 - Each registered bus is part of a doubly list.
 - Each device inherits rte_bus
 - Each driver inherits rte_bus
 - Existing Device and Drivers lists would be moved into rte_bus

(more below)

> 
> create generic functionality device/driver and pmd level deal with
> these. different functionalities inherit from generic functionality logic
> 
> and use rte_device/rte_driver as glue logic for all these.
> 
> This makes easy to add new bus or functionality device/drivers without
> breaking existing logic.
> 
> 
> Something like this:
> 
> struct rte_device {
> 	char *name;
> 	struct rte_driver *drv;
> 	struct rte_bus_device *bus_dev;
> 	struct rte_funcional_device *func_dev;
> 	*devargs
> }
> 
> struct rte_bus_device {
> 	struct rte_device *dev;
> 	/* generic bus device */
> }

This is where you lost me. From what I understood from your text:
(CMIIW)

- Most abstract class is 'rte_device'.
- A bus is a device. So, it points to a rte_device
- But, a rte_device belongs to a bus, so it points to a rte_bus_device.

Isn't that contradictory?

This is how I view the physical layout of devices on which DPDK works:

         +---------+           +----------+
         |Driver 1A|           |Driver 2B |
         |servicing|           |servicing | (*)
         |Bus A    |           |Bus B     |
         +---------+           +----------+
                    \             /
                  +---------+   +-------+
                  | bus  A  |---| bus B |--- ...
                  +---------+   +-------+
                 / \             \  \
                /   \_            \  \
     +---------+     /            /   \
     | device 1|    /   +--------+     \
     | on Bus A|   /    |Device 3|     |
     +---------+  /     |on bus B|     |
       +---------+      +--------+     |
       | device 2|             +--------+
       | on Bus A|             |Device 4|
       +---------+             |on bus B|
                               +--------+

 (*) Multiple drivers servicing a Bus.

Now, if we introduce the abstraction for functionality (assuming net/crypto) as
two functionalities currently applicable:

         +---------+           +----------+
         |Driver 1A|           |Driver 2B |
         |servicing|           |servicing | (*)
         |Bus A    |           |Bus B     |
         +---------'           +---.------+
                    \             /
                  +---------+   +-------+
                  | bus  A  |---| bus B |--- ...
                  +---------+   +-------+
                 / \             \  \
                /   \_            \  \
     +---------'     /            /   \
     | device 1|    /   +--------'     \
     | on Bus A|   /    |Device 3|     |
     +---------+  /     |on bus B|     |
    /  +---------'      +-|------+     |
   |   | device 2|        /    +-------'+
   |   | on Bus A|       /     |Device 4|
    \  +--|------+ _____/      |on bus B|
     \    \_____  /            +--------+
      |      .--\'            /
      |     /    \        ___/
    +-'----'-+   +'------'+
    |Func X  |   |Func Y  |
    |(Net)   |   |(Crypto)|
    +--------|   +--------+

So that means, a device would be a 'net' or 'crypto' device bases on the
Functionality it attaches to.

From a physical layout view, is that correct understanding of your argument?

> 
> struct rte_pci_device {
> 	struct rte_bus_device bus_dev;
> 	/* generic pci bus */
> }
> 
> struct rte_vdev_device {
> 	struct rte_bus_device bus_dev;
> 	/* generic vdev bus */
> }
> 
> struct rte_funcional_device {
> 	struct rte_device *dev;
> }

I understand your point of 'pluggable' functionality. It would be helpful if
same driver would like to move between being a crypto and net. But is that a
plausible use case for DPDK right now?

To me, it seems as one more layer of redirection/abstraction.

This is what the view I have as of now:

                                  __ rte_bus_list
                                 /
                     +----------'---+
                     |rte_bus       |
                     | driver_list  |
                     | device_list  |
                     | scan         |
                     | match        |
                     | remove       |
                     | ..some refcnt|
                     +--|------|----+
              _________/        \_________
    +--------/----+                     +-\--------------+
    |rte_device   |                     |rte_driver      |
    | rte_bus     |                     | rte_bus        |
    |  flags (3*) |                     | probe (1*)     |
    |  init       |                     | remove         |
    |  uninit     |                     | ...            |
    +---||--------+                     | 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.

So, unlike your model, rte_bus remains the topmost class which is neither a
device, not a driver. It is just a class.
Further, as specific information exists in each specific device and driver,
that is not generalized.

> 
> struct rte_eth_device {
> 	struct rte_funcional_device func_dev;
> 	/* generic eth device */
> }
> 
> struct rte_crypto_device {
> 	struct rte_funcional_device func_dev;
> 	/* generic crypto device */
> }
> 

I tried thinking of this breakup but I couldn't understand what common things
a rte_functional_device would contain except init/uninit (similar to what you
have also mentioned below) which can be part of rte_device itself.

> 
> struct rte_driver {
> 	char *name;
> 	struct rte_device *dev;
> 	struct rte_bus_driver *bus_drv;
> 	struct rte_funcional_driver *func_drv;
> }
> 
> struct rte_bus_driver {
> 	struct rte_driver *drv;
> 	rte_bus_probe_t *probe(dev, drv);
> 	rte_bus_remove_t *remove(dev);
> 	/* generic bus driver */
> }

I still think a bus is neither a device, nor a driver. Yes, if we draw
physical analogy, buses are indeed devices on PCB - but they are not anything
functional with respect to an application view. They exist only to provide way
for application to understand device layout.

> 
> struct rte_pci_driver {
> 	struct rte_bus_driver bus_drv;
> 	*id_table;
> 	/* generic pci bus */
> }
> 
> struct rte_vdev_driver {
> 	struct rte_bus_driver bus_drv;
> 	/* generic vdev bus */
> }
> 
> struct rte_funcional_driver {
> 	struct rte_driver *drv;
> 	rte_funcional_init_t *init;
> 	rte_funcional_uninit_t *uninit;
> }
> 
> struct rte_eth_driver {
> 	struct rte_funcional_driver func_drv;
> 	/* generic eth driver */
> }
> 
> struct rte_crypto_driver {
> 	struct rte_funcional_driver func_drv;
> 	/* generic crypto driver */
> }
> 
> pci_scan_one()
> {
> 	dev = create();
> 	pci_dev = create();
> 
> 	dev->bus_dev = pci_dev;
> 	pci_dev->bus_dev.dev = dev;
> 
> 	insert(bus_dev_list);
> }
> 
> register_drv(drv)
> {
> 	insert(bus_drv_list)
> 	insert(func_drv_list)
> 	insert(drv_list)
> }
> 
> rte_eal_bus_probe()
>   for bus_dev in bus_dev_list
> 	bus_probe_all_drivers(bus_dev)
> 	  for bus_drv in bus_drv_list
> 	    bus_probe_one_driver(bus_drv, bus_dev)
> 		  bus_dev->dev->drv = bus_drv->drv;
> 		  bus_drv->drv->dev = bus_dev->dev;
> 		  probe(drv, dev)
>

Agree.
 
> probe(drv, dev)
> {
> 	dev->func_dev = create();

In my case, it would be creating a rte_device; rte_pci_dev in case of
PCI probe, pointing back to rte_device, rte_bus of PCI type.

> 	func_dev->dev = dev;
> 
> 	func_drv = drv->func_drv;
> 	func_drv->init(func_dev);

Effectively, it would be rte_device->init();

> }
> 
> eht_init(func_dev)
> {
> 	eth_dev = (struct rte_eth_dev)func_dev;
> 	drv = func_dev->dev->drv;
> }
> 
> 

I am not against what you have stated. Creating a functional device is just
one more layer of abstraction in my view. Mostly abstraction/classification
makes life easier for applications to visualize underlying hierarchy. If this
serves a purpose, I am OK with that. At least right now, I think it would
only end up being like eth_driver which is just a holder.

__
Shreyansh