Re: Terrible interactivity with 2.6.0-t9-mm3

john stultz
2003-11-17T21:32:40Z

On Mon, 2003-11-17 at 11:46, Thomas Schlichter wrote:
> On Monday 17 November 2003 14:12, Prakash K. Cheemplavam wrote:
> > Ronny V. Vindenes wrote:
> > > I've found that neither linus.patch nor
> > > context-switch-accounting-fix.patch is causing the problem, but rather
> > > acpi-pm-timer-fixes.patch & acpi-pm-timer.patch
> > >
> > > With these applied my cpu (athlon64) is detected as 0.0Mhz, bogomips
> > > drops to 50% and anything cpu intensive destroys interactivity. Revert
> > > them and performance is back at -mm2 level.
> >
> > Yup, works for me too. Reverting those patches and my machine is smooth
> > again. :)

Not configuring the ACPI PM time source in should also work as well.

> I was able to reproduce the interactivity problem, too. My simple testcase
> was:
> 1. open a KDE Konsole
> 2. execute "while true; do a=2; done" in the Konsole
> 3. Move the Konsole window.
>
> With test9-mm3 booted with "clock=pit" or "clock=pmtmr" moving the window was
> very sluggish... Booted with "clock=tsc" made it work fine again. (Btw. which
> kind of hardware is needed to make "clock=hpet" work?)
>
> The problem is that sched_clock() uses the TSC if the hardware supports it.
> But the needed scaling factors are only initialized in init_tsc() and
> init_hpet(). So there are 2 possibilities to fix this:
> 1. Call the neccessary parts of init_tsc() in init_pmtmr() and init_pit().
> 2. Use the TSC in sched_clock() only if "clock=tsc" was set.

As far as sched_clock() goes, I haven't followed its development
closely, but it seem that it is very close to monotonic_clock() in
functionality. The benefit of monotonic_clock is that it is implemented
for each time source (however its not implemented for every arch). For
i386 at least, we may want to make sched_clock just call
monotonic_clock, but I need to look into the details.

> I've attached a small patch that does the 2nd thing. For me it fixed the
> interactivity problem...

That tentatively looks OK to me, but I suspect that sched_clock needs
further work on i386.

> Btw. the BogoMIPS value is the argument for the __delay() function needed to
> wait 1 jiffy. The difference is that the TSC version of this function uses
> the clock cycle count as its argument whereas the PMTMR and PIT versions take
> the count of loops to wait as their argument. Well, and it seems that each
> loop iteration needs 2 clock cycles.

__delay() is called with loops as an argument. calibrate_delay() sets
loops_per_jiffy such that mdelay() and udelay() will delay for the
proper amount of time. BogoMIPS may not line up w/ your cpu frequency,
but don't let that worry you.

> The problem with the shown CPU frequency is, that cpu_khz is only set in
> init_tsc() and init_hpet(). But I don't know how this can be fixed without
> using the TSC...

You're correct, I forgot to initialize cpu_khz in the ACPI PM timesource
init code. This patch fixes that.

thanks for the great testing and feedback!
-john

diff -Nru a/arch/i386/kernel/timers/common.c b/arch/i386/kernel/timers/common.c
--- a/arch/i386/kernel/timers/common.c Mon Nov 17 13:23:43 2003
+++ b/arch/i386/kernel/timers/common.c Mon Nov 17 13:23:43 2003
@@ -137,3 +137,23 @@
}
#endif

+/* calculate cpu_khz */
+void __init init_cpu_khz(void)
+{
+ if (cpu_has_tsc) {
+ unsigned long tsc_quotient = calibrate_tsc();
+ if (tsc_quotient) {
+ /* report CPU clock rate in Hz.
+ * The formula is (10^6 * 2^32) / (2^32 * 1 / (clocks/us)) =
+ * clock/second. Our precision is about 100 ppm.
+ */
+ { unsigned long eax=0, edx=1000;
+ __asm__("divl %2"
+ :"=a" (cpu_khz), "=d" (edx)
+ :"r" (tsc_quotient),
+ "0" (eax), "1" (edx));
+ printk("Detected %lu.%03lu MHz processor.\n", cpu_khz / 1000, cpu_khz % 1000);
+ }
+ }
+ }
+}
diff -Nru a/arch/i386/kernel/timers/timer_cyclone.c b/arch/i386/kernel/timers/timer_cyclone.c
--- a/arch/i386/kernel/timers/timer_cyclone.c Mon Nov 17 13:23:43 2003
+++ b/arch/i386/kernel/timers/timer_cyclone.c Mon Nov 17 13:23:43 2003
@@ -212,27 +212,8 @@
}
}

- /* init cpu_khz.
- * XXX - This should really be done elsewhere,
- * and in a more generic fashion. -johnstul@us.ibm.com
- */
- if (cpu_has_tsc) {
- unsigned long tsc_quotient = calibrate_tsc();
- if (tsc_quotient) {
- /* report CPU clock rate in Hz.
- * The formula is (10^6 * 2^32) / (2^32 * 1 / (clocks/us)) =
- * clock/second. Our precision is about 100 ppm.
- */
- { unsigned long eax=0, edx=1000;
- __asm__("divl %2"
- :"=a" (cpu_khz), "=d" (edx)
- :"r" (tsc_quotient),
- "0" (eax), "1" (edx));
- printk("Detected %lu.%03lu MHz processor.\n", cpu_khz / 1000, cpu_khz % 1000);
- }
- }
- }
-
+ init_cpu_khz();
+
/* Everything looks good! */
return 0;
}
diff -Nru a/arch/i386/kernel/timers/timer_pm.c b/arch/i386/kernel/timers/timer_pm.c
--- a/arch/i386/kernel/timers/timer_pm.c Mon Nov 17 13:23:43 2003
+++ b/arch/i386/kernel/timers/timer_pm.c Mon Nov 17 13:23:43 2003
@@ -57,14 +57,18 @@
if (value2 == value1)
continue;
if (value2 > value1)
- return 0;
+ goto pm_good;
if ((value2 < value1) && ((value2) < 0xFFF))
- return 0;
+ goto pm_good;
printk(KERN_INFO "PM-Timer had inconsistent results: 0x%#x, 0x%#x - aborting.\n", value1, value2);
return -EINVAL;
}
printk(KERN_INFO "PM-Timer had no reasonable result: 0x%#x - aborting.\n", value1);
return -ENODEV;
+
+pm_good:
+ init_cpu_khz();
+ return 0;
}

static inline u32 cyc2us(u32 cycles)
diff -Nru a/include/asm-i386/timer.h b/include/asm-i386/timer.h
--- a/include/asm-i386/timer.h Mon Nov 17 13:23:43 2003
+++ b/include/asm-i386/timer.h Mon Nov 17 13:23:43 2003
@@ -39,6 +39,7 @@
#endif

extern unsigned long calibrate_tsc(void);
+extern void init_cpu_khz(void);
#ifdef CONFIG_HPET_TIMER
extern struct timer_opts timer_hpet;
extern unsigned long calibrate_tsc_hpet(unsigned long *tsc_hpet_quotient_ptr);