From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH 00/13] Introducing EAL Bus-Device-Driver Model Date: Tue, 13 Dec 2016 12:26:14 +0530 Message-ID: <768defac-1538-e425-7f78-0d88d3303d06@nxp.com> References: <1480846288-2517-1-git-send-email-shreyansh.jain@nxp.com> <1697fe66-962d-0848-5e68-615249b52dad@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: David Marchand , "dev@dpdk.org" , Thomas Monjalon , Ferruh Yigit To: Jianbo Liu Return-path: Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0079.outbound.protection.outlook.com [104.47.34.79]) by dpdk.org (Postfix) with ESMTP id 1D651282 for ; Tue, 13 Dec 2016 07:53:33 +0100 (CET) In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Jianbo, On Monday 12 December 2016 08:05 PM, Jianbo Liu wrote: > Hi Shreyansh, > > On 7 December 2016 at 21:10, Shreyansh Jain wrote: >> On Wednesday 07 December 2016 05:47 PM, David Marchand wrote: >>> >>> Hello Shreyansh, >>> >>> On Wed, Dec 7, 2016 at 10:55 AM, Shreyansh Jain >>> wrote: >>>> >>>> On Wednesday 07 December 2016 02:22 AM, David Marchand wrote: >>>>>> >>>>>> 0002~0003: Introducing the basic Bus model and associated test case >>>>>> 0005: Support insertion of device rather than addition to tail >>>>> >>>>> >>>>> >>>>> Patch 2 and 5 could be squashed. >>>> >>>> >>>> >>>> I deliberately kept them separate. I intent to extend the Patch 5 for >>>> hotplugging. But, if I don't end up adding support for that in this >>>> series, >>>> I will merge these two. >>> >>> >>> Fine. >>> >>> >>>>> The constructor priority stuff seems unneeded as long as we use >>>>> explicit reference to a global (or local, did not check) bus symbol >>>>> rather than a runtime lookup. >>>> >>>> >>>> >>>> I didn't understand your point here. >>>> IMO, constructor priority (or some other way to handle this) is >>>> important. I >>>> faced this issue while verifying it at my end when the drivers were >>>> getting >>>> registered before the bus. >>>> >>>> Can you elaborate more on '..use explicit reference to a global...'? >>> >>> >>> The drivers register themselves to a bus using this bus specific api. >>> >>> For pci, this is rte_eal_pci_register(). >>> The pci_bus object must be moved to eal_common_pci.c (we can stil >>> internally expose for bsd / linux specific implementations). >>> Then, rte_eal_pci_register() can add the pci driver to the pci_bus >>> drivers list even if this pci_bus object is not registered yet to the >>> buses list. >> >> >> So, in eal_common_bus.c >> >> --->8--- >> >> struct rte_bus *global_ptr_to_pci_bus = NULL; >> >> struct rte_bus pci_bus = { ... }; >> >> rte_eal_pci_register() { >> if (global_ptr_to_pci_bus == NULL) >> rte_eal_bus_register(&pci_bus) >> else >> // continue as if PCI bus is registered >> } >> >> --->8--- >> >> so, no RTE_REGISTER_BUS()? >> >> If yes, then RTE_REGISTER_BUS() should also check for an existing >> registration for duplication. >> >> I was banking on a model where bus handlers (or bus drivers) are independent >> entities, just like PMDs. So, we have a bus XYZ without any drivers >> necessarily based on it. >> >> By making registration dependent on driver registration, it becomes implicit >> that buses don't exist without drivers. >> I am not in favor of this - or maybe I lack enough reason for this (about >> how it will make framework/PMD life better). >> >>> >>> And no constructor order issue ? >>> >>> >>>>> >>>>> >>>>>> 0004: Add scan and match callbacks for the Bus and updated test >>>>>> case >>>>> >>>>> >>>>> >>>>> Why do you push back the bus object in the 'scan' method ? >>>>> This method is bus specific which means that the code "knows" the >>>>> object registered with the callback. >>>> >>>> >>>> >>>> This 'knows' is the grey area for me. >>>> The bus (for example, PCI) after scanning needs to call >>>> rte_eal_bus_add_device() to link the device in bus's device_list. >>>> >>>> Two options: >>>> 1. Have a global reference to "pci" bus (rte_bus) somewhere in eal_pci.c >>>> 2. Call rte_eal_get_bus() every time someone needs the reference. >>>> 3. C++ style, 'this->'. >>>> >>>> I have taken the 3rd path. It simplifies my code to not assume a handle >>>> as >>>> well as not allow for reference fetch calls every now and then. >>>> >>>> As a disadvantage: it means passing this as argument - and some cases >>>> maintaining it as __rte_unused. >>>> >>>> Taking (1) or (2) is not advantageous than this approach. >>> >>> >>> 1) is the simplest one. >>> >>> When you write a pci_scan method and embed it in you pci_bus object, >>> but this pci_scan method still wonders which bus object it is supposed >>> to work on, this is a bit like Schizophrenia ;-). >> >> >> :) >> This now is linked to the above issue of constructor priority and having a >> global bus reference. I don't personally prefer it. >> I will still give this a serious thought, though. >> > > I'm also in favor of (3). Thank you. I was almost done with v2 and in that I had changed to what David had suggested. My preference too is (3). Now, I will prefer sticking with it - until someone comes with technical issue (like compiler compatibility etc) which I am unaware of. @David: Can you re-think if you still prefer (1)? If so, I will change it in v3 (I will send v2 in a day or two max). > >>> >>> >>>>> Is is that you want to have a single scan method used by multiple buses >>>>> ? >>>> >>>> >>>> >>>> Yes, but only as a use case. For example, platform devices are of various >>>> types - what if we have a south-bound bus over a platform bus. In which >>>> case, a hierarchical bus layout is possible. >>>> But, this is far-fetched idea for now. >>> > > How to express the hierarchical bus layout as the bus in your design > is more like independent objects to hold drivers and their devices? What I had in mind was something on the lines of: 1) Add a new linked list 'bus_list' in rte_bus 2) OR, embed rte_device in rte_bus (1) is for maintaining buses as independent entity; (2) is for treating buses like devices (very similar to what Ferruh once suggested [2]). I prefer (1), but I think programmatically (2) is much more symmetrical. I am assuming (1) below. If we have: (taking hint from [1]) CPU | ====,============`============= PCI Bus 0 | PCI-PCI Bridge | =,='=======,====== PCI Bus 1 | | SCSI Ethernet PCI Bus 0 (rte_bus)pci_bus_0 `.-> scan(): this calls knows it is a PCI-PCI bridge. It would allocate | a new pci_bus_1 (rte_bus object) and attach to bus_list. | Then, assign generic SCSI scan functions to pci_bus_1->scan | and pci_bus_1->match. `-> eal/probe() - bus->match() is called with rte_device/driver. In this case, it would move over all the buses in bus_list pivoted on pci_bus_0 and call rte_bus->match. - (#) For each matched entry, subsequently call the rte_bus->probe() - (*) Cascading calls to rte_driver->probe() for pci_bus_1 (#) there is still an open discussion about whether bus->probe() should exist or not. (I am not convinced buses should probe, but DPDK model doesn't bode well without it) (*) pci_bus_0->probe() would get rte_device/rte_driver as NULL and rotate over each device/driver scanned in pci_bus_1 calling bus->probe. [1] http://www.tldp.org/LDP/tlk/dd/pci.html [2] http://dpdk.org/ml/archives/dev/2016-August/045947.html (Note: I agree that there are minor holes in above theory, specifically from implementation point. But, I am confident that with minor changes this is achievable). > >>> >>> Well, if you have no usecase at the moment, let's keep it simple, please. >>> >> >> Ok. >> >>> >>>>> >>>>>> 0006: Integrate bus scan/match with EAL, without any effective >>>>>> driver >>>>> >>>>> >>>>> >>>>> Hard to find a right balance in patch splittng, but patch 4 and 6 are >>>>> linked, I would squash them into one. >>>> >>>> >>>> >>>> Yes, it is hard and sometimes there is simply no strong rationale for >>>> splitting or merging. This is one of those cases. >>>> My idea was that one patch _only_ introduces Bus services (structures, >>>> functions etc) and another should enable the calls to it from EAL. >>>> In that sense, I still think 4 and 6 should remain separate, may be >>>> consecutive, though. >>> >>> >>> Ok, will see in next version of the patchset. >> >> >> Is there anything specific that you are looking for in patchset v2? >> I was thinking of: >> 0. fixing BSD compilation issue reported by CI >> 1. improving the test_pci.c >> 2. hotplugging >> 3. trying to move PCI to drives/bus/pci/linux/* and resolving how drivers >> link to it, and how EAL resources like devargs are consumed. >> >> Anything else? >> >>> >>> >>>>> >>>>>> 0007: rte_pci_driver->probe replaced with rte_driver->probe >>>>> >>>>> >>>>> >>>>> This patch is too big, please separate in two patches: eal changes >>>>> then ethdev/driver changes. >>>> >>>> >>>> >>>> I don't think that can be done. One change is incomplete without the >>>> other. >>>> >>>> Changes to all files are only for rte_pci_driver->probe to >>>> rte_driver->probe >>>> movement. EAL changes is to allow rte_eth_dev_pci_probe function after >>>> such >>>> a change as rte_driver->probe has different arguments as compared to >>>> rte_pci_driver->probe. The patches won't compile if I split. >>>> >>>> Am I missing something? >>>>> >>>>> >>>>> Why do you push back the driver object in the 'probe' method ? (idem >>>>> rte_bus->scan). >>>> >>>> >>>> >>>> I am assuming you are referring to rte_driver->probe(). >>>> This is being done so that implementations (specific to drivers on a >>>> particular bus) can start extracting the rte_xxx_driver, if need be. >>>> >>>> For example, for e1000/em_ethdev.c, rte_driver->probe() have been set to >>>> rte_eth_dev_pci_probe() which requires rte_pci_driver to work with. In >>>> absence of the rte_driver object, this function cannot call >>>> rte_pci_driver->probe (for example) for driver specific operations. >>> >>> >>> Sorry, I am thinking a step ahead with eth_driver out of the picture. >>> But once eth_driver disappears, I can see no reason to keep this >>> driver in the probe method (Schizophrenia again). >> >> >> When eth_driver disappears, i was thinking of accomodating the eth_dev_init >> into the rte_pci_driver->probe/init. >> But, this is still a nascent thought. >> I am yet to start working on eth_driver. >> >>> >>> >>> >> > Thanks for your comments. - Shreyansh