From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 7C8282BFF for ; Fri, 9 Sep 2016 18:14:50 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga105.jf.intel.com with ESMTP; 09 Sep 2016 09:14:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,305,1470726000"; d="scan'208";a="1037616421" Received: from dwdohert-dpdk.ir.intel.com ([163.33.210.152]) by fmsmga001.fm.intel.com with ESMTP; 09 Sep 2016 09:14:48 -0700 To: Shreyansh Jain , dev@dpdk.org References: <1466510566-9240-1-git-send-email-shreyansh.jain@nxp.com> <1473257297-7221-1-git-send-email-shreyansh.jain@nxp.com> Cc: hemant.agrawal@nxp.com From: Declan Doherty Message-ID: Date: Fri, 9 Sep 2016 17:11:36 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <1473257297-7221-1-git-send-email-shreyansh.jain@nxp.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v9 00/25] Introducing rte_driver/rte_device generalization X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Sep 2016 16:14:51 -0000 On 07/09/16 15:07, Shreyansh Jain wrote: > Based on master (e22856313) > > 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 | > | | > +-------------------------------+ > > Representing from above, it would be: > > +--------------+ > | rte_driver | > | name | > | | > +------^-------+ pci_driver_list > | / vdev_driver_list > `---. <> / / > |\____________/_______ / > | / \ / > +-----------/-----+ +--------/---------+ > | rte_pci_driver | | rte_vdev_driver | > | pci_devinit() | | vdev_devinit() | > | pci_devuninit()| | vdev_devuninit()| > | | | | > +-----------------+ +------------------+ > > > +--------------+ > | rte_device | > | name | > | | > +------^-------+ pci_device_list > | / xxx_device_list > `---. <> / / > |\____________/________ / > | / \ / > +-----------/-----+ +--------/---------+ > | rte_pci_device | | rte_xxx_device | > | | | | > | | | | > | | | | > +-----------------+ +------------------+ > > * Each driver type has its own structure which derives from the generic > rte_driver structure. > \- Each driver type maintains its own list, at the same time, rte_driver > list also exists - so that *all* drivers can be looped on, if required. > * Each device, associated with one or more drivers, has its own type > derived from rte_device > \- Each device _may_ maintain its own list (for example, in current > implementation, vdev is not maintaining it). > > ==Introducing a new device/driver type implies== > - creating their own rte_.h file which contains the device/driver > definitions. > - defining the DRIVER_REGISTER_XXX helpers > > ==Hotplugging Support== > - devices should be able to support attach/detach operations. > - Earlier these functions were part of ethdev. They have been moved to eal > to be more generic. > > This patch is part of larger aim to: > > --------------------+ > eth_driver (PMD) |-------------> rte_driver > crypto_driver (PMD) | ^ > | | > --------------------+ | > / > +-----------------------/+ > | rte_pci_driver | > | rte_vdev_driver | > | rte_soc_driver | > | rte_xxx_driver | > > Where PMD devices (rte_eth_dev, rte_cryptodev_dev) are connected to their > drivers rather than explicitly inheriting type specific driver (PCI, etc). > > .... > > Notes: > ====== > > * Some sign-off were already provided by Jan on the original v5; But, as a > large number of merges have been made, I am leaving those out just in case > it is not sync with initial understanding. > > * This might not be in-sync with pmdinfogen as PMD_REGISTER_DRIVER is > removed [7]. > > Future Work/Pending: > =================== > - Presently eth_driver, rte_eth_dev are not aligned to the rte_driver/ > rte_device model. eth_driver still is a PCI specific entity. This > has been highlighted by comments from Ferruh in [9]. > - cryptodev driver too still remains to be normalized over the rte_driver > model > - Some variables, like drv_name (as highlighted by Ferruh), are getting > duplicated across rte_xxx_driver/device and rte_driver/device. > .... > Hey Shreyansh, I got some time over the last couple of days to got through this patchset and your soc changes, and it's really a great clean up of the driver and device infrastructure code. I guess I'm coming to this party a bit late but I have some thoughts on the the object model that you've outlined it improves things a huge amount but I wonder does it go far enough. I think that currently the coupling of the rte_driver to instances of rte_pci_driver, rte_vdev_driver etc still really clouds the abstraction. I think if we introduced a rte_bus and rte_bus_data abstractions with all rte_drivers having a rte_bus instance then we could further simplify the driver and device management, I think this would greatly reduce the number of driver/device APIs in the EAL as I think most bus functionality could be abstracted into a simpler API. This would also make it possible to create new driver and device types and allowing them to be plug-able components into DPDK and which I don't think would work currently even with the changes included here. http://imgur.com/a/3b7XM The link above is to a png of a visio which outlines the object model that I'm thinking of. The main extension from your current patchset would be that rte_eth_driver and rte_crypto_driver would now always be inheriting from rte_driver and not rte_soc_driver/rte_pci_driver, etv but the rte_driver object would have rte_bus instance which could be of type rte_bus_pci/ rte_bus_virtual/ rte_bus_soc etc. This I think would allow the EAL to be written in such a way that it only every operates on the base object types rte_device/rte_driver directly never the derivative objects, like the rte_vdev_driver, rte_pci_driver. Also I prefer the abstraction that a driver has a bus rather than it is the bus. Also I think would be the devices layer to written driver/bus agnostic, so the eth_dev layer shouldn't know if and underlying PMD is PCI/memory based or virtual. Only the poll mode driver itself, and the part of the EAL which manages driver initialization should have knowledge of the drivers bus type. Anyway have a look, and let me know what you think. Thanks Declan