From owner-cvs-all@FreeBSD.ORG Sat May 1 03:01:45 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 CCAC516A4E5 for ; Sat, 1 May 2004 03:01:45 -0700 (PDT) Received: from relay.pair.com (relay.pair.com [209.68.1.20]) by mx1.FreeBSD.org (Postfix) with SMTP id 2432C43D48 for ; Sat, 1 May 2004 03:01:41 -0700 (PDT) (envelope-from silby@silby.com) Received: (qmail 69624 invoked from network); 1 May 2004 10:01:38 -0000 Received: from niwun.pair.com (HELO localhost) (209.68.2.70) by relay.pair.com with SMTP; 1 May 2004 10:01:38 -0000 X-pair-Authenticated: 209.68.2.70 Date: Sat, 1 May 2004 05:01:36 -0500 (CDT) From: Mike Silbersack To: Bruce Evans In-Reply-To: <20040501155815.L19822@gamplex.bde.org> Message-ID: <20040501044209.L704@odysseus.silby.com> References: <200404272003.i3RK3RFZ048001@repoman.freebsd.org> <20040430221434.J749@odysseus.silby.com> <20040501155815.L19822@gamplex.bde.org> 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 10:01:46 -0000 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