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

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

[Jason Wang]

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

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

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

[Cornelia Huck]

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

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

[Jason Wang]

On 2016年08月01日 22:08, Michael S. Tsirkin wrote:
> 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 <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) {
>> +        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?

Looks not, since it will be only called when

- backend does not support interrupt masking (vhost-user), the patch 
should work for vhost-kernel too
- mask or unmask an vector but the patch only work for a virtqueue of 
VIRTIO_NO_VECTOR

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

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 <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?

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.

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

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

[Cornelia Huck]

On Tue, 2 Aug 2016 10:39:22 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2016年08月02日 02:00, Cornelia Huck wrote:
> > On Mon,  1 Aug 2016 16:07:58 +0800
> > Jason Wang <jasowang@redhat.com> wrote:

> >> +    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?

Yes. The guest will either be notified for any virtqueue (if it
registered indicators; this is always done for every vq of the device
at once), or for none.

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

What we could do is check whether the driver has registed indicators
and disable setting up notification for any vq of the device if not.
But I'm not sure an always-polling driver is worth optimizing for.

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

[Jason Wang]

On 2016年08月02日 14:37, Cornelia Huck wrote:
> On Tue, 2 Aug 2016 10:39:22 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2016年08月02日 02:00, Cornelia Huck wrote:
>>> On Mon,  1 Aug 2016 16:07:58 +0800
>>> Jason Wang <jasowang@redhat.com> wrote:
>>>> +    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?
> Yes. The guest will either be notified for any virtqueue (if it
> registered indicators; this is always done for every vq of the device
> at once), or for none.
>
>>> 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.
> What we could do is check whether the driver has registed indicators
> and disable setting up notification for any vq of the device if not.
> But I'm not sure an always-polling driver is worth optimizing for.

It's worth for at least pci transport. Consider virito-net pmd in guest, 
a NULL vring call can save unnecessary userspace memory access and 
memory barriers.