Re: [PATCH] make tsc=reliable override boot time stability checks

[Alok Kataria <akataria@vmware.com>]

Hi John,

On Mon, 2009-08-17 at 16:40 -0700, john stultz wrote:
> On Mon, 2009-08-17 at 16:27 -0700, Stephen Hemminger wrote:
> > On Mon, 17 Aug 2009 16:17:54 -0700
> > john stultz <johnstul@us.ibm.com> wrote:
> > 
> > > On Mon, 2009-08-17 at 16:02 -0700, Stephen Hemminger wrote:
> > > > This adds tsc, but makes it first?  it is reliable, but do I want
> > > > to make it most important?
> > > > 
> > > > $ cat /sys/devices/system/clocksource/clocksource0/available_clocksource
> > > > tsc acpi_pm
> > > 
> > > 
> > > Well, if you're overriding the system saying that its safe, then sure,
> > > its better then anything else, why wouldn't we?
> > > 
> > 
> > That's acceptable, maybe add change to Documentation/kernel-parameters.txt
> > 
> > 	tsc=		Disable clocksource-must-verify flag for TSC.
> > 			Format: <string>
> > 			[x86] reliable: mark tsc clocksource as reliable and
> >                         makes tsc the default clocksource; this
> > 			disables clocksource verification at runtime.
> > 			Used to enable high-resolution timer mode on older
> > 			hardware, and in virtualized environment.
> > 
> 
> Sounds good. Thanks so much for the bug report and testing!
> 
> 
> This patch makes the tsc=reliable option disable the boot time stability
> checks. Currently the option only disables the runtime watchdog checks.
> This change allows folks who want to override the boot time TSC
> stability checks and use the TSC when the system would otherwise
> disqualify it.
> 
> There still are some situations that the TSC will be disqualified, such
> as cpufreq scaling. But these are situations where the box will hang if
> allowed.
> 

I had purposefully kept the tsc=reliable separate from the TSC
synchronous checks. 
With this patch TSC is marked as usable though the hardware below
doesn't export a CONSTANT_TSC, it might not be a problem generally, but
since TSC has the highest rating, don't you think that timekeeping might
be wayward on such systems ? 
Having said that, I don't think I have a particular problem with the
patch as far as we are explicitly mentioning the fact that TSC=reliable
means TSC is blindly trusted on this system, and time might be little
off on some systems.


Alok


> Patch also includes a fix for an issue found by Thomas Gleixner, where
> the TSC disqualification message wouldn't be printed after a call to
> unsynchronized_tsc().


> 
> I'd recommend queuing this for 2.6.32, since it probably should get more
> testing then we have time for in 2.6.31.
> 
> thanks
> -john
> 
> 
> Signed-off-by: John Stultz <johnstul@us.ibm.com>
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 7936b80..4c6b415 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2484,12 +2484,13 @@ and is between 256 and 4096 characters. It is defined in the file
>  			Format:
>  			<io>,<irq>,<dma>,<dma2>,<sb_io>,<sb_irq>,<sb_dma>,<mpu_io>,<mpu_irq>
>  
> -	tsc=		Disable clocksource-must-verify flag for TSC.
> +	tsc=		Disable clocksource stability checks for TSC.
>  			Format: <string>
>  			[x86] reliable: mark tsc clocksource as reliable, this
> -			disables clocksource verification at runtime.
> -			Used to enable high-resolution timer mode on older
> -			hardware, and in virtualized environment.
> +			disables clocksource verification at runtime, as well
> +			as the stability checks done at bootup.	Used to enable
> +			high-resolution timer mode on older hardware, and in
> +			virtualized environment.
>  
>  	turbografx.map[2|3]=	[HW,JOY]
>  			TurboGraFX parallel port interface
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 71f4368..648fb26 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -825,6 +825,9 @@ __cpuinit int unsynchronized_tsc(void)
>  
>  	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
>  		return 0;
> +
> +	if (tsc_clocksource_reliable)
> +		return 0;
>  	/*
>  	 * Intel systems are normally all synchronized.
>  	 * Exceptions must mark TSC as unstable:
> @@ -832,10 +835,10 @@ __cpuinit int unsynchronized_tsc(void)
>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
>  		/* assume multi socket systems are not synchronized: */
>  		if (num_possible_cpus() > 1)
> -			tsc_unstable = 1;
> +			return 1;
>  	}
>  
> -	return tsc_unstable;
> +	return 0;
>  }
>  
>  static void __init init_tsc_clocksource(void)
> 
> 
>