Re: [PATCH 3/5] virtio-scsi: default num_queues to -smp N

[Cornelia Huck]

On Thu, 16 Jan 2020 10:58:40 +0000
Stefan Hajnoczi <address@hidden> 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 <address@hidden>
> ---
>  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,