Issue:
Current tsc based delay_calibration can result in significant errors in loops_per_jiffy count when the platform events like SMIs (System Management Interrupts that are non-maskable) are present. This could lead to potential kernel panic(). This issue is becoming more visible with 2.6 kernel (as default HZ is 1000) and on platforms with higher SMI handling latencies. During the boot time, SMIs are mostly used by BIOS (for things like legacy keyboard emulation).
Description:
The psuedocode for current delay calibration with tsc based delay looks like
(0) Estimate a value for loops_per_jiffy
(1) While (loops_per_jiffy estimate is accurate enough)
(2) wait for jiffy transition (jiffy1)
(3) Note down current tsc (tsc1)
(4) loop until tsc becomes tsc1 + loops_per_jiffy
(5) check whether jiffy changed since jiffy1 or not and refine loops_per_jiffy estimate
Consider the following cases
Case 1:
If SMIs happen between (2) and (3) above, we can end up with a loops_per_jiffy value that is too low. This results in shorted delays and kernel can panic () during boot (Mostly at IOAPIC timer initialization timer_irq_works() as we don't have enough timer interrupts in a specified interval).
Case 2:
If SMIs happen between (3) and (4) above, then we can end up with a loops_per_jiffy value that is too high. And with current i386 code, too high lpj value (greater than 17M) can result in a overflow in delay.c:__const_udelay() again resulting in shorter delay and panic().
Solution:
The patch below makes the calibration routine aware of asynchronous events like SMIs. We increase the delay calibration time and also identify any significant errors (greater than 12.5%) in the calibration and notify it to user. Like to know your comments on this.
Thanks,
Venki
Signed-off-by: Venkatesh Pallipadi <[email protected]>
--- linux-2.6.10/./arch/i386/kernel/timers/timer_tsc.c.org 2005-01-05 16:06:52.000000000 -0800
+++ linux-2.6.10/./arch/i386/kernel/timers/timer_tsc.c 2005-01-19 12:38:20.000000000 -0800
@@ -552,6 +552,7 @@ static struct timer_opts timer_tsc = {
.get_offset = get_offset_tsc,
.monotonic_clock = monotonic_clock_tsc,
.delay = delay_tsc,
+ .calibrate_delay = calibrate_delay_tsc,
};
struct init_timer_opts __initdata timer_tsc_init = {
--- linux-2.6.10/./arch/i386/kernel/timers/common.c.org 2005-01-11 17:51:28.000000000 -0800
+++ linux-2.6.10/./arch/i386/kernel/timers/common.c 2005-01-19 12:38:20.000000000 -0800
@@ -158,3 +158,49 @@ void __init init_cpu_khz(void)
}
}
}
+
+unsigned long calibrate_delay_tsc(void)
+{
+ unsigned long pre_start, start, post_start;
+ unsigned long pre_end, end, post_end;
+ unsigned long start_jiffies;
+ unsigned long tsc_rate_min, tsc_rate_max;
+
+ if (!cpu_has_tsc)
+ return 0;
+
+#define DELAY_CALIBRATION_TICKS ((HZ < 100) ? 1 : (HZ/100))
+ pre_start = 0;
+ rdtscl(start);
+ start_jiffies = jiffies;
+ while (jiffies <= (start_jiffies + 1)) {
+ pre_start = start;
+ rdtscl(start);
+ }
+ rdtscl(post_start);
+ pre_end = 0;
+ end = post_start;
+ while (jiffies <= (start_jiffies + 1 + DELAY_CALIBRATION_TICKS)) {
+ pre_end = end;
+ rdtscl(end);
+ }
+ rdtscl(post_end);
+
+ tsc_rate_max = (post_end - pre_start) / DELAY_CALIBRATION_TICKS;
+ tsc_rate_min = (pre_end - post_start) / DELAY_CALIBRATION_TICKS;
+
+ /*
+ * If the upper limit and lower limit of the tsc_rate is more than
+ * 12.5% apart.
+ */
+ if (pre_start == 0 || pre_end == 0 ||
+ (tsc_rate_max - tsc_rate_min) > (tsc_rate_max >> 3)) {
+ printk(KERN_WARNING "TSC calibration may not be precise. "
+ "Too many SMIs? "
+ "Consider running with \"lpj=\" boot option\n");
+ return 0;
+ }
+
+ return tsc_rate_max;
+}
+
--- linux-2.6.10/./arch/i386/kernel/timers/timer_hpet.c.org 2005-01-11 17:52:31.000000000 -0800
+++ linux-2.6.10/./arch/i386/kernel/timers/timer_hpet.c 2005-01-19 12:38:20.000000000 -0800
@@ -183,6 +183,7 @@ static struct timer_opts timer_hpet = {
.get_offset = get_offset_hpet,
.monotonic_clock = monotonic_clock_hpet,
.delay = delay_hpet,
+ .calibrate_delay = calibrate_delay_tsc,
};
struct init_timer_opts __initdata timer_hpet_init = {
--- linux-2.6.10/./arch/i386/kernel/timers/timer_pm.c.org 2005-01-11 17:55:55.000000000 -0800
+++ linux-2.6.10/./arch/i386/kernel/timers/timer_pm.c 2005-01-19 12:38:20.000000000 -0800
@@ -246,6 +246,7 @@ static struct timer_opts timer_pmtmr = {
.get_offset = get_offset_pmtmr,
.monotonic_clock = monotonic_clock_pmtmr,
.delay = delay_pmtmr,
+ .calibrate_delay = calibrate_delay_tsc,
};
struct init_timer_opts __initdata timer_pmtmr_init = {
--- linux-2.6.10/./arch/i386/kernel/timers/calibrate.c.org 2005-01-19 12:25:23.000000000 -0800
+++ linux-2.6.10/./arch/i386/kernel/timers/calibrate.c 2005-01-19 12:37:59.000000000 -0800
@@ -0,0 +1,96 @@
+/* calibrate.c: i386 specific delay calibration
+ *
+ * Mostly from init/calibrate.c with addition of __calibrate_delay() for arch
+ * specific stuff.
+ * Copyright (C) 1991, 1992 Linus Torvalds
+ */
+
+#include <linux/sched.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+
+#include <asm/timer.h>
+
+static unsigned long __init __calibrate_delay()
+{
+ if (cur_timer->calibrate_delay)
+ return cur_timer->calibrate_delay();
+
+ return 0;
+}
+
+static unsigned long preset_lpj;
+static int __init lpj_setup(char *str)
+{
+ preset_lpj = simple_strtoul(str,NULL,0);
+ return 1;
+}
+
+__setup("lpj=", lpj_setup);
+
+/*
+ * This is the number of bits of precision for the loops_per_jiffy. Each
+ * bit takes on average 1.5/HZ seconds. This (like the original) is a little
+ * better than 1%
+ */
+#define LPS_PREC 8
+
+void __devinit calibrate_delay(void)
+{
+ unsigned long ticks, loopbit;
+ int lps_precision = LPS_PREC;
+
+ if (preset_lpj) {
+ loops_per_jiffy = preset_lpj;
+ printk("Calibrating delay loop (skipped)... "
+ "%lu.%02lu BogoMIPS preset\n",
+ loops_per_jiffy/(500000/HZ),
+ (loops_per_jiffy/(5000/HZ)) % 100);
+ } else if ((loops_per_jiffy = __calibrate_delay()) != 0) {
+ printk("Calibrating delay using timer specific routine.. ");
+ printk("%lu.%02lu BogoMIPS (lpj=%lu)\n",
+ loops_per_jiffy/(500000/HZ),
+ (loops_per_jiffy/(5000/HZ)) % 100,
+ loops_per_jiffy);
+ } else {
+ loops_per_jiffy = (1<<12);
+
+ printk(KERN_DEBUG "Calibrating delay loop... ");
+ while ((loops_per_jiffy <<= 1) != 0) {
+ /* wait for "start of" clock tick */
+ ticks = jiffies;
+ while (ticks == jiffies)
+ /* nothing */;
+ /* Go .. */
+ ticks = jiffies;
+ __delay(loops_per_jiffy);
+ ticks = jiffies - ticks;
+ if (ticks)
+ break;
+ }
+
+ /*
+ * Do a binary approximation to get loops_per_jiffy set to
+ * equal one clock (up to lps_precision bits)
+ */
+ loops_per_jiffy >>= 1;
+ loopbit = loops_per_jiffy;
+ while (lps_precision-- && (loopbit >>= 1)) {
+ loops_per_jiffy |= loopbit;
+ ticks = jiffies;
+ while (ticks == jiffies)
+ /* nothing */;
+ ticks = jiffies;
+ __delay(loops_per_jiffy);
+ if (jiffies != ticks) /* longer than 1 tick */
+ loops_per_jiffy &= ~loopbit;
+ }
+
+ /* Round the value and print it */
+ printk("%lu.%02lu BogoMIPS (lpj=%lu)\n",
+ loops_per_jiffy/(500000/HZ),
+ (loops_per_jiffy/(5000/HZ)) % 100,
+ loops_per_jiffy);
+ }
+
+}
--- linux-2.6.10/./arch/i386/kernel/timers/Makefile.org 2005-01-19 12:25:30.000000000 -0800
+++ linux-2.6.10/./arch/i386/kernel/timers/Makefile 2005-01-19 12:26:00.000000000 -0800
@@ -2,7 +2,7 @@
# Makefile for x86 timers
#
-obj-y := timer.o timer_none.o timer_tsc.o timer_pit.o common.o
+obj-y :=calibrate.o timer.o timer_none.o timer_tsc.o timer_pit.o common.o
obj-$(CONFIG_X86_CYCLONE_TIMER) += timer_cyclone.o
obj-$(CONFIG_HPET_TIMER) += timer_hpet.o
--- linux-2.6.10/./arch/i386/Kconfig.org 2005-01-19 12:24:13.000000000 -0800
+++ linux-2.6.10/./arch/i386/Kconfig 2005-01-19 12:24:37.000000000 -0800
@@ -373,10 +373,6 @@ config RWSEM_XCHGADD_ALGORITHM
depends on !M386
default y
-config GENERIC_CALIBRATE_DELAY
- bool
- default y
-
config X86_PPRO_FENCE
bool
depends on M686 || M586MMX || M586TSC || M586 || M486 || M386
--- linux-2.6.10/./include/asm-i386/timer.h.org 2005-01-05 14:07:04.000000000 -0800
+++ linux-2.6.10/./include/asm-i386/timer.h 2005-01-19 12:38:20.000000000 -0800
@@ -22,6 +22,7 @@ struct timer_opts {
unsigned long (*get_offset)(void);
unsigned long long (*monotonic_clock)(void);
void (*delay)(unsigned long);
+ unsigned long (*calibrate_delay)(void);
};
struct init_timer_opts {
@@ -52,6 +53,7 @@ extern struct init_timer_opts timer_cycl
#endif
extern unsigned long calibrate_tsc(void);
+extern unsigned long calibrate_delay_tsc(void);
extern void init_cpu_khz(void);
#ifdef CONFIG_HPET_TIMER
extern struct init_timer_opts timer_hpet_init;
Please don't send emails which contain 500-column lines?
Venkatesh Pallipadi <[email protected]> wrote:
>
> Current tsc based delay_calibration can result in significant errors in
> loops_per_jiffy count when the platform events like SMIs (System Management
> Interrupts that are non-maskable) are present.
This seems like an unsolveable problem.
> Solution:
> The patch below makes the calibration routine aware of asynchronous events
> like SMIs. We increase the delay calibration time and also identify any
> significant errors (greater than 12.5%) in the calibration and notify it
> to user. Like to know your comments on this.
I find calibrate_delay_tsc() quite confusing. Are you sure that the
variable names are correct?
+ tsc_rate_max = (post_end - pre_start) / DELAY_CALIBRATION_TICKS;
+ tsc_rate_min = (pre_end - post_start) / DELAY_CALIBRATION_TICKS;
that looks strange. I'm sure it all makes sense if one understands the
algorithm, but it shouldn't be this hard. Please reissue the patch with
adequate comments which describe what the code is doing.
Shouldn't calibrate_delay_tsc() be __devinit? (That may generate warnings
from reference_discarded.pl, but they're false positives)
>From a maintainability POV it's not good that x86 is no longer using the
generic calibrate_delay() code. Can you rework the code so that all
architectures must implement arch_calibrate_delay(), then provide stubs for
all except x86? After all, other architectures/platforms may have the same
problem.
Venkatesh Pallipadi <[email protected]> writes:
> +
> + /*
> + * If the upper limit and lower limit of the tsc_rate is more than
> + * 12.5% apart.
> + */
> + if (pre_start == 0 || pre_end == 0 ||
> + (tsc_rate_max - tsc_rate_min) > (tsc_rate_max >> 3)) {
> + printk(KERN_WARNING "TSC calibration may not be precise. "
> + "Too many SMIs? "
> + "Consider running with \"lpj=\" boot option\n");
> + return 0;
> + }
I think it would be better to rerun it a few times automatically
before giving up. This way it would hopefully work transparently but slower
for most users. The message is too obscure too to be usable and needs
more explanation.
And also in case the platforms in questions support EM64T
x86-64 would need to be changed too :)
-Andi
>-----Original Message-----
>From: Andrew Morton [mailto:[email protected]]
>Subject: Re: [Discuss][i386] Platform SMIs and their
>interferance with tsc based delay calibration
>
>
>Please don't send emails which contain 500-column lines?
Sorry. Something got messed up during cut and paste onto my mailer.
>> Solution:
>> The patch below makes the calibration routine aware of
>asynchronous events
>> like SMIs. We increase the delay calibration time and also
>identify any
>> significant errors (greater than 12.5%) in the calibration
>and notify it
>> to user. Like to know your comments on this.
>
>I find calibrate_delay_tsc() quite confusing. Are you sure that the
>variable names are correct?
>
> + tsc_rate_max = (post_end - pre_start) / DELAY_CALIBRATION_TICKS;
> + tsc_rate_min = (pre_end - post_start) / DELAY_CALIBRATION_TICKS;
>
>that looks strange. I'm sure it all makes sense if one understands the
>algorithm, but it shouldn't be this hard. Please reissue the
>patch with
>adequate comments which describe what the code is doing.
>
I will resend the patch soon with more comments. I think the variable
names here are bit confusing.
>Shouldn't calibrate_delay_tsc() be __devinit? (That may
>generate warnings
>from reference_discarded.pl, but they're false positives)
>
>
>From a maintainability POV it's not good that x86 is no longer
>using the
>generic calibrate_delay() code. Can you rework the code so that all
>architectures must implement arch_calibrate_delay(), then
>provide stubs for
>all except x86? After all, other architectures/platforms may
>have the same
>problem.
>
Agreed. I will add a stub in other architectures. That way we don't
have to duplicate the current delay_calibration under i386.
Thanks,
Venki
>-----Original Message-----
>From: Andi Kleen [mailto:[email protected]]
>Sent: Friday, January 28, 2005 10:24 PM
>To: Pallipadi, Venkatesh
>Cc: Seth, Rohit; Mallick, Asit K;
>[email protected]; [email protected]
>Subject: Re: [Discuss][i386] Platform SMIs and their
>interferance with tsc based delay calibration
>
>Venkatesh Pallipadi <[email protected]> writes:
>> +
>> + /*
>> + * If the upper limit and lower limit of the tsc_rate
>is more than
>> + * 12.5% apart.
>> + */
>> + if (pre_start == 0 || pre_end == 0 ||
>> + (tsc_rate_max - tsc_rate_min) > (tsc_rate_max >> 3)) {
>> + printk(KERN_WARNING "TSC calibration may not be
>precise. "
>> + "Too many SMIs? "
>> + "Consider running with \"lpj=\" boot option\n");
>> + return 0;
>> + }
>
>I think it would be better to rerun it a few times automatically
>before giving up. This way it would hopefully work
>transparently but slower
>for most users.
Agreed. Actually, I was doing that earlier, with each retry
calibrating for 1 HZ. But, once I moved to 10 HZ, I removed the retires.
>The message is too obscure too to be usable and needs
>more explanation.
I will try to improve the message in next revision of the patch.
>And also in case the platforms in questions support EM64T
>x86-64 would need to be changed too :)
Yes. I will send out a patch for x86-64 too, once the i386 patch
gets finalized. I wanted to have a shorted patch reviewed first :).
Thanks,
Venki
On Fri, 28 Jan 2005, Venkatesh Pallipadi wrote:
>
> Current tsc based delay_calibration can result in significant errors in
> loops_per_jiffy count when the platform events like SMIs (System
> Management Interrupts that are non-maskable) are present. This could
> lead to potential kernel panic(). This issue is becoming more visible
> with 2.6 kernel (as default HZ is 1000) and on platforms with higher SMI
> handling latencies. During the boot time, SMIs are mostly used by BIOS
> (for things like legacy keyboard emulation).
Hmm. I see the problem, but I don't know that I'm 100% happy with this
patch, though.
In particular, I don't see why you didn't just put this in the generic
calibrate_delay() routine. You seem to have basically duplicated it, and
added the "guess from an external timer" code - and I don't see what's
non-generic about that, except for some trivial "what's the current timer"
lookup.
Linus
Venkatesh Pallipadi wrote:
> (0) Estimate a value for loops_per_jiffy
> (1) While (loops_per_jiffy estimate is accurate enough)
> (2) wait for jiffy transition (jiffy1)
> (3) Note down current tsc (tsc1)
> (4) loop until tsc becomes tsc1 + loops_per_jiffy
> (5) check whether jiffy changed since jiffy1 or not
> and refine loops_per_jiffy estimate
>
> Case 2: If SMIs happen between (3) and (4) above, then we can end up
> with a loops_per_jiffy value that is too high.
I don't think this can really happen (at least not on the bootstrap CPU)
because even if an SMI occurs, the CPU must first serve the timer
interrupt and increment jiffies before it continues looping.
However "LPJ too short" is what needs to be avoided at all cost.
Regards and thanks for working on this problem,
Martin
Linus Torvalds wrote:
> In particular, I don't see why you didn't just put this in the generic
> calibrate_delay() routine. You seem to have basically duplicated it, and
> added the "guess from an external timer" code - and I don't see what's
> non-generic about that, except for some trivial "what's the current timer"
> lookup.
The trivial lookup is hidden in the __delay() function. Making it return
the number of "loops" it actually waited would help having a robust
generic code.
Regards
Martin