Re: Clarification for eth_driver changes

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/12/2016 5:44 PM, Shreyansh Jain wrote:
> Hello Ferruh,
> 
> (Please ignore if line wrappings are not correct. Using a possibly
> unconfigured mail client).
> 
>> -----Original Message-----
>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>> Sent: Saturday, November 12, 2016 12:46 AM
>> To: Shreyansh Jain <shreyansh.jain@nxp.com>; David Marchand
>> <david.marchand@6wind.com>
>> Cc: dev@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@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?

What I was thinking is:

rte_device/driver are not abstract classes.

rte_bus device/driver is an abstract class and any bus inherited from
this class.
rte_func device/driver is and abstract class and eth/crypto inherited
from this class.

eal layer only deal with rte_bus
pmd's only deal with functional device/driver

but still, it is required to know device <-> driver, and functional <->
bus, relations. rte_dev/rte_driver are to provide this links.

But yes this add extra layer and with second thought I am not sure if it
is really possible to separate bus and functionality, this was just an
idea ..

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

probe() is now done in bus level, right? pci_probe(), vdev_probe(), ...
And I guess, eth_dev and crypto_dev are created during probe(), how
probe() knows if to create eth_dev or crypto_dev?

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

I think it is good to add this to be consistent.

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

<...>

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

I agree with you, mine was more like a brain exercise, may have gaps and
I need to think more, thanks for your comments.

Thanks,
ferruh