[PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Kouya Shimura]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 328 bytes --]

Hi,

I've found that modern windows OS never use the PIT timer,
and neither cpu#0's LAPIC timer after boot.
Despite that, xen emulates them busily. It's inefficient.

Note: this patch ignores the IRQ mask of legacy i8259 since
rombios frequently modifies it.

Thanks,
Kouya

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>


[-- Attachment #2: freeze_vpt.patch --]
[-- Type: text/x-patch, Size: 5928 bytes --]

diff -r d2a32e24fe50 xen/arch/x86/hvm/i8254.c
--- a/xen/arch/x86/hvm/i8254.c	Wed Sep 09 16:39:41 2009 +0100
+++ b/xen/arch/x86/hvm/i8254.c	Thu Sep 10 09:55:29 2009 +0900
@@ -421,6 +421,8 @@ static int pit_load(struct domain *d, hv
 
     spin_unlock(&pit->lock);
 
+    pt_freeze_pit_timer_if_irqmasked(d);
+
     return 0;
 }
 
diff -r d2a32e24fe50 xen/arch/x86/hvm/vioapic.c
--- a/xen/arch/x86/hvm/vioapic.c	Wed Sep 09 16:39:41 2009 +0100
+++ b/xen/arch/x86/hvm/vioapic.c	Thu Sep 10 09:55:29 2009 +0900
@@ -158,6 +158,9 @@ static void vioapic_write_redirent(
         pent->fields.remote_irr = 1;
         vioapic_deliver(vioapic, idx);
     }
+
+    if ( idx == hvm_isa_irq_to_gsi(0) && !top_word )
+        pt_freeze_pit_timer_if_irqmasked(d);
 
     spin_unlock(&d->arch.hvm_domain.irq_lock);
 }
diff -r d2a32e24fe50 xen/arch/x86/hvm/vlapic.c
--- a/xen/arch/x86/hvm/vlapic.c	Wed Sep 09 16:39:41 2009 +0100
+++ b/xen/arch/x86/hvm/vlapic.c	Thu Sep 10 09:55:29 2009 +0900
@@ -655,6 +655,8 @@ static int vlapic_write(struct vcpu *v, 
         vlapic_set_reg(vlapic, offset, val);
         if ( offset == APIC_LVT0 )
             vlapic_adjust_i8259_target(v->domain);
+        if ( offset == APIC_LVTT )
+            pt_freeze_lapic_timer_if_irqmasked(&vlapic->pt);
         break;
 
     case APIC_TMICT:
@@ -770,10 +772,13 @@ void vlapic_adjust_i8259_target(struct d
     v = d->vcpu ? d->vcpu[0] : NULL;
 
  found:
-    if ( d->arch.hvm_domain.i8259_target == v )
-        return;
-    d->arch.hvm_domain.i8259_target = v;
-    pt_adjust_global_vcpu_target(v);
+    if ( d->arch.hvm_domain.i8259_target != v )
+    {
+        d->arch.hvm_domain.i8259_target = v;
+        pt_adjust_global_vcpu_target(v);
+    }
+
+    pt_freeze_pit_timer_if_irqmasked(d);
 }
 
 int vlapic_has_pending_irq(struct vcpu *v)
@@ -934,6 +939,7 @@ static int lapic_load_regs(struct domain
 
     vlapic_adjust_i8259_target(d);
     lapic_rearm(s);
+    pt_freeze_lapic_timer_if_irqmasked(&s->pt);
     return 0;
 }
 
diff -r d2a32e24fe50 xen/arch/x86/hvm/vpt.c
--- a/xen/arch/x86/hvm/vpt.c	Wed Sep 09 16:39:41 2009 +0100
+++ b/xen/arch/x86/hvm/vpt.c	Thu Sep 10 09:55:29 2009 +0900
@@ -211,6 +211,10 @@ static void pt_timer_fn(void *data)
         pt_process_missed_ticks(pt);
         set_timer(&pt->timer, pt->scheduled);
     }
+    else
+    {
+        pt->frozen_by_irqmask = 0;
+    }
 
     if ( !pt_irq_masked(pt) )
         vcpu_kick(pt->vcpu);
@@ -394,6 +398,7 @@ void create_periodic_time(
 
     pt->on_list = 1;
     list_add(&pt->list, &v->arch.hvm_vcpu.tm_list);
+    pt->frozen_by_irqmask = 0;
 
     init_timer(&pt->timer, pt_timer_fn, pt, v->processor);
     set_timer(&pt->timer, pt->scheduled);
@@ -411,6 +416,7 @@ void destroy_periodic_time(struct period
     if ( pt->on_list )
         list_del(&pt->list);
     pt->on_list = 0;
+    pt->frozen_by_irqmask = 0;
     pt_unlock(pt);
 
     /*
@@ -469,3 +475,75 @@ void pt_adjust_global_vcpu_target(struct
         pt_adjust_vcpu(&pl_time->vhpet.pt[i], v);
     spin_unlock(&pl_time->vhpet.lock);
 }
+
+
+static void pt_freeze_timer(struct periodic_time *pt)
+{
+    struct vcpu *v = NULL;
+
+    pt_lock(pt);
+    if ( pt->on_list )
+    {
+        v = pt->vcpu;
+        pt->frozen_by_irqmask = 1;
+        pt->on_list = 0;
+        list_del(&pt->list);
+        stop_timer(&pt->timer);
+    }
+    pt_unlock(pt);
+
+    if ( v )
+        gdprintk(XENLOG_INFO, "vcpu%d freeze %s timer(irq=%d)\n", v->vcpu_id,
+                 pt->source == PTSRC_lapic ? "LAPIC" : "ISA", pt->irq);
+}
+
+static void pt_thaw_timer(struct periodic_time *pt)
+{
+    struct vcpu *v = NULL;
+
+    pt_lock(pt);
+    if ( !pt->on_list && pt->frozen_by_irqmask )
+    {
+        v = pt->vcpu;
+        pt->on_list = 1;
+        list_add(&pt->list, &v->arch.hvm_vcpu.tm_list);
+        migrate_timer(&pt->timer, v->processor);
+        /*
+         * It may set the past time, however, TIMER_SOFTIRQ is raised
+         * and pt_timer_fn() will reset the timer appropriately.
+         */
+        set_timer(&pt->timer, pt->scheduled);
+    }
+    pt->frozen_by_irqmask = 0;
+    pt_unlock(pt);
+
+    if ( v )
+        gdprintk(XENLOG_INFO, "vcpu%d thaw %s timer(irq=%d)\n", v->vcpu_id,
+                 pt->source == PTSRC_lapic ? "LAPIC" : "ISA", pt->irq);
+}
+
+void pt_freeze_pit_timer_if_irqmasked(struct domain *d)
+{
+    struct periodic_time *pt = &d->arch.hvm_domain.pl_time.vpit.pt0;
+    unsigned int gsi = hvm_isa_irq_to_gsi(pt->irq);
+
+    if ( pt->vcpu == NULL )
+        return;
+
+    if ( !vlapic_accept_pic_intr(pt->vcpu) &&
+         domain_vioapic(d)->redirtbl[gsi].fields.mask )
+        pt_freeze_timer(pt);
+    else
+        pt_thaw_timer(pt);
+}
+
+void pt_freeze_lapic_timer_if_irqmasked(struct periodic_time *pt)
+{
+    if ( pt->vcpu == NULL )
+        return;
+
+    if (vlapic_get_reg(vcpu_vlapic(pt->vcpu), APIC_LVTT) & APIC_LVT_MASKED)
+        pt_freeze_timer(pt);
+    else
+        pt_thaw_timer(pt);
+}
diff -r d2a32e24fe50 xen/include/asm-x86/hvm/vpt.h
--- a/xen/include/asm-x86/hvm/vpt.h	Wed Sep 09 16:39:41 2009 +0100
+++ b/xen/include/asm-x86/hvm/vpt.h	Thu Sep 10 09:55:29 2009 +0900
@@ -44,6 +44,7 @@ struct periodic_time {
     bool_t do_not_freeze;
     bool_t irq_issued;
     bool_t warned_timeout_too_short;
+    bool_t frozen_by_irqmask;
 #define PTSRC_isa    1 /* ISA time source */
 #define PTSRC_lapic  2 /* LAPIC time source */
     u8 source;                  /* PTSRC_ */
@@ -145,7 +146,10 @@ void pt_adjust_global_vcpu_target(struct
     ((d)->arch.hvm_domain.i8259_target ? : (d)->vcpu ? (d)->vcpu[0] : NULL)
 
 /* Is given periodic timer active? */
-#define pt_active(pt) ((pt)->on_list)
+#define pt_active(pt) ((pt)->on_list || (pt)->frozen_by_irqmask)
+
+void pt_freeze_pit_timer_if_irqmasked(struct domain *d);
+void pt_freeze_lapic_timer_if_irqmasked(struct periodic_time *pt);
 
 /*
  * Create/destroy a periodic (or one-shot!) timer.

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Keir Fraser]

Although I can see the sense here, the patch makes me a bit uneasy. Also I
think neater code would result from simply making vpt.c's handling of
periodic timers less stupid. By this I mean that calling set_timer() from
the timer handler is not really necessary -- you already know an interrupt
is now pending and the vcpu is kicked. May as well not set_timer() until the
pending interrupt(s) are delivered. That would then effectively get your
optimisation "for free", wouldn't it?

 -- Keir

On 10/09/2009 06:47, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:

> Hi,
> 
> I've found that modern windows OS never use the PIT timer,
> and neither cpu#0's LAPIC timer after boot.
> Despite that, xen emulates them busily. It's inefficient.
> 
> Note: this patch ignores the IRQ mask of legacy i8259 since
> rombios frequently modifies it.
> 
> Thanks,
> Kouya
> 
> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
> 

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Kouya Shimura]

Well, this patch has two aims.  One is to stop the xen's timer by
stop_timer(). And another is to simplify pt_update_irq() which
searches the earliest platform timer.

To answer the latter aim, this patch gets rid of unused (masked
by IRQ) platform timer from vcpu->arch.hvm_vcpu.tm_list.
Your suggestion satisfies the first but the latter, I think.
Besides, pt_update_irq() is more critical since it is called on
every VM_EXIT.

Although I understand your uneasiness, is there any other way?

Thanks,
Kouya

Keir Fraser writes:
> Although I can see the sense here, the patch makes me a bit uneasy. Also I
> think neater code would result from simply making vpt.c's handling of
> periodic timers less stupid. By this I mean that calling set_timer() from
> the timer handler is not really necessary -- you already know an interrupt
> is now pending and the vcpu is kicked. May as well not set_timer() until the
> pending interrupt(s) are delivered. That would then effectively get your
> optimisation "for free", wouldn't it?
> 
>  -- Keir
> 
> On 10/09/2009 06:47, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:
> 
> > Hi,
> > 
> > I've found that modern windows OS never use the PIT timer,
> > and neither cpu#0's LAPIC timer after boot.
> > Despite that, xen emulates them busily. It's inefficient.
> > 
> > Note: this patch ignores the IRQ mask of legacy i8259 since
> > rombios frequently modifies it.
> > 
> > Thanks,
> > Kouya
> > 
> > Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
> > 
> 

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Keir Fraser]

Do you get a measurable performance win from this patch?

Not using pt_irq_masked() seems odd. Is it to avoid 859 IRQ mask checks, and
if so why are you doing that? Why do we care that rombios twiddles the 8259
masks a bunch during boot?

 -- Keir

On 10/09/2009 09:22, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:

> Well, this patch has two aims.  One is to stop the xen's timer by
> stop_timer(). And another is to simplify pt_update_irq() which
> searches the earliest platform timer.
> 
> To answer the latter aim, this patch gets rid of unused (masked
> by IRQ) platform timer from vcpu->arch.hvm_vcpu.tm_list.
> Your suggestion satisfies the first but the latter, I think.
> Besides, pt_update_irq() is more critical since it is called on
> every VM_EXIT.
> 
> Although I understand your uneasiness, is there any other way?
> 
> Thanks,
> Kouya
> 
> Keir Fraser writes:
>> Although I can see the sense here, the patch makes me a bit uneasy. Also I
>> think neater code would result from simply making vpt.c's handling of
>> periodic timers less stupid. By this I mean that calling set_timer() from
>> the timer handler is not really necessary -- you already know an interrupt
>> is now pending and the vcpu is kicked. May as well not set_timer() until the
>> pending interrupt(s) are delivered. That would then effectively get your
>> optimisation "for free", wouldn't it?
>> 
>>  -- Keir
>> 
>> On 10/09/2009 06:47, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:
>> 
>>> Hi,
>>> 
>>> I've found that modern windows OS never use the PIT timer,
>>> and neither cpu#0's LAPIC timer after boot.
>>> Despite that, xen emulates them busily. It's inefficient.
>>> 
>>> Note: this patch ignores the IRQ mask of legacy i8259 since
>>> rombios frequently modifies it.
>>> 
>>> Thanks,
>>> Kouya
>>> 
>>> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
>>> 
>> 

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Kouya Shimura]

Keir Fraser writes:
> Do you get a measurable performance win from this patch?

Sorry, I don't measure yet. Probably a little bit win.

> Not using pt_irq_masked() seems odd. Is it to avoid 859 IRQ mask checks, and
> if so why are you doing that? Why do we care that rombios twiddles the 8259
> masks a bunch during boot?

I guessed that the cost of freezing/thawing a timer with the 8259 mask
is higher than timer emulation.  And the PIT timer is actually active
while rombios masks the 8259.

If using pt_irq_masked() is desirable, I'll remake it.

Thanks,
Kouya

> 
>  -- Keir
> 
> On 10/09/2009 09:22, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:
> 
> > Well, this patch has two aims.  One is to stop the xen's timer by
> > stop_timer(). And another is to simplify pt_update_irq() which
> > searches the earliest platform timer.
> > 
> > To answer the latter aim, this patch gets rid of unused (masked
> > by IRQ) platform timer from vcpu->arch.hvm_vcpu.tm_list.
> > Your suggestion satisfies the first but the latter, I think.
> > Besides, pt_update_irq() is more critical since it is called on
> > every VM_EXIT.
> > 
> > Although I understand your uneasiness, is there any other way?
> > 
> > Thanks,
> > Kouya
> > 
> > Keir Fraser writes:
> >> Although I can see the sense here, the patch makes me a bit uneasy. Also I
> >> think neater code would result from simply making vpt.c's handling of
> >> periodic timers less stupid. By this I mean that calling set_timer() from
> >> the timer handler is not really necessary -- you already know an interrupt
> >> is now pending and the vcpu is kicked. May as well not set_timer() until the
> >> pending interrupt(s) are delivered. That would then effectively get your
> >> optimisation "for free", wouldn't it?
> >> 
> >>  -- Keir
> >> 
> >> On 10/09/2009 06:47, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:
> >> 
> >>> Hi,
> >>> 
> >>> I've found that modern windows OS never use the PIT timer,
> >>> and neither cpu#0's LAPIC timer after boot.
> >>> Despite that, xen emulates them busily. It's inefficient.
> >>> 
> >>> Note: this patch ignores the IRQ mask of legacy i8259 since
> >>> rombios frequently modifies it.
> >>> 
> >>> Thanks,
> >>> Kouya
> >>> 
> >>> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
> >>> 
> >> 
> 

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Keir Fraser]

On 10/09/2009 10:54, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:

>> Not using pt_irq_masked() seems odd. Is it to avoid 859 IRQ mask checks, and
>> if so why are you doing that? Why do we care that rombios twiddles the 8259
>> masks a bunch during boot?
> 
> I guessed that the cost of freezing/thawing a timer with the 8259 mask
> is higher than timer emulation.  And the PIT timer is actually active
> while rombios masks the 8259.
> 
> If using pt_irq_masked() is desirable, I'll remake it.

If your patch works without even any theoretical lost of correctness, then
that makes it more attractive, obviously.

Some evidence that the cost saving is at all measurable would also be good.
Premature optimisation being evil, etc. ;-)

 -- Keir

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Konrad Rzeszutek Wilk]

On Thu, Sep 10, 2009 at 02:47:22PM +0900, Kouya Shimura wrote:
Content-Description: message body text
> Hi,
> 
> I've found that modern windows OS never use the PIT timer,

What about older Linux versions? Like RHEL3?

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Kouya Shimura]

Konrad Rzeszutek Wilk writes:
> On Thu, Sep 10, 2009 at 02:47:22PM +0900, Kouya Shimura wrote:
> Content-Description: message body text
> > Hi,
> > 
> > I've found that modern windows OS never use the PIT timer,
> 
> What about older Linux versions? Like RHEL3?

older Linux uses the PIT timer. Although I don't know about RHEL3,
RHEL5 uses the PIT timer actually.

Any way, my patch doesn't give a benefit to Linux.  For example,
linux-2.6.28 (ubuntu 9.04) stops the PIT timer by itself.

Thanks,
Kouya

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Kouya Shimura]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1194 bytes --]

I remade the patch and measured the performance win. Attached is
a benchmark program which I wrote. It is complied by cygwin's gcc
by -O2 and runs on Windows XP(32bit). And my cpu is 
Intel Core2 Quad Q9450@2.66GHz.

The result is that my patch saves 32 cycles(TSC) per one VM_EXIT(cpuid).
(2696 tsc => 2664 tsc)

The patch is split to two. I'll post them in another mail.

Thanks,
Kouya

Keir Fraser writes:
> On 10/09/2009 10:54, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:
> 
> >> Not using pt_irq_masked() seems odd. Is it to avoid 859 IRQ mask checks, and
> >> if so why are you doing that? Why do we care that rombios twiddles the 8259
> >> masks a bunch during boot?
> > 
> > I guessed that the cost of freezing/thawing a timer with the 8259 mask
> > is higher than timer emulation.  And the PIT timer is actually active
> > while rombios masks the 8259.
> > 
> > If using pt_irq_masked() is desirable, I'll remake it.
> 
> If your patch works without even any theoretical lost of correctness, then
> that makes it more attractive, obviously.
> 
> Some evidence that the cost saving is at all measurable would also be good.
> Premature optimisation being evil, etc. ;-)
> 
>  -- Keir


[-- Attachment #2: cpuidbench.c --]
[-- Type: text/plain, Size: 368 bytes --]

#include <stdio.h>
#include <stdint.h>

#define rdtsc(val) asm volatile("rdtsc" : "=a" (val))

static void cpuid(uint32_t idx)
{
    uint32_t a,b,c,d;
    asm volatile("cpuid": "=a"(a), "=b"(b), "=c"(c), "=d"(d) : "0"(idx) );
}

main()
{
    uint32_t tsc1, tsc2;
    while(1) {
	rdtsc(tsc1);
	cpuid(0x40000000);
	rdtsc(tsc2);
	printf("tsc=%d\n", tsc2 - tsc1);
    }
}

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 1/2] x86 hvm: don't set periodical timer again until its IRQ is delivered.

[Kouya Shimura]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 536 bytes --]

Modern Windows OS (ex XP,2003,2008) never use the PIT timer,
and neither cpu#0's LAPIC timer after boot.
Despite that, xen emulates them busily. It's inefficient.

With this patch, setting a timer is defered while its IRQ is masked.

The reasons why pt_timer_fn() simply calls vcpu_kick() are:
- checking by pt_irq_masked() is duplicated. pt_update_irq() also does.
- pt_timer_fn() is likely called on the same processor
  as pt->vcpu->processor. Hence vcpu_kick() hardly send IPI.

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>


[-- Attachment #2: vpt1.patch --]
[-- Type: text/x-patch, Size: 2364 bytes --]

# HG changeset patch
# User Kouya Shimura <kouya@jp.fujitsu.com>
# Date 1253081156 -32400
# Node ID 864f20763d03ef989501fbc43d74034027679a56
# Parent  d2a32e24fe504b9626e6732b4f213c7cb1bc8b57
x86 hvm: don't set periodical timer again until its IRQ is delivered.

Modern Windows OS (ex XP,2003,2008) never use the PIT timer,
and neither cpu#0's LAPIC timer after boot.
Despite that, xen emulates them busily. It's inefficient.

With this patch, setting a timer is defered while its IRQ is masked.

The reasons why pt_timer_fn() simply calls vcpu_kick() are:
- checking by pt_irq_masked() is duplicated. pt_update_irq() also does.
- pt_timer_fn() is likely called on the same processor
  as pt->vcpu->processor. Hence vcpu_kick() hardly send IPI.

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>

diff -r d2a32e24fe50 -r 864f20763d03 xen/arch/x86/hvm/vpt.c
--- a/xen/arch/x86/hvm/vpt.c	Wed Sep 09 16:39:41 2009 +0100
+++ b/xen/arch/x86/hvm/vpt.c	Wed Sep 16 15:05:56 2009 +0900
@@ -187,8 +187,11 @@ void pt_restore_timer(struct vcpu *v)
 
     list_for_each_entry ( pt, head, list )
     {
-        pt_process_missed_ticks(pt);
-        set_timer(&pt->timer, pt->scheduled);
+        if ( pt->pending_intr_nr == 0 )
+        {
+            pt_process_missed_ticks(pt);
+            set_timer(&pt->timer, pt->scheduled);
+        }
     }
 
     pt_thaw_time(v);
@@ -205,15 +208,7 @@ static void pt_timer_fn(void *data)
     pt->pending_intr_nr++;
     pt->do_not_freeze = 0;
 
-    if ( !pt->one_shot )
-    {
-        pt->scheduled += pt->period;
-        pt_process_missed_ticks(pt);
-        set_timer(&pt->timer, pt->scheduled);
-    }
-
-    if ( !pt_irq_masked(pt) )
-        vcpu_kick(pt->vcpu);
+    vcpu_kick(pt->vcpu);
 
     pt_unlock(pt);
 }
@@ -302,6 +297,9 @@ void pt_intr_post(struct vcpu *v, struct
     }
     else
     {
+        pt->scheduled += pt->period;
+        pt_process_missed_ticks(pt);
+
         if ( mode_is(v->domain, one_missed_tick_pending) ||
              mode_is(v->domain, no_missed_ticks_pending) )
         {
@@ -313,6 +311,9 @@ void pt_intr_post(struct vcpu *v, struct
             pt->last_plt_gtime += pt->period;
             pt->pending_intr_nr--;
         }
+
+        if ( pt->pending_intr_nr == 0 )
+            set_timer(&pt->timer, pt->scheduled);
     }
 
     if ( mode_is(v->domain, delay_for_missed_ticks) &&

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 2/2] x86 hvm: suspend platform timer emulation while its IRQ is masked

[Kouya Shimura]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 904 bytes --]

This patch gets rid of a timer which IRQ is masked from vcpu's timer list.
It reduces the overhead of VM EXIT and context switch of vm.

Also fixes a potential bug.
(1) VCPU#0: mask the IRQ of a timer. (ex. vioapic.redir[2].mask=1)
(2) VCPU#1: pt_timer_fn() is invoked by expiration of the timer.
(3) VCPU#1: pt_update_irq() is called but does nothing by pt_irq_masked()=1.
(4) VCPU#1: sleep by halt.
(5) VCPU#0: unmask the IRQ of the timer.
After that, no one wakes up the VCPU#1.

IRQ of ISA is masked by:
 - PIC's IMR
 - IOAPIC's redir[0]
 - IOAPIC's redir[N].mask
 - LAPIC's LVT0
 - LAPIC enabled/disabled

IRQ of LAPIC timer is masked by:
 - LAPIC's LVTT
 - LAPIC disabled

When above stuffs are changed, the corresponding vcpu is kicked and
suspended timer emulation is resumed.

In addition, a small bug fix in pt_adjust_global_vcpu_target().

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>


[-- Attachment #2: vpt2.patch --]
[-- Type: text/x-patch, Size: 8310 bytes --]

# HG changeset patch
# User Kouya Shimura <kouya@jp.fujitsu.com>
# Date 1253080328 -32400
# Node ID b62a5e4c6125e62fd5fd1e28f7ae1c8a093b689d
# Parent  58ef1439ca982cdd2f7e4698ce9bdba1f4b38853
x86 hvm: suspend platform timer emulation while its IRQ is masked

This patch gets rid of a timer which IRQ is masked from vcpu's timer list.
It reduces the overhead of VM EXIT and context switch of vm.

Also fixes a potential bug.
(1) VCPU#0: mask the IRQ of a timer. (ex. vioapic.redir[2].mask=1)
(2) VCPU#1: pt_timer_fn() is invoked by expiration of the timer.
(3) VCPU#1: pt_update_irq() is called but does nothing by pt_irq_masked()=1.
(4) VCPU#1: sleep by halt.
(5) VCPU#0: unmask the IRQ of the timer.
After that, no one wakes up the VCPU#1.

IRQ of ISA is masked by:
 - PIC's IMR
 - IOAPIC's redir[0]
 - IOAPIC's redir[N].mask
 - LAPIC's LVT0
 - LAPIC enabled/disabled

IRQ of LAPIC timer is masked by:
 - LAPIC's LVTT
 - LAPIC disabled

When above stuffs are changed, the corresponding vcpu is kicked and
suspended timer emulation is resumed.

In addition, a small bug fix in pt_adjust_global_vcpu_target().

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>

diff -r 58ef1439ca98 -r b62a5e4c6125 xen/arch/x86/hvm/vioapic.c
--- a/xen/arch/x86/hvm/vioapic.c	Tue Sep 15 10:08:12 2009 +0100
+++ b/xen/arch/x86/hvm/vioapic.c	Wed Sep 16 14:52:08 2009 +0900
@@ -125,6 +125,7 @@ static void vioapic_write_redirent(
     struct domain *d = vioapic_domain(vioapic);
     struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq;
     union vioapic_redir_entry *pent, ent;
+    int unmasked = 0;
 
     spin_lock(&d->arch.hvm_domain.irq_lock);
 
@@ -138,10 +139,12 @@ static void vioapic_write_redirent(
     }
     else
     {
+        unmasked = ent.fields.mask;
         /* Remote IRR and Delivery Status are read-only. */
         ent.bits = ((ent.bits >> 32) << 32) | val;
         ent.fields.delivery_status = 0;
         ent.fields.remote_irr = pent->fields.remote_irr;
+        unmasked = unmasked && !ent.fields.mask;
     }
 
     *pent = ent;
@@ -160,6 +163,9 @@ static void vioapic_write_redirent(
     }
 
     spin_unlock(&d->arch.hvm_domain.irq_lock);
+
+    if ( idx == 0 || unmasked )
+        pt_may_unmask_irq(d, NULL);
 }
 
 static void vioapic_write_indirect(
diff -r 58ef1439ca98 -r b62a5e4c6125 xen/arch/x86/hvm/vlapic.c
--- a/xen/arch/x86/hvm/vlapic.c	Tue Sep 15 10:08:12 2009 +0100
+++ b/xen/arch/x86/hvm/vlapic.c	Wed Sep 16 14:52:08 2009 +0900
@@ -624,7 +624,10 @@ static int vlapic_write(struct vcpu *v, 
             }
         }
         else
+        {
             vlapic->hw.disabled &= ~VLAPIC_SW_DISABLED;
+            pt_may_unmask_irq(vlapic_domain(vlapic), &vlapic->pt);
+        }
         break;
 
     case APIC_ESR:
@@ -654,7 +657,12 @@ static int vlapic_write(struct vcpu *v, 
         val &= vlapic_lvt_mask[(offset - APIC_LVTT) >> 4];
         vlapic_set_reg(vlapic, offset, val);
         if ( offset == APIC_LVT0 )
+        {
             vlapic_adjust_i8259_target(v->domain);
+            pt_may_unmask_irq(v->domain, NULL);
+        }
+        if ( (offset == APIC_LVTT) && !(val & APIC_LVT_MASKED) )
+            pt_may_unmask_irq(NULL, &vlapic->pt);
         break;
 
     case APIC_TMICT:
@@ -719,10 +727,12 @@ void vlapic_msr_set(struct vlapic *vlapi
         {
             vlapic_reset(vlapic);
             vlapic->hw.disabled &= ~VLAPIC_HW_DISABLED;
+            pt_may_unmask_irq(vlapic_domain(vlapic), &vlapic->pt);
         }
         else
         {
             vlapic->hw.disabled |= VLAPIC_HW_DISABLED;
+            pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
         }
     }
 
diff -r 58ef1439ca98 -r b62a5e4c6125 xen/arch/x86/hvm/vpic.c
--- a/xen/arch/x86/hvm/vpic.c	Tue Sep 15 10:08:12 2009 +0100
+++ b/xen/arch/x86/hvm/vpic.c	Wed Sep 16 14:52:08 2009 +0900
@@ -178,7 +178,7 @@ static void vpic_ioport_write(
     struct hvm_hw_vpic *vpic, uint32_t addr, uint32_t val)
 {
     int priority, cmd, irq;
-    uint8_t mask;
+    uint8_t mask, unmasked = 0;
 
     vpic_lock(vpic);
 
@@ -190,6 +190,7 @@ static void vpic_ioport_write(
             /* Clear edge-sensing logic. */
             vpic->irr &= vpic->elcr;
 
+            unmasked = vpic->imr;
             /* No interrupts masked or in service. */
             vpic->imr = vpic->isr = 0;
 
@@ -268,6 +269,7 @@ static void vpic_ioport_write(
         {
         case 0:
             /* OCW1 */
+            unmasked = vpic->imr & (~val);
             vpic->imr = val;
             break;
         case 1:
@@ -295,6 +297,9 @@ static void vpic_ioport_write(
     vpic_update_int_output(vpic);
 
     vpic_unlock(vpic);
+
+    if ( unmasked )
+        pt_may_unmask_irq(vpic_domain(vpic), NULL);
 }
 
 static uint32_t vpic_ioport_read(struct hvm_hw_vpic *vpic, uint32_t addr)
diff -r 58ef1439ca98 -r b62a5e4c6125 xen/arch/x86/hvm/vpt.c
--- a/xen/arch/x86/hvm/vpt.c	Tue Sep 15 10:08:12 2009 +0100
+++ b/xen/arch/x86/hvm/vpt.c	Wed Sep 16 14:52:08 2009 +0900
@@ -221,19 +221,30 @@ void pt_update_irq(struct vcpu *v)
 void pt_update_irq(struct vcpu *v)
 {
     struct list_head *head = &v->arch.hvm_vcpu.tm_list;
-    struct periodic_time *pt, *earliest_pt = NULL;
+    struct periodic_time *pt, *temp, *earliest_pt = NULL;
     uint64_t max_lag = -1ULL;
     int irq, is_lapic;
 
     spin_lock(&v->arch.hvm_vcpu.tm_lock);
 
-    list_for_each_entry ( pt, head, list )
+    list_for_each_entry_safe ( pt, temp, head, list )
     {
-        if ( pt->pending_intr_nr && !pt_irq_masked(pt) &&
-             ((pt->last_plt_gtime + pt->period) < max_lag) )
+        if ( pt->pending_intr_nr )
         {
-            max_lag = pt->last_plt_gtime + pt->period;
-            earliest_pt = pt;
+            if ( pt_irq_masked(pt) )
+            {
+                /* suspend timer emulation */
+                list_del(&pt->list);
+                pt->on_list = 0;
+            }
+            else
+            {
+                if ( (pt->last_plt_gtime + pt->period) < max_lag )
+                {
+                    max_lag = pt->last_plt_gtime + pt->period;
+                    earliest_pt = pt;
+                }
+            }
         }
     }
 
@@ -411,6 +422,7 @@ void destroy_periodic_time(struct period
     if ( pt->on_list )
         list_del(&pt->list);
     pt->on_list = 0;
+    pt->pending_intr_nr = 0;
     pt_unlock(pt);
 
     /*
@@ -450,11 +462,13 @@ static void pt_adjust_vcpu(struct period
 
 void pt_adjust_global_vcpu_target(struct vcpu *v)
 {
-    struct pl_time *pl_time = &v->domain->arch.hvm_domain.pl_time;
+    struct pl_time *pl_time;
     int i;
 
     if ( v == NULL )
         return;
+
+    pl_time = &v->domain->arch.hvm_domain.pl_time;
 
     spin_lock(&pl_time->vpit.lock);
     pt_adjust_vcpu(&pl_time->vpit.pt0, v);
@@ -469,3 +483,35 @@ void pt_adjust_global_vcpu_target(struct
         pt_adjust_vcpu(&pl_time->vhpet.pt[i], v);
     spin_unlock(&pl_time->vhpet.lock);
 }
+
+
+static void pt_resume(struct periodic_time *pt)
+{
+    if ( pt->vcpu == NULL )
+        return;
+
+    pt_lock(pt);
+    if ( pt->pending_intr_nr && !pt->on_list )
+    {
+        pt->on_list = 1;
+        list_add(&pt->list, &pt->vcpu->arch.hvm_vcpu.tm_list);
+        vcpu_kick(pt->vcpu);
+    }
+    pt_unlock(pt);
+}
+
+void pt_may_unmask_irq(struct domain *d, struct periodic_time *vlapic_pt)
+{
+    int i;
+
+    if ( d )
+    {
+        pt_resume(&d->arch.hvm_domain.pl_time.vpit.pt0);
+        pt_resume(&d->arch.hvm_domain.pl_time.vrtc.pt);
+        for ( i = 0; i < HPET_TIMER_NUM; i++ )
+            pt_resume(&d->arch.hvm_domain.pl_time.vhpet.pt[i]);
+    }
+
+    if ( vlapic_pt )
+        pt_resume(vlapic_pt);
+}
diff -r 58ef1439ca98 -r b62a5e4c6125 xen/include/asm-x86/hvm/vpt.h
--- a/xen/include/asm-x86/hvm/vpt.h	Tue Sep 15 10:08:12 2009 +0100
+++ b/xen/include/asm-x86/hvm/vpt.h	Wed Sep 16 14:52:08 2009 +0900
@@ -144,8 +144,10 @@ void pt_adjust_global_vcpu_target(struct
 #define pt_global_vcpu_target(d) \
     ((d)->arch.hvm_domain.i8259_target ? : (d)->vcpu ? (d)->vcpu[0] : NULL)
 
+void pt_may_unmask_irq(struct domain *d, struct periodic_time *vlapic_pt);
+
 /* Is given periodic timer active? */
-#define pt_active(pt) ((pt)->on_list)
+#define pt_active(pt) ((pt)->on_list || (pt)->pending_intr_nr)
 
 /*
  * Create/destroy a periodic (or one-shot!) timer.

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Keir Fraser]

On 16/09/2009 07:46, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:

> I remade the patch and measured the performance win. Attached is
> a benchmark program which I wrote. It is complied by cygwin's gcc
> by -O2 and runs on Windows XP(32bit). And my cpu is
> Intel Core2 Quad Q9450@2.66GHz.
> 
> The result is that my patch saves 32 cycles(TSC) per one VM_EXIT(cpuid).
> (2696 tsc => 2664 tsc)
> 
> The patch is split to two. I'll post them in another mail.

That's really not enough of a win to bother with, is it.

 -- Keir

RE: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Cui, Dexuan]

Looks the little win doesn't deserve the increased complexity in code.

BTW, recent Intel CPUs run much faster with respect to VMEntry/VMExit and VMREAD/VMWRITE, so I don't think the SW optimizatin is appearling here. :-)

Thanks,
-- Dexuan



-----Original Message-----
From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
Sent: 2009?9?16? 15:41
To: Kouya Shimura
Cc: xen-devel@lists.xensource.com
Subject: Re: [Xen-devel] [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

On 16/09/2009 07:46, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:

> I remade the patch and measured the performance win. Attached is
> a benchmark program which I wrote. It is complied by cygwin's gcc
> by -O2 and runs on Windows XP(32bit). And my cpu is
> Intel Core2 Quad Q9450@2.66GHz.
> 
> The result is that my patch saves 32 cycles(TSC) per one VM_EXIT(cpuid).
> (2696 tsc => 2664 tsc)
> 
> The patch is split to two. I'll post them in another mail.

That's really not enough of a win to bother with, is it.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Keir Fraser]

On 16/09/2009 09:00, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:

> Looks the little win doesn't deserve the increased complexity in code.
> 
> BTW, recent Intel CPUs run much faster with respect to VMEntry/VMExit and
> VMREAD/VMWRITE, so I don't think the SW optimizatin is appearling here. :-)

Makes it more appealing doesn't it? As this path will be a greater
proportion of an overall vmexit/vmenter cycle.

 -- Keir

RE: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Kouya Shimura]

Cui, Dexuan writes:
> Looks the little win doesn't deserve the increased complexity in code.

Agreed. I don't strongly push my patches. 
But this win is given by 2/2 patch and it fixes a certain bug.
Besides, 1/2 patch simplifies the code, I think.

> BTW, recent Intel CPUs run much faster with respect to VMEntry/VMExit and VMREAD/VMWRITE, so I don't think the SW optimizatin is appearling here. :-)

I have a "Mottainai"(http://en.wikipedia.org/wiki/Mottainai) spirit. :-)

Thanks,
Kouya

> 
> Thanks,
> -- Dexuan
> 
> 
> 
> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
> Sent: 2009?9?16? 15:41
> To: Kouya Shimura
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked
> 
> On 16/09/2009 07:46, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:
> 
> > I remade the patch and measured the performance win. Attached is
> > a benchmark program which I wrote. It is complied by cygwin's gcc
> > by -O2 and runs on Windows XP(32bit). And my cpu is
> > Intel Core2 Quad Q9450@2.66GHz.
> > 
> > The result is that my patch saves 32 cycles(TSC) per one VM_EXIT(cpuid).
> > (2696 tsc => 2664 tsc)
> > 
> > The patch is split to two. I'll post them in another mail.
> 
> That's really not enough of a win to bother with, is it.
> 
>  -- Keir
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Keir Fraser]

I liked both patches when I looked at them, so I checked them in and we'll
see how well they work!

 -- Keir

On 16/09/2009 09:28, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:

> Cui, Dexuan writes:
>> Looks the little win doesn't deserve the increased complexity in code.
> 
> Agreed. I don't strongly push my patches.
> But this win is given by 2/2 patch and it fixes a certain bug.
> Besides, 1/2 patch simplifies the code, I think.
> 
>> BTW, recent Intel CPUs run much faster with respect to VMEntry/VMExit and
>> VMREAD/VMWRITE, so I don't think the SW optimizatin is appearling here. :-)
> 
> I have a "Mottainai"(http://en.wikipedia.org/wiki/Mottainai) spirit. :-)
> 
> Thanks,
> Kouya
> 
>> 
>> Thanks,
>> -- Dexuan
>> 
>> 
>> 
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xensource.com
>> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
>> Sent: 2009?9?16? 15:41
>> To: Kouya Shimura
>> Cc: xen-devel@lists.xensource.com
>> Subject: Re: [Xen-devel] [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation
>> while its IRQ is masked
>> 
>> On 16/09/2009 07:46, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:
>> 
>>> I remade the patch and measured the performance win. Attached is
>>> a benchmark program which I wrote. It is complied by cygwin's gcc
>>> by -O2 and runs on Windows XP(32bit). And my cpu is
>>> Intel Core2 Quad Q9450@2.66GHz.
>>> 
>>> The result is that my patch saves 32 cycles(TSC) per one VM_EXIT(cpuid).
>>> (2696 tsc => 2664 tsc)
>>> 
>>> The patch is split to two. I'll post them in another mail.
>> 
>> That's really not enough of a win to bother with, is it.
>> 
>>  -- Keir
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel

RE: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Cui, Dexuan]

I'm not sure if the number of the saved cycles is the same on recent Intel CPUs, so the proportion may not be greater.  :-)

Thanks,
-- Dexuan

-----Original Message-----
From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] 
Sent: 2009?9?16? 16:21
To: Cui, Dexuan; Kouya Shimura
Cc: xen-devel@lists.xensource.com
Subject: Re: [Xen-devel] [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

On 16/09/2009 09:00, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:

> Looks the little win doesn't deserve the increased complexity in code.
> 
> BTW, recent Intel CPUs run much faster with respect to VMEntry/VMExit and
> VMREAD/VMWRITE, so I don't think the SW optimizatin is appearling here. :-)

Makes it more appealing doesn't it? As this path will be a greater
proportion of an overall vmexit/vmenter cycle.

 -- Keir

Re: [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation while its IRQ is masked

[Keir Fraser]

Perhaps. Anyway, the patches fixed bugs and generally cleaned up some logic,
so I think they are good for reasons other than the few saved cycles on
vmenter.

 -- Keir

On 16/09/2009 09:40, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:

> I'm not sure if the number of the saved cycles is the same on recent Intel
> CPUs, so the proportion may not be greater.  :-)
> 
> Thanks,
> -- Dexuan
> 
> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: 2009?9?16? 16:21
> To: Cui, Dexuan; Kouya Shimura
> Cc: xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH] x86 hvm: freeze PIT/LAPIC timer emulation
> while its IRQ is masked
> 
> On 16/09/2009 09:00, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:
> 
>> Looks the little win doesn't deserve the increased complexity in code.
>> 
>> BTW, recent Intel CPUs run much faster with respect to VMEntry/VMExit and
>> VMREAD/VMWRITE, so I don't think the SW optimizatin is appearling here. :-)
> 
> Makes it more appealing doesn't it? As this path will be a greater
> proportion of an overall vmexit/vmenter cycle.
> 
>  -- Keir