Messages in this thread Patch in this message | | | From | Linus Torvalds <> | Date | Mon, 16 Jan 2012 16:18:46 -0800 | Subject | Re: [patch] x86, tsc: fix SMI induced variation in quick_pit_calibrate() |
| |
On Mon, Jan 16, 2012 at 12:15 PM, Suresh Siddha <suresh.b.siddha@intel.com> wrote: > Linus, We are seeing NTP failures on a big cluster as a result of big > variation in calibrated TSC values. Our debug showed that it is indeed > because of the SMI and its effect on quick pit calibration. Appended > patch helps fix it. It ran over the weekend boot tests with out any > failures.
Ok, I think your patch is wrong.
HOWEVER.
I think it may be *close* to right.
So what happens is that right now we calculate the "deltatsc" over the last PIT read - the one that returned the new MSB.
But what we *should* do is to calculate deltatsc over the *two* last PIT reads - so that we have the time delay over seeing both the old MSB _and_ seeign the new one. That's the true measure of how precisely we caught the "MSB changed" thing, after all!
So I think your patch is a total hack that just compares the two last TSC deltas, but it's actually close to the "correct' thing in that it does start taking the time to see the last of the previous MSB into account.
So this patch changes pit_expect_msb() so that
- the 'tsc' is the TSC in between the two reads that read the MSB change from the PIT (same as before)
- the 'delta' is the difference in TSC from *before* the MSB changed to *after* the MSB changed.
Now the delta is twice as big as before (it covers four PIT accesses, roughly 4us), so the comments might have to be updated to match, but the rest of the code should "just work" (except it might loop a bit longer, and maybe it gives closer to 250 ppm precision).
Does this fix it for you? I have NOT tested it in any way.
Linus arch/x86/kernel/tsc.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c index c0dd5b603749..6b742da41304 100644 --- a/arch/x86/kernel/tsc.c +++ b/arch/x86/kernel/tsc.c @@ -290,14 +290,15 @@ static inline int pit_verify_msb(unsigned char val) static inline int pit_expect_msb(unsigned char val, u64 *tscp, unsigned long *deltap) { int count; - u64 tsc = 0; + u64 tsc = 0, prev_tsc = 0; for (count = 0; count < 50000; count++) { if (!pit_verify_msb(val)) break; + prev_tsc = tsc; tsc = get_cycles(); } - *deltap = get_cycles() - tsc; + *deltap = get_cycles() - prev_tsc; *tscp = tsc; /* | |