Re: [PATCH] vhost-net: fix reversed logic in mask notifiers
On 05/25/2010 05:00 PM, Michael S. Tsirkin wrote: When guest notifier is assigned, we set mask notifier, which will assign kvm irqfd. When guest notifier is unassigned, mask notifier is unset, which should unassign kvm irqfd. The way to do this is to call mask notifier telling it to mask the vector. This, unless vector is already masked which unassigns irqfd already. The logic in unassign was reversed, which left kvm irqfd assigned. This patch is qemu-kvm only as irqfd is not upstream. Applied, thanks. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] vhost-net: fix reversed logic in mask notifiers
When guest notifier is assigned, we set mask notifier,
which will assign kvm irqfd.
When guest notifier is unassigned, mask notifier is unset,
which should unassign kvm irqfd.
The way to do this is to call mask notifier telling it to mask the vector.
This, unless vector is already masked which unassigns irqfd already.
The logic in unassign was reversed, which left kvm irqfd assigned.
This patch is qemu-kvm only as irqfd is not upstream.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reported-by: Amit Shah amit.s...@redhat.com
---
hw/msix.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/hw/msix.c b/hw/msix.c
index 8f9a621..1398680 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -617,6 +617,7 @@ int msix_set_mask_notifier(PCIDevice *dev, unsigned vector,
void *opaque)
assert(opaque);
assert(!dev-msix_mask_notifier_opaque[vector]);
+/* Unmask the new notifier unless vector is masked. */
if (msix_is_masked(dev, vector)) {
return 0;
}
@@ -638,12 +639,13 @@ int msix_unset_mask_notifier(PCIDevice *dev, unsigned
vector)
assert(dev-msix_mask_notifier);
assert(dev-msix_mask_notifier_opaque[vector]);
+/* Mask the old notifier unless it is already masked. */
if (msix_is_masked(dev, vector)) {
return 0;
}
r = dev-msix_mask_notifier(dev, vector,
dev-msix_mask_notifier_opaque[vector],
-msix_is_masked(dev, vector));
+!msix_is_masked(dev, vector));
if (r 0) {
return r;
}
--
1.7.1.12.g42b7f
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] vhost-net: fix reversed logic in mask notifiers
On (Tue) May 25 2010 [17:00:43], Michael S. Tsirkin wrote: When guest notifier is assigned, we set mask notifier, which will assign kvm irqfd. When guest notifier is unassigned, mask notifier is unset, which should unassign kvm irqfd. The way to do this is to call mask notifier telling it to mask the vector. This, unless vector is already masked which unassigns irqfd already. The logic in unassign was reversed, which left kvm irqfd assigned. This patch is qemu-kvm only as irqfd is not upstream. Signed-off-by: Michael S. Tsirkin m...@redhat.com Reported-by: Amit Shah amit.s...@redhat.com Acked-by: Amit Shah amit.s...@redhat.com Amit -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] vhost-net: fix reversed logic in mask notifiers
Michael S. Tsirkin m...@redhat.com wrote:
When guest notifier is assigned, we set mask notifier,
which will assign kvm irqfd.
When guest notifier is unassigned, mask notifier is unset,
which should unassign kvm irqfd.
The way to do this is to call mask notifier telling it to mask the vector.
This, unless vector is already masked which unassigns irqfd already.
The logic in unassign was reversed, which left kvm irqfd assigned.
This patch is qemu-kvm only as irqfd is not upstream.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reported-by: Amit Shah amit.s...@redhat.com
---
hw/msix.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/hw/msix.c b/hw/msix.c
index 8f9a621..1398680 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -617,6 +617,7 @@ int msix_set_mask_notifier(PCIDevice *dev, unsigned
vector, void *opaque)
assert(opaque);
assert(!dev-msix_mask_notifier_opaque[vector]);
+/* Unmask the new notifier unless vector is masked. */
if (msix_is_masked(dev, vector)) {
return 0;
}
@@ -638,12 +639,13 @@ int msix_unset_mask_notifier(PCIDevice *dev, unsigned
vector)
assert(dev-msix_mask_notifier);
assert(dev-msix_mask_notifier_opaque[vector]);
+/* Mask the old notifier unless it is already masked. */
if (msix_is_masked(dev, vector)) {
return 0;
}
r = dev-msix_mask_notifier(dev, vector,
dev-msix_mask_notifier_opaque[vector],
-msix_is_masked(dev, vector));
+!msix_is_masked(dev, vector));
Why don't put just a 1 here?
we have:
if (msix_is_masked())
return 0
r = msix_mask_notifier(., !msix_is_masked());
i.e. at that point msix_is_masked() is false, or we really, really needs
locking.
Puttting a !foo, when we know that it needs to be an 1 looks strange.
Later, Juan.
PD. Yes, I already asked in a previous version to just have two
methods, mask/unmask. we now at call time which one we need.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] vhost-net: fix reversed logic in mask notifiers
On Tue, May 25, 2010 at 04:37:36PM +0200, Juan Quintela wrote:
Michael S. Tsirkin m...@redhat.com wrote:
When guest notifier is assigned, we set mask notifier,
which will assign kvm irqfd.
When guest notifier is unassigned, mask notifier is unset,
which should unassign kvm irqfd.
The way to do this is to call mask notifier telling it to mask the vector.
This, unless vector is already masked which unassigns irqfd already.
The logic in unassign was reversed, which left kvm irqfd assigned.
This patch is qemu-kvm only as irqfd is not upstream.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reported-by: Amit Shah amit.s...@redhat.com
---
hw/msix.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/hw/msix.c b/hw/msix.c
index 8f9a621..1398680 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -617,6 +617,7 @@ int msix_set_mask_notifier(PCIDevice *dev, unsigned
vector, void *opaque)
assert(opaque);
assert(!dev-msix_mask_notifier_opaque[vector]);
+/* Unmask the new notifier unless vector is masked. */
if (msix_is_masked(dev, vector)) {
return 0;
}
@@ -638,12 +639,13 @@ int msix_unset_mask_notifier(PCIDevice *dev, unsigned
vector)
assert(dev-msix_mask_notifier);
assert(dev-msix_mask_notifier_opaque[vector]);
+/* Mask the old notifier unless it is already masked. */
if (msix_is_masked(dev, vector)) {
return 0;
}
r = dev-msix_mask_notifier(dev, vector,
dev-msix_mask_notifier_opaque[vector],
-msix_is_masked(dev, vector));
+!msix_is_masked(dev, vector));
Why don't put just a 1 here?
we have:
if (msix_is_masked())
return 0
r = msix_mask_notifier(., !msix_is_masked());
i.e. at that point msix_is_masked() is false, or we really, really needs
locking.
Puttting a !foo, when we know that it needs to be an 1 looks strange.
Later, Juan.
PD. Yes, I already asked in a previous version to just have two
methods, mask/unmask. we now at call time which one we need.
I find msix_is_masked clearer here than true since you don't need
to look up definition to understand what this 'true' stands for.
The value is clear from code above. What do you think?
--
MST
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] vhost-net: fix reversed logic in mask notifiers
On 05/25/10 16:00, Michael S. Tsirkin wrote: When guest notifier is assigned, we set mask notifier, which will assign kvm irqfd. When guest notifier is unassigned, mask notifier is unset, which should unassign kvm irqfd. The way to do this is to call mask notifier telling it to mask the vector. This, unless vector is already masked which unassigns irqfd already. The logic in unassign was reversed, which left kvm irqfd assigned. This patch is qemu-kvm only as irqfd is not upstream. Signed-off-by: Michael S. Tsirkinm...@redhat.com Reported-by: Amit Shahamit.s...@redhat.com Acked-by: Gerd Hoffmann kra...@redhat.com cheers, Gerd -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] vhost-net: fix reversed logic in mask notifiers
Michael S. Tsirkin m...@redhat.com wrote: On Tue, May 25, 2010 at 04:37:36PM +0200, Juan Quintela wrote: we have: if (msix_is_masked()) return 0 r = msix_mask_notifier(., !msix_is_masked()); i.e. at that point msix_is_masked() is false, or we really, really needs locking. Puttting a !foo, when we know that it needs to be an 1 looks strange. Later, Juan. PD. Yes, I already asked in a previous version to just have two methods, mask/unmask. we now at call time which one we need. I find msix_is_masked clearer here than true since you don't need to look up definition to understand what this 'true' stands for. The value is clear from code above. What do you think? I preffer the change, but it is up to you. at that point, we are using !msix_masked() to mean true i.e. we know that msix_masked() is false. What you want to do is mask. Later, Juan. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] vhost-net: fix reversed logic in mask notifiers
On Tue, May 25, 2010 at 04:58:15PM +0200, Juan Quintela wrote: Michael S. Tsirkin m...@redhat.com wrote: On Tue, May 25, 2010 at 04:37:36PM +0200, Juan Quintela wrote: we have: if (msix_is_masked()) return 0 r = msix_mask_notifier(., !msix_is_masked()); i.e. at that point msix_is_masked() is false, or we really, really needs locking. Puttting a !foo, when we know that it needs to be an 1 looks strange. Later, Juan. PD. Yes, I already asked in a previous version to just have two methods, mask/unmask. we now at call time which one we need. I find msix_is_masked clearer here than true since you don't need to look up definition to understand what this 'true' stands for. The value is clear from code above. What do you think? I preffer the change, but it is up to you. at that point, we are using !msix_masked() to mean true i.e. we know that msix_masked() is false. What you want to do is mask. Later, Juan. Right. I guess I'll keep it as is, when I look at it with a fresh mind next time, I'll clean it all up. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] vhost-net: fix reversed logic in mask notifiers
Michael S. Tsirkin m...@redhat.com wrote: On Tue, May 25, 2010 at 04:58:15PM +0200, Juan Quintela wrote: Michael S. Tsirkin m...@redhat.com wrote: On Tue, May 25, 2010 at 04:37:36PM +0200, Juan Quintela wrote: we have: if (msix_is_masked()) return 0 r = msix_mask_notifier(., !msix_is_masked()); i.e. at that point msix_is_masked() is false, or we really, really needs locking. Puttting a !foo, when we know that it needs to be an 1 looks strange. Later, Juan. PD. Yes, I already asked in a previous version to just have two methods, mask/unmask. we now at call time which one we need. I find msix_is_masked clearer here than true since you don't need to look up definition to understand what this 'true' stands for. The value is clear from code above. What do you think? I preffer the change, but it is up to you. at that point, we are using !msix_masked() to mean true i.e. we know that msix_masked() is false. What you want to do is mask. Later, Juan. Right. I guess I'll keep it as is, when I look at it with a fresh mind next time, I'll clean it all up. ok with me. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
