From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 4ECD93230 for ; Mon, 14 Nov 2016 18:38:41 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP; 14 Nov 2016 09:38:40 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,491,1473145200"; d="scan'208";a="1085058983" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.57]) ([10.237.220.57]) by fmsmga002.fm.intel.com with ESMTP; 14 Nov 2016 09:38:39 -0800 To: Shreyansh Jain , David Marchand References: <86ece536-1574-6d84-661e-9b9e77180344@nxp.com> <1a19615c-1121-fed4-b34f-aa0f4b654085@intel.com> Cc: "dev@dpdk.org" From: Ferruh Yigit Message-ID: <1706a89e-7aa0-6a9d-401b-7529026f8589@intel.com> Date: Mon, 14 Nov 2016 17:38:39 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] Clarification for eth_driver changes 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: Mon, 14 Nov 2016 17:38:43 -0000 On 11/12/2016 5:44 PM, Shreyansh Jain wrote: > Hello Ferruh, > > (Please ignore if line wrappings are not correct. Using a possibly > unconfigured mail client). > >> -----Original Message----- >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] >> Sent: Saturday, November 12, 2016 12:46 AM >> To: Shreyansh Jain ; David Marchand >> >> Cc: dev@dpdk.org >> Subject: Re: [dpdk-dev] Clarification for eth_driver changes >> >> On 11/10/2016 11:05 AM, Shreyansh Jain wrote: >>> Hello David, >>> >>> On Thursday 10 November 2016 01:46 PM, David Marchand wrote: >>>> Hello Shreyansh, >>>> >>>> On Thu, Nov 10, 2016 at 8:26 AM, Shreyansh Jain >> wrote: >>>>> I need some help and clarification regarding some changes I am doing to >>>>> cleanup the EAL code. >>>>> >>>>> There are some changes which should be done for eth_driver/rte_eth_device >>>>> structures: >>>>> >>>>> 1. most obvious, eth_driver should be renamed to rte_eth_driver. >>>>> 2. eth_driver currently has rte_pci_driver embedded in it >>>>> - there can be ethernet devices which are _not_ PCI >>>>> - in which case, this structure should be removed. >>>> >>>> Do we really need to keep a eth_driver ? >>> >>> No. As you have rightly mentioned below (as well as in your Jan'16 >>> post), it is a mere convenience. >> >> Isn't it good to separate the logic related which bus device connected >> and what functionality it provides. Because these two can be flexible: > > Indeed. The very idea of a Bus model is to make a hierarchy which allows > for pluggability/flexibility in terms of devices being used. But, until now I > have only considered placement hierarchy and not functional hierarchy. (more > below) > >> >> device -> virtual_bus -> ethernet_functionality >> device -> pci_bus -> crypto_functionality >> device -> x_bus -> y_function >> > > Ok. > >> >> what about: >> >> create generic bus driver/device and all eal level deal with generic >> bus. different buses inherit from generic bus logic > > From what I had in mind: (and very much similar to what you have already > mentioned in this email) > - a generic bus (not a driver, not a device). I don't know how to categorize > a bus. It is certainly not a device, and then handler for a bus (physical) > can be considered a 'bus driver'. So, just 'rte_bus'. > - there is a bus for each physical implementation (or virtual). So, a rte_bus > Object for PCI, VDEV, ABC, DEF and so on. > - Buses are registered just like a PMD - RTE_PMD_BUS_REGISTER() > -- There is a problem of making sure the constructor for bus registration is > executed before drivers. Probably by putting the bus code within lib* > - Each registered bus is part of a doubly list. > - Each device inherits rte_bus > - Each driver inherits rte_bus > - Existing Device and Drivers lists would be moved into rte_bus > > (more below) > >> >> create generic functionality device/driver and pmd level deal with >> these. different functionalities inherit from generic functionality logic >> >> and use rte_device/rte_driver as glue logic for all these. >> >> This makes easy to add new bus or functionality device/drivers without >> breaking existing logic. >> >> >> Something like this: >> >> struct rte_device { >> char *name; >> struct rte_driver *drv; >> struct rte_bus_device *bus_dev; >> struct rte_funcional_device *func_dev; >> *devargs >> } >> >> struct rte_bus_device { >> struct rte_device *dev; >> /* generic bus device */ >> } > > This is where you lost me. From what I understood from your text: > (CMIIW) > > - Most abstract class is 'rte_device'. > - A bus is a device. So, it points to a rte_device > - But, a rte_device belongs to a bus, so it points to a rte_bus_device. > > Isn't that contradictory? What I was thinking is: rte_device/driver are not abstract classes. rte_bus device/driver is an abstract class and any bus inherited from this class. rte_func device/driver is and abstract class and eth/crypto inherited from this class. eal layer only deal with rte_bus pmd's only deal with functional device/driver but still, it is required to know device <-> driver, and functional <-> bus, relations. rte_dev/rte_driver are to provide this links. But yes this add extra layer and with second thought I am not sure if it is really possible to separate bus and functionality, this was just an idea .. > > This is how I view the physical layout of devices on which DPDK works: > > +---------+ +----------+ > |Driver 1A| |Driver 2B | > |servicing| |servicing | (*) > |Bus A | |Bus B | > +---------+ +----------+ > \ / > +---------+ +-------+ > | bus A |---| bus B |--- ... > +---------+ +-------+ > / \ \ \ > / \_ \ \ > +---------+ / / \ > | device 1| / +--------+ \ > | on Bus A| / |Device 3| | > +---------+ / |on bus B| | > +---------+ +--------+ | > | device 2| +--------+ > | on Bus A| |Device 4| > +---------+ |on bus B| > +--------+ > > (*) Multiple drivers servicing a Bus. > > Now, if we introduce the abstraction for functionality (assuming net/crypto) as > two functionalities currently applicable: > > +---------+ +----------+ > |Driver 1A| |Driver 2B | > |servicing| |servicing | (*) > |Bus A | |Bus B | > +---------' +---.------+ > \ / > +---------+ +-------+ > | bus A |---| bus B |--- ... > +---------+ +-------+ > / \ \ \ > / \_ \ \ > +---------' / / \ > | device 1| / +--------' \ > | on Bus A| / |Device 3| | > +---------+ / |on bus B| | > / +---------' +-|------+ | > | | device 2| / +-------'+ > | | on Bus A| / |Device 4| > \ +--|------+ _____/ |on bus B| > \ \_____ / +--------+ > | .--\' / > | / \ ___/ > +-'----'-+ +'------'+ > |Func X | |Func Y | > |(Net) | |(Crypto)| > +--------| +--------+ > > So that means, a device would be a 'net' or 'crypto' device bases on the > Functionality it attaches to. > > From a physical layout view, is that correct understanding of your argument? > >> >> struct rte_pci_device { >> struct rte_bus_device bus_dev; >> /* generic pci bus */ >> } >> >> struct rte_vdev_device { >> struct rte_bus_device bus_dev; >> /* generic vdev bus */ >> } >> >> struct rte_funcional_device { >> struct rte_device *dev; >> } > > I understand your point of 'pluggable' functionality. It would be helpful if > same driver would like to move between being a crypto and net. But is that a > plausible use case for DPDK right now? > > To me, it seems as one more layer of redirection/abstraction. > > This is what the view I have as of now: > > __ rte_bus_list > / > +----------'---+ > |rte_bus | > | driver_list | > | device_list | > | scan | > | match | > | remove | > | ..some refcnt| > +--|------|----+ > _________/ \_________ > +--------/----+ +-\--------------+ > |rte_device | |rte_driver | > | rte_bus | | rte_bus | > | flags (3*) | | probe (1*) | > | init | | remove | > | uninit | | ... | > +---||--------+ | drv_flags | > || | intr_handle(2*)| > | \ +----------\\\---+ > | \_____________ \\\ > | \ ||| > +------|---------+ +----|----------+ ||| > |rte_pci_device | |rte_xxx_device | (4*) ||| > | PCI specific | | xxx device | ||| > | info (mem,) | | specific fns | / | \ > +----------------+ +---------------+ / | \ > _____________________/ / \ > / ___/ \ > +-------------'--+ +------------'---+ +--'------------+ > |rte_pci_driver | |rte_vdev_driver | |rte_xxx_driver | > | PCI id table, | | | other driver | | nothing> | +---------------+ > | data | +----------------+ > +----------------+ > > (1*) Problem is that probe functions have different arguments. So, > generalizing them might be some rework in the respective device > layers probe() is now done in bus level, right? pci_probe(), vdev_probe(), ... And I guess, eth_dev and crypto_dev are created during probe(), how probe() knows if to create eth_dev or crypto_dev? > (2*) Interrupt handling for each driver type might be different. I am not > sure how to generalize that either. This is grey area for me. > (3*) Probably exposing a bitmask for device capabilities. Nothing similar > exists now to relate it. Don't know if that is useful. Allowing > applications to question a device about what it supports and what it > doesn't - making it more flexible at application layer (but more code > as well.) > (4*) Even vdev would be an instantiated as a device. It is not being done > currently. I think it is good to add this to be consistent. > > So, unlike your model, rte_bus remains the topmost class which is neither a > device, not a driver. It is just a class. > Further, as specific information exists in each specific device and driver, > that is not generalized. > <...> > > I am not against what you have stated. Creating a functional device is just > one more layer of abstraction in my view. Mostly abstraction/classification > makes life easier for applications to visualize underlying hierarchy. If this > serves a purpose, I am OK with that. At least right now, I think it would > only end up being like eth_driver which is just a holder. I agree with you, mine was more like a brain exercise, may have gaps and I need to think more, thanks for your comments. Thanks, ferruh