Re: [PATCH] x86 apic: Ack all pending irqs when crashed/on kexec - V5

[ebiederm@xmission.com (Eric W. Biederman)]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 06/16/2010 02:11 PM, Pan, Jacob jun wrote:
>> 
>> W.R.T. the loop limits, is it possible to use a default max_loops value in
>> case when cpu_khz is not set? The reason is that on Moorestown platform
>> we need to do an early APIC setup before tsc_init(), so cpu_khz is 0 at the
>> time we setup local APIC. The result is that we hit WARN_ON(max_loops<= 0)
>> on Moorestown for early APIC setup.

So you get a nasty warning but the system still boots?

>> The early APIC setup is needed because Moorestown does not have a PIT and the
>> system timer interrupts are routed via IOAPIC.
>> 
>
> Can't MRST install a quick ballpark value into cpu_khz?

Looking at the code the initialization order in init/main.c is:

early_irq_init()
init_IRQ()
init_timers()
time_init()
    tsc_init()

local_irq_enable()

So arguably if we simply switched those lines around we could make
this work, as you must be initializing the tsc with interrupts
disabled.

That said given the current ordering it appears that using the tsc
in setup_local_APIC is just broken because if it is properly called
from init_IRQ() the code doesn't work properly.

The question is what do we consider more important the current code
initialization ordering, or virtual processors having such an expensive
apic_read that we need a 1 second timeout.

I think the virtual processor concern is silly.  Most of the time
this loop should execute exactly once after having confirmed there
is nothing to do.  On a bad day this loop should execute at most twice.
If the vitalization is too expensive that this loop cannot execute
twice, bailing out early is a correctness concern.

I think we should set max_loops to 5.  Leave the WARN_ON, and call it
good.

Does the following patch work cleanly on Moorestown?

Eric


>From fc298618e87233de748c7a289f41bcc68ed8fc64 Mon Sep 17 00:00:00 2001
From: Eric W. Biederman <ebiederm@xmission.com>
Date: Wed, 16 Jun 2010 18:42:39 -0700
Subject: [PATCH] x86/apic:  Don't use the tsc in setup_local_APIC

If everything is initialized in the order of init/main.c we should have:
init_IRQ()
    setup_local_APIC()
time_init()
    tsc_init()
local_irq_enable()

Which means the only reason the current use of the tsc in setup_local_APIC
works is because we are calling setup_local_APIC late on most x86
platforms.

The justification given for using a 1 second timeout on this loop
is because virtualized platforms have a very expensive apic_read().

Typically this loop should execute exactly once after confirming there
is nothing to do.  On a bad day this loop should execute twice
after clearing the ISR and the IRR unacked irqs.  If it is too
expensive to execute this loop twice on a virtualized platform
that is not our problem.

Let's set max_loops to 5.  Which is more than enough and that
removes the need for messing with the tsc.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86/kernel/apic/apic.c |   13 ++-----------
 1 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index c02cc69..d1a2b19 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -51,7 +51,6 @@
 #include <asm/smp.h>
 #include <asm/mce.h>
 #include <asm/kvm_para.h>
-#include <asm/tsc.h>
 
 unsigned int num_processors;
 
@@ -1154,11 +1153,7 @@ void __cpuinit setup_local_APIC(void)
 {
 	unsigned int value, queued;
 	int i, j, acked = 0;
-	unsigned long long tsc = 0, ntsc;
-	long long max_loops = cpu_khz;
-
-	if (cpu_has_tsc)
-		rdtscll(tsc);
+	int max_loops = 5;
 
 	if (disable_apic) {
 		arch_disable_smp_support();
@@ -1229,11 +1224,7 @@ void __cpuinit setup_local_APIC(void)
 			       acked);
 			break;
 		}
-		if (cpu_has_tsc) {
-			rdtscll(ntsc);
-			max_loops = (cpu_khz << 10) - (ntsc - tsc);
-		} else
-			max_loops--;
+		max_loops--;
 	} while (queued && max_loops > 0);
 	WARN_ON(max_loops <= 0);
 
-- 
1.6.5.2.143.g8cc62