[dpdk-dev] [PATCH v5 01/12] eal/bus: introduce bus abstraction

[Shreyansh Jain]

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
>