Re: [GIT PULL] Additional x86 fixes for 2.6.31-rc5

[Ingo Molnar <mingo@elte.hu>]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, 31 Jul 2009, H. Peter Anvin wrote:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git x86-fixes-for-linus
> > 
> > Tan, Wei Chong (1):
> >       x86: SMI workaround for pit_expect_msb
> 
> I really think that last one (commit fb34a8ee86) is 
> incorrect.
> 
> We do _not_ want to do that SMI workaround in the loop, 
> and the logic of that patch is broken anyway.
> 
> If an SMI kicks in, then it's true that tscp will be off, 
> but that's what the error term is there for. If the error 
> term is sufficiently small, then we don't care.
> 
> Sure, we might want the error term to be even smaller, but 
> in no way does it actually invalidate any of the logic - 
> the 'tsc' reading is just a guess anyway. Also, I think 
> that the real issue isn't even an SMI - but the fact that 
> in the very last iteration of the loop, there's no 
> serializing instruction _after_ the last 'rdtsc'. So even 
> in the absense of SMI's, we do have a situation where the 
> cycle counter was read without proper serialization.
> 
> So I think the patch does fix something, I just think it's 
> done the wrong way.
> 
> The last check shouldn't be done the way that commit does 
> it. It should be done outside the outer loop, since 
> _inside_ the outer loop, we'll be testing that the PIT has 
> the right MSB value has the right value in the next 
> iteration.
> 
> So only the _last_ iteration is special, because that's 
> the one that will not check the PIT MSB value any more, 
> and because the final 'get_cycles()' isn't serialized.
> 
> In other words:
>  - I'd like to move the PIT MSB check to after the last 
>  iteration, rather
>    than in every iteration
> 
>  - I think we should comment on the fact that it's also a 
>  serializing
>    instruction and so 'fences in' the TSC read.
> 
> Here's a suggested replacement - BUT NOTE THAT IT'S 
> ENTIRELY UNTESTED!
> 
> (Ok, so the patch is bigger than it strictly needs to be - 
> I hate repeating code, so I made that 'read PIT counter 
> twice and compare MSB' be a new helper routine)
> 
> Hmm? What do you guys think?
> 
> 		Linus
> 
> ---
>  arch/x86/kernel/tsc.c |   29 ++++++++++++++++++++++-------
>  1 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 6e1a368..71f4368 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -275,15 +275,20 @@ static unsigned long pit_calibrate_tsc(u32 latch, unsigned long ms, int loopmin)
>   * use the TSC value at the transitions to calculate a pretty
>   * good value for the TSC frequencty.
>   */
> +static inline int pit_verify_msb(unsigned char val)
> +{
> +	/* Ignore LSB */
> +	inb(0x42);
> +	return inb(0x42) == val;
> +}
> +
>  static inline int pit_expect_msb(unsigned char val, u64 *tscp, unsigned long *deltap)
>  {
>  	int count;
>  	u64 tsc = 0;
>  
>  	for (count = 0; count < 50000; count++) {
> -		/* Ignore LSB */
> -		inb(0x42);
> -		if (inb(0x42) != val)
> +		if (!pit_verify_msb(val))
>  			break;
>  		tsc = get_cycles();
>  	}
> @@ -336,8 +341,7 @@ static unsigned long quick_pit_calibrate(void)
>  	 * to do that is to just read back the 16-bit counter
>  	 * once from the PIT.
>  	 */
> -	inb(0x42);
> -	inb(0x42);
> +	pit_verify_msb(0);
>  
>  	if (pit_expect_msb(0xff, &tsc, &d1)) {
>  		for (i = 1; i <= MAX_QUICK_PIT_ITERATIONS; i++) {
> @@ -348,8 +352,19 @@ static unsigned long quick_pit_calibrate(void)
>  			 * Iterate until the error is less than 500 ppm
>  			 */
>  			delta -= tsc;
> -			if (d1+d2 < delta >> 11)
> -				goto success;
> +			if (d1+d2 >= delta >> 11)
> +				continue;
> +
> +			/*
> +			 * Check the PIT one more time to verify that
> +			 * all TSC reads were stable wrt the PIT.
> +			 *
> +			 * This also guarantees serialization of the
> +			 * last cycle read ('d2') in pit_expect_msb.
> +			 */
> +			if (!pit_verify_msb(0xfe - i))
> +				break;
> +			goto success;

Oh, this reminds me of a patch i saw from John(?) a couple 
of months ago that added a magic final pit_verify_msb() 
call, which solved PIT calibration instabilities.

(Or maybe i did that hack when i tried to write an 
auto-error-boundary calibrator?)

I dont think we committed it because it was a 'black magic' 
hack as per observation and nobody realized the side-effect 
of the TSC synchronization which you mention above.

My memories are sketchy but i think the symptom was one 
failed PIT loop every 10 bootups, and it was solved by a 
patch very similar to yours.

So with a proper changlog and testing i'd very much go for 
this on those grounds ago - and if it also solves the 
problem observed by Wei Chong Tan that would be perfect.

[ Unfortunately Thomas (who too saw some PIT calibration
  weirdnesses IIRC) wont be back for some time. ]

	Ingo