[dpdk-dev] [PATCH v8 00/25] Introducing rte_driver/rte_device generalization

Shreyansh Jain shreyansh.jain at nxp.com
Thu Sep 8 09:03:38 CEST 2016


Hi Ferruh,

On Thursday 01 September 2016 06:06 PM, Shreyansh Jain wrote:
> Hi Ferruh,
>
> Sorry for the delay in my reply.
> Please find some comments inline.
>
> On Tuesday 30 August 2016 06:57 PM, Ferruh Yigit wrote:
>> On 8/26/2016 2:56 PM, Shreyansh Jain wrote:
>>> Based on master (e22856313fff2)
>>>
>>> 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               |
>>>                                      |                               |
>>>                                      +-------------------------------+
>
> I am in agreement to all the points you have raised below.
> Still, there are some comments:
>
>>
>> There are also "eth_driver" and "rte_cryptodev_driver", which can  be
>> represent as:
>> +-----------------------------------+
>> |                                   |
>> | eth_driver / rte_cryptodev_driver |
>> |                                   |
>> | +-------------------------------+ |
>> | |                               | |
>> | | rte_pci_driver                | |
>> | |                               | |
>> | | +---------------------------+ | |
>> | | |                           | | |
>> | | | rte_driver                | | |
>> | | |  char name[]              | | |
>> | | |  int init(rte_device *)   | | |
>> | | |  int uninit(rte_device *) | | |
>> | | |                           | | |
>> | | +---------------------------+ | |
>> | |                               | |
>> | +-------------------------------+ |
>> |                                   |
>> +-----------------------------------+
>>
>> Is eth_driver really needs to be inherited from rte_pci_driver?
>> It is possible to have ethernet driver, without PCI bus, at least we
>> have virtual driver samples.
>
> I agree with this. There would be cases where even non-PCI devices are
> ethernet devices. As of now, in the proposed patchset, rte_soc_driver
> has been added into this but it is not a scalable solution. We cannot go
> on and add all type of devices into this.
>
> Having said this, back reference to type of ethernet device would be
> important if the ethernet driver needs to refer to some hardware
> specific information as part of rte_xxx_driver.
>
>>
>> How about:
>> +-------------------------------+
>> |                               |
>> | rte_pci_driver /              |
>> |           rte_vdev_driver     |
>> | +---------------------------+ |      +-------------------+
>> | |                           | |      | eth_driver        |
>> | | rte_driver                |<---------- driver          |
>> | |  char name[]              | |      |   eth_dev_init    |
>> | |  int init(rte_device *)   | |      |   eth_dev_uninit  |
>> | |  int uninit(rte_device *) | |      |                   |
>> | |                           | |      |                   |
>> | +---------------------------+ |      |                   |
>> | functional_driver ------------------>|                   |
>> +-------------------------------+      +-------------------+
>
> Even while I was re-creating this patchset I was wondering how to
> realign the eth_driver<=>rte_eth_dev (even naming is inconsistent) with
> rte_*_driver/device and rte_driver/device.
> I agree with you that we should link eth_driver->rte_driver.
>
> My only concern being a case where a PCI (or other) based ethernet
> device needs access to driver structure for some info. Is that even a
> possible usecase?
>
>>
>> Currently there is no way to reference rte_vdev_driver from eth_driver,
>> since it only has pci_drv field.
>
> True.
>
>>
>> With this update, it can be possible to create eth_driver and
>> rte_eth_vdev_probe() for virtual drivers for example. (Although this may
>> not necessary right now, this gives ability to do.)
>>
>
> Agree with you. This change can help various other similar cases.
>
>>
>>
>> Another think, what do you think moving "dev_private_size" from
>> eth_driver and rte_cryptodev_driver to rte_driver, since this looks like
>> generic need for drivers.
>
> I don't see any problem in this as well.
>
>>
>>
>>
>> And last think, what do you think renaming eth_driver to rte_eth_driver
>> to be consistent?
>
> Yes, I think it should have been a most basic patch -
> eth_driver<->rte_eth_dev combination has existed too long.
>
>>
>>
>> Thanks,
>> ferruh
>>
>>
>
> -
> Shreyansh
>

In the v9 that I have posted, above changes have not been incorporated 
yet. All the above changes are dependent on one change: to introduce 
rte_driver<->eth_driver relationship:

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

If I go ahead with above model, PMDs would now have to define 
rte_driver, rte_pci_driver and eth_driver. As of now, PMDs only define a 
eth_driver and embed pci_driver into it. This change makes the current 
patch quite large - which is one of the primary reasons I postponed it 
for another patchset/phase.

Further, now that eth_driver no longer embeds rte_pci_driver, it becomes 
unused in the PMDs - it was only being used to access the init fns. I am 
still to find a way around this - whether to completely do away with it 
or have some macro attach it to another list.

I will work on this and post another RFC style series based on current 
rte_driver/device so as to take more inputs.

-
Shreyansh


More information about the dev mailing list