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

[Cornelia Huck]

On Thu, 16 Jan 2020 14:16:54 +0000
Stefan Hajnoczi <address@hidden> 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 <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.

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?

pgpmRk1hJONGb.pgp
Description: OpenPGP digital signature