From owner-freebsd-current@FreeBSD.ORG Fri Dec 3 20:04:10 2010 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from [127.0.0.1] (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by hub.freebsd.org (Postfix) with ESMTP id 916F71065674; Fri, 3 Dec 2010 20:04:10 +0000 (UTC) (envelope-from jkim@FreeBSD.org) From: Jung-uk Kim To: Andriy Gapon Date: Fri, 3 Dec 2010 15:03:59 -0500 User-Agent: KMail/1.6.2 References: <4CF92852.20705@freebsd.org> <201012031305.53750.jkim@FreeBSD.org> <4CF93395.3060601@freebsd.org> In-Reply-To: <4CF93395.3060601@freebsd.org> MIME-Version: 1.0 Content-Disposition: inline Content-Type: Multipart/Mixed; boundary="Boundary-00=_y0U+MLBy9CoocEU" Message-Id: <201012031504.02532.jkim@FreeBSD.org> Cc: freebsd-current@freebsd.org Subject: Re: non-invariant tsc and cputicker X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Dec 2010 20:04:10 -0000 --Boundary-00=_y0U+MLBy9CoocEU Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline On Friday 03 December 2010 01:14 pm, Andriy Gapon wrote: > on 03/12/2010 20:05 Jung-uk Kim said the following: > > On Friday 03 December 2010 12:26 pm, Andriy Gapon wrote: > >> FreeBSD uses cpu_ticks [function pointer] in a few places for a > >> few things like process CPU time accounting. On x86 cpu_ticks > >> always points to rdtsc. If TSC is not invariant that leads to > >> incorrect accounting of "CPU ticks". The code pretends to try to > >> handle changing cpufreq levels, but does that incorrectly. > > > > Arg... Probably it is my fault. :-( > > > >> I think that we could use a selected timecounter instead of > >> "raw" TSC if the latter is not invariant. In this case > >> cpu_ticks calls would be slightly costlier, but always correct. > >> > >> The change is quite trivial: > >> http://people.freebsd.org/~avg/tsc-cputicker.diff > >> > >> What do you think? > > > > Why don't we just fix it properly? > > Patch? :-) Attached. > It seems that it is not too trivial to do and is prone to error > accumulation given how the ticks are added up. > Besides, why using a timecounter would not be a proper fix? Well, it is not that simple, unfortunately. Because init_TSC() is called very early, your patch will select dummy timecounter as a CPU ticker if my memory serves. It is very hard to implement right on x86 arch. :-( Jung-uk Kim > >> P.S. it's probably a good idea to merge i386 and amd64 tsc.c > >> files into a common x86 version, which would be the same as i386 > >> version, which seems to be generic enough. > > > > Agreed. > > Cool! --Boundary-00=_y0U+MLBy9CoocEU Content-Type: text/plain; charset="iso-8859-1"; name="tsc.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="tsc.diff" Index: sys/i386/i386/tsc.c =================================================================== --- sys/i386/i386/tsc.c (revision 216155) +++ sys/i386/i386/tsc.c (working copy) @@ -174,6 +174,9 @@ tsc_levels_changed(void *arg, int unit) int count, error; uint64_t max_freq; + if (tsc_is_invariant) + return; + /* Only use values from the first CPU, assuming all are equal. */ if (unit != 0) return; Index: sys/amd64/amd64/tsc.c =================================================================== --- sys/amd64/amd64/tsc.c (revision 216155) +++ sys/amd64/amd64/tsc.c (working copy) @@ -146,6 +146,9 @@ tsc_levels_changed(void *arg, int unit) int count, error; uint64_t max_freq; + if (tsc_is_invariant) + return; + /* Only use values from the first CPU, assuming all are equal. */ if (unit != 0) return; --Boundary-00=_y0U+MLBy9CoocEU--