[PATCH 2/4] CPUIDLE: Avoid remnant LAPIC timer intr while force hpetbroadcast

[PATCH 2/4] CPUIDLE: Avoid remnant LAPIC timer intr while force hpetbroadcast

[Wei, Gang]

[-- Attachment #1: Type: text/plain, Size: 1121 bytes --]

CPUIDLE: Avoid remnant LAPIC timer intr while force hpetbroadcast

LAPIC will stop during C3, and resume to work after exit from C3. Considering below case:
The LAPIC timer was programmed to expire after 1000us, but CPU enter C3 after 100us and exit C3 at 9xxus.

   0us: reprogram_timer(1000us)
 100us: entry C3, LAPIC timer stop
 9xxus: exit C3 due to unexpected event, LAPIC timer continue running
10xxus: reprogram_timer(1000us), fail due to the past expiring time.
......: no timer softirq raised, no change to LAPIC timer.
......: if entry C3 again, HPET will be forced reprogramed to now+small_slop.
......: if entry C2, no change to LAPIC.
18xxus: LAPIC timer expires unexpectedly if no C3 entries after 10xxus.

>From above sequences, we can find this case will either introduce extra HPET intrs or put off the softtimer expiring.
This patch simply stops the LAPIC timer first (avoid immediate unnecessary expiring) and raise a softirq (continue the softtimer handling process, which will correct the LAPIC timer) when reprogram LAPIC timer fails.

Signed-off-by: Wei Gang <gang.wei@intel.com>

[-- Attachment #2: cancel_useless_timer_intr_0909.patch --]
[-- Type: application/octet-stream, Size: 2012 bytes --]

CPUIDLE: Avoid remnant LAPIC timer intr while force hpetbroadcast

LAPIC will stop during C3, and resume to work after exit from C3. Considering below case:
The LAPIC timer was programmed to expire after 1000us, but CPU enter C3 after 100us and exit C3 at 9xxus.

   0us: reprogram_timer(1000us)
 100us: entry C3, LAPIC timer stop
 9xxus: exit C3 due to unexpected event, LAPIC timer continue running
10xxus: reprogram_timer(1000us), fail due to the past expiring time.
......: no timer softirq raised, no change to LAPIC timer.
......: if entry C3 again, hpet will be forced reprogramed to now+small_slop.
......: if entry C2, no change to LAPIC.
18xxus: LAPIC timer expires unexpectedly if no C3 entries after 10xxus.

From above sequences, we can find this case will either introduce extra hpet intrs or put off the softtimer expiring.
This patch simply stops the LAPIC timer first (avoid immediate unnecessary expiring) and raise a softirq (continue the softtimer handling process, which will correct the LAPIC timer) when reprogram apic timer fails.

Signed-off-by: Wei Gang <gang.wei@intel.com>

diff -r da8cf43032a9 xen/arch/x86/hpet.c
--- a/xen/arch/x86/hpet.c	Tue Sep 09 09:07:17 2008 +0800
+++ b/xen/arch/x86/hpet.c	Tue Sep 09 09:47:16 2008 +0800
@@ -221,6 +221,8 @@ void hpet_broadcast_enter(void)
 
     spin_lock(&ch->lock);
 
+    disable_APIC_timer();
+
     cpu_set(smp_processor_id(), ch->cpumask);
 
     /* reprogram if current cpu expire time is nearer */
@@ -239,11 +241,17 @@ void hpet_broadcast_exit(void)
 
     if ( cpu_test_and_clear(cpu, ch->cpumask) )
     {
-        reprogram_timer(per_cpu(timer_deadline, cpu));
+        if ( !reprogram_timer(per_cpu(timer_deadline, cpu)) )
+        {
+            reprogram_timer(0);
+            raise_softirq(TIMER_SOFTIRQ);
+        }
 
         if ( cpus_empty(ch->cpumask) && ch->next_event != STIME_MAX )
             reprogram_hpet_evt_channel(ch, STIME_MAX, 0, 0);
     }
+
+    enable_APIC_timer();
 
     spin_unlock_irq(&ch->lock);
 }

[-- 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 2/4] CPUIDLE: Avoid remnant LAPIC timer intr while force hpetbroadcast

[Keir Fraser]

On 9/9/08 09:28, "Wei, Gang" <gang.wei@intel.com> wrote:

>> From above sequences, we can find this case will either introduce extra HPET
>> intrs or put off the softtimer expiring.
> This patch simply stops the LAPIC timer first (avoid immediate unnecessary
> expiring) and raise a softirq (continue the softtimer handling process, which
> will correct the LAPIC timer) when reprogram LAPIC timer fails.

It's not clear to me the disable/enable_LAPIC_timer() work is worthwhile for
the few timer interrupts it is likely to avoid. The other bit of the patch
is a good bugfix though. I've taken just the latter part.

 -- Keir

RE: [PATCH 2/4] CPUIDLE: Avoid remnant LAPIC timer intr while force hpetbroadcast

[Wei, Gang]

> It's not clear to me the disable/enable_LAPIC_timer() work is worthwhile for
> the few timer interrupts it is likely to avoid. The other bit of the patch
> is a good bugfix though. I've taken just the latter part.

Thanks for accept most of these patches. As to disable/enable_LAPIC_timer(), I add them because some platforms require the LAPIC timer intr being disabled before entering C3, otherwise there may be some unexpected things. As a reference, Linux kernel always do so.

Jimmy

RE: [PATCH 2/4] CPUIDLE: Avoid remnant LAPIC timer intrwhile force hpetbroadcast

[Tian, Kevin]

>From: Wei, Gang
>Sent: 2008年9月10日 22:25
>
>> It's not clear to me the disable/enable_LAPIC_timer() work 
>is worthwhile for
>> the few timer interrupts it is likely to avoid. The other 
>bit of the patch
>> is a good bugfix though. I've taken just the latter part.
>
>Thanks for accept most of these patches. As to 
>disable/enable_LAPIC_timer(), I add them because some 
>platforms require the LAPIC timer intr being disabled before 
>entering C3, otherwise there may be some unexpected things. As 
>a reference, Linux kernel always do so.
>

Also this is conceptually clearer, as at given time we only
want one clock source to drive timers. It'd make little sense
to have APIC timer freely counting down to trigger spurious
interrupt when HPET/PIT already plays the role before APIC 
timer is reprogrammed upon C3 exit. :-)

Thanks,
Kevin

Re: [PATCH 2/4] CPUIDLE: Avoid remnant LAPIC timer intr while force hpetbroadcast

[Keir Fraser]

On 10/9/08 15:25, "Wei, Gang" <gang.wei@intel.com> wrote:

>> It's not clear to me the disable/enable_LAPIC_timer() work is worthwhile for
>> the few timer interrupts it is likely to avoid. The other bit of the patch
>> is a good bugfix though. I've taken just the latter part.
> 
> Thanks for accept most of these patches. As to disable/enable_LAPIC_timer(), I
> add them because some platforms require the LAPIC timer intr being disabled
> before entering C3, otherwise there may be some unexpected things. As a
> reference, Linux kernel always do so.

Is this a documented aspect of C3, or just one of those things? Do you know
what kind of 'unexpected things' can happen?

 -- Keir

RE: [PATCH 2/4] CPUIDLE: Avoid remnant LAPIC timer intr while force hpetbroadcast

[Wei, Gang]

On Thursday, September 11, 2008 3:38 PM, Keir Fraser wrote:
> On 10/9/08 15:25, "Wei, Gang" <gang.wei@intel.com> wrote:
>
>>> It's not clear to me the disable/enable_LAPIC_timer() work is worthwhile for
>>> the few timer interrupts it is likely to avoid. The other bit of the patch
>>> is a good bugfix though. I've taken just the latter part.
>>
>> Thanks for accept most of these patches. As to disable/enable_LAPIC_timer(),
>> I add them because some platforms require the LAPIC timer intr being disabled
>> before entering C3, otherwise there may be some unexpected things. As a
>> reference, Linux kernel always do so.
>
> Is this a documented aspect of C3, or just one of those things? Do you know
> what kind of 'unexpected things' can happen?

Currently let's just regard it as one of those things. I think kevin's reply can explain it better from another perspective.

Jimmy

Re: [PATCH 2/4] CPUIDLE: Avoid remnant LAPIC timer intr while force hpetbroadcast

[Keir Fraser]

On 10/9/08 15:25, "Wei, Gang" <gang.wei@intel.com> wrote:

>> It's not clear to me the disable/enable_LAPIC_timer() work is worthwhile for
>> the few timer interrupts it is likely to avoid. The other bit of the patch
>> is a good bugfix though. I've taken just the latter part.
> 
> Thanks for accept most of these patches. As to disable/enable_LAPIC_timer(), I
> add them because some platforms require the LAPIC timer intr being disabled
> before entering C3, otherwise there may be some unexpected things. As a
> reference, Linux kernel always do so.

Can you point out where Linux does so? It's not obvious to me.

Also, in your patch, you unmask LVTT after reprogramming the LAPIC counter.
Isn't there a race where the LAPIC timer generates an interrupt event before
you unmask the LVTT and hence you lose the interrupt (since I assume the
LAPIC interrupt is basically an internal one-shot signal which does not get
latched in any way)? So you'd probably need to reprogram_timer(0), then
enable the timer, then reprogram_timer(<actual value>).

 -- Keir

RE: [PATCH 2/4] CPUIDLE: Avoid remnant LAPIC timerintr while force hpetbroadcast

[Tian, Kevin]

>From: Keir Fraser
>Sent: 2008年9月11日 18:38
>
>On 10/9/08 15:25, "Wei, Gang" <gang.wei@intel.com> wrote:
>
>>> It's not clear to me the disable/enable_LAPIC_timer() work 
>is worthwhile for
>>> the few timer interrupts it is likely to avoid. The other 
>bit of the patch
>>> is a good bugfix though. I've taken just the latter part.
>> 
>> Thanks for accept most of these patches. As to 
>disable/enable_LAPIC_timer(), I
>> add them because some platforms require the LAPIC timer intr 
>being disabled
>> before entering C3, otherwise there may be some unexpected 
>things. As a
>> reference, Linux kernel always do so.
>
>Can you point out where Linux does so? It's not obvious to me.

acpi_state_timer_broadcast
clockevents_notify
tick_notify
tick_broadcast_oneshot_control
clockevents_set_mode(dev, CLOCK_EVT_MODE_SHUTDOWN)
lapic_timer_setup:
        case CLOCK_EVT_MODE_SHUTDOWN:
                v = apic_read(APIC_LVTT);
                v |= (APIC_LVT_MASKED | LOCAL_TIMER_VECTOR);
                apic_write(APIC_LVTT, v);
                break;

>
>Also, in your patch, you unmask LVTT after reprogramming the 
>LAPIC counter.
>Isn't there a race where the LAPIC timer generates an 
>interrupt event before
>you unmask the LVTT and hence you lose the interrupt (since I 
>assume the
>LAPIC interrupt is basically an internal one-shot signal which 
>does not get
>latched in any way)? So you'd probably need to reprogram_timer(0), then
>enable the timer, then reprogram_timer(<actual value>).
>

You're correct. It will be fixed.

Thanks,
Kevin

Re: [PATCH 2/4] CPUIDLE: Avoid remnant LAPIC timerintr while force hpetbroadcast

[Keir Fraser]

On 11/9/08 11:59, "Tian, Kevin" <kevin.tian@intel.com> wrote:

>> Also, in your patch, you unmask LVTT after reprogramming the
>> LAPIC counter.
>> Isn't there a race where the LAPIC timer generates an
>> interrupt event before
>> you unmask the LVTT and hence you lose the interrupt (since I
>> assume the
>> LAPIC interrupt is basically an internal one-shot signal which
>> does not get
>> latched in any way)? So you'd probably need to reprogram_timer(0), then
>> enable the timer, then reprogram_timer(<actual value>).
>> 
> 
> You're correct. It will be fixed.

Thanks. I'm not sure whether the reprogram_timer(0) before re-enabling is
really required. It looks like Linux doesn't do similar (although if course
it does re-programming after re-enabling to avoid the above race).

I suppose repogram_timer(0) is cheap so you might choose to do it anyway.
It's up to you...

 -- Keir

RE: [PATCH 2/4] CPUIDLE: Avoid remnant LAPIC timerintr while force hpetbroadcast

[Wei, Gang]

[-- Attachment #1: Type: text/plain, Size: 1115 bytes --]

On Thursday, September 11, 2008 7:07 PM, Keir Fraser wrote:
> On 11/9/08 11:59, "Tian, Kevin" <kevin.tian@intel.com> wrote:
>
>>> Also, in your patch, you unmask LVTT after reprogramming the
>>> LAPIC counter.
>>> Isn't there a race where the LAPIC timer generates an
>>> interrupt event before
>>> you unmask the LVTT and hence you lose the interrupt (since I
>>> assume the
>>> LAPIC interrupt is basically an internal one-shot signal which
>>> does not get
>>> latched in any way)? So you'd probably need to reprogram_timer(0), then
>>> enable the timer, then reprogram_timer(<actual value>).
>>>
>>
>> You're correct. It will be fixed.
>
> Thanks. I'm not sure whether the reprogram_timer(0) before re-enabling is
> really required. It looks like Linux doesn't do similar (although if course
> it does re-programming after re-enabling to avoid the above race).
>
> I suppose repogram_timer(0) is cheap so you might choose to do it anyway.
> It's up to you...

Do reprogram_timer(0) anyway make things well controlled. So let's do it this way. Attached is the additional patch.

Jimmy

[-- Attachment #2: disable_apic_timer_in_c3_0911.patch --]
[-- Type: application/octet-stream, Size: 1437 bytes --]

CPUIDLE: Disable APIC timer intr during C3 while force hpetbroadcast

It can avoid C3 early exit, and also this is conceptually clearer, as at given time we only want one clock source to drive timers.

Signed-off-by: Wei Gang <gang.wei@intel.com>
Signed-off-by: Keir Fraser <keir.fraser@citrix.com>

diff -r f5e72cbfbb17 xen/arch/x86/hpet.c
--- a/xen/arch/x86/hpet.c	Wed Sep 10 11:26:16 2008 +0100
+++ b/xen/arch/x86/hpet.c	Thu Sep 11 23:27:05 2008 +0800
@@ -215,6 +215,8 @@ void hpet_broadcast_enter(void)
 
     spin_lock(&ch->lock);
 
+    disable_APIC_timer();
+
     cpu_set(smp_processor_id(), ch->cpumask);
 
     /* reprogram if current cpu expire time is nearer */
@@ -233,15 +235,14 @@ void hpet_broadcast_exit(void)
 
     if ( cpu_test_and_clear(cpu, ch->cpumask) )
     {
+        /* Cancel any outstanding LAPIC event */
+        reprogram_timer(0);
+
+        enable_APIC_timer();
+        
+        /* The deadline must have passed -- trigger timer work now. */
         if ( !reprogram_timer(per_cpu(timer_deadline, cpu)) )
-        {
-            /*
-             * The deadline must have passed -- trigger timer work now.
-             * Also cancel any outstanding LAPIC event.
-             */
-            reprogram_timer(0);
             raise_softirq(TIMER_SOFTIRQ);
-        }
 
         if ( cpus_empty(ch->cpumask) && ch->next_event != STIME_MAX )
             reprogram_hpet_evt_channel(ch, STIME_MAX, 0, 0);

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

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