[dpdk-dev] [PATCH v4 01/24] eal: introduce one device scan

[Gaëtan Rivet]

On Tue, Jun 26, 2018 at 12:26:05PM +0000, Zhang, Qi Z wrote:
> 
> 
> > -----Original Message-----
> > From: Burakov, Anatoly
> > Sent: Tuesday, June 26, 2018 7:48 PM
> > To: Zhang, Qi Z <qi.z.zhang at intel.com>; thomas at monjalon.net
> > Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org;
> > Richardson, Bruce <bruce.richardson at intel.com>; Yigit, Ferruh
> > <ferruh.yigit at intel.com>; Shelton, Benjamin H
> > <benjamin.h.shelton at intel.com>; Vangati, Narender
> > <narender.vangati at intel.com>
> > Subject: Re: [PATCH v4 01/24] eal: introduce one device scan
> > 
> > On 26-Jun-18 8:08 AM, Qi Zhang wrote:
> > > When hot plug a new device, it is not necessary to scan everything on
> > > the bus since the devname and devargs are already there. So new
> > > rte_bus ops "scan_one" is introduced, bus driver can implement this
> > > function to simplify the hotplug process.
> > >
> > > Signed-off-by: Qi Zhang <qi.z.zhang at intel.com>
> > > ---
> > >
> > 
> > <snip>
> > 
> > > + *	NULL for unsuccessful scan
> > > + */
> > > +typedef struct rte_device *(*rte_bus_scan_one_t)(struct rte_devargs
> > > +*devargs);
> > > +
> > > +/**
> > >    * Implementation specific probe function which is responsible for linking
> > >    * devices on that bus with applicable drivers.
> > >    *
> > > @@ -204,6 +219,7 @@ struct rte_bus {
> > >   	TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
> > >   	const char *name;            /**< Name of the bus */
> > >   	rte_bus_scan_t scan;         /**< Scan for devices attached to
> > bus */
> > > +	rte_bus_scan_one_t scan_one; /**< Scan one device using devargs */
> > >   	rte_bus_probe_t probe;       /**< Probe devices on bus */
> > >   	rte_bus_find_device_t find_device; /**< Find a device on the bus */
> > >   	rte_bus_plug_t plug;         /**< Probe single device for drivers
> > */
> > >
> > 
> > Does changing this structure break ABI for bus drivers?
> 
> For bus driver, I think yes, but I'm not sure what I should do for this, since this is not for application
> 
> 

This should be appropriately announced in advance, in general.
However, it seems there is some leeway if the new field will not move
the others and not make the structure grow (i.e. replace a padding).

There is an ABI check script that can be used.

This however breaks the bus ABI, which breaks the EAL ABI.
This is usually an issue.

More generally, I was in favor of changing the whole bus scan process to a
per-device iteration. I was shut down on this when adding hotplug.
As a result, bus->scan() process was made to require the operation to be
idempotent.

Adding a new ops adds noise to the bus API. It should be kept as clean
as possible. This new one seems unnecessary, now that all bus scans are
idempotent (when supporting hotplug).

Regards,
-- 
Gaëtan Rivet
6WIND