[RFC PATCH 0/2] block: virtio-blk: support multi vq per virtio-blk

Hi,

This patches try to support multi virtual queues(multi-vq) in one
virtio-blk device, and maps each virtual queue(vq) to blk-mq's
hardware queue.

With this approach, both scalability and performance problems on
virtio-blk device get improved.

For verifying the improvement, I implements virtio-blk multi-vq over
qemu's dataplane feature, and both handling host notification
from each vq and processing host I/O are still kept in the per-device
iothread context, the changes are based on qemu v2.0.0 release, and
can be accessed from below tree:

	git://kernel.ubuntu.com/ming/qemu.git #v2.0.0-virtblk-dataplane-mq

For enabling the multi-vq feature, 'num_queues=N' need to be added into
'-device virtio-blk-pci ...' of qemu command line, and suggest to pass
'vectors=N+1' to keep one MSI irq vector per each vq, and the feature
depends on x-data-plane.

Fio(libaio, randread, iodepth=64, bs=4K, jobs=N) is run inside VM to
verify the improvement.

I just create a small quadcore VM and run fio inside the VM, and
num_queues of the virtio-blk device is set as 2, but looks the
improvement is still obvious.

1), about scalability
- without mutli-vq feature
	-- jobs=2, thoughput: 145K iops
	-- jobs=4, thoughput: 100K iops
- without mutli-vq feature
	-- jobs=2, thoughput: 186K iops
	-- jobs=4, thoughput: 199K iops

2), about thoughput
- without mutli-vq feature
	-- top thoughput: 145K iops
- with mutli-vq feature
    -- top thoughput: 199K iops

So even for one quadcore VM, if the virtqueue number is increased
from 1 to 2, both scalability and performance can get improved a
lot.

Thanks,
--
Ming Lei

Current virtio-blk spec only supports one virtual queue for transfering
data between VM and host, and inside VM all kinds of operations on
the virtual queue needs to hold one lock, so cause below problems:

	- no scalability
    - bad throughput

So this patch requests to introduce feature of VIRTIO_BLK_F_MQ
so that more than one virtual queues can be used to virtio-blk
device, then above problems can be solved or eased.

Signed-off-by: Ming Lei <redacted>
---
 include/uapi/linux/virtio_blk.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index 6d8e61c..c5a2751 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h

@@ -40,6 +40,7 @@
 #define VIRTIO_BLK_F_WCE	9	/* Writeback mode enabled after reset */
 #define VIRTIO_BLK_F_TOPOLOGY	10	/* Topology information is available */
 #define VIRTIO_BLK_F_CONFIG_WCE	11	/* Writeback mode available in config */
+#define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
 
 #ifndef __KERNEL__
 /* Old (deprecated) name for VIRTIO_BLK_F_WCE. */

@@ -77,6 +78,9 @@ struct virtio_blk_config {
 
 	/* writeback mode (if VIRTIO_BLK_F_CONFIG_WCE) */
 	__u8 wce;
+
+	/* number of vqs, only available when VIRTIO_BLK_F_MQ is set */
+	__u16 num_queues;
 } __attribute__((packed));
 
 /*

-- 
1.7.9.5

Firstly this patch supports more than one virtual queues for virtio-blk
device.

Secondly this patch maps the virtual queue to blk-mq's hardware queue.

With this approach, both scalability and performance problem can be improved.

Signed-off-by: Ming Lei <redacted>
---
 drivers/block/virtio_blk.c |   75 ++++++++++++++++++++++++++++++++------------
 1 file changed, 55 insertions(+), 20 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index f63d358..e0d077d 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c

@@ -16,6 +16,8 @@
 
 #define PART_BITS 4
 
+#define MAX_NUM_VQ 16
+
 static int major;
 static DEFINE_IDA(vd_index_ida);
 

@@ -24,8 +26,8 @@ static struct workqueue_struct *virtblk_wq;
 struct virtio_blk
 {
 	struct virtio_device *vdev;
-	struct virtqueue *vq;
-	spinlock_t vq_lock;
+	struct virtqueue *vq[MAX_NUM_VQ];
+	spinlock_t vq_lock[MAX_NUM_VQ];
 
 	/* The disk structure for the kernel. */
 	struct gendisk *disk;

@@ -47,6 +49,9 @@ struct virtio_blk
 
 	/* Ida index - used to track minor number allocations. */
 	int index;
+
+	/* num of vqs */
+	int num_vqs;
 };
 
 struct virtblk_req

@@ -133,14 +138,15 @@ static void virtblk_done(struct virtqueue *vq)
 {
 	struct virtio_blk *vblk = vq->vdev->priv;
 	bool req_done = false;
+	int qid = vq->index;
 	struct virtblk_req *vbr;
 	unsigned long flags;
 	unsigned int len;
 
-	spin_lock_irqsave(&vblk->vq_lock, flags);
+	spin_lock_irqsave(&vblk->vq_lock[qid], flags);
 	do {
 		virtqueue_disable_cb(vq);
-		while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
+		while ((vbr = virtqueue_get_buf(vblk->vq[qid], &len)) != NULL) {
 			blk_mq_complete_request(vbr->req);
 			req_done = true;
 		}

@@ -151,7 +157,7 @@ static void virtblk_done(struct virtqueue *vq)
 	/* In case queue is stopped waiting for more buffers. */
 	if (req_done)
 		blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
-	spin_unlock_irqrestore(&vblk->vq_lock, flags);
+	spin_unlock_irqrestore(&vblk->vq_lock[qid], flags);
 }
 
 static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)

@@ -160,6 +166,7 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
 	struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
 	unsigned long flags;
 	unsigned int num;
+	int qid = hctx->queue_num;
 	const bool last = (req->cmd_flags & REQ_END) != 0;
 	int err;
 	bool notify = false;

@@ -202,12 +209,12 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
 			vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
 	}
 
-	spin_lock_irqsave(&vblk->vq_lock, flags);
-	err = __virtblk_add_req(vblk->vq, vbr, vbr->sg, num);
+	spin_lock_irqsave(&vblk->vq_lock[qid], flags);
+	err = __virtblk_add_req(vblk->vq[qid], vbr, vbr->sg, num);
 	if (err) {
-		virtqueue_kick(vblk->vq);
+		virtqueue_kick(vblk->vq[qid]);
 		blk_mq_stop_hw_queue(hctx);
-		spin_unlock_irqrestore(&vblk->vq_lock, flags);
+		spin_unlock_irqrestore(&vblk->vq_lock[qid], flags);
 		/* Out of mem doesn't actually happen, since we fall back
 		 * to direct descriptors */
 		if (err == -ENOMEM || err == -ENOSPC)

@@ -215,12 +222,12 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx, struct request *req)
 		return BLK_MQ_RQ_QUEUE_ERROR;
 	}
 
-	if (last && virtqueue_kick_prepare(vblk->vq))
+	if (last && virtqueue_kick_prepare(vblk->vq[qid]))
 		notify = true;
-	spin_unlock_irqrestore(&vblk->vq_lock, flags);
+	spin_unlock_irqrestore(&vblk->vq_lock[qid], flags);
 
 	if (notify)
-		virtqueue_notify(vblk->vq);
+		virtqueue_notify(vblk->vq[qid]);
 	return BLK_MQ_RQ_QUEUE_OK;
 }
 

@@ -377,12 +384,40 @@ static void virtblk_config_changed(struct virtio_device *vdev)
 static int init_vq(struct virtio_blk *vblk)
 {
 	int err = 0;
+	int i;
+	vq_callback_t *callbacks[MAX_NUM_VQ];
+	const char *names[MAX_NUM_VQ];
+	unsigned short num_vqs;
+	struct virtio_device *vdev = vblk->vdev;
 
-	/* We expect one virtqueue, for output. */
-	vblk->vq = virtio_find_single_vq(vblk->vdev, virtblk_done, "requests");
-	if (IS_ERR(vblk->vq))
-		err = PTR_ERR(vblk->vq);
+	if (virtio_has_feature(vdev, VIRTIO_BLK_F_MQ))
+		err = virtio_cread_feature(vdev, VIRTIO_BLK_F_MQ,
+				   struct virtio_blk_config, num_queues,
+				   &num_vqs);
+	else
+		num_vqs = 1;
+
+	if (err)
+		goto out;
 
+	if (num_vqs > MAX_NUM_VQ)
+		num_vqs = MAX_NUM_VQ;
+
+	for (i = 0; i < num_vqs; i++) {
+		callbacks[i] = virtblk_done;
+		names[i] = "requests";
+	}
+
+	/* Discover virtqueues and write information to configuration.  */
+	err = vdev->config->find_vqs(vdev, num_vqs, vblk->vq,
+			callbacks, names);
+	if (err)
+		goto out;
+
+	for (i = 0; i < num_vqs; i++)
+		spin_lock_init(&vblk->vq_lock[i]);
+	vblk->num_vqs = num_vqs;
+out:
 	return err;
 }
 

@@ -551,7 +586,6 @@ static int virtblk_probe(struct virtio_device *vdev)
 	err = init_vq(vblk);
 	if (err)
 		goto out_free_vblk;
-	spin_lock_init(&vblk->vq_lock);
 
 	/* FIXME: How many partitions?  How long is a piece of string? */
 	vblk->disk = alloc_disk(1 << PART_BITS);

@@ -562,7 +596,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 
 	/* Default queue sizing is to fill the ring. */
 	if (!virtblk_queue_depth) {
-		virtblk_queue_depth = vblk->vq->num_free;
+		virtblk_queue_depth = vblk->vq[0]->num_free;
 		/* ... but without indirect descs, we use 2 descs per req */
 		if (!virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC))
 			virtblk_queue_depth /= 2;

@@ -570,7 +604,6 @@ static int virtblk_probe(struct virtio_device *vdev)
 
 	memset(&vblk->tag_set, 0, sizeof(vblk->tag_set));
 	vblk->tag_set.ops = &virtio_mq_ops;
-	vblk->tag_set.nr_hw_queues = 1;
 	vblk->tag_set.queue_depth = virtblk_queue_depth;
 	vblk->tag_set.numa_node = NUMA_NO_NODE;
 	vblk->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;

@@ -578,6 +611,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 		sizeof(struct virtblk_req) +
 		sizeof(struct scatterlist) * sg_elems;
 	vblk->tag_set.driver_data = vblk;
+	vblk->tag_set.nr_hw_queues = vblk->num_vqs;
 
 	err = blk_mq_alloc_tag_set(&vblk->tag_set);
 	if (err)

@@ -777,7 +811,8 @@ static const struct virtio_device_id id_table[] = {
 static unsigned int features[] = {
 	VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE, VIRTIO_BLK_F_SCSI,
-	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE
+	VIRTIO_BLK_F_WCE, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
+	VIRTIO_BLK_F_MQ,
 };
 
 static struct virtio_driver virtio_blk = {

-- 
1.7.9.5

On 06/13/2014 11:29 AM, Ming Lei wrote:

Hi,

This patches try to support multi virtual queues(multi-vq) in one
virtio-blk device, and maps each virtual queue(vq) to blk-mq's
hardware queue.

With this approach, both scalability and performance problems on
virtio-blk device get improved.

For verifying the improvement, I implements virtio-blk multi-vq over
qemu's dataplane feature, and both handling host notification
from each vq and processing host I/O are still kept in the per-device
iothread context, the changes are based on qemu v2.0.0 release, and
can be accessed from below tree:

	git://kernel.ubuntu.com/ming/qemu.git #v2.0.0-virtblk-dataplane-mq

For enabling the multi-vq feature, 'num_queues=N' need to be added into
'-device virtio-blk-pci ...' of qemu command line, and suggest to pass
'vectors=N+1' to keep one MSI irq vector per each vq, and the feature
depends on x-data-plane.

Fio(libaio, randread, iodepth=64, bs=4K, jobs=N) is run inside VM to
verify the improvement.

I just create a small quadcore VM and run fio inside the VM, and
num_queues of the virtio-blk device is set as 2, but looks the
improvement is still obvious.

1), about scalability
- without mutli-vq feature
	-- jobs=2, thoughput: 145K iops
	-- jobs=4, thoughput: 100K iops
- without mutli-vq feature
	-- jobs=2, thoughput: 186K iops
	-- jobs=4, thoughput: 199K iops

Awesome! I was hoping someone would do that, and make virtio-blk take
full advantage of blk-mq.

-- 
Jens Axboe

Ming Lei [off-list ref] writes:

+	if (virtio_has_feature(vdev, VIRTIO_BLK_F_MQ))
+		err = virtio_cread_feature(vdev, VIRTIO_BLK_F_MQ,
+				   struct virtio_blk_config, num_queues,
+				   &num_vqs);
+	else
+		num_vqs = 1;

This is redundant: virtio_cread_feature() checks the feature.

So, either:
        if (virtio_has_feature(vdev, VIRTIO_BLK_F_MQ))
                virtio_cread(vdev, struct virtio_blk_config, num_queues,
          		     &num_vqs);
        else
                num_vqs = 1;

Or:
	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_MQ,
			   struct virtio_blk_config, num_queues,
			   &num_vqs);
        if (err)
                num_vqs = 1;

Otherwise, the patch looks pretty straight-forward.

Cheers,
Rusty.

Ming Lei [off-list ref] writes:

Hmm, please pad this like so:

        __u8 unused;

        __u16 num_queues;

That avoids weird alignment.

Thanks,
Rusty.

On Sat, Jun 14, 2014 at 1:29 AM, Ming Lei [off-list ref] wrote:

It would be nice to allocate virtqueues dynamically instead of
hardcoding the limit.  virtio-scsi also allocates virtqueues
dynamically.

Stefan

On Tue, Jun 17, 2014 at 10:40 AM, Stefan Hajnoczi [off-list ref] wrote:

On Sat, Jun 14, 2014 at 1:29 AM, Ming Lei [off-list ref] wrote:

quoted

Firstly this patch supports more than one virtual queues for virtio-blk
device.

Secondly this patch maps the virtual queue to blk-mq's hardware queue.

With this approach, both scalability and performance problem can be improved.

Signed-off-by: Ming Lei <redacted>
---
 drivers/block/virtio_blk.c |   75 ++++++++++++++++++++++++++++++++------------
 1 file changed, 55 insertions(+), 20 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index f63d358..e0d077d 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c

@@ -16,6 +16,8 @@

 #define PART_BITS 4

+#define MAX_NUM_VQ 16

It would be nice to allocate virtqueues dynamically instead of
hardcoding the limit.  virtio-scsi also allocates virtqueues
dynamically.

virtio-scsi may have lots of LUN, but virtio-blk only has one disk
which needn't lots of hardware queues.

Also it doesn't matter since it isn't part of ABI.

If change on virtio_blk_config is agreed, both host side and
guest side can choose to support dynamic length or pre-defined
length freely.

Thanks,
--
Ming Lei

Il 17/06/2014 17:50, Ming Lei ha scritto:

quoted

quoted

It would be nice to allocate virtqueues dynamically instead of
hardcoding the limit.  virtio-scsi also allocates virtqueues
dynamically.

virtio-scsi may have lots of LUN, but virtio-blk only has one disk
which needn't lots of hardware queues.

If you want to do queue steering based on the guest VCPU number, the 
number of queues must be = to the number of VCPUs shouldn't it?

I tried using a divisor of the number of VCPUs, but couldn't get the 
block layer to deliver interrupts to the right VCPU.

Paolo

On Tue, Jun 17, 2014 at 11:53 PM, Paolo Bonzini [off-list ref] wrote:

Il 17/06/2014 17:50, Ming Lei ha scritto:

quoted

quoted

quoted

It would be nice to allocate virtqueues dynamically instead of
hardcoding the limit.  virtio-scsi also allocates virtqueues
dynamically.

virtio-scsi may have lots of LUN, but virtio-blk only has one disk
which needn't lots of hardware queues.

If you want to do queue steering based on the guest VCPU number, the number
of queues must be = to the number of VCPUs shouldn't it?

I tried using a divisor of the number of VCPUs, but couldn't get the block
layer to deliver interrupts to the right VCPU.

For blk-mq's hardware queue, that won't be necessary to equal to
VCPUs number, and irq affinity per hw queue can be simply set as
blk_mq_hw_ctx->cpumask.

Thanks,
--
Ming Lei

Il 17/06/2014 18:00, Ming Lei ha scritto:

quoted

quoted

If you want to do queue steering based on the guest VCPU number, the number
of queues must be = to the number of VCPUs shouldn't it?

I tried using a divisor of the number of VCPUs, but couldn't get the block
layer to deliver interrupts to the right VCPU.

For blk-mq's hardware queue, that won't be necessary to equal to
VCPUs number, and irq affinity per hw queue can be simply set as
blk_mq_hw_ctx->cpumask.

Yes, but on top of that you want to have each request processed exactly 
by the CPU that sent it.  Unless the cpumasks are singletons, most of 
the benefit went away in my virtio-scsi tests.  Perhaps blk-mq is smarter.

Can you try benchmarking a 16 VCPU guest with 8 and 16 queues?

Paolo

On Wed, Jun 18, 2014 at 12:34 AM, Paolo Bonzini [off-list ref] wrote:

Il 17/06/2014 18:00, Ming Lei ha scritto:

quoted

quoted

quoted

If you want to do queue steering based on the guest VCPU number, the
number
of queues must be = to the number of VCPUs shouldn't it?

I tried using a divisor of the number of VCPUs, but couldn't get the
block
layer to deliver interrupts to the right VCPU.

For blk-mq's hardware queue, that won't be necessary to equal to
VCPUs number, and irq affinity per hw queue can be simply set as
blk_mq_hw_ctx->cpumask.

Yes, but on top of that you want to have each request processed exactly by
the CPU that sent it.  Unless the cpumasks are singletons, most of the
benefit went away in my virtio-scsi tests.  Perhaps blk-mq is smarter.

Can you try benchmarking a 16 VCPU guest with 8 and 16 queues?

From VM side, it might be better to use one hardware queue per vCPU,

since in theory it can remove vq lock contention.

But from host side, there is still disadvantage with more queues, since
more queues means more notify times, in my virtio-blk test, even with
ioeventfd, one notification may take ~3us averagely on qemu-system-x86_64.

For virtio-blk, I don't think it is always better to take more queues, and
we need to leverage below things in host side:

- host storage top performance, generally it reaches that with more
than 1 jobs with libaio(suppose it is N, so basically we can use N
iothread per device in qemu to try to get top performance)

- iothreads' loading(if iothreads are at full loading, increasing
queues doesn't help at all)

In my test, I only use the current per-dev iothread(x-dataplane)
in qemu to handle 2 vqs' notification and precess all I/O from
the 2 vqs, and looks it can improve IOPS by ~30%.

For virtio-scsi, the current usage doesn't make full use of blk-mq's
advantage too because only one vq is active at the same time, so I
guess the multi vqs' benefit won't be very much and I'd like to post
patches to support that first, then provide test data with
more queues(8, 16).


Thanks,
--
Ming Lei

On 18/06/2014 06:04, Ming Lei wrote:

For virtio-blk, I don't think it is always better to take more queues, and
we need to leverage below things in host side:

- host storage top performance, generally it reaches that with more
than 1 jobs with libaio(suppose it is N, so basically we can use N
iothread per device in qemu to try to get top performance)

- iothreads' loading(if iothreads are at full loading, increasing
queues doesn't help at all)

In my test, I only use the current per-dev iothread(x-dataplane)
in qemu to handle 2 vqs' notification and precess all I/O from
the 2 vqs, and looks it can improve IOPS by ~30%.

For virtio-scsi, the current usage doesn't make full use of blk-mq's
advantage too because only one vq is active at the same time, so I
guess the multi vqs' benefit won't be very much and I'd like to post
patches to support that first, then provide test data with
more queues(8, 16).

Hi Ming Lei,

would you like to repost these patches now that MQ support is in the kernel?

Also, I changed my mind about moving linux-aio to AioContext.  I now
think it's a good idea, because it limits the number of io_getevents
syscalls. O:-)  So I would be happy to review your patches for that as well.

Paolo

Hi Paolo,

On Mon, Dec 14, 2015 at 6:31 PM, Paolo Bonzini [off-list ref] wrote:

On 18/06/2014 06:04, Ming Lei wrote:

quoted

For virtio-blk, I don't think it is always better to take more queues, and
we need to leverage below things in host side:

- host storage top performance, generally it reaches that with more
than 1 jobs with libaio(suppose it is N, so basically we can use N
iothread per device in qemu to try to get top performance)

- iothreads' loading(if iothreads are at full loading, increasing
queues doesn't help at all)

In my test, I only use the current per-dev iothread(x-dataplane)
in qemu to handle 2 vqs' notification and precess all I/O from
the 2 vqs, and looks it can improve IOPS by ~30%.

For virtio-scsi, the current usage doesn't make full use of blk-mq's
advantage too because only one vq is active at the same time, so I
guess the multi vqs' benefit won't be very much and I'd like to post
patches to support that first, then provide test data with
more queues(8, 16).

Hi Ming Lei,

would you like to repost these patches now that MQ support is in the kernel?

Also, I changed my mind about moving linux-aio to AioContext.  I now
think it's a good idea, because it limits the number of io_getevents
syscalls. O:-)  So I would be happy to review your patches for that as well.

OK, I try to figure out a new version, and it might take a while since it is
close to festival season, :-)

Thanks,