Re: Regression in 2.6.27 caused by commit bfc0f59

[Linus Torvalds]

On Mon, 1 Sep 2008, Thomas Gleixner wrote:
> 
> If the PIT interrupts are delayed by SMM code

Btw, this sentence of yours just doesn't seem to make much sense.

The thing is, the calibration code doesn't even use interrupts. It just 
reads the PIT timer value. 

And the thing is, the 64-bit code really does things that the 32-bit code 
does _not_ do. 

Just as an example, the old 32-bit code (the thing that was deleted) tried 
to actually round things correctly, while the 64-bit code does not. Look 
at what the 64-bit code does:

        outb((inb(0x61) & ~0x02) | 0x01, 0x61);

        outb(0xb0, 0x43);
        outb((CLOCK_TICK_RATE / (1000 / 50)) & 0xff, 0x42);
        outb((CLOCK_TICK_RATE / (1000 / 50)) >> 8, 0x42);
        tr1 = get_cycles();  
        while ((inb(0x61) & 0x20) == 0);
        tr2 = get_cycles();

and notice three things:

 - no comments (can you actually see what it does?)

 - no rounding of the inevitable errors of trying to wait 1/50th of a 
   second

 - not a single try to even account for the fact that there might be 
   things going on that perturb the value.

Now, look at what the 32-bit code _used_ to do. The good code. The code 
that was _deleted_.

Really. I don't think you really even looked. It did:

        /* run 3 times to ensure the cache is warm and to get an accurate reading */
        for (i = 0; i < 3; i++) {
                mach_prepare_counter();
                rdtscll(start);
                mach_countup(&count);
                rdtscll(end);

		.. ignore bad values ..

                /*
                 * We want the minimum time of all runs in case one of them
                 * is inaccurate due to SMI or other delay
                 */
                delta64 = min(delta64, (end - start));
	}

and if you actually look at those counter things, you'll see:

	#define CALIBRATE_TIME_MSEC 30 /* 30 msecs */
	#define CALIBRATE_LATCH \
	        ((CLOCK_TICK_RATE * CALIBRATE_TIME_MSEC + 1000/2)/1000)
	
	static inline void mach_prepare_counter(void)
	{
	       /* Set the Gate high, disable speaker */
	        outb((inb(0x61) & ~0x02) | 0x01, 0x61);

        	/*
	         * Now let's take care of CTC channel 2
	         *
	         * Set the Gate high, program CTC channel 2 for mode 0,
	         * (interrupt on terminal count mode), binary count,
	         * load 5 * LATCH count, (LSB and MSB) to begin countdown.
	         *
	         * Some devices need a delay here.
	         */
	        outb(0xb0, 0x43);                       /* binary, mode 0, LSB/MSB, Ch 2 */
	        outb_p(CALIBRATE_LATCH & 0xff, 0x42);   /* LSB of count */
	        outb_p(CALIBRATE_LATCH >> 8, 0x42);       /* MSB of count */
	}

ie look how it actually tries to round to the nearest latch value, an how 
it actually comments on what it is doing.

Now, which piece of code is better?

Honestly?

Have you tried the better version (for example, boot a 32-bit kernel 
_before_ the unification on that machine to try).

				Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/