Re: [PATCH] Fix TSC calibration issues

[Alok Kataria <akataria@vmware.com>]

On Wed, 2008-09-03 at 02:11 -0700, Thomas Gleixner wrote:
> On Tue, 2 Sep 2008, Linus Torvalds wrote:
> > 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.
> 
> True. Just kept them at the place where my debug patches had left them.

Hi Thomas, 

I agree with Linus that we should move the tsc_read_refs call outside of
the loop. I did those changes and ran some boot-halt tests at my end.
The frequency calibration against the pmtimer/hpet was surely more fine
tuned with this change, the variance that i see now in repeated reboots
is very minimal.

Please have a look at the patch below. 

--
x86: Fine tune TSC calibration.

From: Alok N Kataria <akataria@vmware.com>

As Linus suggested, we should be moving the tsc_read_refs outside of the
loop, this gives us more accurate TSC calibration when calibrating against
hpet/pmtimer, since we are now calibrating over a period of 250ms.


With SMI_THRESHOLD equals to 50000, in the worst case, tsc values read by
tsc_read_refs, could be off by 50000 ticks. On a 2Ghz processor this would
mean an error of 25us. The tsc frequency is calibrated over a period of 250ms
with this patch, hence the worst case error would be around 100ppm down from
500ppm previously without the patch.

Signed-off-by: Alok N Kataria <akataria@vmware.com>
---

 arch/x86/kernel/tsc.c |   54 ++++++++++++++++++++++++++-----------------------
 1 files changed, 29 insertions(+), 25 deletions(-)


diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 346cae5..bc2c1a2 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -188,7 +188,7 @@ static unsigned long pit_calibrate_tsc(void)
 unsigned long native_calibrate_tsc(void)
 {
 	u64 tsc1, tsc2, delta, pm1, pm2, hpet1, hpet2;
-	unsigned long tsc_pit_min = ULONG_MAX, tsc_ref_min = ULONG_MAX;
+	unsigned long tsc_pit_min = ULONG_MAX, tsc_ref = ULONG_MAX;
 	unsigned long flags;
 	int hpet = is_hpet_enabled(), i;
 
@@ -216,31 +216,35 @@ unsigned long native_calibrate_tsc(void)
 	 * calibration delay loop as we have to wait for a certain
 	 * amount of time anyway.
 	 */
+
+	local_irq_save(flags);
+
+	/*
+	 * Read the start value and the reference count of
+	 * hpet/pmtimer when available. Then do the PIT
+	 * calibration iteratively, which will take at least 250ms,
+	 * and read the end value.
+	 */
+	tsc1 = tsc_read_refs(&pm1, hpet ? &hpet1 : NULL);
+
 	for (i = 0; i < 5; i++) {
 		unsigned long tsc_pit_khz;
 
-		/*
-		 * Read the start value and the reference count of
-		 * hpet/pmtimer when available. Then do the PIT
-		 * calibration, which will take at least 50ms, and
-		 * read the end value.
-		 */
-		local_irq_save(flags);
-		tsc1 = tsc_read_refs(&pm1, hpet ? &hpet1 : NULL);
 		tsc_pit_khz = pit_calibrate_tsc();
-		tsc2 = tsc_read_refs(&pm2, hpet ? &hpet2 : NULL);
-		local_irq_restore(flags);
 
 		/* Pick the lowest PIT TSC calibration so far */
 		tsc_pit_min = min(tsc_pit_min, tsc_pit_khz);
 
-		/* hpet or pmtimer available ? */
-		if (!hpet && !pm1 && !pm2)
-			continue;
+	}
+
+	tsc2 = tsc_read_refs(&pm2, hpet ? &hpet2 : NULL);
+	local_irq_restore(flags);
 
-		/* Check, whether the sampling was disturbed by an SMI */
-		if (tsc1 == ULLONG_MAX || tsc2 == ULLONG_MAX)
-			continue;
+	/*
+	 * Check that, either HPET or PM timer is available and,
+	 * the sampling was not disturbed by an SMI.
+	 */
+	if ((hpet || pm1) && (tsc1 != ULLONG_MAX && tsc2 != ULLONG_MAX)) {
 
 		tsc2 = (tsc2 - tsc1) * 1000000LL;
 
@@ -259,7 +263,7 @@ unsigned long native_calibrate_tsc(void)
 		}
 
 		do_div(tsc2, tsc1);
-		tsc_ref_min = min(tsc_ref_min, (unsigned long) tsc2);
+		tsc_ref = tsc2;
 	}
 
 	/*
@@ -277,7 +281,7 @@ unsigned long native_calibrate_tsc(void)
 		}
 
 		/* The alternative source failed as well, disable TSC */
-		if (tsc_ref_min == ULONG_MAX) {
+		if (tsc_ref == ULONG_MAX) {
 			printk(KERN_WARNING "TSC: HPET/PMTIMER calibration "
 			       "failed due to SMI disturbance.\n");
 			return 0;
@@ -287,7 +291,7 @@ unsigned long native_calibrate_tsc(void)
 		printk(KERN_INFO "TSC: using %s reference calibration\n",
 		       hpet ? "HPET" : "PMTIMER");
 
-		return tsc_ref_min;
+		return tsc_ref;
 	}
 
 	/* We don't have an alternative source, use the PIT calibration value */
@@ -297,7 +301,7 @@ unsigned long native_calibrate_tsc(void)
 	}
 
 	/* The alternative source failed, use the PIT calibration value */
-	if (tsc_ref_min == ULONG_MAX) {
+	if (tsc_ref == ULONG_MAX) {
 		printk(KERN_WARNING "TSC: HPET/PMTIMER calibration failed due "
 		       "to SMI disturbance. Using PIT calibration\n");
 		return tsc_pit_min;
@@ -305,7 +309,7 @@ unsigned long native_calibrate_tsc(void)
 
 	/* Check the reference deviation */
 	delta = ((u64) tsc_pit_min) * 100;
-	do_div(delta, tsc_ref_min);
+	do_div(delta, tsc_ref);
 
 	/*
 	 * If both calibration results are inside a 5% window, the we
@@ -316,13 +320,13 @@ unsigned long native_calibrate_tsc(void)
 		printk(KERN_INFO "TSC: PIT calibration confirmed by %s.\n",
 		       hpet ? "HPET" : "PMTIMER");
 		printk(KERN_INFO "TSC: using %s calibration value\n",
-		       tsc_pit_min <= tsc_ref_min ? "PIT" :
+		       tsc_pit_min <= tsc_ref ? "PIT" :
 		       hpet ? "HPET" : "PMTIMER");
-		return tsc_pit_min <= tsc_ref_min ? tsc_pit_min : tsc_ref_min;
+		return tsc_pit_min <= tsc_ref ? tsc_pit_min : tsc_ref;
 	}
 
 	printk(KERN_WARNING "TSC: PIT calibration deviates from %s: %lu %lu.\n",
-	       hpet ? "HPET" : "PMTIMER", tsc_pit_min, tsc_ref_min);
+	       hpet ? "HPET" : "PMTIMER", tsc_pit_min, tsc_ref);
 
 	/*
 	 * The calibration values differ too much. In doubt, we use