[dpdk-dev,02/22] bus/vdev: enable one device scan
diff mbox series

Message ID 20180607123849.14439-3-qi.z.zhang@intel.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • enable hotplug on multi-process
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Zhang, Qi Z June 7, 2018, 12:38 p.m. UTC
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(+)

Comments

Shreyansh Jain June 8, 2018, 12:08 p.m. UTC | #1
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;
>   }
>   

[...]
Zhang, Qi Z June 13, 2018, 1:32 p.m. UTC | #2
> -----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;
> >   }
> >
> 
> [...]

Patch
diff mbox series

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,