Re: [PATCH] Fix TSC calibration issues

[Linus Torvalds <torvalds@linux-foundation.org>]

On Wed, 3 Sep 2008, Thomas Gleixner wrote:
>
> +	/*
> +	 * Run 5 calibration loops to get the lowest frequency value
> +	 * (the best estimate). We use two different calibration modes
> +	 * here:
> +	 *
> +	 * 1) PIT loop. We set the PIT Channel 2 to oneshot mode and
> +	 * load a timeout of 50ms. We read the time right after we
> +	 * started the timer and wait until the PIT count down reaches
> +	 * zero. In each wait loop iteration we read the TSC and check
> +	 * the delta to the previous read. We keep track of the min
> +	 * and max values of that delta. The delta is mostly defined
> +	 * by the IO time of the PIT access, so we can detect when a
> +	 * SMI/SMM disturbance happend between the two reads. If the
> +	 * maximum time is significantly larger than the minimum time,
> +	 * then we discard the result and have another try.
> +	 *
> +	 * 2) Reference counter. If available we use the HPET or the
> +	 * PMTIMER as a reference to check the sanity of that value.
> +	 * We use separate TSC readouts and check inside of the
> +	 * reference read for a SMI/SMM disturbance. We dicard
> +	 * disturbed values here as well. We do that around the PIT
> +	 * calibration delay loop as we have to wait for a certain
> +	 * amount of time anyway.
> +	 */
> +	for (i = 0; i < 5; i++) {
> +		tscmin = ULONG_MAX;
> +		tscmax = 0;
> +		pitcnt = 0;
> +
> +		local_irq_save(flags);
> +
> +		/*
> +		 * Read the start value and the reference count of
> +		 * hpet/pmtimer when available:
> +		 */
> +		tsc1 = tsc_read_refs(&pm1, hpet ? &hpet1 : NULL);

This is "wrongish".

You really should do the

	tsc1 = tsc_read_refs(&pm1, hpet ? &hpet1 : NULL);   
	...
	tsc2 = tsc_read_refs(&pm2, hpet ? &hpet2 : NULL);

around the whole loop, because they get more exact with more time inside, 
and they don't improve from looping around.

Also, that code is already _too_ unreadable. How about starting by just 
moving the PIT calibration into a function of its own, like the appended 
patch. And then as a separate patch, improving the heuristics for just the 
PIT calibration.

The others are independent of this issue..

		Linus

---
 arch/x86/kernel/tsc.c |   37 ++++++++++++++++++++-----------------
 1 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 8e786b0..bf7ff5a 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -122,19 +122,9 @@ static u64 tsc_read_refs(u64 *pm, u64 *hpet)
 	return ULLONG_MAX;
 }
 
-/**
- * native_calibrate_tsc - calibrate the tsc on boot
- */
-unsigned long native_calibrate_tsc(void)
+static unsigned long PIT_calibrate_tsc(void)
 {
-	unsigned long flags;
-	u64 tsc1, tsc2, tr1, tr2, delta, pm1, pm2, hpet1, hpet2;
-	int hpet = is_hpet_enabled();
-	unsigned int tsc_khz_val = 0;
-
-	local_irq_save(flags);
-
-	tsc1 = tsc_read_refs(&pm1, hpet ? &hpet1 : NULL);
+	u64 tr1, tr2, delta;
 
 	outb((inb(0x61) & ~0x02) | 0x01, 0x61);
 
@@ -145,17 +135,30 @@ unsigned long native_calibrate_tsc(void)
 	while ((inb(0x61) & 0x20) == 0);
 	tr2 = get_cycles();
 
-	tsc2 = tsc_read_refs(&pm2, hpet ? &hpet2 : NULL);
-
-	local_irq_restore(flags);
-
 	/*
 	 * Preset the result with the raw and inaccurate PIT
 	 * calibration value
 	 */
 	delta = (tr2 - tr1);
 	do_div(delta, 50);
-	tsc_khz_val = delta;
+	return delta;
+}
+
+/**
+ * native_calibrate_tsc - calibrate the tsc on boot
+ */
+unsigned long native_calibrate_tsc(void)
+{
+	unsigned long flags;
+	u64 tsc1, tsc2, pm1, pm2, hpet1, hpet2;
+	int hpet = is_hpet_enabled();
+	unsigned int tsc_khz_val;
+
+	local_irq_save(flags);
+	tsc1 = tsc_read_refs(&pm1, hpet ? &hpet1 : NULL);
+	tsc_khz_val = PIT_calibrate_tsc();
+	tsc2 = tsc_read_refs(&pm2, hpet ? &hpet2 : NULL);
+	local_irq_restore(flags);
 
 	/* hpet or pmtimer available ? */
 	if (!hpet && !pm1 && !pm2) {