Messages in this thread | | | Date | Fri, 31 Jul 2009 21:57:05 +0200 | From | Ingo Molnar <> | Subject | Re: [GIT PULL] Additional x86 fixes for 2.6.31-rc5 |
| |
* 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
| |