Re: [Qemu-devel] [PATCH V2 03/11] virito: introduce bus specific queue limit

[Jason Wang <jasowang@redhat.com>]

On Thu, Feb 26, 2015 at 8:57 PM, Cornelia Huck 
<cornelia.huck@de.ibm.com> wrote:
> On Thu, 26 Feb 2015 15:04:38 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
>>  This patch introduces a bus specific queue limitation. It will be
>>  useful for increasing the limit for one of the bus without 
>> disturbing
>>  other buses.
>>  
>>  Cc: Anthony Liguori <aliguori@amazon.com>
>>  Cc: Michael S. Tsirkin <mst@redhat.com>
>>  Cc: Alexander Graf <agraf@suse.de>
>>  Cc: Richard Henderson <rth@twiddle.net>
>>  Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>>  Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>>  Cc: Paolo Bonzini <pbonzini@redhat.com>
>>  Signed-off-by: Jason Wang <jasowang@redhat.com>
>>  ---
>>   hw/net/virtio-net.c            |  4 ++--
>>   hw/s390x/s390-virtio-bus.c     |  1 +
>>   hw/s390x/virtio-ccw.c          |  1 +
>>   hw/scsi/virtio-scsi.c          |  4 ++--
>>   hw/virtio/virtio-mmio.c        |  1 +
>>   hw/virtio/virtio-pci.c         |  1 +
>>   hw/virtio/virtio.c             | 52 
>> +++++++++++++++++++++++++++++++-----------
>>   include/hw/virtio/virtio-bus.h |  1 +
>>   include/hw/virtio/virtio.h     |  1 +
>>   9 files changed, 49 insertions(+), 17 deletions(-)
>>  
> 
>>  diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>  index ffc22e8..5a806b5 100644
>>  --- a/hw/virtio/virtio.c
>>  +++ b/hw/virtio/virtio.c
>>  @@ -541,6 +541,14 @@ void virtio_update_irq(VirtIODevice *vdev)
>>       virtio_notify_vector(vdev, VIRTIO_NO_VECTOR);
>>   }
>>  
>>  +int virtio_get_queue_max(VirtIODevice *vdev)
>>  +{
>>  +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>>  +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>>  +
>>  +    return k->queue_max;
>>  +}
>>  +
> 
> Are all callers of this in the slow path? So we don't introduce
> processing overhead.

Looks not. For overhead, do you mean one introduced by 
VIRTIO_BUS_GET_CLASS()? Not sure how much it will affact but we've 
already used something like this in the datapath, e.g 
virtio_notify_vector(). 
> 
> 
>>   void virtio_set_status(VirtIODevice *vdev, uint8_t val)
>>   {
>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> 
> (...)
> 
> 
>>  @@ -777,27 +790,35 @@ void virtio_queue_notify(VirtIODevice *vdev, 
>> int n)
>>  
>>   uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
>>   {
>>  -    return n < VIRTIO_PCI_QUEUE_MAX ? vdev->vq[n].vector :
>>  +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>>  +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>>  +
>>  +    return n < k->queue_max ? vdev->vq[n].vector :
> 
> Any reason you're not using the newly introduced 
> virtio_get_queue_max()?

Yes, better use virtio_get_queue_max().
> 
> 
>>           VIRTIO_NO_VECTOR;
>>   }
> 
> Do we need to care about migration if the target supports a different
> number of queues?

See patch 9, if a target support a different number of queues, it can 
patch the k->queue_max during its initialization. 
> 
> 
> I'm also not sure whether it would be sufficient to allow transports 
> to
> limit queues to a lower number than the core supports. We'd basically
> only need to block off queue creation in the individual transports, I
> think.

Not sure I understand the issue correctly, with this series. There's 
not core limitation but only transport limitation.