diff mbox

[dpdk-dev,v2] bus/vdev: fix scope of device list lock

Message ID 20180521164544.26421-1-thomas@monjalon.net (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

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

Commit Message

Thomas Monjalon May 21, 2018, 4:45 p.m. UTC 
  The lock vdev_device_list_lock was taken before calling
"remove" function for the device.
So it prevents to remove sub-devices (as in failsafe) inside
its own "remove" function, because of a deadlock.

The lock is now only protecting the device list inside
the bus driver.

Fixes: 35f462839b69 ("bus/vdev: add lock on device list")

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
v2: reduce scope more by moving unlock
---
 drivers/bus/vdev/vdev.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Matan Azrad May 21, 2018, 5:28 p.m. UTC | #1 
From: Thomas Monjalon
> The lock vdev_device_list_lock was taken before calling "remove" function for
> the device.
> So it prevents to remove sub-devices (as in failsafe) inside its own "remove"
> function, because of a deadlock.
> 
> The lock is now only protecting the device list inside the bus driver.
> 
> Fixes: 35f462839b69 ("bus/vdev: add lock on device list")
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Tested-by: Matan Azrad <matan@mellanox.com>
Gaëtan Rivet May 22, 2018, 9:11 a.m. UTC | #2 
On Mon, May 21, 2018 at 05:28:52PM +0000, Matan Azrad wrote:
> 
> 
> From: Thomas Monjalon
> > The lock vdev_device_list_lock was taken before calling "remove" function for
> > the device.
> > So it prevents to remove sub-devices (as in failsafe) inside its own "remove"
> > function, because of a deadlock.
> > 
> > The lock is now only protecting the device list inside the bus driver.
> > 
> > Fixes: 35f462839b69 ("bus/vdev: add lock on device list")
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Tested-by: Matan Azrad <matan@mellanox.com>

If these locks were necessary, they would be missing as well for
rte_devargs. Jianfeng inquired about it, I think it should be followed
upon.

Restricting the scope of the lock here could maybe re-introduce the bug
that motivated their introduction in the first place, as the
devargs_remove() is not in the critical section anymore.

However, this is an rte_devargs issue, not a vdev bus one, so
the fix makes sense and I'd like to have it ASAP for failsafe.
Without a vdev bus maintainer left:

Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
diff mbox

Patch

diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 099b9ff85..470cff46c 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -293,25 +293,24 @@  rte_vdev_uninit(const char *name)
 	if (name == NULL)
 		return -EINVAL;
 
-	rte_spinlock_lock(&vdev_device_list_lock);
-
 	dev = find_vdev(name);
 	if (!dev) {
 		ret = -ENOENT;
-		goto unlock;
+		return ret;
 	}
 
 	ret = vdev_remove_driver(dev);
 	if (ret)
-		goto unlock;
+		return ret;
 
+	rte_spinlock_lock(&vdev_device_list_lock);
 	TAILQ_REMOVE(&vdev_device_list, dev, next);
+	rte_spinlock_unlock(&vdev_device_list_lock);
+
 	devargs = dev->device.devargs;
 	rte_devargs_remove(devargs->bus->name, devargs->name);
 	free(dev);
 
-unlock:
-	rte_spinlock_unlock(&vdev_device_list_lock);
 	return ret;
 }