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

[Stefan Hajnoczi]

On Tue, Jul 07, 2020 at 05:44:53PM +0200, Cornelia Huck wrote:
> On Mon,  6 Jul 2020 14:56:48 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> Maybe mention 'pci' in the subject as well?

I think this patch does too many things. I'll split up the generic and
PCI parts so that the commit message is more accurate.

> > 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.
> > 
> > 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.  The
> > maximum number of MSI-X vectors and virtqueues limit are respected.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  include/hw/virtio/virtio-scsi.h |  2 ++
> >  hw/core/machine.c               |  3 +++
> >  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, 35 insertions(+), 12 deletions(-)
> 
> (...)
> 
> > diff --git a/hw/virtio/virtio-scsi-pci.c b/hw/virtio/virtio-scsi-pci.c
> > index 3ff9eb7ef6..fa4b3bfb50 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;
> >      }
> >  
> >      /*
> 
> One corner case where the setup may end up being a bit odd is a
> situation where nvectors was specified, but num_queues was not, and the
> device suddenly ends up with more queues than vectors. But I don't see
> a reason why you would want to specify nvectors but not num_queues in a
> real word scenario, so I think we can ignore that corner case.

I agree, I've ignored that case. Other options include printing a
warning or even an error when num_queues disagrees with nvectors.

> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

signature.asc
Description: PGP signature