Skip to contentSkip to conversation

vhost: locking/rcu cleanup

PATCH In review Michael S. Tsirkin 2 participants 3 messages Jul 29, 2010 – Aug 1, 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: locking/rcu cleanup

I saw WARN_ON(!list_empty(&dev->work_list)) trigger so our custom flush is not as airtight as need be.

This patch switches to a simple atomic counter + srcu instead of the custom locked queue + flush implementation.

This will slow down the setup ioctls, which should not matter - it's slow path anyway. We use the expedited flush to at least make sure it has a sane time bound.

Works fine for me. I got reports that with many guests, work lock is highly contended, and this patch should in theory fix this as well - but I haven't tested this yet.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/vhost/net.c   |   55 +++++--------------
 drivers/vhost/vhost.c |  140 ++++++++++++++++++++++---------------------------
 drivers/vhost/vhost.h |   47 +++++++++-------
 3 files changed, 103 insertions(+), 139 deletions(-)
drivers/vhost/net.c +14 -41
index f13e56b..ee69c51 100644
@@ -111,8 +111,9 @@ static void tx_poll_start(struct vhost_net *net, struct socket *sock)
 
 /* Expects to be always run from workqueue - which acts as
  * read-size critical section for our kind of RCU. */
-static void handle_tx(struct vhost_net *net)
+static void handle_tx(struct vhost_dev *dev)
 {
+	struct vhost_net *net = container_of(dev, struct vhost_net, dev);
 	struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
 	unsigned out, in, s;
 	int head;

@@ -127,7 +128,7 @@ static void handle_tx(struct vhost_net *net)
 	size_t len, total_len = 0;
 	int err, wmem;
 	size_t hdr_size;
-	struct socket *sock = rcu_dereference(vq->private_data);
+	struct socket *sock = vhost_vq_data(vq, &net->dev);
 	if (!sock)
 		return;
 

@@ -305,7 +306,7 @@ static void handle_rx_big(struct vhost_net *net)
 	size_t len, total_len = 0;
 	int err;
 	size_t hdr_size;
-	struct socket *sock = rcu_dereference(vq->private_data);
+	struct socket *sock = vhost_vq_data(vq, &net->dev);
 	if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
 		return;
 

@@ -416,7 +417,7 @@ static void handle_rx_mergeable(struct vhost_net *net)
 	int err, headcount;
 	size_t vhost_hlen, sock_hlen;
 	size_t vhost_len, sock_len;
-	struct socket *sock = rcu_dereference(vq->private_data);
+	struct socket *sock = vhost_vq_data(vq, &net->dev);
 	if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
 		return;
 

@@ -500,46 +501,15 @@ static void handle_rx_mergeable(struct vhost_net *net)
 	unuse_mm(net->dev.mm);
 }
 
-static void handle_rx(struct vhost_net *net)
+static void handle_rx(struct vhost_dev *dev)
 {
+	struct vhost_net *net = container_of(dev, struct vhost_net, dev);
 	if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF))
 		handle_rx_mergeable(net);
 	else
 		handle_rx_big(net);
 }
 
-static void handle_tx_kick(struct vhost_work *work)
-{
-	struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
-						  poll.work);
-	struct vhost_net *net = container_of(vq->dev, struct vhost_net, dev);
-
-	handle_tx(net);
-}
-
-static void handle_rx_kick(struct vhost_work *work)
-{
-	struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
-						  poll.work);
-	struct vhost_net *net = container_of(vq->dev, struct vhost_net, dev);
-
-	handle_rx(net);
-}
-
-static void handle_tx_net(struct vhost_work *work)
-{
-	struct vhost_net *net = container_of(work, struct vhost_net,
-					     poll[VHOST_NET_VQ_TX].work);
-	handle_tx(net);
-}
-
-static void handle_rx_net(struct vhost_work *work)
-{
-	struct vhost_net *net = container_of(work, struct vhost_net,
-					     poll[VHOST_NET_VQ_RX].work);
-	handle_rx(net);
-}
-
 static int vhost_net_open(struct inode *inode, struct file *f)
 {
 	struct vhost_net *n = kmalloc(sizeof *n, GFP_KERNEL);

@@ -550,16 +520,18 @@ static int vhost_net_open(struct inode *inode, struct file *f)
 		return -ENOMEM;
 
 	dev = &n->dev;
-	n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
-	n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;
+	vhost_work_set_fn(&n->vqs[VHOST_NET_VQ_TX].work, handle_tx);
+	vhost_work_set_fn(&n->vqs[VHOST_NET_VQ_RX].work, handle_rx);
 	r = vhost_dev_init(dev, n->vqs, VHOST_NET_VQ_MAX);
 	if (r < 0) {
 		kfree(n);
 		return r;
 	}
 
-	vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT, dev);
-	vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN, dev);
+	vhost_poll_init(n->poll + VHOST_NET_VQ_TX,
+			&n->vqs[VHOST_NET_VQ_TX].work, POLLOUT, dev);
+	vhost_poll_init(n->poll + VHOST_NET_VQ_RX,
+			&n->vqs[VHOST_NET_VQ_RX].work, POLLIN, dev);
 	n->tx_poll_state = VHOST_NET_POLL_DISABLED;
 
 	f->private_data = n;

@@ -640,6 +612,7 @@ static int vhost_net_release(struct inode *inode, struct file *f)
 	/* We do an extra flush before freeing memory,
 	 * since jobs can re-queue themselves. */
 	vhost_net_flush(n);
+	vhost_dev_free(&n->dev);
 	kfree(n);
 	return 0;
 }
drivers/vhost/vhost.c +62 -78

231 lines not on this page · raw message

drivers/vhost/vhost.h +27 -20

106 lines not on this page · raw message

Signature · 1 line
1.7.2.rc0.14.g41c1c
#1 lore raw

Hello,

Quoted 2 lines · Michael S. Tsirkin

I saw WARN_ON(!list_empty(&dev->work_list)) trigger so our custom flush is not as airtight as need be.

Could be but it's also possible that something has queued something after the last flush? Is the problem reproducible?

Quoted 10 lines

This patch switches to a simple atomic counter + srcu instead of the custom locked queue + flush implementation.

This will slow down the setup ioctls, which should not matter - it's slow path anyway. We use the expedited flush to at least make sure it has a sane time bound.

Works fine for me. I got reports that with many guests, work lock is highly contended, and this patch should in theory fix this as well - but I haven't tested this yet.

Hmmm... vhost_poll_flush() becomes synchronize_srcu_expedited(). Can you please explain how it works? synchronize_srcu_expedited() is an extremely heavy operation involving scheduling the cpu_stop task on all cpus. I'm not quite sure whether doing it from every flush is a good idea. Is flush supposed to be a very rare operation?

Having custom implementation is fine too but let's try to implement something generic if at all possible.

Thanks.

Signature · 1 line
tejun
author maintainer
#2 lore raw
Quoted 9 lines · Tejun Heo

Hello,

Quoted 2 lines · Michael S. Tsirkin

I saw WARN_ON(!list_empty(&dev->work_list)) trigger so our custom flush is not as airtight as need be.

Could be but it's also possible that something has queued something after the last flush?
Is the problem reproducible?

Well, We do requeue from the job itself. So need to be careful with what we do with indexes here. Bug seemed to happen all the time when qemu was killed under stress but now I can't reproduce anymore :( Will try again later.

Quoted 11 lines
Quoted 10 lines

This patch switches to a simple atomic counter + srcu instead of the custom locked queue + flush implementation.

This will slow down the setup ioctls, which should not matter - it's slow path anyway. We use the expedited flush to at least make sure it has a sane time bound.

Works fine for me. I got reports that with many guests, work lock is highly contended, and this patch should in theory fix this as well - but I haven't tested this yet.

Quoted 5 lines

Hmmm... vhost_poll_flush() becomes synchronize_srcu_expedited(). Can you please explain how it works? synchronize_srcu_expedited() is an extremely heavy operation involving scheduling the cpu_stop task on all cpus. I'm not quite sure whether doing it from every flush is a good idea. Is flush supposed to be a very rare operation?

It is rare - happens on guest reboot typically. I guess I will switch to regular synchronize_srcu.

Quoted 4 lines

Having custom implementation is fine too but let's try to implement something generic if at all possible.

Thanks.

Sure. It does seem that avoiding list lock would be pretty hard in generic code though.

Quoted 2 lines
Signature · 1 line
tejun