[Qemu-devel] [PATCH 0/3] virtio: don't poll masked vectors with irqfd

[Qemu-devel] [PATCH 0/3] virtio: don't poll masked vectors with irqfd

[Michael S. Tsirkin]

At the moment when vector is masked virtio will poll it
in userspace, even if it is handled by irqfd.
This is done in order to update pending bits, but
it's not really required until someone reads the pending bits.
On the other hand this read results in extra io thread wakeups.

As we only implement the pending bits as a compatibility
feature (read - real drivers don't use it), we can defer
the irqfd poll until the read actually happens.

This does not seem to affect vhost-net speed
in simple benchmarks but could help block: both
vhost-blk and dataplane when using irqfd,
and I also think this is cleaner than enabling/disabling
notifiers all the time.

This will also be the basis for future optimizations.

Michael S. Tsirkin (3):
  msi: add API to get notified about pending bit poll
  msix: expose access to masked/pending state
  virtio-pci: don't poll masked vectors

 hw/pci/msix.c   | 19 +++++++++++++++----
 hw/pci/msix.h   |  6 +++++-
 hw/pci/pci.h    |  4 ++++
 hw/vfio_pci.c   |  2 +-
 hw/virtio-pci.c | 53 +++++++++++++++++++++++++++++++++++++++++------------
 5 files changed, 66 insertions(+), 18 deletions(-)

-- 
MST

[Qemu-devel] [PATCH 3/3] virtio-pci: don't poll masked vectors

[Michael S. Tsirkin]

At the moment, when irqfd is in use but a vector is masked,
qemu will poll it and handle vector masks in userspace.
Since almost no one ever looks at the pending bits,
it is better to defer this until pending bits
are actually read.
Implement this optimization using the new poll notifier.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio-pci.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 40 insertions(+), 12 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 1c03bb5..bc6b4e0 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -509,8 +509,6 @@ static int kvm_virtio_pci_vq_vector_use(VirtIOPCIProxy *proxy,
         }
         return ret;
     }
-
-    virtio_queue_set_guest_notifier_fd_handler(vq, true, true);
     return 0;
 }
 
@@ -529,8 +527,6 @@ static void kvm_virtio_pci_vq_vector_release(VirtIOPCIProxy *proxy,
     if (--irqfd->users == 0) {
         kvm_irqchip_release_virq(kvm_state, irqfd->virq);
     }
-
-    virtio_queue_set_guest_notifier_fd_handler(vq, true, false);
 }
 
 static int kvm_virtio_pci_vector_use(PCIDevice *dev, unsigned vector,
@@ -581,7 +577,36 @@ static void kvm_virtio_pci_vector_release(PCIDevice *dev, unsigned vector)
     }
 }
 
-static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign)
+static void kvm_virtio_pci_vector_poll(PCIDevice *dev,
+                                       unsigned int vector_start,
+                                       unsigned int vector_end)
+{
+    VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
+    VirtIODevice *vdev = proxy->vdev;
+    int queue_no;
+    unsigned int vector;
+    EventNotifier *notifier;
+    VirtQueue *vq;
+
+    for (queue_no = 0; queue_no < VIRTIO_PCI_QUEUE_MAX; queue_no++) {
+        if (!virtio_queue_get_num(vdev, queue_no)) {
+            break;
+        }
+        vector = virtio_queue_vector(vdev, queue_no);
+        if (vector < vector_start || vector >= vector_end ||
+            !msix_is_masked(dev, vector)) {
+            continue;
+        }
+        vq = virtio_get_queue(vdev, queue_no);
+        notifier = virtio_queue_get_guest_notifier(vq);
+        if (event_notifier_test_and_clear(notifier)) {
+            msix_set_pending(dev, vector);
+        }
+    }
+}
+
+static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign,
+                                         bool with_irqfd)
 {
     VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
     VirtQueue *vq = virtio_get_queue(proxy->vdev, n);
@@ -592,9 +617,9 @@ static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign)
         if (r < 0) {
             return r;
         }
-        virtio_queue_set_guest_notifier_fd_handler(vq, true, false);
+        virtio_queue_set_guest_notifier_fd_handler(vq, true, with_irqfd);
     } else {
-        virtio_queue_set_guest_notifier_fd_handler(vq, false, false);
+        virtio_queue_set_guest_notifier_fd_handler(vq, false, with_irqfd);
         event_notifier_cleanup(notifier);
     }
 
@@ -612,9 +637,11 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign)
     VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
     VirtIODevice *vdev = proxy->vdev;
     int r, n;
+    bool with_irqfd = msix_enabled(&proxy->pci_dev) &&
+        kvm_msi_via_irqfd_enabled();
 
     /* Must unset vector notifier while guest notifier is still assigned */
-    if (kvm_msi_via_irqfd_enabled() && !assign) {
+    if (with_irqfd && !assign) {
         msix_unset_vector_notifiers(&proxy->pci_dev);
         g_free(proxy->vector_irqfd);
         proxy->vector_irqfd = NULL;
@@ -625,21 +652,22 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign)
             break;
         }
 
-        r = virtio_pci_set_guest_notifier(d, n, assign);
+        r = virtio_pci_set_guest_notifier(d, n, assign,
+                                          kvm_msi_via_irqfd_enabled());
         if (r < 0) {
             goto assign_error;
         }
     }
 
     /* Must set vector notifier after guest notifier has been assigned */
-    if (kvm_msi_via_irqfd_enabled() && assign) {
+    if (with_irqfd && assign) {
         proxy->vector_irqfd =
             g_malloc0(sizeof(*proxy->vector_irqfd) *
                       msix_nr_vectors_allocated(&proxy->pci_dev));
         r = msix_set_vector_notifiers(&proxy->pci_dev,
                                       kvm_virtio_pci_vector_use,
                                       kvm_virtio_pci_vector_release,
-                                      NULL);
+                                      kvm_virtio_pci_vector_poll);
         if (r < 0) {
             goto assign_error;
         }
@@ -651,7 +679,7 @@ assign_error:
     /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */
     assert(assign);
     while (--n >= 0) {
-        virtio_pci_set_guest_notifier(d, n, !assign);
+        virtio_pci_set_guest_notifier(d, n, !assign, with_irqfd);
     }
     return r;
 }
-- 
MST

[Qemu-devel] [PATCH 2/3] msix: expose access to masked/pending state

[Michael S. Tsirkin]

For use by poll handler.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci/msix.c | 6 +++---
 hw/pci/msix.h | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 1f31975..9df0ffb 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -65,7 +65,7 @@ static int msix_is_pending(PCIDevice *dev, int vector)
     return *msix_pending_byte(dev, vector) & msix_pending_mask(vector);
 }
 
-static void msix_set_pending(PCIDevice *dev, int vector)
+void msix_set_pending(PCIDevice *dev, unsigned int vector)
 {
     *msix_pending_byte(dev, vector) |= msix_pending_mask(vector);
 }
@@ -75,13 +75,13 @@ static void msix_clr_pending(PCIDevice *dev, int vector)
     *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector);
 }
 
-static bool msix_vector_masked(PCIDevice *dev, int vector, bool fmask)
+static bool msix_vector_masked(PCIDevice *dev, unsigned int vector, bool fmask)
 {
     unsigned offset = vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL;
     return fmask || dev->msix_table[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT;
 }
 
-static bool msix_is_masked(PCIDevice *dev, int vector)
+bool msix_is_masked(PCIDevice *dev, unsigned int vector)
 {
     return msix_vector_masked(dev, vector, dev->msix_function_masked);
 }
diff --git a/hw/pci/msix.h b/hw/pci/msix.h
index ea85d02..d0c4429 100644
--- a/hw/pci/msix.h
+++ b/hw/pci/msix.h
@@ -26,6 +26,9 @@ void msix_load(PCIDevice *dev, QEMUFile *f);
 int msix_enabled(PCIDevice *dev);
 int msix_present(PCIDevice *dev);
 
+bool msix_is_masked(PCIDevice *dev, unsigned vector);
+void msix_set_pending(PCIDevice *dev, unsigned vector);
+
 int msix_vector_use(PCIDevice *dev, unsigned vector);
 void msix_vector_unuse(PCIDevice *dev, unsigned vector);
 void msix_unuse_all_vectors(PCIDevice *dev);
-- 
MST

[Qemu-devel] [PATCH 1/3] msi: add API to get notified about pending bit poll

[Michael S. Tsirkin]

Update all users.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci/msix.c   | 13 ++++++++++++-
 hw/pci/msix.h   |  3 ++-
 hw/pci/pci.h    |  4 ++++
 hw/vfio_pci.c   |  2 +-
 hw/virtio-pci.c |  3 ++-
 5 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 917327b..1f31975 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -192,6 +192,11 @@ static uint64_t msix_pba_mmio_read(void *opaque, hwaddr addr,
                                    unsigned size)
 {
     PCIDevice *dev = opaque;
+    if (dev->msix_vector_poll_notifier) {
+        unsigned vector_start = addr * 8;
+        unsigned vector_end = MIN(addr + size * 8, dev->msix_entries_nr);
+        dev->msix_vector_poll_notifier(dev, vector_start, vector_end);
+    }
 
     return pci_get_long(dev->msix_pba + addr);
 }
@@ -515,7 +520,8 @@ static void msix_unset_notifier_for_vector(PCIDevice *dev, unsigned int vector)
 
 int msix_set_vector_notifiers(PCIDevice *dev,
                               MSIVectorUseNotifier use_notifier,
-                              MSIVectorReleaseNotifier release_notifier)
+                              MSIVectorReleaseNotifier release_notifier,
+                              MSIVectorPollNotifier poll_notifier)
 {
     int vector, ret;
 
@@ -523,6 +529,7 @@ int msix_set_vector_notifiers(PCIDevice *dev,
 
     dev->msix_vector_use_notifier = use_notifier;
     dev->msix_vector_release_notifier = release_notifier;
+    dev->msix_vector_poll_notifier = poll_notifier;
 
     if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
         (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
@@ -533,6 +540,9 @@ int msix_set_vector_notifiers(PCIDevice *dev,
             }
         }
     }
+    if (dev->msix_vector_poll_notifier) {
+        dev->msix_vector_poll_notifier(dev, 0, dev->msix_entries_nr);
+    }
     return 0;
 
 undo:
@@ -559,4 +569,5 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
     }
     dev->msix_vector_use_notifier = NULL;
     dev->msix_vector_release_notifier = NULL;
+    dev->msix_vector_poll_notifier = NULL;
 }
diff --git a/hw/pci/msix.h b/hw/pci/msix.h
index ff07ae2..ea85d02 100644
--- a/hw/pci/msix.h
+++ b/hw/pci/msix.h
@@ -36,6 +36,7 @@ void msix_reset(PCIDevice *dev);
 
 int msix_set_vector_notifiers(PCIDevice *dev,
                               MSIVectorUseNotifier use_notifier,
-                              MSIVectorReleaseNotifier release_notifier);
+                              MSIVectorReleaseNotifier release_notifier,
+                              MSIVectorPollNotifier poll_notifier);
 void msix_unset_vector_notifiers(PCIDevice *dev);
 #endif
diff --git a/hw/pci/pci.h b/hw/pci/pci.h
index 41e5ddd..f80f8fb 100644
--- a/hw/pci/pci.h
+++ b/hw/pci/pci.h
@@ -187,6 +187,9 @@ typedef void (*PCIINTxRoutingNotifier)(PCIDevice *dev);
 typedef int (*MSIVectorUseNotifier)(PCIDevice *dev, unsigned int vector,
                                       MSIMessage msg);
 typedef void (*MSIVectorReleaseNotifier)(PCIDevice *dev, unsigned int vector);
+typedef void (*MSIVectorPollNotifier)(PCIDevice *dev,
+                                      unsigned int vector_start,
+                                      unsigned int vector_end);
 
 struct PCIDevice {
     DeviceState qdev;
@@ -271,6 +274,7 @@ struct PCIDevice {
     /* MSI-X notifiers */
     MSIVectorUseNotifier msix_vector_use_notifier;
     MSIVectorReleaseNotifier msix_vector_release_notifier;
+    MSIVectorPollNotifier msix_vector_poll_notifier;
 };
 
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
diff --git a/hw/vfio_pci.c b/hw/vfio_pci.c
index 45d90ab..80c11de 100644
--- a/hw/vfio_pci.c
+++ b/hw/vfio_pci.c
@@ -697,7 +697,7 @@ static void vfio_enable_msix(VFIODevice *vdev)
     vdev->interrupt = VFIO_INT_MSIX;
 
     if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
-                                  vfio_msix_vector_release)) {
+                                  vfio_msix_vector_release, NULL)) {
         error_report("vfio: msix_set_vector_notifiers failed\n");
     }
 
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 518fb8a..1c03bb5 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -638,7 +638,8 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign)
                       msix_nr_vectors_allocated(&proxy->pci_dev));
         r = msix_set_vector_notifiers(&proxy->pci_dev,
                                       kvm_virtio_pci_vector_use,
-                                      kvm_virtio_pci_vector_release);
+                                      kvm_virtio_pci_vector_release,
+                                      NULL);
         if (r < 0) {
             goto assign_error;
         }
-- 
MST

Re: [Qemu-devel] [PATCH 0/3] virtio: don't poll masked vectors with irqfd

[Michael S. Tsirkin]

On Tue, Dec 18, 2012 at 02:20:20PM +0200, Michael S. Tsirkin wrote:
> At the moment when vector is masked virtio will poll it
> in userspace, even if it is handled by irqfd.
> This is done in order to update pending bits, but
> it's not really required until someone reads the pending bits.
> On the other hand this read results in extra io thread wakeups.
> 
> As we only implement the pending bits as a compatibility
> feature (read - real drivers don't use it), we can defer
> the irqfd poll until the read actually happens.
> 
> This does not seem to affect vhost-net speed
> in simple benchmarks but could help block: both
> vhost-blk and dataplane when using irqfd,
> and I also think this is cleaner than enabling/disabling
> notifiers all the time.
> 
> This will also be the basis for future optimizations.

Note: this is on top of the typesafe bindings patch v3
I sent previously.
You can get the whole bundle from:
git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git pci

-- 
MST

Re: [Qemu-devel] [PATCH 0/3] virtio: don't poll masked vectors with irqfd

[Asias He]

On 12/18/2012 08:39 PM, Michael S. Tsirkin wrote:
> At the moment when vector is masked virtio will poll it
> in userspace, even if it is handled by irqfd.
> This is done in order to update pending bits, but
> it's not really required until someone reads the pending bits.
> On the other hand this read results in extra io thread wakeups.
> 
> As we only implement the pending bits as a compatibility
> feature (read - real drivers don't use it), we can defer
> the irqfd poll until the read actually happens.
> 
> This does not seem to affect vhost-net speed
> in simple benchmarks but could help block: both
> vhost-blk and dataplane when using irqfd,
> and I also think this is cleaner than enabling/disabling
> notifiers all the time.
> 
> This will also be the basis for future optimizations.
> 
> Michael S. Tsirkin (3):
>   msi: add API to get notified about pending bit poll
>   msix: expose access to masked/pending state
>   virtio-pci: don't poll masked vectors
> 
>  hw/pci/msix.c   | 19 +++++++++++++++----
>  hw/pci/msix.h   |  6 +++++-
>  hw/pci/pci.h    |  4 ++++
>  hw/vfio_pci.c   |  2 +-
>  hw/virtio-pci.c | 53 +++++++++++++++++++++++++++++++++++++++++------------
>  5 files changed, 66 insertions(+), 18 deletions(-)

The performance boost is significant here. It is close to the
result of dropping msix_fire_vector_notifier() hack.

-- 
Asias

Re: [Qemu-devel] [PATCH 3/3] virtio-pci: don't poll masked vectors

[Asias He]

On 12/18/2012 08:39 PM, Michael S. Tsirkin wrote:
> At the moment, when irqfd is in use but a vector is masked,
> qemu will poll it and handle vector masks in userspace.
> Since almost no one ever looks at the pending bits,
> it is better to defer this until pending bits
> are actually read.
> Implement this optimization using the new poll notifier.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio-pci.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 1c03bb5..bc6b4e0 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -509,8 +509,6 @@ static int kvm_virtio_pci_vq_vector_use(VirtIOPCIProxy *proxy,
>          }
>          return ret;
>      }
> -
> -    virtio_queue_set_guest_notifier_fd_handler(vq, true, true);
>      return 0;
>  }
>  
> @@ -529,8 +527,6 @@ static void kvm_virtio_pci_vq_vector_release(VirtIOPCIProxy *proxy,
>      if (--irqfd->users == 0) {
>          kvm_irqchip_release_virq(kvm_state, irqfd->virq);
>      }
> -
> -    virtio_queue_set_guest_notifier_fd_handler(vq, true, false);
>  }
>  
>  static int kvm_virtio_pci_vector_use(PCIDevice *dev, unsigned vector,
> @@ -581,7 +577,36 @@ static void kvm_virtio_pci_vector_release(PCIDevice *dev, unsigned vector)
>      }
>  }
>  
> -static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign)
> +static void kvm_virtio_pci_vector_poll(PCIDevice *dev,
> +                                       unsigned int vector_start,
> +                                       unsigned int vector_end)
> +{
> +    VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
> +    VirtIODevice *vdev = proxy->vdev;
> +    int queue_no;
> +    unsigned int vector;
> +    EventNotifier *notifier;
> +    VirtQueue *vq;
> +
> +    for (queue_no = 0; queue_no < VIRTIO_PCI_QUEUE_MAX; queue_no++) {
> +        if (!virtio_queue_get_num(vdev, queue_no)) {
> +            break;
> +        }
> +        vector = virtio_queue_vector(vdev, queue_no);
> +        if (vector < vector_start || vector >= vector_end ||
> +            !msix_is_masked(dev, vector)) {
> +            continue;
> +        }
> +        vq = virtio_get_queue(vdev, queue_no);
> +        notifier = virtio_queue_get_guest_notifier(vq);
> +        if (event_notifier_test_and_clear(notifier)) {
> +            msix_set_pending(dev, vector);
> +        }
> +    }
> +}
> +
> +static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign,
> +                                         bool with_irqfd)
>  {
>      VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
>      VirtQueue *vq = virtio_get_queue(proxy->vdev, n);
> @@ -592,9 +617,9 @@ static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign)
>          if (r < 0) {
>              return r;
>          }
> -        virtio_queue_set_guest_notifier_fd_handler(vq, true, false);
> +        virtio_queue_set_guest_notifier_fd_handler(vq, true, with_irqfd);

+  virtio_queue_set_guest_notifier_fd_handler(vq, assign, with_irqfd);

>      } else {
> -        virtio_queue_set_guest_notifier_fd_handler(vq, false, false);
> +        virtio_queue_set_guest_notifier_fd_handler(vq, false, with_irqfd);

+  virtio_queue_set_guest_notifier_fd_handler(vq, assign, with_irqfd);

Use the 'assign' variable instead of the constants?

>          event_notifier_cleanup(notifier);
>      }
>  
> @@ -612,9 +637,11 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign)
>      VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
>      VirtIODevice *vdev = proxy->vdev;
>      int r, n;
> +    bool with_irqfd = msix_enabled(&proxy->pci_dev) &&
> +        kvm_msi_via_irqfd_enabled();
>  
>      /* Must unset vector notifier while guest notifier is still assigned */
> -    if (kvm_msi_via_irqfd_enabled() && !assign) {
> +    if (with_irqfd && !assign) {
>          msix_unset_vector_notifiers(&proxy->pci_dev);
>          g_free(proxy->vector_irqfd);
>          proxy->vector_irqfd = NULL;
> @@ -625,21 +652,22 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign)
>              break;
>          }
>  
> -        r = virtio_pci_set_guest_notifier(d, n, assign);
> +        r = virtio_pci_set_guest_notifier(d, n, assign,
> +                                          kvm_msi_via_irqfd_enabled());
>          if (r < 0) {
>              goto assign_error;
>          }
>      }
>  
>      /* Must set vector notifier after guest notifier has been assigned */
> -    if (kvm_msi_via_irqfd_enabled() && assign) {
> +    if (with_irqfd && assign) {
>          proxy->vector_irqfd =
>              g_malloc0(sizeof(*proxy->vector_irqfd) *
>                        msix_nr_vectors_allocated(&proxy->pci_dev));
>          r = msix_set_vector_notifiers(&proxy->pci_dev,
>                                        kvm_virtio_pci_vector_use,
>                                        kvm_virtio_pci_vector_release,
> -                                      NULL);
> +                                      kvm_virtio_pci_vector_poll);
>          if (r < 0) {
>              goto assign_error;
>          }
> @@ -651,7 +679,7 @@ assign_error:
>      /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */
>      assert(assign);
>      while (--n >= 0) {
> -        virtio_pci_set_guest_notifier(d, n, !assign);
> +        virtio_pci_set_guest_notifier(d, n, !assign, with_irqfd);
>      }
>      return r;
>  }
> 


-- 
Asias

Re: [Qemu-devel] [PATCH 3/3] virtio-pci: don't poll masked vectors

[Michael S. Tsirkin]

On Wed, Dec 19, 2012 at 04:59:05PM +0800, Asias He wrote:
> On 12/18/2012 08:39 PM, Michael S. Tsirkin wrote:
> > At the moment, when irqfd is in use but a vector is masked,
> > qemu will poll it and handle vector masks in userspace.
> > Since almost no one ever looks at the pending bits,
> > it is better to defer this until pending bits
> > are actually read.
> > Implement this optimization using the new poll notifier.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/virtio-pci.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 40 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > index 1c03bb5..bc6b4e0 100644
> > --- a/hw/virtio-pci.c
> > +++ b/hw/virtio-pci.c
> > @@ -509,8 +509,6 @@ static int kvm_virtio_pci_vq_vector_use(VirtIOPCIProxy *proxy,
> >          }
> >          return ret;
> >      }
> > -
> > -    virtio_queue_set_guest_notifier_fd_handler(vq, true, true);
> >      return 0;
> >  }
> >  
> > @@ -529,8 +527,6 @@ static void kvm_virtio_pci_vq_vector_release(VirtIOPCIProxy *proxy,
> >      if (--irqfd->users == 0) {
> >          kvm_irqchip_release_virq(kvm_state, irqfd->virq);
> >      }
> > -
> > -    virtio_queue_set_guest_notifier_fd_handler(vq, true, false);
> >  }
> >  
> >  static int kvm_virtio_pci_vector_use(PCIDevice *dev, unsigned vector,
> > @@ -581,7 +577,36 @@ static void kvm_virtio_pci_vector_release(PCIDevice *dev, unsigned vector)
> >      }
> >  }
> >  
> > -static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign)
> > +static void kvm_virtio_pci_vector_poll(PCIDevice *dev,
> > +                                       unsigned int vector_start,
> > +                                       unsigned int vector_end)
> > +{
> > +    VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
> > +    VirtIODevice *vdev = proxy->vdev;
> > +    int queue_no;
> > +    unsigned int vector;
> > +    EventNotifier *notifier;
> > +    VirtQueue *vq;
> > +
> > +    for (queue_no = 0; queue_no < VIRTIO_PCI_QUEUE_MAX; queue_no++) {
> > +        if (!virtio_queue_get_num(vdev, queue_no)) {
> > +            break;
> > +        }
> > +        vector = virtio_queue_vector(vdev, queue_no);
> > +        if (vector < vector_start || vector >= vector_end ||
> > +            !msix_is_masked(dev, vector)) {
> > +            continue;
> > +        }
> > +        vq = virtio_get_queue(vdev, queue_no);
> > +        notifier = virtio_queue_get_guest_notifier(vq);
> > +        if (event_notifier_test_and_clear(notifier)) {
> > +            msix_set_pending(dev, vector);
> > +        }
> > +    }
> > +}
> > +
> > +static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign,
> > +                                         bool with_irqfd)
> >  {
> >      VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
> >      VirtQueue *vq = virtio_get_queue(proxy->vdev, n);
> > @@ -592,9 +617,9 @@ static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign)
> >          if (r < 0) {
> >              return r;
> >          }
> > -        virtio_queue_set_guest_notifier_fd_handler(vq, true, false);
> > +        virtio_queue_set_guest_notifier_fd_handler(vq, true, with_irqfd);
> 
> +  virtio_queue_set_guest_notifier_fd_handler(vq, assign, with_irqfd);
> 
> >      } else {
> > -        virtio_queue_set_guest_notifier_fd_handler(vq, false, false);
> > +        virtio_queue_set_guest_notifier_fd_handler(vq, false, with_irqfd);
> 
> +  virtio_queue_set_guest_notifier_fd_handler(vq, assign, with_irqfd);
> 
> Use the 'assign' variable instead of the constants?

I don't care either way (this is within if (assign)).
But all this patch does is add a parameter,
unrelated code refactorings should be separate.

> >          event_notifier_cleanup(notifier);
> >      }
> >  
> > @@ -612,9 +637,11 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign)
> >      VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
> >      VirtIODevice *vdev = proxy->vdev;
> >      int r, n;
> > +    bool with_irqfd = msix_enabled(&proxy->pci_dev) &&
> > +        kvm_msi_via_irqfd_enabled();
> >  
> >      /* Must unset vector notifier while guest notifier is still assigned */
> > -    if (kvm_msi_via_irqfd_enabled() && !assign) {
> > +    if (with_irqfd && !assign) {
> >          msix_unset_vector_notifiers(&proxy->pci_dev);
> >          g_free(proxy->vector_irqfd);
> >          proxy->vector_irqfd = NULL;
> > @@ -625,21 +652,22 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign)
> >              break;
> >          }
> >  
> > -        r = virtio_pci_set_guest_notifier(d, n, assign);
> > +        r = virtio_pci_set_guest_notifier(d, n, assign,
> > +                                          kvm_msi_via_irqfd_enabled());
> >          if (r < 0) {
> >              goto assign_error;
> >          }
> >      }
> >  
> >      /* Must set vector notifier after guest notifier has been assigned */
> > -    if (kvm_msi_via_irqfd_enabled() && assign) {
> > +    if (with_irqfd && assign) {
> >          proxy->vector_irqfd =
> >              g_malloc0(sizeof(*proxy->vector_irqfd) *
> >                        msix_nr_vectors_allocated(&proxy->pci_dev));
> >          r = msix_set_vector_notifiers(&proxy->pci_dev,
> >                                        kvm_virtio_pci_vector_use,
> >                                        kvm_virtio_pci_vector_release,
> > -                                      NULL);
> > +                                      kvm_virtio_pci_vector_poll);
> >          if (r < 0) {
> >              goto assign_error;
> >          }
> > @@ -651,7 +679,7 @@ assign_error:
> >      /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */
> >      assert(assign);
> >      while (--n >= 0) {
> > -        virtio_pci_set_guest_notifier(d, n, !assign);
> > +        virtio_pci_set_guest_notifier(d, n, !assign, with_irqfd);
> >      }
> >      return r;
> >  }
> > 
> 
> 
> -- 
> Asias

Re: [Qemu-devel] [PATCH 0/3] virtio: don't poll masked vectors with irqfd

[Michael S. Tsirkin]

On Wed, Dec 19, 2012 at 04:53:22PM +0800, Asias He wrote:
> On 12/18/2012 08:39 PM, Michael S. Tsirkin wrote:
> > At the moment when vector is masked virtio will poll it
> > in userspace, even if it is handled by irqfd.
> > This is done in order to update pending bits, but
> > it's not really required until someone reads the pending bits.
> > On the other hand this read results in extra io thread wakeups.
> > 
> > As we only implement the pending bits as a compatibility
> > feature (read - real drivers don't use it), we can defer
> > the irqfd poll until the read actually happens.
> > 
> > This does not seem to affect vhost-net speed
> > in simple benchmarks but could help block: both
> > vhost-blk and dataplane when using irqfd,
> > and I also think this is cleaner than enabling/disabling
> > notifiers all the time.
> > 
> > This will also be the basis for future optimizations.
> > 
> > Michael S. Tsirkin (3):
> >   msi: add API to get notified about pending bit poll
> >   msix: expose access to masked/pending state
> >   virtio-pci: don't poll masked vectors
> > 
> >  hw/pci/msix.c   | 19 +++++++++++++++----
> >  hw/pci/msix.h   |  6 +++++-
> >  hw/pci/pci.h    |  4 ++++
> >  hw/vfio_pci.c   |  2 +-
> >  hw/virtio-pci.c | 53 +++++++++++++++++++++++++++++++++++++++++------------
> >  5 files changed, 66 insertions(+), 18 deletions(-)
> 
> The performance boost is significant here. It is close to the
> result of dropping msix_fire_vector_notifier() hack.

Okay great. I have one other last optimization up my sleeve,
that should hopefully make it as fast as with the hack.

> -- 
> Asias

Re: [Qemu-devel] [PATCH 3/3] virtio-pci: don't poll masked vectors

[Stefan Hajnoczi]

On Tue, Dec 18, 2012 at 02:39:19PM +0200, Michael S. Tsirkin wrote:
> -static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign)
> +static void kvm_virtio_pci_vector_poll(PCIDevice *dev,
> +                                       unsigned int vector_start,
> +                                       unsigned int vector_end)
> +{
> +    VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
> +    VirtIODevice *vdev = proxy->vdev;
> +    int queue_no;
> +    unsigned int vector;
> +    EventNotifier *notifier;
> +    VirtQueue *vq;
> +
> +    for (queue_no = 0; queue_no < VIRTIO_PCI_QUEUE_MAX; queue_no++) {
> +        if (!virtio_queue_get_num(vdev, queue_no)) {
> +            break;
> +        }
> +        vector = virtio_queue_vector(vdev, queue_no);
> +        if (vector < vector_start || vector >= vector_end ||
> +            !msix_is_masked(dev, vector)) {
> +            continue;
> +        }
> +        vq = virtio_get_queue(vdev, queue_no);
> +        notifier = virtio_queue_get_guest_notifier(vq);
> +        if (event_notifier_test_and_clear(notifier)) {
> +            msix_set_pending(dev, vector);
> +        }

Small difference to virtio_queue_guest_notifier_read() here: we do not
set vq->vdev->isr |= 0x01.  I guess no guest drivers use isr with MSIX
but should we still set it or even just call
virtio_queue_guest_notifier_read() instead of duplicating that event
notifier read here?

Stefan