| Message ID | 20200116105842.271179-4-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio: enable blk and scsi multi-queue by default | expand |
On Thu, 16 Jan 2020 10:58:40 +0000 Stefan Hajnoczi <stefanha@redhat.com> wrote: > Automatically size the number of request virtqueues to match the number > of vCPUs. This ensures that completion interrupts are handled on the > same vCPU that submitted the request. No IPI is necessary to complete > an I/O request and performance is improved. > > Remember that virtqueue numbering assumptions are being removed from the > virtio-pci proxy object, so the Control and Event virtqueues are counted > by ->get_num_virtqueues() and we only add 1 for the Configuration Change > interrupt: > > if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { > - vpci_dev->nvectors = vs->conf.num_queues + 3; > + vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1; > } > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/core/machine.c | 3 +++ > hw/scsi/vhost-scsi.c | 2 ++ > hw/scsi/vhost-user-scsi.c | 2 ++ > hw/scsi/virtio-scsi.c | 18 ++++++++++++++++++ > hw/virtio/vhost-scsi-pci.c | 4 ++-- > hw/virtio/vhost-user-scsi-pci.c | 4 ++-- > hw/virtio/virtio-scsi-pci.c | 4 ++-- > include/hw/virtio/virtio-scsi.h | 1 + > 8 files changed, 32 insertions(+), 6 deletions(-) > > @@ -878,6 +879,18 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = { > .load_request = virtio_scsi_load_request, > }; > > +static uint32_t virtio_scsi_get_num_virtqueues(VirtIODevice *vdev) > +{ > + VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev); > + uint32_t request_queues = s->conf.num_queues; > + > + if (s->conf.num_queues == 1 && s->conf.auto_num_queues) { > + request_queues = current_machine->smp.cpus; > + } > + > + return VIRTIO_SCSI_VQ_NUM_FIXED + request_queues; I'm not sure doing this at the device level is the right thing to do. For now, only virtio-pci will call this function, and there basing the number of virtqueues off the number of cpus makes sense; but that's a property of the transport, not of the device. Consider e.g. a virtio-scsi-ccw device: If we wanted to introduce a way to automatically pick a good number of virtqueues there, this functions likely would not return a particularly useful value, as queue interrupt processing does not really relate to the number of cpus with adapter interrupts. It's not a problem right now, as only virtio-pci calls this, but someone looking at this callback is likely getting the impression that this is a generically useful function. > +} > + > void virtio_scsi_common_realize(DeviceState *dev, > VirtIOHandleOutput ctrl, > VirtIOHandleOutput evt,
On Thu, Jan 16, 2020 at 12:53:49PM +0100, Cornelia Huck wrote: > On Thu, 16 Jan 2020 10:58:40 +0000 > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > Automatically size the number of request virtqueues to match the number > > of vCPUs. This ensures that completion interrupts are handled on the > > same vCPU that submitted the request. No IPI is necessary to complete > > an I/O request and performance is improved. > > > > Remember that virtqueue numbering assumptions are being removed from the > > virtio-pci proxy object, so the Control and Event virtqueues are counted > > by ->get_num_virtqueues() and we only add 1 for the Configuration Change > > interrupt: > > > > if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { > > - vpci_dev->nvectors = vs->conf.num_queues + 3; > > + vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1; > > } > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > hw/core/machine.c | 3 +++ > > hw/scsi/vhost-scsi.c | 2 ++ > > hw/scsi/vhost-user-scsi.c | 2 ++ > > hw/scsi/virtio-scsi.c | 18 ++++++++++++++++++ > > hw/virtio/vhost-scsi-pci.c | 4 ++-- > > hw/virtio/vhost-user-scsi-pci.c | 4 ++-- > > hw/virtio/virtio-scsi-pci.c | 4 ++-- > > include/hw/virtio/virtio-scsi.h | 1 + > > 8 files changed, 32 insertions(+), 6 deletions(-) > > > > > @@ -878,6 +879,18 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = { > > .load_request = virtio_scsi_load_request, > > }; > > > > +static uint32_t virtio_scsi_get_num_virtqueues(VirtIODevice *vdev) > > +{ > > + VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev); > > + uint32_t request_queues = s->conf.num_queues; > > + > > + if (s->conf.num_queues == 1 && s->conf.auto_num_queues) { > > + request_queues = current_machine->smp.cpus; > > + } > > + > > + return VIRTIO_SCSI_VQ_NUM_FIXED + request_queues; > > I'm not sure doing this at the device level is the right thing to do. > For now, only virtio-pci will call this function, and there basing the > number of virtqueues off the number of cpus makes sense; but that's a > property of the transport, not of the device. > > Consider e.g. a virtio-scsi-ccw device: If we wanted to introduce a way > to automatically pick a good number of virtqueues there, this functions > likely would not return a particularly useful value, as queue interrupt > processing does not really relate to the number of cpus with adapter > interrupts. It's not a problem right now, as only virtio-pci calls > this, but someone looking at this callback is likely getting the > impression that this is a generically useful function. That's a great point. Maybe the direction should be reversed so that the device asks the transport to suggest the optimal number of queues when auto-num-queues is enabled. Stefan
On Thu, 16 Jan 2020 14:16:54 +0000 Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Thu, Jan 16, 2020 at 12:53:49PM +0100, Cornelia Huck wrote: > > On Thu, 16 Jan 2020 10:58:40 +0000 > > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > > Automatically size the number of request virtqueues to match the number > > > of vCPUs. This ensures that completion interrupts are handled on the > > > same vCPU that submitted the request. No IPI is necessary to complete > > > an I/O request and performance is improved. > > > > > > Remember that virtqueue numbering assumptions are being removed from the > > > virtio-pci proxy object, so the Control and Event virtqueues are counted > > > by ->get_num_virtqueues() and we only add 1 for the Configuration Change > > > interrupt: > > > > > > if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { > > > - vpci_dev->nvectors = vs->conf.num_queues + 3; > > > + vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1; > > > } > > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > --- > > > hw/core/machine.c | 3 +++ > > > hw/scsi/vhost-scsi.c | 2 ++ > > > hw/scsi/vhost-user-scsi.c | 2 ++ > > > hw/scsi/virtio-scsi.c | 18 ++++++++++++++++++ > > > hw/virtio/vhost-scsi-pci.c | 4 ++-- > > > hw/virtio/vhost-user-scsi-pci.c | 4 ++-- > > > hw/virtio/virtio-scsi-pci.c | 4 ++-- > > > include/hw/virtio/virtio-scsi.h | 1 + > > > 8 files changed, 32 insertions(+), 6 deletions(-) > > > > > > > > @@ -878,6 +879,18 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = { > > > .load_request = virtio_scsi_load_request, > > > }; > > > > > > +static uint32_t virtio_scsi_get_num_virtqueues(VirtIODevice *vdev) > > > +{ > > > + VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev); > > > + uint32_t request_queues = s->conf.num_queues; > > > + > > > + if (s->conf.num_queues == 1 && s->conf.auto_num_queues) { > > > + request_queues = current_machine->smp.cpus; > > > + } > > > + > > > + return VIRTIO_SCSI_VQ_NUM_FIXED + request_queues; > > > > I'm not sure doing this at the device level is the right thing to do. > > For now, only virtio-pci will call this function, and there basing the > > number of virtqueues off the number of cpus makes sense; but that's a > > property of the transport, not of the device. > > > > Consider e.g. a virtio-scsi-ccw device: If we wanted to introduce a way > > to automatically pick a good number of virtqueues there, this functions > > likely would not return a particularly useful value, as queue interrupt > > processing does not really relate to the number of cpus with adapter > > interrupts. It's not a problem right now, as only virtio-pci calls > > this, but someone looking at this callback is likely getting the > > impression that this is a generically useful function. > > That's a great point. Maybe the direction should be reversed so that > the device asks the transport to suggest the optimal number of queues > when auto-num-queues is enabled. Maybe the device also specifying that it needs at least m queues, and the transport can then return max(m, n) (with n being the number of cpus in the pci case). My next problem is that I'm not sure what 'optimal number of queues' would mean from a ccw viewpoint. "Remaining free bits in the indicator area" will be way too much in the general case :) Giving an upper limit is easy, but not a value that we'd want to autoconfigure, and I really don't want to return 42 all the time. Would it make sense to make this feature opt-in per transport?
diff --git a/hw/core/machine.c b/hw/core/machine.c index 3e288bfceb..41da19d85b 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -30,8 +30,11 @@ GlobalProperty hw_compat_4_2[] = { { "virtio-blk-device", "x-enable-wce-if-config-wce", "off" }, { "virtio-blk-device", "seg-max-adjust", "off"}, + { "virtio-scsi-device", "auto_num_queues", "off"}, { "virtio-scsi-device", "seg_max_adjust", "off"}, { "vhost-blk-device", "seg_max_adjust", "off"}, + { "vhost-scsi", "auto_num_queues", "off"}, + { "vhost-user-scsi", "auto_num_queues", "off"}, { "usb-host", "suppress-remote-wake", "off" }, { "usb-redir", "suppress-remote-wake", "off" }, }; diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index 26f710d3ec..1dfc269a29 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -273,6 +273,8 @@ static Property vhost_scsi_properties[] = { DEFINE_PROP_STRING("wwpn", VirtIOSCSICommon, conf.wwpn), DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0), DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1), + DEFINE_PROP_BOOL("auto_num_queues", VirtIOSCSICommon, + conf.auto_num_queues, true), DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size, 128), DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSICommon, conf.seg_max_adjust, diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c index eb37733bd0..92dbdf1042 100644 --- a/hw/scsi/vhost-user-scsi.c +++ b/hw/scsi/vhost-user-scsi.c @@ -164,6 +164,8 @@ static Property vhost_user_scsi_properties[] = { DEFINE_PROP_CHR("chardev", VirtIOSCSICommon, conf.chardev), DEFINE_PROP_UINT32("boot_tpgt", VirtIOSCSICommon, conf.boot_tpgt, 0), DEFINE_PROP_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues, 1), + DEFINE_PROP_BOOL("auto_num_queues", VirtIOSCSICommon, + conf.auto_num_queues, true), DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSICommon, conf.virtqueue_size, 128), DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors, diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 5622dd54e5..73c6030b22 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -16,6 +16,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "standard-headers/linux/virtio_ids.h" +#include "hw/boards.h" #include "hw/virtio/virtio-scsi.h" #include "migration/qemu-file-types.h" #include "qemu/error-report.h" @@ -878,6 +879,18 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = { .load_request = virtio_scsi_load_request, }; +static uint32_t virtio_scsi_get_num_virtqueues(VirtIODevice *vdev) +{ + VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev); + uint32_t request_queues = s->conf.num_queues; + + if (s->conf.num_queues == 1 && s->conf.auto_num_queues) { + request_queues = current_machine->smp.cpus; + } + + return VIRTIO_SCSI_VQ_NUM_FIXED + request_queues; +} + void virtio_scsi_common_realize(DeviceState *dev, VirtIOHandleOutput ctrl, VirtIOHandleOutput evt, @@ -905,6 +918,8 @@ void virtio_scsi_common_realize(DeviceState *dev, "must be > 2", s->conf.virtqueue_size); return; } + s->conf.num_queues = virtio_scsi_get_num_virtqueues(vdev) - + VIRTIO_SCSI_VQ_NUM_FIXED; s->cmd_vqs = g_new0(VirtQueue *, s->conf.num_queues); s->sense_size = VIRTIO_SCSI_SENSE_DEFAULT_SIZE; s->cdb_size = VIRTIO_SCSI_CDB_DEFAULT_SIZE; @@ -959,6 +974,8 @@ 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_BOOL("auto_num_queues", VirtIOSCSI, + parent_obj.conf.auto_num_queues, true), DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI, parent_obj.conf.virtqueue_size, 128), DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI, @@ -1011,6 +1028,7 @@ static void virtio_scsi_class_init(ObjectClass *klass, void *data) vdc->reset = virtio_scsi_reset; vdc->start_ioeventfd = virtio_scsi_dataplane_start; vdc->stop_ioeventfd = virtio_scsi_dataplane_stop; + vdc->get_num_virtqueues = virtio_scsi_get_num_virtqueues; hc->pre_plug = virtio_scsi_pre_hotplug; hc->plug = virtio_scsi_hotplug; hc->unplug = virtio_scsi_hotunplug; diff --git a/hw/virtio/vhost-scsi-pci.c b/hw/virtio/vhost-scsi-pci.c index e8dfbfc60f..94f64afde4 100644 --- a/hw/virtio/vhost-scsi-pci.c +++ b/hw/virtio/vhost-scsi-pci.c @@ -47,10 +47,10 @@ static void vhost_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) { VHostSCSIPCI *dev = VHOST_SCSI_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); - VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); + VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { - vpci_dev->nvectors = vs->conf.num_queues + 3; + vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1; } qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); diff --git a/hw/virtio/vhost-user-scsi-pci.c b/hw/virtio/vhost-user-scsi-pci.c index ff13af7030..6b518e6d72 100644 --- a/hw/virtio/vhost-user-scsi-pci.c +++ b/hw/virtio/vhost-user-scsi-pci.c @@ -53,10 +53,10 @@ static void vhost_user_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) { VHostUserSCSIPCI *dev = VHOST_USER_SCSI_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); - VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); + VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { - vpci_dev->nvectors = vs->conf.num_queues + 3; + vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1; } qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c index 3c55dc19a1..447344869d 100644 --- a/hw/virtio/virtio-scsi-pci.c +++ b/hw/virtio/virtio-scsi-pci.c @@ -46,12 +46,12 @@ static void virtio_scsi_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) { VirtIOSCSIPCI *dev = VIRTIO_SCSI_PCI(vpci_dev); DeviceState *vdev = DEVICE(&dev->vdev); - VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev); + VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); DeviceState *proxy = DEVICE(vpci_dev); char *bus_name; if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { - vpci_dev->nvectors = vs->conf.num_queues + 3; + vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1; } /* diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index 9f293bcb80..672e703bd2 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -50,6 +50,7 @@ typedef struct virtio_scsi_config VirtIOSCSIConfig; struct VirtIOSCSIConf { uint32_t num_queues; + bool auto_num_queues; uint32_t virtqueue_size; bool seg_max_adjust; uint32_t max_sectors;
Automatically size the number of request virtqueues to match the number of vCPUs. This ensures that completion interrupts are handled on the same vCPU that submitted the request. No IPI is necessary to complete an I/O request and performance is improved. Remember that virtqueue numbering assumptions are being removed from the virtio-pci proxy object, so the Control and Event virtqueues are counted by ->get_num_virtqueues() and we only add 1 for the Configuration Change interrupt: if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) { - vpci_dev->nvectors = vs->conf.num_queues + 3; + vpci_dev->nvectors = vdc->get_num_virtqueues(VIRTIO_DEVICE(vdev)) + 1; } Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/core/machine.c | 3 +++ hw/scsi/vhost-scsi.c | 2 ++ hw/scsi/vhost-user-scsi.c | 2 ++ hw/scsi/virtio-scsi.c | 18 ++++++++++++++++++ hw/virtio/vhost-scsi-pci.c | 4 ++-- hw/virtio/vhost-user-scsi-pci.c | 4 ++-- hw/virtio/virtio-scsi-pci.c | 4 ++-- include/hw/virtio/virtio-scsi.h | 1 + 8 files changed, 32 insertions(+), 6 deletions(-)