From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v5 01/12] eal/bus: introduce bus abstraction Date: Mon, 9 Jan 2017 11:54:10 +0530 Message-ID: References: <1482756644-13726-1-git-send-email-shreyansh.jain@nxp.com> <12579803.ZHBMo4GdSA@xps13> <105987546.PAaz77144n@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: , To: Thomas Monjalon Return-path: Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0061.outbound.protection.outlook.com [104.47.34.61]) by dpdk.org (Postfix) with ESMTP id 0968B1E2F for ; Mon, 9 Jan 2017 07:20:44 +0100 (CET) In-Reply-To: <105987546.PAaz77144n@xps13> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Friday 06 January 2017 08:25 PM, Thomas Monjalon wrote: > 2017-01-06 16:01, Shreyansh Jain: >> On Wednesday 04 January 2017 03:22 AM, Thomas Monjalon wrote: >>> 2016-12-26 18:53, Shreyansh Jain: >>>> +/** >>>> + * A structure describing a generic bus. >>>> + */ >>>> +struct rte_bus { >>>> + TAILQ_ENTRY(rte_bus) next; /**< Next bus object in linked list */ >>>> + struct rte_driver_list driver_list; >>>> + /**< List of all drivers on bus */ >>>> + struct rte_device_list device_list; >>>> + /**< List of all devices on bus */ >>>> + const char *name; /**< Name of the bus */ >>>> +}; >>> >>> I am not convinced we should link a generic bus to drivers and devices. >>> What do you think of having rte_pci_bus being a rte_bus and linking >>> with rte_pci_device and rte_pci_driver lists? >> >> This is different from what I had in mind. >> You are saying: >> >> Class: rte_bus >> `-> No object instantiated for this class >> Class: rte_pci_bus inheriting rte_bus >> `-> object instantiated for this class. >> >> Here, rte_bus is being treated as an abstract class which is only >> inherited and rte_pci_bus is the base class which is instantiated. >> >> And I was thinking: >> >> Class: rte_bus >> `-> Object: pci_bus (defined in */eal/eal_pci.c) >> >> Here, rte_bus is that base class which is instantiated. >> >> I agree that what you are suggesting is inline with current model: >> rte_device -> abstract class (no one instantiates it) >> `-> rte_pci_device -> Base class which inherits rte_device and >> is instantiated. > > Yes > >> When I choose not to create rte_pci_bus, it was because I didn't want >> another indirection in form of rte_bus->rte_pci_bus->object. >> There were no 'non-generic' bus functions which were only applicable for >> rte_pci_bus. Eventually, rte_pci_bus ended up being a direct inheritance >> of rte_bus. >> >>> I'm thinking to something like that: >>> >>> struct rte_bus { >>> TAILQ_ENTRY(rte_bus) next; >>> const char *name; >>> rte_bus_scan_t scan; >>> rte_bus_match_t match; >>> }; >>> struct rte_pci_bus { >>> struct rte_bus bus; >>> struct rte_pci_driver_list pci_drivers; >>> struct rte_pci_device_list pci_devices; >>> }; >> >> if we go by rte_bus->rte_pci_bus->(instance of rte_pci_bus), above is >> fine. Though, I am in favor of rte_bus->(instance of rte_bus for PCI) >> because I don't see any 'non-generic' information in rte_pci_bus which >> can't be put in rte_bus. > > The lists of drivers and devices are specific to the bus. > Your proposal was to list them as generic rte_driver/rte_device and > cast them. I'm just saying we can directly declare them with the right type, > e.g. rte_pci_driver/rte_pci_device. Ok. I get your point. Already changing the code to reflect this. > > In the same logic, the functions probe/remove are specifics for the bus and > should be declared in rte_pci_driver instead of the generic rte_driver. Yes, I agree with this after above argument. > > >>>> +/** Helper for Bus registration. The constructor has higher priority than >>>> + * PMD constructors >>>> + */ >>>> +#define RTE_REGISTER_BUS(nm, bus) \ >>>> +static void __attribute__((constructor(101), used)) businitfn_ ##nm(void) \ >>>> +{\ >>>> + (bus).name = RTE_STR(nm);\ >>>> + rte_eal_bus_register(&bus); \ >>>> +} >>> >>> By removing the lists from rte_bus as suggested above, do you still need >>> a priority for this constructor? >> >> I think yes. >> Even if we have rte_pci_bus as a class, object of rte_bus should be part >> of Bus list *before* registration of a driver (because, driver >> registration searches for bus by name). >> >> (This is assuming that no global PCI/VDEV/XXX bus object is created >> which is directly used within all PCI specific bus operations). >> >> There was another suggestion on list which was to check for existence of >> bus _within_ each driver registration and create/instantiate an object >> in case no bus is registered. I didn't like the approach so I didn't use >> it. From David [1], and me [2] >> >> [1] http://dpdk.org/ml/archives/dev/2016-December/051689.html >> [2] http://dpdk.org/ml/archives/dev/2016-December/051698.html > > OK, we can keep your approach of prioritize bus registrations. > If we see an issue later, we could switch to a bus registration done > when a first driver is registered on the bus. Thanks for confirmation. > > >>>> struct rte_device { >>>> TAILQ_ENTRY(rte_device) next; /**< Next device */ >>>> + struct rte_bus *bus; /**< Device connected to this bus */ >>>> const struct rte_driver *driver;/**< Associated driver */ >>>> int numa_node; /**< NUMA node connection */ >>>> struct rte_devargs *devargs; /**< Device user arguments */ >>>> @@ -148,6 +149,7 @@ void rte_eal_device_remove(struct rte_device *dev); >>>> */ >>>> struct rte_driver { >>>> TAILQ_ENTRY(rte_driver) next; /**< Next in list. */ >>>> + struct rte_bus *bus; /**< Bus serviced by this driver */ >>>> const char *name; /**< Driver name. */ >>>> const char *alias; /**< Driver alias. */ >>>> }; >>> >>> Do we need to know the bus associated to a driver in rte_driver? >>> Bus and driver are already associated in rte_device. >> >> Two reasons: >> 1/ A driver should be associated with a bus so that if required, all bus >> can be directly extracted - even when probing has not been done. > > I do not understand this need. For example, Looping over all drivers for plugging them out. We need to know which bus a driver is on so that we can unplug the devices associated with the driver on that bus. > >> 2/ device->driver would only be updated after probe. device->driver->bus >> would not be valid in such cases, if required. > > We can update device->driver on match. Yes, we can. > > Please let's do not over-engineer if not needed. > In this case, I think we can skip rte_driver->bus. Hm, Ok. This was more of prospective step. We can avoid it without much impact. I will change the code. > > >> Overall, I don't have objections for rte_bus->rte_pci_bus=>object as >> compared to rte_bus=>PCI-object. But, I would still like to get a final >> confirmation of a more preferred way. >> >> Meanwhile, I will make changes to accommodate this change to save time >> in case rte_pci_bus class is final/preferred method. > > It looks more natural to me to avoid class casting and use specialized classes > when possible. So yes I prefer instantiating rte_pci_bus. > However, I could be wrong, and will consider any argument. Ok. I will go with your argument - mostly because I am OK either way and we can always come back if framework changes are stable. > > Thanks >