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

[Bruce Evans]

On Sat, 1 May 2004, Mike Silbersack wrote:

> 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!

I think you missed that the hardware timer is count-down.  We don't assume
anything about entire counter cycles; we just miss them and wait another
timer0_max_count cycles.  The adjustment by timer0_max count is just to
to convert the counter from count-down to count-up.  it works right for
that:

		delta = 4 - 65535 = -65531
		if (-65531 < 0)
			delta += 65535		// delta = 4
		ticks_left -= 4;

We don't try to determine or guess whether the counter wrapped since
unlike in i8254_get_timecount(), the accuracy of DELAY() is not critical
and determination of wraps would not improve it signifantly but would
have significant cost.  (DELAY() may take arbitrarily longer than
the specified delay since it doesn't and shouldn't mask interrupts,
but it usually doesn't get interrupted for long enough for the counter
to overflow (1/HZ seconds).  If it does get interrupted for longer than
this then it has already delayed for much longer than the specified
delay for any reasonable specified delay so delaying a little longer
won't make much much difference.)

>
> Wouldn't be it be more accurate to restructure the code as follows?
>                 prev_tick = tick;
>
>                 delta = prev_tick - tick;
>                 if (delta < 0) {
>                         delta += tick;
>                 }
>                 prev_tick = tick;
>                 ticks_left -= delta;

That would introduce lots of bugs :-).  The initial delta is always
negative since the timer is count-down.  So in the usual case where
the timer counts down without reaching 0, the above gives
delta = prev_tick - tick + tick = prev_tick.  The average error is
very large (almost the average of prev_tick, which is approx.
tiemr0_max_count / 2).

The second adjustment for the delta < 0 case in the original code is
more technical and could be improved.  getit() is now faked if ddb
is active, so it can be arranged that the second adjustment never occurs.
Perhaps it already doesn't occur.  While investigating this, I noticed
that the fake getit() is buggy.  It implements a count-up timer, but
a count_down timer is needed.  This gives similar bugs to your modified
version (almost timer0_max_count).

> 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. :)

:-)

I would have have expected an error of off by a factor of 550 or so
with HZ=100, since the average value of prev_tick is about
timer0_max_count / 2 which is about 550 for HZ=100.

> 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.

Depends how much more complicated and slower you want it to be :-).
A simple loop using microtime() should be used if the caller wants
maximal accuracy and doesn't care much about speed and doesn't run
at boot time.  There are few or no such callers.

HZ=1000 instead of the default of HZ=10 increases the chance of the
timer wrapping while DELAY() is interrupted.  This shouldn't be a
problem for TSC calibration.

My version of TSC calibration uses a much more precise algorithm.  It
still uses the i8254 since this is the best clock to compare with
for technical reasons.  I will send this in private mail.  You could
also try RELENG_4 to get calibration of the TSC relative to the RTC.
This is more precise than the current calibration, but may be less
accurate since the nominal RTC frequency may be less accurate than
the nominal i8254 frequency.

Bruce