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