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

[Stefan Hajnoczi]

On Thu, Jan 16, 2020 at 12:53:49PM +0100, Cornelia Huck wrote:
> 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.

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

signature.asc
Description: PGP signature