From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>,
Pankaj Gupta <pankaj.gupta.linux@gmail.com>,
Eduardo Habkost <ehabkost@redhat.com>,
qemu-block@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>,
cohuck@redhat.com, qemu-devel@nongnu.org,
Max Reitz <mreitz@redhat.com>,
Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>,
Raphael Norwitz <raphael.norwitz@nutanix.com>
Subject: Re: [PATCH v4 3/5] virtio-scsi: default num_queues to -smp N
Date: Wed, 27 May 2020 11:38:17 +0100 [thread overview]
Message-ID: <20200527103817.GI2665520@redhat.com> (raw)
In-Reply-To: <20200527102925.128013-4-stefanha@redhat.com>
On Wed, May 27, 2020 at 11:29:23AM +0100, Stefan Hajnoczi wrote:
> Automatically size the number of virtio-scsi-pci, vhost-scsi-pci, and
> vhost-user-scsi-pci request virtqueues to match the number of vCPUs.
> Other transports continue to default to 1 request virtqueue.
IIRC this was raised on earlier versions of the series, but i don't
recall the outcome and no caveats are mentioned here...
Is this default still valid for very large $vCPUs. eg if I run QEMU
with "-smp 512" or even larger (I've seen people discussing 1000's of
CPUs), is this going to cause problems with the virtio-scsi default
queue counts ? Is there such a thing as "too large" for the num
of queues setting ? num vectors defaults to a value derived from
num queues, so is there concept of "too large" for num of vectors
setting ?
Ideally the commit message would answer these questions for future
reference. Same for the next patch to virtio-blk
>
> A 1:1 virtqueue:vCPU mapping 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.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> include/hw/virtio/virtio-scsi.h | 2 ++
> hw/core/machine.c | 6 +++++-
> hw/scsi/vhost-scsi.c | 3 ++-
> hw/scsi/vhost-user-scsi.c | 3 ++-
> hw/scsi/virtio-scsi.c | 6 +++++-
> hw/virtio/vhost-scsi-pci.c | 10 +++++++---
> hw/virtio/vhost-user-scsi-pci.c | 10 +++++++---
> hw/virtio/virtio-scsi-pci.c | 10 +++++++---
> 8 files changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index 9f293bcb80..c0b8e4dd7e 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -39,6 +39,8 @@
> /* Number of virtqueues that are always present */
> #define VIRTIO_SCSI_VQ_NUM_FIXED 2
>
> +#define VIRTIO_SCSI_AUTO_NUM_QUEUES UINT32_MAX
> +
> typedef struct virtio_scsi_cmd_req VirtIOSCSICmdReq;
> typedef struct virtio_scsi_cmd_resp VirtIOSCSICmdResp;
> typedef struct virtio_scsi_ctrl_tmf_req VirtIOSCSICtrlTMFReq;
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index bb3a7b18b1..df7664bc8d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -28,7 +28,11 @@
> #include "hw/mem/nvdimm.h"
> #include "migration/vmstate.h"
>
> -GlobalProperty hw_compat_5_0[] = {};
> +GlobalProperty hw_compat_5_0[] = {
> + { "virtio-scsi-device", "num_queues", "1"},
> + { "vhost-scsi", "num_queues", "1"},
> + { "vhost-user-scsi", "num_queues", "1"},
> +};
> const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
>
> GlobalProperty hw_compat_4_2[] = {
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index c1b012aea4..5e3bc319ab 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -272,7 +272,8 @@ static Property vhost_scsi_properties[] = {
> DEFINE_PROP_STRING("vhostfd", VirtIOSCSICommon, conf.vhostfd),
> 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_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues,
> + VIRTIO_SCSI_AUTO_NUM_QUEUES),
> 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 f8bd158c31..4b56de97a8 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -163,7 +163,8 @@ static void vhost_user_scsi_unrealize(DeviceState *dev)
> 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_UINT32("num_queues", VirtIOSCSICommon, conf.num_queues,
> + VIRTIO_SCSI_AUTO_NUM_QUEUES),
> 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 f3d60683bd..f055ae7389 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -891,6 +891,9 @@ void virtio_scsi_common_realize(DeviceState *dev,
> virtio_init(vdev, "virtio-scsi", VIRTIO_ID_SCSI,
> sizeof(VirtIOSCSIConfig));
>
> + if (s->conf.num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
> + s->conf.num_queues = 1;
> + }
> if (s->conf.num_queues == 0 ||
> s->conf.num_queues > VIRTIO_QUEUE_MAX - VIRTIO_SCSI_VQ_NUM_FIXED) {
> error_setg(errp, "Invalid number of queues (= %" PRIu32 "), "
> @@ -964,7 +967,8 @@ static void virtio_scsi_device_unrealize(DeviceState *dev)
> }
>
> static Property virtio_scsi_properties[] = {
> - DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues, 1),
> + DEFINE_PROP_UINT32("num_queues", VirtIOSCSI, parent_obj.conf.num_queues,
> + VIRTIO_SCSI_AUTO_NUM_QUEUES),
> DEFINE_PROP_UINT32("virtqueue_size", VirtIOSCSI,
> parent_obj.conf.virtqueue_size, 256),
> DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI,
> diff --git a/hw/virtio/vhost-scsi-pci.c b/hw/virtio/vhost-scsi-pci.c
> index 2b25db9a3c..9f377a84cf 100644
> --- a/hw/virtio/vhost-scsi-pci.c
> +++ b/hw/virtio/vhost-scsi-pci.c
> @@ -47,11 +47,15 @@ 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);
> + VirtIOSCSIConf *conf = &dev->vdev.parent_obj.parent_obj.conf;
> +
> + if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
> + conf->num_queues =
> + virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED);
> + }
>
> if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> - vpci_dev->nvectors = vs->conf.num_queues +
> - VIRTIO_SCSI_VQ_NUM_FIXED + 1;
> + vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 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 80710ab170..2a4c0d27f1 100644
> --- a/hw/virtio/vhost-user-scsi-pci.c
> +++ b/hw/virtio/vhost-user-scsi-pci.c
> @@ -53,11 +53,15 @@ 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);
> + VirtIOSCSIConf *conf = &dev->vdev.parent_obj.parent_obj.conf;
> +
> + if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
> + conf->num_queues =
> + virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED);
> + }
>
> if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> - vpci_dev->nvectors = vs->conf.num_queues +
> - VIRTIO_SCSI_VQ_NUM_FIXED + 1;
> + vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 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 c52d68053a..c3e2f8625a 100644
> --- a/hw/virtio/virtio-scsi-pci.c
> +++ b/hw/virtio/virtio-scsi-pci.c
> @@ -46,13 +46,17 @@ 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);
> DeviceState *proxy = DEVICE(vpci_dev);
> + VirtIOSCSIConf *conf = &dev->vdev.parent_obj.conf;
> char *bus_name;
>
> + if (conf->num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
> + conf->num_queues =
> + virtio_pci_optimal_num_queues(VIRTIO_SCSI_VQ_NUM_FIXED);
> + }
> +
> if (vpci_dev->nvectors == DEV_NVECTORS_UNSPECIFIED) {
> - vpci_dev->nvectors = vs->conf.num_queues +
> - VIRTIO_SCSI_VQ_NUM_FIXED + 1;
> + vpci_dev->nvectors = conf->num_queues + VIRTIO_SCSI_VQ_NUM_FIXED + 1;
> }
>
> /*
> --
> 2.25.4
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
next prev parent reply other threads:[~2020-05-27 10:39 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-27 10:29 [PATCH v4 0/5] virtio-pci: enable blk and scsi multi-queue by default Stefan Hajnoczi
2020-05-27 10:29 ` [PATCH v4 1/5] virtio-pci: add virtio_pci_optimal_num_queues() helper Stefan Hajnoczi
2020-05-28 15:35 ` Cornelia Huck
2020-06-09 15:37 ` Michael S. Tsirkin
2020-07-06 13:25 ` Stefan Hajnoczi
2020-07-06 14:14 ` Cornelia Huck
2020-05-27 10:29 ` [PATCH v4 2/5] virtio-scsi: introduce a constant for fixed virtqueues Stefan Hajnoczi
2020-05-28 14:18 ` Pankaj Gupta
2020-05-28 15:22 ` Philippe Mathieu-Daudé
2020-05-31 2:43 ` Raphael Norwitz
2020-05-27 10:29 ` [PATCH v4 3/5] virtio-scsi: default num_queues to -smp N Stefan Hajnoczi
2020-05-27 10:38 ` Daniel P. Berrangé [this message]
2020-05-28 8:50 ` Stefan Hajnoczi
2020-05-27 10:29 ` [PATCH v4 4/5] virtio-blk: " Stefan Hajnoczi
2020-05-28 14:45 ` Pankaj Gupta
2020-05-27 10:29 ` [PATCH v4 5/5] vhost-user-blk: " Stefan Hajnoczi
2020-05-31 2:42 ` Raphael Norwitz
2020-06-09 15:36 ` Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200527103817.GI2665520@redhat.com \
--to=berrange@redhat.com \
--cc=cohuck@redhat.com \
--cc=ehabkost@redhat.com \
--cc=fam@euphon.net \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=mst@redhat.com \
--cc=pankaj.gupta.linux@gmail.com \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=raphael.norwitz@nutanix.com \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.