Re: [Qemu-devel] [PATCH] vhost: don't set vring call if no vector

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Mon,  1 Aug 2016 16:07:58 +0800
Jason Wang <jasowang@redhat.com> wrote:

> We used to set vring call fd unconditionally even if guest driver does
> not use MSIX for this vritqueue at all. This will cause lots of
> unnecessary userspace access and other checks for drivers does not use
> interrupt at all (e.g virtio-net pmd). So check and clean vring call
> fd if guest does not use any vector for this virtqueue at
> all.

So the basic idea is: don't setup signalling via eventfd if the guest
did not enable interrupts for this queue, right?

> 
> Perf diffs (on rx) shows lots of cpus wasted on vhost_signal() were saved:
> 
> #
>     28.12%  -27.82%  [vhost]           [k] vhost_signal
>     14.44%   -1.69%  [kernel.vmlinux]  [k] copy_user_generic_string
>      7.05%   +1.53%  [kernel.vmlinux]  [k] __free_page_frag
>      6.51%   +5.53%  [vhost]           [k] vhost_get_vq_desc
> ...
> 
> Pktgen tests shows 15.8% improvement on rx pps and 6.5% on tx pps.
> 
> Before: RX 2.08Mpps TX 1.35Mpps
> After:  RX 2.41Mpps TX 1.44Mpps
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/vhost.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 3d0c807..bd051ab 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -822,6 +822,9 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
>                                  struct vhost_virtqueue *vq,
>                                  unsigned idx)
>  {
> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> +    VirtioBusState *vbus = VIRTIO_BUS(qbus);
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>      hwaddr s, l, a;
>      int r;
>      int vhost_vq_index = dev->vhost_ops->vhost_get_vq_index(dev, idx);
> @@ -912,8 +915,19 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
>          vhost_virtqueue_mask(dev, vdev, idx, false);
>      }
> 
> +    if (k->query_guest_notifiers &&
> +        k->query_guest_notifiers(qbus->parent) &&
> +        virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR) {

I'm trying to imagine what this means for virtio-ccw. Keep in mind that
we don't have the concept of setting a 'vector' by the OS (the vector
is setup internally to the queue index and the OS does not see it.)

->query_guest_notifiers() is true if the OS has enabled the subchannel
of the proxy device (i.e., if it is enabled for doing *anything* with
the subchannel, regardless whether the OS wants to be notified or is
planning to poll.) The second condition will never hold true for any
valid queue once the OS has setup the queues.

So this won't break anything for virtio-ccw AFAICS, but I don't think
we gain anything.

> +        file.fd = -1;
> +        r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> +        if (r) {
> +            goto fail_vector;
> +        }
> +    }
> +
>      return 0;
> 
> +fail_vector:
>  fail_kick:
>  fail_alloc:
>      cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev, idx),