[PATCH] tsc_khz= boot option to avoid TSC calibration variance

[PATCH] tsc_khz= boot option to avoid TSC calibration variance

[john stultz]

All, 

Despite recent tweaking, TSC calibration variance is still biting users
who care about keeping close sync with NTP servers over reboots.

Here's a recent example:
http://lkml.indiana.edu/hypermail/linux/kernel/0905.0/02061.html

The problem is, each reboot, we have to calibrate the TSC, and any
error, regardless of how small, in the calibrated freq has to be
corrected for by NTP. Assuming the error is within 500ppm NTP can
correct this, but until it finds the proper correction value for the new
TSC freq, users may see time offsets from the NTP server.

In my experience, its fairly easy to see 100khz variance from reboot to
reboot with 2.6.30-rc.

While I think its worth trying to improve the calibration further, there
will likely be a trade-off between very accurate calibration and fast
boot times.

To mitigate this, I wanted to provide a tsc_khz= boot option. This would
allow users to set the tsc_khz value at boot-up, assuming they are
within 1Mhz of the calibrated value (to protect against bad values).
Once the tsc_khz value is set in grub, the box will always boot with the
same value, so the NTP drift value prior to reboot will still be correct
after rebooting.

Thanks to George Spelvin for the idea:
	http://lkml.indiana.edu/hypermail/linux/kernel/0905.0/02807.html

Also thanks to George Spelvin for noticing and fixing the bogus
frequency comparison check in my original RFC'ed patch. This version of
the patch includes his much better comparison.

Signed-off-by: John Stultz <johnstul@us.ibm.com>

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index e87bdbf..4c5d6aa 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2402,6 +2402,13 @@ and is between 256 and 4096 characters. It is defined in the file
 			Used to enable high-resolution timer mode on older
 			hardware, and in virtualized environment.
 
+	tsc_khz=	[x86] Set the TSC freq value.
+			Format: <khz>
+			Used to avoid TSC calibration error.
+			The specified tsc_khz value must be accurate to the
+			calibrated tsc value by 0.1%. Otherwise the calibrated
+			value will be used.
+
 	turbografx.map[2|3]=	[HW,JOY]
 			TurboGraFX parallel port interface
 			Format:
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index d57de05..d47c871 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -825,15 +825,40 @@ static void __init init_tsc_clocksource(void)
 	clocksource_register(&clocksource_tsc);
 }
 
+unsigned long tsc_khz_specified;
+static int __init tsc_khz_specified_setup(char *str)
+{
+	tsc_khz_specified = simple_strtoul(str, NULL, 0);
+	return 1;
+}
+
+__setup("tsc_khz=", tsc_khz_specified_setup);
+
+
 void __init tsc_init(void)
 {
 	u64 lpj;
 	int cpu;
+	long difference;
 
 	if (!cpu_has_tsc)
 		return;
 
 	tsc_khz = calibrate_tsc();
+
+	/*
+	 * If the calibrated TSC freq and user specified
+	 * TSC freq are close enough, pick the what the
+	 * user told us.
+	 */
+	difference = abs(tsc_khz - tsc_khz_specified);
+	if (difference <= tsc_khz >> 10) {      /* 1/1024 = 976 ppm */
+		printk(KERN_INFO "Using user defined TSC freq: %lu.%03lu MHz\n",
+			tsc_khz_specified/1000,
+			tsc_khz_specified%1000);
+		tsc_khz = tsc_khz_specified;
+	}
+
 	cpu_khz = tsc_khz;
 
 	if (!tsc_khz) {

Re: [PATCH] tsc_khz= boot option to avoid TSC calibration variance

[Ulrich Windl]

On 11 May 2009 at 19:12, john stultz wrote:

> All, 
> 
> Despite recent tweaking, TSC calibration variance is still biting users
> who care about keeping close sync with NTP servers over reboots.
> 
> Here's a recent example:
> http://lkml.indiana.edu/hypermail/linux/kernel/0905.0/02061.html
> 
> The problem is, each reboot, we have to calibrate the TSC, and any
> error, regardless of how small, in the calibrated freq has to be
> corrected for by NTP. Assuming the error is within 500ppm NTP can
> correct this, but until it finds the proper correction value for the new
> TSC freq, users may see time offsets from the NTP server.
> 
> In my experience, its fairly easy to see 100khz variance from reboot to
> reboot with 2.6.30-rc.
> 
> While I think its worth trying to improve the calibration further, there
> will likely be a trade-off between very accurate calibration and fast
> boot times.
> 
> To mitigate this, I wanted to provide a tsc_khz= boot option. This would
> allow users to set the tsc_khz value at boot-up, assuming they are
> within 1Mhz of the calibrated value (to protect against bad values).
> Once the tsc_khz value is set in grub, the box will always boot with the
> same value, so the NTP drift value prior to reboot will still be correct
> after rebooting.

[...]

Hi everyone!

I was thinking a little bit on the issue, and I wondered (assuming the TSC 
calibration and the hardware is somewhat stable) whether one should invent a 
mechanism similar to the RTC or random pool; i.e. load the last value that was 
stored or accumulated before. For the TSC calibaration one could build the average 
of previous calibration values (if the value jumps between different numbers).
That would require a sysctl (or equivalent) interface however.

Regards,
Ulrich

Re: [PATCH] tsc_khz= boot option to avoid TSC calibration variance

[Serge Belyshev]

john stultz <johnstul@us.ibm.com> writes:

> Once the tsc_khz value is set in grub, the box will always boot with the
                                                      ^^^^^^
> same value, so the NTP drift value prior to reboot will still be correct
> after rebooting.
No, it won't, because...

>  	tsc_khz = calibrate_tsc();
> +
> +	/*
> +	 * If the calibrated TSC freq and user specified
> +	 * TSC freq are close enough, pick the what the
> +	 * user told us.
> +	 */
> +	difference = abs(tsc_khz - tsc_khz_specified);
> +	if (difference <= tsc_khz >> 10) {      /* 1/1024 = 976 ppm */
> +		printk(KERN_INFO "Using user defined TSC freq: %lu.%03lu MHz\n",
> +			tsc_khz_specified/1000,
> +			tsc_khz_specified%1000);
> +		tsc_khz = tsc_khz_specified;
> +	}
> +

... of this "if".

Please *please* don't set arbitrary limits.  Just use user supplied value.

Or at the very least print big red warning if you are going to ignore a user
supplied option (and have another tsc_khz_really= option to override faulty
calibration routine).

Re: [PATCH] tsc_khz= boot option to avoid TSC calibration variance

[john stultz]

On Tue, 2009-05-12 at 10:36 +0200, Ulrich Windl wrote:
> On 11 May 2009 at 19:12, john stultz wrote:
> 
> > All, 
> > 
> > Despite recent tweaking, TSC calibration variance is still biting users
> > who care about keeping close sync with NTP servers over reboots.
> > 
> > Here's a recent example:
> > http://lkml.indiana.edu/hypermail/linux/kernel/0905.0/02061.html
> > 
> > The problem is, each reboot, we have to calibrate the TSC, and any
> > error, regardless of how small, in the calibrated freq has to be
> > corrected for by NTP. Assuming the error is within 500ppm NTP can
> > correct this, but until it finds the proper correction value for the new
> > TSC freq, users may see time offsets from the NTP server.
> > 
> > In my experience, its fairly easy to see 100khz variance from reboot to
> > reboot with 2.6.30-rc.
> > 
> > While I think its worth trying to improve the calibration further, there
> > will likely be a trade-off between very accurate calibration and fast
> > boot times.
> > 
> > To mitigate this, I wanted to provide a tsc_khz= boot option. This would
> > allow users to set the tsc_khz value at boot-up, assuming they are
> > within 1Mhz of the calibrated value (to protect against bad values).
> > Once the tsc_khz value is set in grub, the box will always boot with the
> > same value, so the NTP drift value prior to reboot will still be correct
> > after rebooting.
> 
> [...]
> 
> Hi everyone!
> 
> I was thinking a little bit on the issue, and I wondered (assuming the TSC 
> calibration and the hardware is somewhat stable) whether one should invent a 
> mechanism similar to the RTC or random pool; i.e. load the last value that was 
> stored or accumulated before. For the TSC calibaration one could build the average 
> of previous calibration values (if the value jumps between different numbers).
> That would require a sysctl (or equivalent) interface however.

I still feel that a sysctrl or /sys/ interface for this sort of thing is
overkill. It creates yet another interface we have to manage, and really
doesn't improve the situation more then the boot option would.
Additionally it would require quite a bit of work at the clocksource
level to allow for re-calibration (currently we avoid this by
disqualifying the TSC if it changes freq). 

That said, I've gotten very few positive comments from my patch, so I'm
going to give it one more spin (to address Serge's point) and if folks
are still feeling blah about it I'll stop pushing it.

thanks
-john

Re: [PATCH] tsc_khz= boot option to avoid TSC calibration variance

[Thomas Gleixner]

On Tue, 12 May 2009, john stultz wrote:
> > stored or accumulated before. For the TSC calibaration one could build the average 
> > of previous calibration values (if the value jumps between different numbers).
> > That would require a sysctl (or equivalent) interface however.
> 
> I still feel that a sysctrl or /sys/ interface for this sort of thing is
> overkill. It creates yet another interface we have to manage, and really
> doesn't improve the situation more then the boot option would.
> Additionally it would require quite a bit of work at the clocksource
> level to allow for re-calibration (currently we avoid this by
> disqualifying the TSC if it changes freq). 
> 
> That said, I've gotten very few positive comments from my patch, so I'm
> going to give it one more spin (to address Serge's point) and if folks
> are still feeling blah about it I'll stop pushing it.

I'm fine with the command line option, but I refuse to add some sys/
thingy which makes us add extra calibration stuff.

Honestly all this is just the futile attempt to fix the flaws of NTP
via (super)user interaction.

Darn, it can not be that hard to adjust the math to do what you think
it should do. I'm not an expert on that NTP stuff, but blindly
stuffing the last known value into the kernel and expect that the
calibration value did not change is nonsense.

You know the calibration value which created the last known parameters
and you want an extra interface to inject this last known calibration
value into the kernel instead of doing the math of adjusting the NTP
parameters according to the change of calibration values ?

Thanks,

	tglx

Re: [PATCH] tsc_khz= boot option to avoid TSC calibration variance

[john stultz]

On Wed, 2009-05-13 at 00:42 +0200, Thomas Gleixner wrote:
> On Tue, 12 May 2009, john stultz wrote:
> > > stored or accumulated before. For the TSC calibaration one could build the average 
> > > of previous calibration values (if the value jumps between different numbers).
> > > That would require a sysctl (or equivalent) interface however.
> > 
> > I still feel that a sysctrl or /sys/ interface for this sort of thing is
> > overkill. It creates yet another interface we have to manage, and really
> > doesn't improve the situation more then the boot option would.
> > Additionally it would require quite a bit of work at the clocksource
> > level to allow for re-calibration (currently we avoid this by
> > disqualifying the TSC if it changes freq). 
> > 
> > That said, I've gotten very few positive comments from my patch, so I'm
> > going to give it one more spin (to address Serge's point) and if folks
> > are still feeling blah about it I'll stop pushing it.
> 
> I'm fine with the command line option, but I refuse to add some sys/
> thingy which makes us add extra calibration stuff.
> 
> Honestly all this is just the futile attempt to fix the flaws of NTP
> via (super)user interaction.
> 
> Darn, it can not be that hard to adjust the math to do what you think
> it should do. I'm not an expert on that NTP stuff, but blindly
> stuffing the last known value into the kernel and expect that the
> calibration value did not change is nonsense.

Well, only nonsense when using the TSC. Given that every other bit of
hardware either sticks to a fixed freq or provides its frequency to the
OS.

And the fact that boot-to-boot the TSC calibration code provides such
varying results illustrates how poor our calibration is, or how poor the
TSC's interface is for not providing its freq.

That said, the TSC is fast and fine grained, so its hard to tell folks
not to use it. Hard enough to convince people to avoid it when its
halting and changing frequency, and really can't be used for
timekeeping.

> You know the calibration value which created the last known parameters
> and you want an extra interface to inject this last known calibration
> value into the kernel instead of doing the math of adjusting the NTP
> parameters according to the change of calibration values ?

Again, I'd really not suggest NTP try to handle TSC calibration error
compensation itself. Either through the kernel or internally.

A) It would have to add linux-x86 specific kludge code
B) It would have to know to look at which clocksource was being used and
decide what to do from there (since ACPI_PM, HPET, PIT and even jiffies
doesn't need this extra tweaking).

Instead I propose, 
1) Improving the calibration code as best we can, given the time
constraints at boot-up.

A neat idea from Miroslav Lichvar: Round the TSC calibration results to
the nearest 100ppm. Although I suspect we already see variations beyond
100ppm, so some tweaking would probably be necessary. I'll be looking at
this option soon.

2) Provide a workaround (that doesn't create some sort of userland API
we have to manage forever) for users who really care about the
super-fine details of tsc calibration and its interactions with NTP.

This patch only provides #2 above.

thanks
-john

Re: [PATCH] tsc_khz= boot option to avoid TSC calibration variance

[john stultz]

On Tue, 2009-05-12 at 17:20 +0400, Serge Belyshev wrote:
> Please *please* don't set arbitrary limits.  Just use user supplied value.

Ok, fair enough. As I was not trying to deal with incorrect calibration
results, just calibration variance, I was hoping to avoid dealing with
error reports where users pushed the tsc_khz value outside of a sane
range.

But I guess throwing a warning when its outside of a sane range is
better then ignoring user defined boot options.

thanks
-john

So one more time....

Despite recent tweaking, TSC calibration variance is still biting users
who care about keeping close sync with NTP servers over reboots.

Here's a recent example:
http://lkml.indiana.edu/hypermail/linux/kernel/0905.0/02061.html

The problem is, each reboot, we have to calibrate the TSC, and any
error, regardless of how small, in the calibrated freq has to be
corrected for by NTP. Assuming the error is within 500ppm NTP can
correct this, but until it finds the proper correction value for the new
TSC freq, users may see time offsets from the NTP server.

In my experience, its fairly easy to see 100khz variance from reboot to
reboot with 2.6.30-rc.

While I think its worth trying to improve the calibration further, there
will likely be a trade-off between very accurate calibration and fast
boot times.

To mitigate this, I wanted to provide a tsc_khz= boot option. This would
allow users to set the tsc_khz value at boot-up, assuming they are
within 1Mhz of the calibrated value (to protect against bad values).
Once the tsc_khz value is set in grub, the box will always boot with the
same value, so the NTP drift value prior to reboot will still be correct
after rebooting.

Thanks to George Spelvin for the idea:
        http://lkml.indiana.edu/hypermail/linux/kernel/0905.0/02807.html

Also thanks to George Spelvin for noticing and fixing the bogus
frequency comparison check in my original RFC'ed patch. This version of
the patch includes his much better comparison.

Also thanks to Serge Belyshev for suggesting instead of ignoring out of
range values, to always use the user provided tsc_khz value but throw a
warning.

Signed-off-by: John Stultz <johnstul@us.ibm.com>

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index e87bdbf..cc5b2c1 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2402,6 +2402,13 @@ and is between 256 and 4096 characters. It is defined in the file
 			Used to enable high-resolution timer mode on older
 			hardware, and in virtualized environment.
 
+	tsc_khz=	[x86] Set the TSC freq value.
+			Format: <khz>
+			Used to override the calibrated TSC freq.
+			This can be useful to avoid TSC calibration error
+			causing problems with NTP synchronization across
+			reboots.
+
 	turbografx.map[2|3]=	[HW,JOY]
 			TurboGraFX parallel port interface
 			Format:
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index d57de05..31a1b27 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -825,6 +825,16 @@ static void __init init_tsc_clocksource(void)
 	clocksource_register(&clocksource_tsc);
 }
 
+unsigned long tsc_khz_specified;
+static int __init tsc_khz_specified_setup(char *str)
+{
+	tsc_khz_specified = simple_strtoul(str, NULL, 0);
+	return 1;
+}
+
+__setup("tsc_khz=", tsc_khz_specified_setup);
+
+
 void __init tsc_init(void)
 {
 	u64 lpj;
@@ -834,6 +844,25 @@ void __init tsc_init(void)
 		return;
 
 	tsc_khz = calibrate_tsc();
+
+	if (tsc_khz_specified) {
+		long difference = abs(tsc_khz - tsc_khz_specified);
+		/*
+		 * Make a fair amount of noise if tsc_khz boot option
+		 * is more then 0.1% off of the calibrated tsc_khz value
+		 */
+		if (difference > tsc_khz/1000) {
+			printk(KERN_WARNING "WARNING! Specified tsc_khz is"
+				" more then 0.1%% off from the calibrated TSC"
+				" freq.\n\tThis may cause severe time"
+				" problems.\n");
+		}
+		printk(KERN_INFO "Using user defined TSC freq: %lu.%03lu MHz\n",
+			tsc_khz_specified/1000,
+			tsc_khz_specified%1000);
+		tsc_khz = tsc_khz_specified;
+	}
+
 	cpu_khz = tsc_khz;
 
 	if (!tsc_khz) {

Re: [PATCH] tsc_khz= boot option to avoid TSC calibration variance

[Ulrich Windl]

On 13 May 2009 at 0:42, Thomas Gleixner wrote:

[...]
> I'm fine with the command line option, but I refuse to add some sys/
> thingy which makes us add extra calibration stuff.
> 
> Honestly all this is just the futile attempt to fix the flaws of NTP
> via (super)user interaction.

This has nothing to do with flaws in NTP (AFAIK).

> 
> Darn, it can not be that hard to adjust the math to do what you think
> it should do. I'm not an expert on that NTP stuff, but blindly
> stuffing the last known value into the kernel and expect that the
> calibration value did not change is nonsense.

Well, the assumption most likely is that if the hardware didn't change, kernel's 
calibration results also shouldn't change.

> 
> You know the calibration value which created the last known parameters
> and you want an extra interface to inject this last known calibration
> value into the kernel instead of doing the math of adjusting the NTP
> parameters according to the change of calibration values ?

I think, the wole issue is somewhat "cosmetics of statistics": People don't want 
to see a "clock frequency hops" after rebooting the machine (and restarting NTP). 
Usually there are thermal effects anyway, so what... Maybe it's just about "making 
everything perfect". For most people it's "good enough" already probably.

Regards,
Ulrich



> 
> Thanks,
> 
> 	tglx
> 
> 

Re: [PATCH] tsc_khz= boot option to avoid TSC calibration variance

[Petri Kaukasoina]

On Wed, May 13, 2009 at 09:59:59AM +0200, Ulrich Windl wrote:
> On 13 May 2009 at 0:42, Thomas Gleixner wrote:
> 
> > Honestly all this is just the futile attempt to fix the flaws of NTP
> > via (super)user interaction.
> 
> This has nothing to do with flaws in NTP (AFAIK).

Correct. When ntp is NOT RUNNING at all, if the TSC calibration gives a MHz
value differing e.g. 100 ppm from the previous value, the clock will run at
a different speed, 100 ppm making a difference of 17 seconds a day. On the
other hand, if ntp is running, it will eventually compensate the offset. But
it takes quite a long time to settle, IMO too long for a quality time
server.

The real changes in the clock frequency between bootups are in the order of
1 ppm, but the TSC calibration can be even 100 ppm off randomly.

-Petri

Re: [PATCH] tsc_khz= boot option to avoid TSC calibration variance

[Petri Kaukasoina]

On Tue, May 12, 2009 at 04:02:46PM -0700, john stultz wrote:
> A neat idea from Miroslav Lichvar: Round the TSC calibration results to
> the nearest 100ppm. Although I suspect we already see variations beyond
> 100ppm, so some tweaking would probably be necessary. I'll be looking at
> this option soon.

This does not sound good. Say, e.g., the variations were 40 ppm. Then, with
luck, for some hardware you can get rid of the variations completely. But,
if the average value for some hardware is near the value where you decide
either to round up or down, it will amplify the variation to 100 ppm. I
think you can't reduce the average of the variations for an ensemble of
computers this way.

> 2) Provide a workaround (that doesn't create some sort of userland API
> we have to manage forever) for users who really care about the
> super-fine details of tsc calibration and its interactions with NTP.
> 
> This patch only provides #2 above.

I have used this method on my campus ntp time servers for over a year now. I
just didn't know how to make a command line parameter, so I just wrote a
constant value to cpu_khz in tsc.c, instead. The command line option is much
better, of course.

-Petri

Re: [PATCH] tsc_khz= boot option to avoid TSC calibration variance

[Petri Kaukasoina]

On Wed, May 13, 2009 at 12:23:35PM +0300, Petri Kaukasoina wrote:
> 100 ppm making a difference of 17 seconds a day.

Sorry, 200 ppm is 17 seconds. 100 ppm is just 8.6 s.

-Petri

Re: [PATCH] tsc_khz= boot option to avoid TSC calibration variance

[George Spelvin]

> No, it won't, because...  
> <cde snippet>
> ... of this "if".
> 
> Please *please* don't set arbitrary limits.  Just use user supplied value.
> 
> Or at the very least print big red warning if you are going to ignore a user
> supplied option (and have another tsc_khz_really= option to override faulty
> calibration routine).

I'd like to disagree.  The calibration is accurate and reliable.
It has worked without any override for millions of users for many
years.  What is the plausible reason to need to set it to a wildly
divergent value?

On the other hand, what if my CPU gets fried and I replace it with one
with a different clock multiplier?  Or the motherboard fries and I move
the boot hard drive to a different machine?  That's a situation that I
have found myself in.  And I don't want to figure out how to edit the
LILO options before booting the machine, even if I remember to do it
at all; this is an emergency replacement.

In such a case, having the kernel believe the command line could result
in it not booting, or the system not working properly.

Much better that it basically work, even if some fine-tuning remains
to be done.

And that's the idea: this is a fine-tuning option.  If the calibration
says the option value is grossly wrong, then something strange has
happened, and the kernel calibration value is safer.

If you think a coarse-tuning option is required, I can extend the patch
so that a trailing ! (e.g. "tsc_khz=3000000!") disables the sanity check.
Bit I'd still like to see an argument for why it is required.  Remember,
it would be far better to make the kernel self-calibration more accurate,
so people don't even have to use this option.  It's only there because
it's the simplest way (simplest for the lazy programmer, not the user!)
to guarantee getting exactly the same value each boot.

>From a *user* point of view, I'd rather the kernel did it automagically
and I didn't need to futz with the kernel command line at all.