From owner-cvs-all@FreeBSD.ORG Sat May 1 06:30:40 2004 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 6E64B16A4CE; Sat, 1 May 2004 06:30:40 -0700 (PDT) Received: from mailout2.pacific.net.au (mailout2.pacific.net.au [61.8.0.85]) by mx1.FreeBSD.org (Postfix) with ESMTP id 5939743D39; Sat, 1 May 2004 06:30:39 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from mailproxy1.pacific.net.au (mailproxy1.pacific.net.au [61.8.0.86])i41DUc5v029496; Sat, 1 May 2004 23:30:38 +1000 Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) i41DUYI2011008; Sat, 1 May 2004 23:30:35 +1000 Date: Sat, 1 May 2004 23:30:32 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Mike Silbersack In-Reply-To: <20040501044209.L704@odysseus.silby.com> Message-ID: <20040501214146.N20783@gamplex.bde.org> References: <200404272003.i3RK3RFZ048001@repoman.freebsd.org> <20040430221434.J749@odysseus.silby.com> <20040501044209.L704@odysseus.silby.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: cvs-src@freebsd.org cc: src-committers@freebsd.org cc: cvs-all@freebsd.org cc: John Baldwin Subject: Re: cvs commit: src/sys/i386/isa clock.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 01 May 2004 13:30:40 -0000 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