[v4,01/24] eal: introduce one device scan
Checks
Commit Message
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@intel.com>
---
lib/librte_eal/common/eal_common_dev.c | 17 +++++++++++++----
lib/librte_eal/common/include/rte_bus.h | 16 ++++++++++++++++
2 files changed, 29 insertions(+), 4 deletions(-)
Comments
On 26/06/2018 08:08, Qi Zhang wrote:
[..]
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>
> lib/librte_eal/common/eal_common_dev.c | 17 +++++++++++++----
> lib/librte_eal/common/include/rte_bus.h | 16 ++++++++++++++++
Acked-by: Remy Horton <remy.horton@intel.com>
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@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?
> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Tuesday, June 26, 2018 7:48 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@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@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
>
> --
> Thanks,
> Anatoly
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@intel.com>; thomas@monjalon.net
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> > Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Shelton, Benjamin H
> > <benjamin.h.shelton@intel.com>; Vangati, Narender
> > <narender.vangati@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@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,
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Wednesday, June 27, 2018 12:34 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; thomas@monjalon.net;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 01/24] eal: introduce one device scan
>
> 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@intel.com>; thomas@monjalon.net
> > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > > dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Yigit,
> > > Ferruh <ferruh.yigit@intel.com>; Shelton, Benjamin H
> > > <benjamin.h.shelton@intel.com>; Vangati, Narender
> > > <narender.vangati@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@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.
OK, since we are able to invoke IPC request in a separate thread and also with below
fix, there is no issue on the vdev->scan during hotplug on secondary.
https://patches.dpdk.org/patch/41647/
ABI break is not necessary, I will withdraw patch 1 and 2.
Thanks
Qi
>
> 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
@@ -147,11 +147,20 @@ int __rte_experimental rte_eal_hotplug_add(const char *busname, const char *devn
if (ret)
goto err_devarg;
- ret = bus->scan();
- if (ret)
- goto err_devarg;
+ /**
+ * if bus support to scan specific device by devargs,
+ * we don't need to scan all devices on the bus.
+ */
+ if (bus->scan_one) {
+ dev = bus->scan_one(da);
+ } else {
+ ret = bus->scan();
+ if (ret)
+ goto err_devarg;
+
+ dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
+ }
- dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
if (dev == NULL) {
RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
devname);
@@ -84,6 +84,21 @@ enum rte_iova_mode {
typedef int (*rte_bus_scan_t)(void);
/**
+ * Bus specific scan for one specific device attached on the bus.
+ * For each bus object, the scan would be responsible for finding the specific
+ * device and adding it to its private device list, and the device object will
+ * be return also.
+ *
+ * @param devargs
+ * Device arguments be used to identify the device.
+ *
+ * @return
+ * !NULL for successful scan
+ * 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 */