cvs commit: src/sys/i386/isa clock.c

[Mike Silbersack]

On Sat, 1 May 2004, Bruce Evans wrote:

> The TSC calibration is lower level than the bug fixed in the commit.
> It just uses DELAY(1000000) without even adjusting for DELAY()'s
> overhead.  Perhaps acpi is working "better" and throttling the cpu
> during the calibration, or something is interrupting the calibration.
> I hope FreeBSD still has interrupts disabled at that point, but there
> may be SMM interrupts especially with acpi.

Ok, I've read through the calibration code + DELAY now, and I see how much
the i8254 stinks.  But still, there's one piece of DELAY that seems
suspicious to me:

                delta = prev_tick - tick;
                prev_tick = tick;
                if (delta < 0) {
                        delta += timer0_max_count;
                        /*
                         * Guard against timer0_max_count being wrong.
                         * This shouldn't happen in normal operation,
                         * but it may happen if set_timer_freq() is
                         * traced.
                         */
                        if (delta < 0)
                                delta = 0;
                }
                ticks_left -= delta;

So this says to me that if timer wraps and we miss it, we assume that an
entire cycle has passed.  However, suppose that on our given machine that
we take 5 counter ticks to run through this loop.  In that case, if we
last read the counter when it was at 4, we'd then read it again at 65535,
and fall into the if statement.  We'd then be subtracting timer0_max_count
from ticks_list, when we really should have subtracted 5!

Wouldn't be it be more accurate to restructure the code as follows?

                delta = prev_tick - tick;
                if (delta < 0) {
                        delta += tick;
                }
                prev_tick = tick;
                ticks_left -= delta;

This assumes that if we missed reading the count before it looped probably
only missed it by a small amount, rather than by a large amount.

On my Athlon 1800+ this changes

CPU: AMD Athlon(tm) XP 1800+ (1526.85-MHz 686-class CPU)

to

CPU: AMD Athlon(tm) XP 1800+ (1527.00-MHz 686-class CPU)

And when run with the troublesome Mobile Celeron 1.6G:

CPU: Mobile Intel(R) Celeron(R) CPU 1.60GHz (797.19-MHz 686-class CPU)

Wow!  I think there's something wrong with my patch. :)

And more seriously, I guess the wrapping case is happening A LOT!

Could we possibly look at how many interrupts have happened during the
wrapping in order to make this part of DELAY more accurate?  The code
that's present in the code above seems just as wrong as my patch does.

Mike "Silby" Silbersack