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

[Jason Wang]

On 2016年08月02日 02:00, Cornelia Huck wrote:
> On Mon,  1 Aug 2016 16:07:58 +0800
> Jason Wang <address@hidden> 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?

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 <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) {
> 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.

I see, so If I understand correctly, there's no way to detect whether or 
not guest will use a specific virtqueue interrupt?

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

Yes, but if we could infer whether or not polling is used in the driver, 
this is probably all we can do for ccw.

Thanks

> > +        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),