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

[Shreyansh Jain]

On Wednesday 04 January 2017 03:22 AM, Thomas Monjalon wrote:
> 2016-12-26 18:53, Shreyansh Jain:
>> +DPDK_17.02 {
>> +	global:
>> +
>> +	rte_bus_list;
>> +	rte_eal_bus_add_device;
>> +	rte_eal_bus_add_driver;
>> +	rte_eal_bus_get;
>> +	rte_eal_bus_dump;
>> +	rte_eal_bus_register;
>> +	rte_eal_bus_insert_device;
>> +	rte_eal_bus_remove_device;
>> +	rte_eal_bus_remove_driver;
>> +	rte_eal_bus_unregister;
>
> I think the prefix can be just rte_bus_ instead of rte_eal_bus_.

Ok. I will change these.
I thought as these symbols are part of librte_eal/*, naming should 
reflect that.

>
>> +/** Double linked list of buses */
>> +TAILQ_HEAD(rte_bus_list, rte_bus);
>> +
>> +/* Global Bus list */
>> +extern struct rte_bus_list rte_bus_list;
>
> Why the bus list is public?

I will revisit this. When I made it public, intention was that some
element external to EAL (e.g. drivers/bus) might require to iterate
over this list. But, as of now I don't think any such user/case exists
(except test*).

>
>> +/**
>> + * 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.

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.

>
>> +/** 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


>
>>  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.
2/ device->driver would only be updated after probe. device->driver->bus 
would not be valid in such cases, if required.

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.

-
Shreyansh