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
Re: [PATCH] vhost-net: fix reversed logic in mask notifiers
"Michael S. Tsirkin" wrote: > On Tue, May 25, 2010 at 04:58:15PM +0200, Juan Quintela wrote: >> "Michael S. Tsirkin" 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
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" 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" 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 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. Tsirkin Reported-by: Amit Shah Acked-by: Gerd Hoffmann 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
On Tue, May 25, 2010 at 04:37:36PM +0200, Juan Quintela wrote:
> "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
> > Reported-by: Amit Shah
> > ---
> > 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
"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
> Reported-by: Amit Shah
> ---
> 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 [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 > Reported-by: Amit Shah Acked-by: Amit Shah 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
