| Message ID | 20180607123849.14439-3-qi.z.zhang@intel.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series |
|
| Related | show |
| Context | Check | Description |
|---|---|---|
| ci/Intel-compilation | fail | Compilation issues |
| ci/checkpatch | success | coding style OK |
On 6/7/2018 6:08 PM, Qi Zhang wrote: > Implemented the bus ops scan_one, besides this improve the scan > efficiency in hotplug case, it aslo avoid sync IPC invoke (which ^^^^ also > happens in vdev->scan on secondary process). The benifit is it ^^^^^^^ benefit > removes the potiential deadlock in the case when secondary process ^^^^^^^^^^ potential > receive a request from primary process to attach a new device, since > vdev->scan will be invoked on mp thread itself at this case. ^^^^^^^ in that Besides the above spells, is it possible to re-write the commit? You mention it "...improves the scan efficiency..." - how? Is that an implicit output of introducing the new scan_one for vdev? > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com> > --- > drivers/bus/vdev/vdev.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c > index 6139dd551..cdbd77df0 100644 > --- a/drivers/bus/vdev/vdev.c > +++ b/drivers/bus/vdev/vdev.c > @@ -467,6 +467,35 @@ vdev_scan(void) > return 0; > } > [...]
> -----Original Message----- > From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com] > Sent: Friday, June 8, 2018 8:08 PM > To: Zhang, Qi Z <qi.z.zhang@intel.com> > Cc: thomas@monjalon.net; Burakov, Anatoly <anatoly.burakov@intel.com>; > 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 02/22] bus/vdev: enable one device scan > > On 6/7/2018 6:08 PM, Qi Zhang wrote: > > Implemented the bus ops scan_one, besides this improve the scan > > efficiency in hotplug case, it aslo avoid sync IPC invoke (which > ^^^^ > also > > > happens in vdev->scan on secondary process). The benifit is it > ^^^^^^^ > benefit > > > removes the potiential deadlock in the case when secondary process > ^^^^^^^^^^ > potential > > > receive a request from primary process to attach a new device, since > > vdev->scan will be invoked on mp thread itself at this case. > ^^^^^^^ > in that > > > Besides the above spells, is it possible to re-write the commit? > You mention it "...improves the scan efficiency..." - how? Is that an implicit > output of introducing the new scan_one for vdev? "Improve scan efficiency" should be general to all buses in hot plug case. since compare to bus->scan, bus->scan_one no need to iterate all devargs. But yes, it's not the original purpose for this patch set, but a bonus. I will re-write comment with below format to make it more clear. The patch implemented bus ops scan_one for vdev, it gives two benefits 1. improve scan efficiency .... 2. avoid sync IPC invoke ..... Regards Qi > > > > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com> > > --- > > drivers/bus/vdev/vdev.c | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c index > > 6139dd551..cdbd77df0 100644 > > --- a/drivers/bus/vdev/vdev.c > > +++ b/drivers/bus/vdev/vdev.c > > @@ -467,6 +467,35 @@ vdev_scan(void) > > return 0; > > } > > > > [...]
diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c index 6139dd551..cdbd77df0 100644 --- a/drivers/bus/vdev/vdev.c +++ b/drivers/bus/vdev/vdev.c @@ -467,6 +467,35 @@ vdev_scan(void) return 0; } +static struct rte_device *vdev_scan_one(struct rte_devargs *devargs) +{ + struct rte_vdev_device *dev = NULL; + + dev = calloc(1, sizeof(*dev)); + if (!dev) { + VDEV_LOG(ERR, "failed to allocate memory for new device"); + return NULL; + } + + rte_spinlock_recursive_lock(&vdev_device_list_lock); + + if (find_vdev(devargs->name)) { + VDEV_LOG(ERR, "device %s already exist", devargs->name); + free(dev); + rte_spinlock_recursive_unlock(&vdev_device_list_lock); + return NULL; + } + + dev->device.devargs = devargs; + dev->device.numa_node = SOCKET_ID_ANY; + dev->device.name = devargs->name; + TAILQ_INSERT_TAIL(&vdev_device_list, dev, next); + + rte_spinlock_recursive_unlock(&vdev_device_list_lock); + + return &dev->device; +} + static int vdev_probe(void) { @@ -531,6 +560,7 @@ vdev_unplug(struct rte_device *dev) static struct rte_bus rte_vdev_bus = { .scan = vdev_scan, + .scan_one = vdev_scan_one, .probe = vdev_probe, .find_device = vdev_find_device, .plug = vdev_plug,
Implemented the bus ops scan_one, besides this improve the scan efficiency in hotplug case, it aslo avoid sync IPC invoke (which happens in vdev->scan on secondary process). The benifit is it removes the potiential deadlock in the case when secondary process receive a request from primary process to attach a new device, since vdev->scan will be invoked on mp thread itself at this case. Signed-off-by: Qi Zhang <qi.z.zhang@intel.com> --- drivers/bus/vdev/vdev.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)