[dpdk-dev] [PATCH v8 00/25] Introducing rte_driver/rte_device generalization
Shreyansh Jain
shreyansh.jain at nxp.com
Thu Sep 1 14:36:04 CEST 2016
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
More information about the dev
mailing list