[lkml]   [2009]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    Patch in this message
    SubjectRe: [GIT PULL] Additional x86 fixes for 2.6.31-rc5

    On Fri, 31 Jul 2009, H. Peter Anvin wrote:
    > 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

    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?


    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))
    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;
    printk("Fast TSC calibration failed\n");

     \ /
      Last update: 2009-10-18 23:28    [W:0.025 / U:68.516 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site