Xen devel - Sep 2008 - [PATCH 2/4] CPUIDLE: Avoid remnant LAPIC timer intr while force hpetbroadcast

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>


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

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



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

> 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

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

>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

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

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



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

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

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

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



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

>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

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

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



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

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


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