Increased memory usage with scsi-mq

I read your slides about scsi-mq and it seems like a significant benefit to large machines, but could the out of the box defaults be made more friendly for small memory machines?

Quoted 3 lines · Richard W.M. Jones

I read your slides about scsi-mq and it seems like a significant benefit to large machines, but could the out of the box defaults be made more friendly for small memory machines?

The default inumber of queues and queue depth and thus memory usage is set by the LLDD.

Try to reduce the can_queue value in virtio_scsi and/or make sure you use the single queue variant in your VM (which should be tunable in qemu).

For now can you apply this testing patch to the guest kernel?

drivers/scsi/virtio_scsi.c +2 -2

index 9be211d68b15..0cbe2c882e1c 100644
@@ -818,7 +818,7 @@ static struct scsi_host_template virtscsi_host_template_single = {
 	.eh_timed_out = virtscsi_eh_timed_out,
 	.slave_alloc = virtscsi_device_alloc,
 
-	.can_queue = 1024,
+	.can_queue = 64,
 	.dma_boundary = UINT_MAX,
 	.use_clustering = ENABLE_CLUSTERING,
 	.target_alloc = virtscsi_target_alloc,

@@ -839,7 +839,7 @@ static struct scsi_host_template virtscsi_host_template_multi = {
 	.eh_timed_out = virtscsi_eh_timed_out,
 	.slave_alloc = virtscsi_device_alloc,
 
-	.can_queue = 1024,
+	.can_queue = 64,
 	.dma_boundary = UINT_MAX,
 	.use_clustering = ENABLE_CLUSTERING,
 	.target_alloc = virtscsi_target_alloc,

Quoted 25 lines · Christoph Hellwig

For now can you apply this testing patch to the guest kernel?

drivers/scsi/virtio_scsi.c +2 -2

index 9be211d68b15..0cbe2c882e1c 100644
@@ -818,7 +818,7 @@ static struct scsi_host_template virtscsi_host_template_single = {
 	.eh_timed_out = virtscsi_eh_timed_out,
 	.slave_alloc = virtscsi_device_alloc,
 
-	.can_queue = 1024,
+	.can_queue = 64,
 	.dma_boundary = UINT_MAX,
 	.use_clustering = ENABLE_CLUSTERING,
 	.target_alloc = virtscsi_target_alloc,

@@ -839,7 +839,7 @@ static struct scsi_host_template virtscsi_host_template_multi = {
 	.eh_timed_out = virtscsi_eh_timed_out,
 	.slave_alloc = virtscsi_device_alloc,
 
-	.can_queue = 1024,
+	.can_queue = 64,
 	.dma_boundary = UINT_MAX,
 	.use_clustering = ENABLE_CLUSTERING,
 	.target_alloc = virtscsi_target_alloc,

drivers/scsi/virtio_scsi.c static int virtscsi_probe(struct virtio_device *vdev) · line 989

Quoted 8 lines

drivers/scsi/virtio_scsi.c +1 -1

@@ -983,7 +983,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
 	shost->max_id = num_targets;
 	shost->max_channel = 0;
 	shost->max_cmd_len = VIRTIO_SCSI_CDB_SIZE;
-	shost->nr_hw_queues = num_queues;
+	shost->nr_hw_queues = 1;
 
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 	if (virtio_has_feature(vdev, VIRTIO_SCSI_F_T10_PI)) {

Yes, that's an improvement, although it's still a little way off the density possible the old way:

With scsi-mq enabled: 175 disks
* With this patch: 319 disks *
With scsi-mq disabled: 1755 disks

Also only the first two hunks are necessary. The kernel behaves exactly the same way with or without the third hunk (ie. num_queues must already be 1).

Can I infer from this that qemu needs a way to specify the can_queue setting to the virtio-scsi driver in the guest kernel?

You could also add a module parameter to the driver, and set it to 64 on the kernel command line (there is an example in drivers/scsi/vmw_pvscsi.c of how to do it).

Also I noticed this code in virtio_scsi.c:

        cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1;
        shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);

but setting cmd_per_lun (as a qemu virtio-scsi-pci parameter) didn't seem to make any difference to memory usage.

can_queue should depend on the virtqueue size, which unfortunately can vary for each virtio-scsi device in theory. The virtqueue size is retrieved by drivers/virtio prior to calling vring_create_virtqueue, and in QEMU it is the second argument to virtio_add_queue.

Quoted 4 lines · Paolo Bonzini

can_queue should depend on the virtqueue size, which unfortunately can vary for each virtio-scsi device in theory. The virtqueue size is retrieved by drivers/virtio prior to calling vring_create_virtqueue, and in QEMU it is the second argument to virtio_add_queue.

Why is that unfortunate? We don't even have to set can_queue in the host template, we can dynamically set it on per-host basis.

Quoted 6 lines · Christoph Hellwig

Quoted 4 lines · Paolo Bonzini

can_queue should depend on the virtqueue size, which unfortunately can vary for each virtio-scsi device in theory. The virtqueue size is retrieved by drivers/virtio prior to calling vring_create_virtqueue, and in QEMU it is the second argument to virtio_add_queue.

Quoted 2 lines

Why is that unfortunate? We don't even have to set can_queue in the host template, we can dynamically set it on per-host basis.

Ah, cool, I thought allocations based on can_queue happened already in scsi_host_alloc, but they happen at scsi_add_host time.

From my point of view, this is a good result, but you should be warned

From e923e49846189b2f55f3f02b70a290d4be237ed5 Mon Sep 17 00:00:00 2001

Quoted 7 lines · Paolo Bonzini

Quoted 6 lines · Christoph Hellwig

Quoted 4 lines · Paolo Bonzini

can_queue should depend on the virtqueue size, which unfortunately can vary for each virtio-scsi device in theory. The virtqueue size is retrieved by drivers/virtio prior to calling vring_create_virtqueue, and in QEMU it is the second argument to virtio_add_queue.

Quoted 5 lines

Quoted 2 lines

Why is that unfortunate? We don't even have to set can_queue in the host template, we can dynamically set it on per-host basis.

Ah, cool, I thought allocations based on can_queue happened already in scsi_host_alloc, but they happen at scsi_add_host time.

I think I've decoded all that information into the patch below.

I tested it, and it appears to work: when I set cmd_per_lun on the qemu command line, I see that the guest can add more disks:

  With scsi-mq enabled:   175 disks
  cmd_per_lun not set:    177 disks  *
  cmd_per_lun=16:         776 disks  *

cmd_per_lun=4: 1160 disks *
With scsi-mq disabled: 1755 disks
* = new result

From my point of view, this is a good result, but you should be warned that I don't fully understand what's going on here and I may have made obvious or not-so-obvious mistakes.

I tested the performance impact and it's not noticable in the libguestfs case even with very small cmd_per_lun settings, but libguestfs is largely serial and so this result won't be applicable to guests in general.

Also, should the guest kernel validate cmd_per_lun to make sure it's not too small or large? And if so, what would the limits be?

Rich.

From e923e49846189b2f55f3f02b70a290d4be237ed5 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <[email protected]>
Date: Thu, 10 Aug 2017 12:21:47 +0100
Subject: [PATCH] scsi: virtio_scsi: Set can_queue based on cmd_per_lun passed
by hypervisor.

Signed-off-by Richard W.M. Jones <[email protected]>

---
 drivers/scsi/virtio_scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/scsi/virtio_scsi.c +1 -1

index 9be211d68b15..b22591e9b16b 100644
@@ -973,7 +973,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
 		goto virtscsi_init_failed;
 
 	cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1;
-	shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
+	shost->cmd_per_lun = shost->can_queue = cmd_per_lun;
 	shost->max_sectors = virtscsi_config_get(vdev, max_sectors) ?: 0xFFFF;
 
 	/* LUNs > 256 are reported with format 1, so they go in the range

can_queue and cmd_per_lun are different. can_queue should be set to the value of vq->vring.num where vq is the command virtqueue (the first one is okay if there's >1).

If you want to change it, you'll have to do so in QEMU.

Quoted 5 lines · Paolo Bonzini

can_queue and cmd_per_lun are different. can_queue should be set to the value of vq->vring.num where vq is the command virtqueue (the first one is okay if there's >1).

If you want to change it, you'll have to do so in QEMU.

Here are a couple more patches I came up with, the first to Linux, the second to qemu.

There are a few problems ...

(1) Setting virtqueue_size to < 128 causes a BUG_ON failure in virtio_ring.c:virtqueue_add at:

    BUG_ON(total_sg > vq->vring.num);

(2) Can/should the ctrl, event and cmd queue sizes be set to the same values? Or should ctrl & event be left at 128?

(3) It seems like it might be a problem that virtqueue_size is not passed through the virtio_scsi_conf struct (which is ABI between the kernel and qemu AFAICT and so not easily modifiable). However I think this is not a problem because virtqueue size is stored in the Virtqueue Descriptor table, which is how the kernel gets the value. Is that right?

Rich.

--- kernel patch ---

drivers/scsi/virtio_scsi.c +2 -1

index 9be211d68b15..d6b4ff634c0d 100644
@@ -972,6 +972,8 @@ static int virtscsi_probe(struct virtio_device *vdev)
 	if (err)
 		goto virtscsi_init_failed;
 
+	shost->can_queue = virtqueue_get_vring_size(vscsi->req_vqs[0].vq);
+
 	cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1;
 	shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
 	shost->max_sectors = virtscsi_config_get(vdev, max_sectors) ?: 0xFFFF;
 
 
--- qemu patch ---

hw/scsi/virtio-scsi.c +4 -3

index eb639442d1..aadd99aad1 100644
@@ -867,10 +867,10 @@ void virtio_scsi_common_realize(DeviceState *dev,
     s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
     s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
 
-    s->ctrl_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, ctrl);
-    s->event_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, evt);
+    s->ctrl_vq = virtio_add_queue(vdev, s->conf.virtqueue_size, ctrl);
+    s->event_vq = virtio_add_queue(vdev, s->conf.virtqueue_size, evt);
     for (i = 0; i < s->conf.num_queues; i++) {
-        s->cmd_vqs[i] = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, cmd);
+        s->cmd_vqs[i] = virtio_add_queue(vdev, s->conf.virtqueue_size, cmd);
     }
 }
 

@@ -917,6 +917,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp)
 
 static Property virtio_scsi_properties[] = {
     DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1),
+    DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, parent_obj.conf.virtqueue_size, 128),
     DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors,
                                                   0xFFFF),
     DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSI, parent_obj.conf.cmd_per_lun,

include/hw/virtio/virtio-scsi.h +1 -0

index de6ae5a9f6..e30a92d3e7 100644
@@ -48,6 +48,7 @@ typedef struct virtio_scsi_config VirtIOSCSIConfig;
 
 struct VirtIOSCSIConf {
     uint32_t num_queues;
+    uint32_t virtqueue_size;
     uint32_t max_sectors;
     uint32_t cmd_per_lun;
 #ifdef CONFIG_VHOST_SCSI

OK this is looking a bit better now.

  With scsi-mq enabled:   175 disks
  virtqueue_size=64:      318 disks *
  virtqueue_size=16:      775 disks *

With scsi-mq disabled: 1755 disks
* = new results

I also ran the whole libguestfs test suite with virtqueue_size=16 (with no failures shown). As this tests many different disk I/O operations, it gives me some confidence that things generally work.

Do you have any other comments about the patches? I'm not sure I know enough to write an intelligent commit message for the kernel patch.

Rich.

--- kernel patch ---

drivers/scsi/virtio_scsi.c +2 -0

index 9be211d68b15..d6b4ff634c0d 100644
@@ -972,6 +972,8 @@ static int virtscsi_probe(struct virtio_device *vdev)
 	if (err)
 		goto virtscsi_init_failed;
 
+	shost->can_queue = virtqueue_get_vring_size(vscsi->req_vqs[0].vq);
+
 	cmd_per_lun = virtscsi_config_get(vdev, cmd_per_lun) ?: 1;
 	shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
 	shost->max_sectors = virtscsi_config_get(vdev, max_sectors) ?: 0xFFFF;

drivers/virtio/virtio_ring.c +3 -3

index 5e1b548828e6..2d7509da9f39 100644
@@ -296,7 +296,6 @@ static inline int virtqueue_add(struct virtqueue *_vq,
 	}
 #endif
 
-	BUG_ON(total_sg > vq->vring.num);
 	BUG_ON(total_sg == 0);
 
 	head = vq->free_head;

@@ -305,8 +304,10 @@ static inline int virtqueue_add(struct virtqueue *_vq,
 	 * buffers, then go indirect. FIXME: tune this threshold */
 	if (vq->indirect && total_sg > 1 && vq->vq.num_free)
 		desc = alloc_indirect(_vq, total_sg, gfp);
-	else
+	else {
 		desc = NULL;
+		WARN_ON_ONCE(total_sg > vq->vring.num && !vq->indirect);
+        }
 
 	if (desc) {
 		/* Use a single buffer which doesn't continue */
 
--- qemu patch ---

hw/scsi/virtio-scsi.c +4 -3

index eb639442d1..aadd99aad1 100644
@@ -867,10 +867,10 @@ void virtio_scsi_common_realize(DeviceState *dev,
     s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE;
     s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE;
 
-    s->ctrl_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, ctrl);
-    s->event_vq = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, evt);
+    s->ctrl_vq = virtio_add_queue(vdev, s->conf.virtqueue_size, ctrl);
+    s->event_vq = virtio_add_queue(vdev, s->conf.virtqueue_size, evt);
     for (i = 0; i < s->conf.num_queues; i++) {
-        s->cmd_vqs[i] = virtio_add_queue(vdev, VIRTIO_SCSI_VQ_SIZE, cmd);
+        s->cmd_vqs[i] = virtio_add_queue(vdev, s->conf.virtqueue_size, cmd);
     }
 }
 

@@ -917,6 +917,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp)
 
 static Property virtio_scsi_properties[] = {
     DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1),
+    DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, parent_obj.conf.virtqueue_size, 128),
     DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors,
                                                   0xFFFF),
     DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSI, parent_obj.conf.cmd_per_lun,

include/hw/virtio/virtio-scsi.h +1 -0

index de6ae5a9f6..e30a92d3e7 100644
@@ -48,6 +48,7 @@ typedef struct virtio_scsi_config VirtIOSCSIConfig;
 
 struct VirtIOSCSIConf {
     uint32_t num_queues;
+    uint32_t virtqueue_size;
     uint32_t max_sectors;
     uint32_t cmd_per_lun;
 #ifdef CONFIG_VHOST_SCSI