[PATCH] xen: correctly check for pending events when restoring irq flags

[PATCH] xen: correctly check for pending events when restoring irq flags

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

In xen_restore_fl_direct(), xen_force_evtchn_callback() was being
called even if no events were pending.  This resulted in (depending on
workload) about a 100 times as many xen_version hypercalls as
necessary.

Fix this by correcting the sense of the conditional jump.

This seems to give a significant performance benefit for some
workloads.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
I'd prefer to do more testing and benchmarking but the gains appear to
be significant.  e.g., lmbench3's 'lat_proc fork' test has improved
from 404 us to 357 us (~11% faster).
---
 arch/x86/xen/xen-asm.S |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
index 79d7362..3e45aa0 100644
--- a/arch/x86/xen/xen-asm.S
+++ b/arch/x86/xen/xen-asm.S
@@ -96,7 +96,7 @@ ENTRY(xen_restore_fl_direct)
 
 	/* check for unmasked and pending */
 	cmpw $0x0001, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
-	jz 1f
+	jnz 1f
 2:	call check_events
 1:
 ENDPATCH(xen_restore_fl_direct)
-- 
1.7.2.5

Re: [PATCH] xen: correctly check for pending events when restoring irq flags

[Konrad Rzeszutek Wilk]

On Thu, Apr 26, 2012 at 07:44:06PM +0100, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> In xen_restore_fl_direct(), xen_force_evtchn_callback() was being
> called even if no events were pending.  This resulted in (depending on
> workload) about a 100 times as many xen_version hypercalls as
> necessary.
> 
> Fix this by correcting the sense of the conditional jump.
> 
> This seems to give a significant performance benefit for some
> workloads.
> 

Will put stable@kernel.org here as well.

Thx!
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
> I'd prefer to do more testing and benchmarking but the gains appear to
> be significant.  e.g., lmbench3's 'lat_proc fork' test has improved
> from 404 us to 357 us (~11% faster).
> ---
>  arch/x86/xen/xen-asm.S |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
> index 79d7362..3e45aa0 100644
> --- a/arch/x86/xen/xen-asm.S
> +++ b/arch/x86/xen/xen-asm.S
> @@ -96,7 +96,7 @@ ENTRY(xen_restore_fl_direct)
>  
>  	/* check for unmasked and pending */
>  	cmpw $0x0001, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
> -	jz 1f
> +	jnz 1f
>  2:	call check_events
>  1:
>  ENDPATCH(xen_restore_fl_direct)
> -- 
> 1.7.2.5

Re: [PATCH] xen: correctly check for pending events when restoring irq flags

[Ian Campbell]

On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> In xen_restore_fl_direct(), xen_force_evtchn_callback() was being
> called even if no events were pending.

In actual fact it seems that the callback was actually being called if
and only if no events were pending? Which makes me wonder how it used to
work at all!

> diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
> index 79d7362..3e45aa0 100644
> --- a/arch/x86/xen/xen-asm.S
> +++ b/arch/x86/xen/xen-asm.S
> @@ -96,7 +96,7 @@ ENTRY(xen_restore_fl_direct)
>  
>  	/* check for unmasked and pending */
>  	cmpw $0x0001, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
> -	jz 1f
> +	jnz 1f
>  2:	call check_events
>  1:

Took me a while, this is a bit tricksy (and it may well be too early for
me to be decoding it) since the check here is trying to check both
pending and masked in a single cmpw, but I think this is correct. It
will call check_events now only when the combined mask+pending word is
0x0001 (aka unmasked, pending).

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Does xen_irq_enable_direct have the same sort of issue? No, in that case
we are doing a straight forward test of pending without involving masked
(since it has just unmasked) and so jz is correct.

Ian.

Re: [PATCH] xen: correctly check for pending events when restoring irq flags

[David Vrabel]

On 27/04/12 09:47, Ian Campbell wrote:
> On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> In xen_restore_fl_direct(), xen_force_evtchn_callback() was being
>> called even if no events were pending.
> 
> In actual fact it seems that the callback was actually being called if
> and only if no events were pending?

Or if events are masked, but this wasn't as bad as it sounds as Xen
would not actually do the upcall.

> Which makes me wonder how it used to work at all!

Xen would have to raise an event during a local_save_flags() /
local_restore_flags() /and/ after missing the call to
xen_force_evtchn_callback(), the guest would have to do no more
hypercalls at all.  This is possible I guess but seems really unlikely
to me.

>> diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
>> index 79d7362..3e45aa0 100644
>> --- a/arch/x86/xen/xen-asm.S
>> +++ b/arch/x86/xen/xen-asm.S
>> @@ -96,7 +96,7 @@ ENTRY(xen_restore_fl_direct)
>>  
>>  	/* check for unmasked and pending */
>>  	cmpw $0x0001, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
>> -	jz 1f
>> +	jnz 1f
>>  2:	call check_events
>>  1:
> 
> Took me a while, this is a bit tricksy (and it may well be too early for
> me to be decoding it) since the check here is trying to check both
> pending and masked in a single cmpw, but I think this is correct. It
> will call check_events now only when the combined mask+pending word is
> 0x0001 (aka unmasked, pending).
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Does xen_irq_enable_direct have the same sort of issue? No, in that case
> we are doing a straight forward test of pending without involving masked
> (since it has just unmasked) and so jz is correct.

Thanks for the review.  This was a tricky one to pin down.

David

Re: [PATCH] xen: correctly check for pending events when restoring irq flags

[Jan Beulich]

>>> On 27.04.12 at 10:47, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>> 
>> In xen_restore_fl_direct(), xen_force_evtchn_callback() was being
>> called even if no events were pending.
> 
> In actual fact it seems that the callback was actually being called if
> and only if no events were pending? Which makes me wonder how it used to
> work at all!
> 
>> diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
>> index 79d7362..3e45aa0 100644
>> --- a/arch/x86/xen/xen-asm.S
>> +++ b/arch/x86/xen/xen-asm.S
>> @@ -96,7 +96,7 @@ ENTRY(xen_restore_fl_direct)
>>  
>>  	/* check for unmasked and pending */
>>  	cmpw $0x0001, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
>> -	jz 1f
>> +	jnz 1f
>>  2:	call check_events
>>  1:
> 
> Took me a while, this is a bit tricksy (and it may well be too early for
> me to be decoding it) since the check here is trying to check both
> pending and masked in a single cmpw, but I think this is correct. It
> will call check_events now only when the combined mask+pending word is
> 0x0001 (aka unmasked, pending).

It is _too much_ trickery, as it implies that the pending field, when set,
will always be 1. This is not sanctioned by the specification (quoting
the hypervisor's xen/include/public/xen.h):

     * 'evtchn_upcall_pending' is written non-zero by Xen to indicate
     * a pending notification for a particular VCPU. It is then cleared 
     * by the guest OS /before/ checking for pending work, thus avoiding

Note that it says "non-zero", not "1".

But yes, this isn't the fault of the patch here, so this is also not an
objection to this patch.

And yes, it can still be done with a single compare afaict, just not
directly on the memory operand.

Jan

> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Does xen_irq_enable_direct have the same sort of issue? No, in that case
> we are doing a straight forward test of pending without involving masked
> (since it has just unmasked) and so jz is correct.
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH] xen: correctly check for pending events when restoring irq flags

[Ian Campbell]

On Fri, 2012-04-27 at 12:42 +0100, Jan Beulich wrote:
> >>> On 27.04.12 at 10:47, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote:
> >> From: David Vrabel <david.vrabel@citrix.com>
> >> 
> >> In xen_restore_fl_direct(), xen_force_evtchn_callback() was being
> >> called even if no events were pending.
> > 
> > In actual fact it seems that the callback was actually being called if
> > and only if no events were pending? Which makes me wonder how it used to
> > work at all!
> > 
> >> diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
> >> index 79d7362..3e45aa0 100644
> >> --- a/arch/x86/xen/xen-asm.S
> >> +++ b/arch/x86/xen/xen-asm.S
> >> @@ -96,7 +96,7 @@ ENTRY(xen_restore_fl_direct)
> >>  
> >>  	/* check for unmasked and pending */
> >>  	cmpw $0x0001, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
> >> -	jz 1f
> >> +	jnz 1f
> >>  2:	call check_events
> >>  1:
> > 
> > Took me a while, this is a bit tricksy (and it may well be too early for
> > me to be decoding it) since the check here is trying to check both
> > pending and masked in a single cmpw, but I think this is correct. It
> > will call check_events now only when the combined mask+pending word is
> > 0x0001 (aka unmasked, pending).
> 
> It is _too much_ trickery, as it implies that the pending field, when set,
> will always be 1. This is not sanctioned by the specification (quoting
> the hypervisor's xen/include/public/xen.h):
> 
>      * 'evtchn_upcall_pending' is written non-zero by Xen to indicate
>      * a pending notification for a particular VCPU. It is then cleared 
>      * by the guest OS /before/ checking for pending work, thus avoiding
> 
> Note that it says "non-zero", not "1".

Hrm, has it ever not been 1 in practice? i.e. could we legitimately
tighten the spec?

> But yes, this isn't the fault of the patch here, so this is also not an
> objection to this patch.
> 
> And yes, it can still be done with a single compare afaict, just not
> directly on the memory operand.

Code size is also a concern here since this sequence of instructions is
used for inline patching (not sure what the size limit actually is
though).

Ian.

Re: [PATCH] xen: correctly check for pending events when restoring irq flags

[Jan Beulich]

>>> On 27.04.12 at 13:58, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2012-04-27 at 12:42 +0100, Jan Beulich wrote:
>> >>> On 27.04.12 at 10:47, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote:
>> >> From: David Vrabel <david.vrabel@citrix.com>
>> >> 
>> >> In xen_restore_fl_direct(), xen_force_evtchn_callback() was being
>> >> called even if no events were pending.
>> > 
>> > In actual fact it seems that the callback was actually being called if
>> > and only if no events were pending? Which makes me wonder how it used to
>> > work at all!
>> > 
>> >> diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
>> >> index 79d7362..3e45aa0 100644
>> >> --- a/arch/x86/xen/xen-asm.S
>> >> +++ b/arch/x86/xen/xen-asm.S
>> >> @@ -96,7 +96,7 @@ ENTRY(xen_restore_fl_direct)
>> >>  
>> >>  	/* check for unmasked and pending */
>> >>  	cmpw $0x0001, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
>> >> -	jz 1f
>> >> +	jnz 1f
>> >>  2:	call check_events
>> >>  1:
>> > 
>> > Took me a while, this is a bit tricksy (and it may well be too early for
>> > me to be decoding it) since the check here is trying to check both
>> > pending and masked in a single cmpw, but I think this is correct. It
>> > will call check_events now only when the combined mask+pending word is
>> > 0x0001 (aka unmasked, pending).
>> 
>> It is _too much_ trickery, as it implies that the pending field, when set,
>> will always be 1. This is not sanctioned by the specification (quoting
>> the hypervisor's xen/include/public/xen.h):
>> 
>>      * 'evtchn_upcall_pending' is written non-zero by Xen to indicate
>>      * a pending notification for a particular VCPU. It is then cleared 
>>      * by the guest OS /before/ checking for pending work, thus avoiding
>> 
>> Note that it says "non-zero", not "1".
> 
> Hrm, has it ever not been 1 in practice?

I don't think so.

> i.e. could we legitimately tighten the spec?

I wouldn't want to recommend this. In particular, we can't all of the
sudden keep guests from storing other non-zero values in here.

While I'm not in favor of this either, what we could do is specify that
Xen will only ever write 0 or 1 in here, while other non-zero values
are okay to be used by guests.

>> But yes, this isn't the fault of the patch here, so this is also not an
>> objection to this patch.
>> 
>> And yes, it can still be done with a single compare afaict, just not
>> directly on the memory operand.
> 
> Code size is also a concern here since this sequence of instructions is
> used for inline patching (not sure what the size limit actually is
> though).

Oh yes, didn't think of that aspect.

Jan

Re: [PATCH] xen: correctly check for pending events when restoring irq flags

[Ian Campbell]

On Fri, 2012-04-27 at 13:46 +0100, Jan Beulich wrote:
> >>> On 27.04.12 at 13:58, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Fri, 2012-04-27 at 12:42 +0100, Jan Beulich wrote:
> >> >>> On 27.04.12 at 10:47, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >> > On Thu, 2012-04-26 at 19:44 +0100, David Vrabel wrote:
> >> >> From: David Vrabel <david.vrabel@citrix.com>
> >> >> 
> >> >> In xen_restore_fl_direct(), xen_force_evtchn_callback() was being
> >> >> called even if no events were pending.
> >> > 
> >> > In actual fact it seems that the callback was actually being called if
> >> > and only if no events were pending? Which makes me wonder how it used to
> >> > work at all!
> >> > 
> >> >> diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
> >> >> index 79d7362..3e45aa0 100644
> >> >> --- a/arch/x86/xen/xen-asm.S
> >> >> +++ b/arch/x86/xen/xen-asm.S
> >> >> @@ -96,7 +96,7 @@ ENTRY(xen_restore_fl_direct)
> >> >>  
> >> >>  	/* check for unmasked and pending */
> >> >>  	cmpw $0x0001, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
> >> >> -	jz 1f
> >> >> +	jnz 1f
> >> >>  2:	call check_events
> >> >>  1:
> >> > 
> >> > Took me a while, this is a bit tricksy (and it may well be too early for
> >> > me to be decoding it) since the check here is trying to check both
> >> > pending and masked in a single cmpw, but I think this is correct. It
> >> > will call check_events now only when the combined mask+pending word is
> >> > 0x0001 (aka unmasked, pending).
> >> 
> >> It is _too much_ trickery, as it implies that the pending field, when set,
> >> will always be 1. This is not sanctioned by the specification (quoting
> >> the hypervisor's xen/include/public/xen.h):
> >> 
> >>      * 'evtchn_upcall_pending' is written non-zero by Xen to indicate
> >>      * a pending notification for a particular VCPU. It is then cleared 
> >>      * by the guest OS /before/ checking for pending work, thus avoiding
> >> 
> >> Note that it says "non-zero", not "1".
> > 
> > Hrm, has it ever not been 1 in practice?
> 
> I don't think so.
> 
> > i.e. could we legitimately tighten the spec?
> 
> I wouldn't want to recommend this. In particular, we can't all of the
> sudden keep guests from storing other non-zero values in here.

Could they be doing this? Whatever they put there is going to get
clobbered by Xen whenever it injects an event, isn't it? Or do you mean
that a guest can force an upcall by writing non-zero itself? Do guests
actually do that? (why?)

> While I'm not in favor of this either, what we could do is specify that
> Xen will only ever write 0 or 1 in here, while other non-zero values
> are okay to be used by guests.

Does Xen ever clear an upcall, isn't that always the guest? Xen only
ever writes 1, at least as far as I can see...

What do you think of the following?

diff -r 107285938c50 xen/include/public/xen.h
--- a/xen/include/public/xen.h	Thu Apr 26 10:03:08 2012 +0100
+++ b/xen/include/public/xen.h	Fri Apr 27 14:09:00 2012 +0100
@@ -559,16 +559,18 @@ typedef struct vcpu_time_info vcpu_time_
 
 struct vcpu_info {
     /*
-     * 'evtchn_upcall_pending' is written non-zero by Xen to indicate
-     * a pending notification for a particular VCPU. It is then cleared 
-     * by the guest OS /before/ checking for pending work, thus avoiding
-     * a set-and-check race. Note that the mask is only accessed by Xen
-     * on the CPU that is currently hosting the VCPU. This means that the
-     * pending and mask flags can be updated by the guest without special
-     * synchronisation (i.e., no need for the x86 LOCK prefix).
-     * This may seem suboptimal because if the pending flag is set by
-     * a different CPU then an IPI may be scheduled even when the mask
-     * is set. However, note:
+     * 'evtchn_upcall_pending' is written to '1' by Xen to indicate a
+     * pending notification for a particular VCPU. Xen will never
+     * write any other value but it is legitimate for a guest to store
+     * any other non-zero value here to trigger an upcall. It is then
+     * cleared by the guest OS /before/ checking for pending work,
+     * thus avoiding a set-and-check race. Note that the mask is only
+     * accessed by Xen on the CPU that is currently hosting the
+     * VCPU. This means that the pending and mask flags can be updated
+     * by the guest without special synchronisation (i.e., no need for
+     * the x86 LOCK prefix).  This may seem suboptimal because if the
+     * pending flag is set by a different CPU then an IPI may be
+     * scheduled even when the mask is set. However, note:
      *  1. The task of 'interrupt holdoff' is covered by the per-event-
      *     channel mask bits. A 'noisy' event that is continually being
      *     triggered can be masked at source at this very precise

Re: [PATCH] xen: correctly check for pending events when restoring irq flags

[Jan Beulich]

>>> On 27.04.12 at 15:09, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2012-04-27 at 13:46 +0100, Jan Beulich wrote:
>> >>> On 27.04.12 at 13:58, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Fri, 2012-04-27 at 12:42 +0100, Jan Beulich wrote:
>> >> >>> On 27.04.12 at 10:47, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > Hrm, has it ever not been 1 in practice?
>> 
>> I don't think so.
>> 
>> > i.e. could we legitimately tighten the spec?
>> 
>> I wouldn't want to recommend this. In particular, we can't all of the
>> sudden keep guests from storing other non-zero values in here.
> 
> Could they be doing this? Whatever they put there is going to get
> clobbered by Xen whenever it injects an event, isn't it? Or do you mean
> that a guest can force an upcall by writing non-zero itself? Do guests
> actually do that? (why?)

Yes, there is such a case even in Linux - see unmask_evtchn(). And
tightening an existing spec in ways that affect things we don't
control seems undesirable (and unnecessary here).

>> While I'm not in favor of this either, what we could do is specify that
>> Xen will only ever write 0 or 1 in here, while other non-zero values
>> are okay to be used by guests.
> 
> Does Xen ever clear an upcall, isn't that always the guest? Xen only
> ever writes 1, at least as far as I can see...

Oh, yes, you're right of course.

> What do you think of the following?

Reads okay.

Jan

> --- a/xen/include/public/xen.h	Thu Apr 26 10:03:08 2012 +0100
> +++ b/xen/include/public/xen.h	Fri Apr 27 14:09:00 2012 +0100
> @@ -559,16 +559,18 @@ typedef struct vcpu_time_info vcpu_time_
>  
>  struct vcpu_info {
>      /*
> -     * 'evtchn_upcall_pending' is written non-zero by Xen to indicate
> -     * a pending notification for a particular VCPU. It is then cleared 
> -     * by the guest OS /before/ checking for pending work, thus avoiding
> -     * a set-and-check race. Note that the mask is only accessed by Xen
> -     * on the CPU that is currently hosting the VCPU. This means that the
> -     * pending and mask flags can be updated by the guest without special
> -     * synchronisation (i.e., no need for the x86 LOCK prefix).
> -     * This may seem suboptimal because if the pending flag is set by
> -     * a different CPU then an IPI may be scheduled even when the mask
> -     * is set. However, note:
> +     * 'evtchn_upcall_pending' is written to '1' by Xen to indicate a
> +     * pending notification for a particular VCPU. Xen will never
> +     * write any other value but it is legitimate for a guest to store
> +     * any other non-zero value here to trigger an upcall. It is then
> +     * cleared by the guest OS /before/ checking for pending work,
> +     * thus avoiding a set-and-check race. Note that the mask is only
> +     * accessed by Xen on the CPU that is currently hosting the
> +     * VCPU. This means that the pending and mask flags can be updated
> +     * by the guest without special synchronisation (i.e., no need for
> +     * the x86 LOCK prefix).  This may seem suboptimal because if the
> +     * pending flag is set by a different CPU then an IPI may be
> +     * scheduled even when the mask is set. However, note:
>       *  1. The task of 'interrupt holdoff' is covered by the per-event-
>       *     channel mask bits. A 'noisy' event that is continually being
>       *     triggered can be masked at source at this very precise