Virtualization - May 2010 - [PATCH RFC] virtio_blk: Use blk-iopoll for host->guest notify

This patch adds blk-iopoll interrupt mitigation to virtio-blk.  Instead
of processing completed requests inside the virtqueue interrupt handler,
a softirq is scheduled to process up to a maximum number of completed
requests in one go.

If the number of complete requests exceeds the maximum number, then another
softirq is scheduled to continue polling.  Otherwise the virtqueue interrupt is
enabled again and we return to interrupt-driven mode.

The patch sets the maximum number of completed requests (aka budget, aka
weight) to 4.  This is a low number but reflects the expensive context
switch between guest and host virtio-blk emulation.

The blk-iopoll infrastructure is enabled system-wide by default:

kernel.blk_iopoll = 1

It can be disabled to always use interrupt-driven mode (useful for comparison):

kernel.blk_iopoll = 0

Signed-off-by: Stefan Hajnoczi <stefanha at linux.vnet.ibm.com>
---
No performance figures yet.

 drivers/block/virtio_blk.c |   71 ++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 2138a7a..1523895 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -6,6 +6,7 @@
 #include <linux/virtio.h>
 #include <linux/virtio_blk.h>
 #include <linux/scatterlist.h>
+#include <linux/blk-iopoll.h>
 
 #define PART_BITS 4
 
@@ -26,6 +27,9 @@ struct virtio_blk
 
 	mempool_t *pool;
 
+	/* Host->guest notify mitigation */
+	struct blk_iopoll iopoll;
+
 	/* What host tells us, plus 2 for header & tailer. */
 	unsigned int sg_elems;
 
@@ -42,16 +46,18 @@ struct virtblk_req
 	u8 status;
 };
 
-static void blk_done(struct virtqueue *vq)
+/* Assumes vblk->lock held */
+static int __virtblk_end_requests(struct virtio_blk *vblk, int weight)
 {
-	struct virtio_blk *vblk = vq->vdev->priv;
 	struct virtblk_req *vbr;
 	unsigned int len;
-	unsigned long flags;
+	int error;
+	int work = 0;
 
-	spin_lock_irqsave(&vblk->lock, flags);
-	while ((vbr = vblk->vq->vq_ops->get_buf(vblk->vq, &len)) !=
NULL) {
-		int error;
+	while (!weight || work < weight) {
+		vbr = vblk->vq->vq_ops->get_buf(vblk->vq, &len);
+		if (!vbr)
+			break;
 
 		switch (vbr->status) {
 		case VIRTIO_BLK_S_OK:
@@ -74,10 +80,53 @@ static void blk_done(struct virtqueue *vq)
 		__blk_end_request_all(vbr->req, error);
 		list_del(&vbr->list);
 		mempool_free(vbr, vblk->pool);
+		work++;
 	}
+
 	/* In case queue is stopped waiting for more buffers. */
 	blk_start_queue(vblk->disk->queue);
+	return work;
+}
+
+static int virtblk_iopoll(struct blk_iopoll *iopoll, int weight)
+{
+	struct virtio_blk *vblk +		container_of(iopoll, struct virtio_blk, iopoll);
+	unsigned long flags;
+	int work;
+
+	spin_lock_irqsave(&vblk->lock, flags);
+
+	work = __virtblk_end_requests(vblk, weight);
+	if (work < weight) {
+		/* Keep polling if there are pending requests. */
+		if (vblk->vq->vq_ops->enable_cb(vblk->vq))
+			__blk_iopoll_complete(&vblk->iopoll);
+		else
+			vblk->vq->vq_ops->disable_cb(vblk->vq);
+	}
+
 	spin_unlock_irqrestore(&vblk->lock, flags);
+	return work;
+}
+
+static void blk_done(struct virtqueue *vq)
+{
+	struct virtio_blk *vblk = vq->vdev->priv;
+	unsigned long flags;
+
+	if (blk_iopoll_enabled) {
+		if (!blk_iopoll_sched_prep(&vblk->iopoll)) {
+			spin_lock_irqsave(&vblk->lock, flags);
+			vblk->vq->vq_ops->disable_cb(vblk->vq);
+			spin_unlock_irqrestore(&vblk->lock, flags);
+			blk_iopoll_sched(&vblk->iopoll);
+		}
+	} else {
+		spin_lock_irqsave(&vblk->lock, flags);
+		__virtblk_end_requests(vblk, 0);
+		spin_unlock_irqrestore(&vblk->lock, flags);
+	}
 }
 
 static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
@@ -289,11 +338,14 @@ static int __devinit virtblk_probe(struct virtio_device
*vdev)
 		goto out_free_vq;
 	}
 
+	blk_iopoll_init(&vblk->iopoll, 4 /* budget */, virtblk_iopoll);
+	blk_iopoll_enable(&vblk->iopoll);
+
 	/* FIXME: How many partitions?  How long is a piece of string? */
 	vblk->disk = alloc_disk(1 << PART_BITS);
 	if (!vblk->disk) {
 		err = -ENOMEM;
-		goto out_mempool;
+		goto out_iopoll;
 	}
 
 	q = vblk->disk->queue = blk_init_queue(do_virtblk_request,
&vblk->lock);
@@ -401,13 +453,13 @@ static int __devinit virtblk_probe(struct virtio_device
*vdev)
 	if (!err && opt_io_size)
 		blk_queue_io_opt(q, blk_size * opt_io_size);
 
-
 	add_disk(vblk->disk);
 	return 0;
 
 out_put_disk:
 	put_disk(vblk->disk);
-out_mempool:
+out_iopoll:
+	blk_iopoll_disable(&vblk->iopoll);
 	mempool_destroy(vblk->pool);
 out_free_vq:
 	vdev->config->del_vqs(vdev);
@@ -430,6 +482,7 @@ static void __devexit virtblk_remove(struct virtio_device
*vdev)
 	del_gendisk(vblk->disk);
 	blk_cleanup_queue(vblk->disk->queue);
 	put_disk(vblk->disk);
+	blk_iopoll_disable(&vblk->iopoll);
 	mempool_destroy(vblk->pool);
 	vdev->config->del_vqs(vdev);
 	kfree(vblk);
-- 
1.7.1

On Friday, May 14, 2010 03:47:37 pm Stefan Hajnoczi
wrote:
> This patch adds blk-iopoll interrupt mitigation to virtio-blk.  Instead
> of processing completed requests inside the virtqueue interrupt handler,
> a softirq is scheduled to process up to a maximum number of completed
> requests in one go.
> 
> If the number of complete requests exceeds the maximum number, then another
> softirq is scheduled to continue polling.  Otherwise the virtqueue
> interrupt is enabled again and we return to interrupt-driven mode.
> 
> The patch sets the maximum number of completed requests (aka budget, aka
> weight) to 4.  This is a low number but reflects the expensive context
> switch between guest and host virtio-blk emulation.
> 
> The blk-iopoll infrastructure is enabled system-wide by default:
> 
> kernel.blk_iopoll = 1
> 
> It can be disabled to always use interrupt-driven mode (useful for
> comparison):
> 
> kernel.blk_iopoll = 0

Any preliminary numbers? latency, throughput, cpu use? What about comparing 
different "weights"?

> 
> Signed-off-by: Stefan Hajnoczi <stefanha at linux.vnet.ibm.com>
> ---
> No performance figures yet.
> 
>  drivers/block/virtio_blk.c |   71
> ++++++++++++++++++++++++++++++++++++++----- 1 files changed, 62
> insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 2138a7a..1523895 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -6,6 +6,7 @@
>  #include <linux/virtio.h>
>  #include <linux/virtio_blk.h>
>  #include <linux/scatterlist.h>
> +#include <linux/blk-iopoll.h>
> 
>  #define PART_BITS 4
> 
> @@ -26,6 +27,9 @@ struct virtio_blk
> 
>  	mempool_t *pool;
> 
> +	/* Host->guest notify mitigation */
> +	struct blk_iopoll iopoll;
> +
>  	/* What host tells us, plus 2 for header & tailer. */
>  	unsigned int sg_elems;
> 
> @@ -42,16 +46,18 @@ struct virtblk_req
>  	u8 status;
>  };
> 
> -static void blk_done(struct virtqueue *vq)
> +/* Assumes vblk->lock held */
> +static int __virtblk_end_requests(struct virtio_blk *vblk, int weight)
>  {
> -	struct virtio_blk *vblk = vq->vdev->priv;
>  	struct virtblk_req *vbr;
>  	unsigned int len;
> -	unsigned long flags;
> +	int error;
> +	int work = 0;
> 
> -	spin_lock_irqsave(&vblk->lock, flags);
> -	while ((vbr = vblk->vq->vq_ops->get_buf(vblk->vq, &len))
!= NULL) {
> -		int error;
> +	while (!weight || work < weight) {
> +		vbr = vblk->vq->vq_ops->get_buf(vblk->vq, &len);
> +		if (!vbr)
> +			break;
> 
>  		switch (vbr->status) {
>  		case VIRTIO_BLK_S_OK:
> @@ -74,10 +80,53 @@ static void blk_done(struct virtqueue *vq)
>  		__blk_end_request_all(vbr->req, error);
>  		list_del(&vbr->list);
>  		mempool_free(vbr, vblk->pool);
> +		work++;
>  	}
> +
>  	/* In case queue is stopped waiting for more buffers. */
>  	blk_start_queue(vblk->disk->queue);
> +	return work;
> +}
> +
> +static int virtblk_iopoll(struct blk_iopoll *iopoll, int weight)
> +{
> +	struct virtio_blk *vblk > +		container_of(iopoll, struct virtio_blk,
iopoll);
> +	unsigned long flags;
> +	int work;
> +
> +	spin_lock_irqsave(&vblk->lock, flags);
> +
> +	work = __virtblk_end_requests(vblk, weight);
> +	if (work < weight) {
> +		/* Keep polling if there are pending requests. */
> +		if (vblk->vq->vq_ops->enable_cb(vblk->vq))
> +			__blk_iopoll_complete(&vblk->iopoll);
> +		else
> +			vblk->vq->vq_ops->disable_cb(vblk->vq);
> +	}
> +
>  	spin_unlock_irqrestore(&vblk->lock, flags);
> +	return work;
> +}
> +
> +static void blk_done(struct virtqueue *vq)
> +{
> +	struct virtio_blk *vblk = vq->vdev->priv;
> +	unsigned long flags;
> +
> +	if (blk_iopoll_enabled) {
> +		if (!blk_iopoll_sched_prep(&vblk->iopoll)) {
> +			spin_lock_irqsave(&vblk->lock, flags);
> +			vblk->vq->vq_ops->disable_cb(vblk->vq);
> +			spin_unlock_irqrestore(&vblk->lock, flags);
> +			blk_iopoll_sched(&vblk->iopoll);
> +		}
> +	} else {
> +		spin_lock_irqsave(&vblk->lock, flags);
> +		__virtblk_end_requests(vblk, 0);
> +		spin_unlock_irqrestore(&vblk->lock, flags);
> +	}
>  }
> 
>  static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
> @@ -289,11 +338,14 @@ static int __devinit virtblk_probe(struct
> virtio_device *vdev) goto out_free_vq;
>  	}
> 
> +	blk_iopoll_init(&vblk->iopoll, 4 /* budget */, virtblk_iopoll);
> +	blk_iopoll_enable(&vblk->iopoll);
> +
>  	/* FIXME: How many partitions?  How long is a piece of string? */
>  	vblk->disk = alloc_disk(1 << PART_BITS);
>  	if (!vblk->disk) {
>  		err = -ENOMEM;
> -		goto out_mempool;
> +		goto out_iopoll;
>  	}
> 
>  	q = vblk->disk->queue = blk_init_queue(do_virtblk_request,
&vblk->lock);
> @@ -401,13 +453,13 @@ static int __devinit virtblk_probe(struct
> virtio_device *vdev) if (!err && opt_io_size)
>  		blk_queue_io_opt(q, blk_size * opt_io_size);
> 
> -
>  	add_disk(vblk->disk);
>  	return 0;
> 
>  out_put_disk:
>  	put_disk(vblk->disk);
> -out_mempool:
> +out_iopoll:
> +	blk_iopoll_disable(&vblk->iopoll);
>  	mempool_destroy(vblk->pool);
>  out_free_vq:
>  	vdev->config->del_vqs(vdev);
> @@ -430,6 +482,7 @@ static void __devexit virtblk_remove(struct
> virtio_device *vdev) del_gendisk(vblk->disk);
>  	blk_cleanup_queue(vblk->disk->queue);
>  	put_disk(vblk->disk);
> +	blk_iopoll_disable(&vblk->iopoll);
>  	mempool_destroy(vblk->pool);
>  	vdev->config->del_vqs(vdev);
>  	kfree(vblk);

On Fri, May 14, 2010 at 05:30:56PM -0500, Brian Jackson
wrote:
> Any preliminary numbers? latency, throughput, cpu use? What about comparing
> different "weights"?
I am running benchmarks and will report results when they are in.

Stefan