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

login
register
mail settings
Submitter Michael S. Tsirkin
Date Oct. 7, 2010, 9:57 a.m.
Message ID <20101007095751.GA7098@redhat.com>
Download mbox | patch
Permalink /patch/67026/
State New
Headers show

Comments

Michael S. Tsirkin - Oct. 7, 2010, 9:57 a.m.
On Wed, Oct 06, 2010 at 04:05:38PM -0600, Alex Williamson wrote:
> 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:

Good idea. I tweaked the code a bit more and came up with this:
I think I will keep this as an incremental patch just to
keep diffs small.

msix: factor out mask notifier code

Move some common code handling msix mask notifiers to a function.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Alex Williamson - Oct. 7, 2010, 3:15 p.m.
On Thu, 2010-10-07 at 11:57 +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 06, 2010 at 04:05:38PM -0600, Alex Williamson wrote:
> > 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:
> 
> Good idea. I tweaked the code a bit more and came up with this:
> I think I will keep this as an incremental patch just to
> keep diffs small.
> 
> msix: factor out mask notifier code
> 
> Move some common code handling msix mask notifiers to a function.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 3d4dd61..5f66d20 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -583,40 +583,26 @@ 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, int masked)
>  {
> -    int r = 0;
> -    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[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;
> -        }
> +    if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) {
> +        return 0;
>      }
> -    return r;
> +    return dev->msix_mask_notifier(dev, vector, masked);
>  }
>  
> -static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> +static int msix_set_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);
> +	/* Notifier has been set. Invoke it on unmasked vectors. */
> +	return msix_notify_if_unmasked(dev, vector, 0); 
> +}

Looks fine except for the extra white space here and copied below.
Thanks,

Alex
 
> -    /* 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;
> +static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> +{
> +	/* Notifier will be unset. Invoke it to mask unmasked entries. */
> +	return msix_notify_if_unmasked(dev, vector, 1); 
>  }
>  
>  int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
Michael S. Tsirkin - Oct. 7, 2010, 3:17 p.m.
> Looks fine except for the extra white space here and copied below.
> Thanks,
> 
> Alex

OK, I fixed this up and queued the patch with your ack.
Thanks for the review!

Patch

diff --git a/hw/msix.c b/hw/msix.c
index 3d4dd61..5f66d20 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -583,40 +583,26 @@  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, int masked)
 {
-    int r = 0;
-    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[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;
-        }
+    if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) {
+        return 0;
     }
-    return r;
+    return dev->msix_mask_notifier(dev, vector, masked);
 }
 
-static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
+static int msix_set_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);
+	/* Notifier has been set. Invoke it on unmasked vectors. */
+	return msix_notify_if_unmasked(dev, vector, 0); 
+}
 
-    /* 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;
+static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
+{
+	/* Notifier will be unset. Invoke it to mask unmasked entries. */
+	return msix_notify_if_unmasked(dev, vector, 1); 
 }
 
 int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)