Skip to contentSkip to conversation

vhost-net: avoid flush under lock

PATCH In review Michael S. Tsirkin 2 participants 5 messages Jul 15, 2010

Subsystems (5)
Networking Network drivers KVM virtio core +1 more subsystems
lore
Share & tools
lore

Michael S. Tsirkin

author maintainer
#0 lore raw

PATCH 1/1 vhost-net: avoid flush under lock

We flush under vq mutex when changing backends. This creates a deadlock as workqueue being flushed needs this lock as well.

https://bugzilla.redhat.com/show_bug.cgi?id=612421

Drop the vq mutex before flush: we have the device mutex which is sufficient to prevent another ioctl from touching the vq.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/vhost/net.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/vhost/net.c +5 -0
index 28d7786..50df58e6 100644
@@ -534,11 +534,16 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 	rcu_assign_pointer(vq->private_data, sock);
 	vhost_net_enable_vq(n, vq);
 done:
+	mutex_unlock(&vq->mutex);
+
 	if (oldsock) {
 		vhost_net_flush_vq(n, index);
 		fput(oldsock->file);
 	}
 
+	mutex_unlock(&n->dev.mutex);
+	return 0;
+
 err_vq:
 	mutex_unlock(&vq->mutex);
 err:
Signature · 1 line
1.7.2.rc0.14.g41c1c
author maintainer
#1 lore raw
Quoted 11 lines · Michael S. Tsirkin

We flush under vq mutex when changing backends. This creates a deadlock as workqueue being flushed needs this lock as well.

https://bugzilla.redhat.com/show_bug.cgi?id=612421

Drop the vq mutex before flush: we have the device mutex which is sufficient to prevent another ioctl from touching the vq.

Signed-off-by Michael S. Tsirkin <[email protected]>

Dave, just to clarify, I'll send pull request to merge it through my tree, there's no need for you to bother with this.

Quoted 27 lines
---
 drivers/vhost/net.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/vhost/net.c +5 -0
index 28d7786..50df58e6 100644
@@ -534,11 +534,16 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 	rcu_assign_pointer(vq->private_data, sock);
 	vhost_net_enable_vq(n, vq);
 done:
+	mutex_unlock(&vq->mutex);
+
 	if (oldsock) {
 		vhost_net_flush_vq(n, index);
 		fput(oldsock->file);
 	}
 
+	mutex_unlock(&n->dev.mutex);
+	return 0;
+
 err_vq:
 	mutex_unlock(&vq->mutex);
 err:
Signature · 1 line
1.7.2.rc0.14.g41c1c
author maintainer
#2 lore raw
In reply to Michael S. Tsirkin
Quoted 11 lines · Michael S. Tsirkin

We flush under vq mutex when changing backends. This creates a deadlock as workqueue being flushed needs this lock as well.

https://bugzilla.redhat.com/show_bug.cgi?id=612421

Drop the vq mutex before flush: we have the device mutex which is sufficient to prevent another ioctl from touching the vq.

Signed-off-by Michael S. Tsirkin <[email protected]>

Dave, just to clarify, I'll send pull request to merge it through my tree, there's no need for you to bother with this.

Quoted 27 lines
---
 drivers/vhost/net.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/vhost/net.c +5 -0
index 28d7786..50df58e6 100644
@@ -534,11 +534,16 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 	rcu_assign_pointer(vq->private_data, sock);
 	vhost_net_enable_vq(n, vq);
 done:
+	mutex_unlock(&vq->mutex);
+
 	if (oldsock) {
 		vhost_net_flush_vq(n, index);
 		fput(oldsock->file);
 	}
 
+	mutex_unlock(&n->dev.mutex);
+	return 0;
+
 err_vq:
 	mutex_unlock(&vq->mutex);
 err:
Signature · 1 line
1.7.2.rc0.14.g41c1c
author maintainer
#3 lore raw
In reply to Michael S. Tsirkin
Quoted 11 lines · Michael S. Tsirkin

We flush under vq mutex when changing backends. This creates a deadlock as workqueue being flushed needs this lock as well.

https://bugzilla.redhat.com/show_bug.cgi?id=612421

Drop the vq mutex before flush: we have the device mutex which is sufficient to prevent another ioctl from touching the vq.

Signed-off-by Michael S. Tsirkin <[email protected]>

Dave, just to clarify, I'll send pull request to merge it through my tree, there's no need for you to bother with this.

Quoted 27 lines
---
 drivers/vhost/net.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/vhost/net.c +5 -0
index 28d7786..50df58e6 100644
@@ -534,11 +534,16 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 	rcu_assign_pointer(vq->private_data, sock);
 	vhost_net_enable_vq(n, vq);
 done:
+	mutex_unlock(&vq->mutex);
+
 	if (oldsock) {
 		vhost_net_flush_vq(n, index);
 		fput(oldsock->file);
 	}
 
+	mutex_unlock(&n->dev.mutex);
+	return 0;
+
 err_vq:
 	mutex_unlock(&vq->mutex);
 err:
Signature · 1 line
1.7.2.rc0.14.g41c1c
#4 lore raw
In reply to Michael S. Tsirkin
Quoted 9 lines · Michael S. Tsirkin

We flush under vq mutex when changing backends. This creates a deadlock as workqueue being flushed needs this lock as well.

https://bugzilla.redhat.com/show_bug.cgi?id=612421

Drop the vq mutex before flush: we have the device mutex which is sufficient to prevent another ioctl from touching the vq.

Why do we need to flush the vq when trying to set the backend and we find that it is already set. Is this just an optimization?

Thanks
Sridhar

Quoted 27 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/vhost/net.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/vhost/net.c +5 -0
index 28d7786..50df58e6 100644
@@ -534,11 +534,16 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 	rcu_assign_pointer(vq->private_data, sock);
 	vhost_net_enable_vq(n, vq);
 done:
+	mutex_unlock(&vq->mutex);
+
 	if (oldsock) {
 		vhost_net_flush_vq(n, index);
 		fput(oldsock->file);
 	}
 
+	mutex_unlock(&n->dev.mutex);
+	return 0;
+
 err_vq:
 	mutex_unlock(&vq->mutex);
 err: