Patchwork Re: [PATCHv2] qemu-kvm/vhost: fix up irqfd support

login
register
mail settings
Submitter Michael S. Tsirkin
Date Oct. 6, 2010, 9:44 p.m.
Message ID <20101006214440.GA17149@redhat.com>
Download mbox | patch
Permalink /patch/66984/
State New
Headers show

Comments

Michael S. Tsirkin - Oct. 6, 2010, 9:44 p.m.
On Wed, Oct 06, 2010 at 11:24:24AM -0600, Alex Williamson wrote:
> You could always keep the functions as separate wrapper callers of the
> common function so you only need to keep true = unset, false = set
> straight in one place.  Thanks,


Just to show why it does not work, I did exactly this: as you see the
code is shorter but the true/false magic gets spread: it was in 2
places, (set/unset) now it is in 4 places and it is within the loop, in
code that is more complex.

So I think I'll stick to the original version and we can
patch it up later if there's a will.
Alex Williamson - Oct. 6, 2010, 10:05 p.m.
On Wed, 2010-10-06 at 23:44 +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 06, 2010 at 11:24:24AM -0600, Alex Williamson wrote:
> > You could always keep the functions as separate wrapper callers of the
> > common function so you only need to keep true = unset, false = set
> > straight in one place.  Thanks,
> 
> 
> Just to show why it does not work, I did exactly this: as you see the
> code is shorter but the true/false magic gets spread: it was in 2
> places, (set/unset) now it is in 4 places and it is within the loop, in
> code that is more complex.

You seem to have missed the wrapper function.  I'm simply suggesting
something like this:

static int __do_msix_mask_notifier_for_vector(PCIDevice *dev, unsigned vector, bool mask)
{
    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
        return 0;

    assert(dev->msix_mask_notifier);

    /* Set the new notifier unless vector is masked. */
    if (!msix_is_masked(dev, vector)) {
        return dev->msix_mask_notifier(dev, vector, mask);
    }
    return 0;
}

static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
{
    return __do_msix_mask_notifier_for_vector(dev, vector, false);
}

static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
{
    return __do_msix_mask_notifier_for_vector(dev, vector, true);
}

Which then doesn't go on to complicate the callers like the below does.
Thanks,

Alex


> So I think I'll stick to the original version and we can
> patch it up later if there's a will.
> 
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 3d4dd61..4b705a0 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -583,40 +583,15 @@ void msix_unuse_all_vectors(PCIDevice *dev)
>      msix_free_irq_entries(dev);
>  }
>  
> -static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> +/* Invoke the notifier if vector entry is used and unmasked. */
> +static int msix_notify_if_unmasked(PCIDevice *dev, unsigned vector, bool masked)
>  {
>      int r = 0;
> -    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> +    if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) {
>          return 0;
> -
> -    assert(dev->msix_mask_notifier);
> -
> -    /* Unmask the new notifier unless vector is masked. */
> -    if (!msix_is_masked(dev, vector)) {
> -        r = dev->msix_mask_notifier(dev, vector, false);
> -        if (r < 0) {
> -            return r;
> -        }
>      }
> -    return r;
> -}
> -
> -static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> -{
> -    int r = 0;
> -    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> -        return 0;
> -
>      assert(dev->msix_mask_notifier);
> -
> -    /* Mask the old notifier unless it is already masked. */
> -    if (!msix_is_masked(dev, vector)) {
> -        r = dev->msix_mask_notifier(dev, vector, true);
> -        if (r < 0) {
> -            return r;
> -        }
> -    }
> -    return r;
> +    return dev->msix_mask_notifier(dev, vector, masked);
>  }
>  
>  int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
> @@ -625,7 +600,7 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
>      assert(!dev->msix_mask_notifier);
>      dev->msix_mask_notifier = f;
>      for (n = 0; n < dev->msix_entries_nr; ++n) {
> -        r = msix_set_mask_notifier_for_vector(dev, n);
> +        r = msix_notify_if_unmasked(dev, n, false);
>          if (r < 0) {
>              goto undo;
>          }
> @@ -634,7 +609,7 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
>  
>  undo:
>      while (--n >= 0) {
> -        msix_unset_mask_notifier_for_vector(dev, n);
> +        msix_notify_if_unmasked(dev, n, true);
>      }
>      dev->msix_mask_notifier = NULL;
>      return r;
> @@ -645,7 +620,7 @@ int msix_unset_mask_notifier(PCIDevice *dev)
>      int r, n;
>      assert(dev->msix_mask_notifier);
>      for (n = 0; n < dev->msix_entries_nr; ++n) {
> -        r = msix_unset_mask_notifier_for_vector(dev, n);
> +        r = msix_notify_if_unmasked(dev, n, true);
>          if (r < 0) {
>              goto undo;
>          }
> @@ -655,7 +630,7 @@ int msix_unset_mask_notifier(PCIDevice *dev)
>  
>  undo:
>      while (--n >= 0) {
> -        msix_set_mask_notifier_for_vector(dev, n);
> +        msix_notify_if_unmasked(dev, n, false);
>      }
>      return r;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/hw/msix.c b/hw/msix.c
index 3d4dd61..4b705a0 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -583,40 +583,15 @@  void msix_unuse_all_vectors(PCIDevice *dev)
     msix_free_irq_entries(dev);
 }
 
-static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
+/* Invoke the notifier if vector entry is used and unmasked. */
+static int msix_notify_if_unmasked(PCIDevice *dev, unsigned vector, bool masked)
 {
     int r = 0;
-    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
+    if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) {
         return 0;
-
-    assert(dev->msix_mask_notifier);
-
-    /* Unmask the new notifier unless vector is masked. */
-    if (!msix_is_masked(dev, vector)) {
-        r = dev->msix_mask_notifier(dev, vector, false);
-        if (r < 0) {
-            return r;
-        }
     }
-    return r;
-}
-
-static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
-{
-    int r = 0;
-    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
-        return 0;
-
     assert(dev->msix_mask_notifier);
-
-    /* Mask the old notifier unless it is already masked. */
-    if (!msix_is_masked(dev, vector)) {
-        r = dev->msix_mask_notifier(dev, vector, true);
-        if (r < 0) {
-            return r;
-        }
-    }
-    return r;
+    return dev->msix_mask_notifier(dev, vector, masked);
 }
 
 int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
@@ -625,7 +600,7 @@  int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
     assert(!dev->msix_mask_notifier);
     dev->msix_mask_notifier = f;
     for (n = 0; n < dev->msix_entries_nr; ++n) {
-        r = msix_set_mask_notifier_for_vector(dev, n);
+        r = msix_notify_if_unmasked(dev, n, false);
         if (r < 0) {
             goto undo;
         }
@@ -634,7 +609,7 @@  int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
 
 undo:
     while (--n >= 0) {
-        msix_unset_mask_notifier_for_vector(dev, n);
+        msix_notify_if_unmasked(dev, n, true);
     }
     dev->msix_mask_notifier = NULL;
     return r;
@@ -645,7 +620,7 @@  int msix_unset_mask_notifier(PCIDevice *dev)
     int r, n;
     assert(dev->msix_mask_notifier);
     for (n = 0; n < dev->msix_entries_nr; ++n) {
-        r = msix_unset_mask_notifier_for_vector(dev, n);
+        r = msix_notify_if_unmasked(dev, n, true);
         if (r < 0) {
             goto undo;
         }
@@ -655,7 +630,7 @@  int msix_unset_mask_notifier(PCIDevice *dev)
 
 undo:
     while (--n >= 0) {
-        msix_set_mask_notifier_for_vector(dev, n);
+        msix_notify_if_unmasked(dev, n, false);
     }
     return r;
 }