[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