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

[Michael S. Tsirkin]

On Mon, Aug 01, 2016 at 04:07:58PM +0800, Jason Wang 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.
> 
> 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 <address@hidden>
> ---
>  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) {
> +        file.fd = -1;
> +        r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> +        if (r) {
> +            goto fail_vector;
> +        }
> +    }
> +

It's rather asymmetrical though. Wouldn't a better place
be in vhost_virtqueue_mask?


>      return 0;
>  
> +fail_vector:
>  fail_kick:
>  fail_alloc:
>      cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev, 
> idx),
> -- 
> 2.7.4