Virtualization - Sep 2011 - [RFC v2 0/2] virtio: Support releasing lock during kick

This patch allows virtio-blk to release its block queue lock while kicking the
host.  This improves scalability on SMP guests who would otherwise spin on the
lock while another vCPU is kicking the host.

This approach can be used for other virtio devices too.  It simply splits the
virtqueue_kick() operation into a prepare step which requires that the lock be
held and the actual notify step which may be performed without the lock.
Existing virtio drivers may continue to use the virtqueue_kick() interface
which now does the prepare and notify steps internally.

I am sending this out as RFC because further performance benchmarking is
required.  Although we have seen good results in the past, gathering number on
a wider range of machines and verifying that there is no regression would be
helpful.

Stefan Hajnoczi (2):
  virtio_ring: split virtqueue_kick prepare/notify
  virtio_blk: unlock vblk->lock during kick

 drivers/block/virtio_blk.c   |   10 ++++++++--
 drivers/virtio/virtio_ring.c |   28 +++++++++++++++++++++-------
 include/linux/virtio.h       |   13 +++++++++++++
 3 files changed, 42 insertions(+), 9 deletions(-)

-- 
1.7.5.4

The virtqueue_kick() operation really has two phases and should be split
so that devices hold locks only when necessary.  This patch splits
virtqueue_kick() into two virtqueue_kick_prepare() and
virtqueue_kick_notify().

virtqueue_kick_prepare() updates the vring and checks whether the host
needs to be notified.  This function must be executed with a lock held
to protect the vring.

virtqueue_kick_notify() notifies the host that the vring has new
buffers.  This function may be executed without holding a lock
protecting the vring.

Signed-off-by: Stefan Hajnoczi <stefanha at linux.vnet.ibm.com>
---
 drivers/virtio/virtio_ring.c |   28 +++++++++++++++++++++-------
 include/linux/virtio.h       |   13 +++++++++++++
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 68b9136..7d059f7 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -237,10 +237,12 @@ add_head:
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_buf_gfp);
 
-void virtqueue_kick(struct virtqueue *_vq)
+bool virtqueue_kick_prepare(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 	u16 new, old;
+	bool r;
+
 	START_USE(vq);
 	/* Descriptors and available array need to be set before we expose the
 	 * new available array entries. */
@@ -253,13 +255,25 @@ void virtqueue_kick(struct virtqueue *_vq)
 	/* Need to update avail index before checking if we should notify */
 	virtio_mb();
 
-	if (vq->event ?
-	    vring_need_event(vring_avail_event(&vq->vring), new, old) :
-	    !(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY))
-		/* Prod other side to tell it about changes. */
-		vq->notify(&vq->vq);
-
+	if (vq->event)
+		r = vring_need_event(vring_avail_event(&vq->vring), new, old);
+	else
+		r = !(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
 	END_USE(vq);
+	return r;
+}
+EXPORT_SYMBOL_GPL(virtqueue_kick_prepare);
+
+void virtqueue_kick_notify(struct virtqueue *_vq)
+{
+	to_vvq(_vq)->notify(_vq);
+}
+EXPORT_SYMBOL_GPL(virtqueue_kick_notify);
+
+void virtqueue_kick(struct virtqueue *_vq)
+{
+	if (virtqueue_kick_prepare(_vq))
+		virtqueue_kick_notify(_vq);
 }
 EXPORT_SYMBOL_GPL(virtqueue_kick);
 
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 7108857..65536cb 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -35,9 +35,18 @@ struct virtqueue {
  *	data: the token identifying the buffer.
  *	gfp: how to do memory allocations (if necessary).
  *      Returns remaining capacity of queue (sg segments) or a negative error.
+ * virtqueue_kick_prepare: update after add_buf and check if kick necessary
+ *	vq: the struct virtqueue
+ *	After one or more add_buf calls, invoke this to check whether kick is
+ *	necessary.
+ * virtqueue_kick_notify: kick the other side
+ *	vq: the struct virtqueue
+ *	Normally virtqueue_kick_prepare is called before this.  Can be done in
+ *	parallel with add_buf/get_buf.
  * virtqueue_kick: update after add_buf
  *	vq: the struct virtqueue
  *	After one or more add_buf calls, invoke this to kick the other side.
+ *	Uses virtqueue_kick_prepare and virtqueue_kick_notify internally.
  * virtqueue_get_buf: get the next used buffer
  *	vq: the struct virtqueue we're talking about.
  *	len: the length written into the buffer
@@ -85,6 +94,10 @@ static inline int virtqueue_add_buf(struct virtqueue *vq,
 	return virtqueue_add_buf_gfp(vq, sg, out_num, in_num, data, GFP_ATOMIC);
 }
 
+bool virtqueue_kick_prepare(struct virtqueue *vq);
+
+void virtqueue_kick_notify(struct virtqueue *vq);
+
 void virtqueue_kick(struct virtqueue *vq);
 
 void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len);
-- 
1.7.5.4

Holding the vblk->lock across kick causes poor scalability in SMP
guests.  If one CPU is doing virtqueue kick and another CPU touches the
vblk->lock it will have to spin until virtqueue kick completes.

This typically results in high system CPU utilization in SMP guests that
are running multithreaded I/O-bound workloads.

Signed-off-by: Stefan Hajnoczi <stefanha at linux.vnet.ibm.com>
---
 drivers/block/virtio_blk.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..c2bf0a9 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -193,8 +193,14 @@ static void do_virtblk_request(struct request_queue *q)
 		issued++;
 	}
 
-	if (issued)
-		virtqueue_kick(vblk->vq);
+	if (!issued)
+		return;
+
+	if (virtqueue_kick_prepare(vblk->vq)) {
+		spin_unlock(&vblk->lock);
+		virtqueue_kick_notify(vblk->vq);
+		spin_lock(&vblk->lock);
+	}
 }
 
 /* return id (s/n) string for *disk to *id_str
-- 
1.7.5.4

On Wed, Sep 07, 2011 at 02:51:39PM +0100, Stefan Hajnoczi
wrote:
> This patch allows virtio-blk to release its block queue lock while kicking
the
> host.  This improves scalability on SMP guests who would otherwise spin on
the
> lock while another vCPU is kicking the host.
> 
> This approach can be used for other virtio devices too.  It simply splits
the
> virtqueue_kick() operation into a prepare step which requires that the lock
be
> held and the actual notify step which may be performed without the lock.
> Existing virtio drivers may continue to use the virtqueue_kick() interface
> which now does the prepare and notify steps internally.
> 
> I am sending this out as RFC because further performance benchmarking is
> required.  Although we have seen good results in the past, gathering number
on
> a wider range of machines and verifying that there is no regression would
be
> helpful.
Makes sense to me.
Acked-by: Michael S. Tsirkin <mst at redhat.com>
> Stefan Hajnoczi (2):
>   virtio_ring: split virtqueue_kick prepare/notify
>   virtio_blk: unlock vblk->lock during kick
> 
>  drivers/block/virtio_blk.c   |   10 ++++++++--
>  drivers/virtio/virtio_ring.c |   28 +++++++++++++++++++++-------
>  include/linux/virtio.h       |   13 +++++++++++++
>  3 files changed, 42 insertions(+), 9 deletions(-)
> 
> -- 
> 1.7.5.4