Date: Fri, 3 Dec 2010 15:03:59 -0500 From: Jung-uk Kim <jkim@FreeBSD.org> To: Andriy Gapon <avg@freebsd.org> Cc: freebsd-current@freebsd.org Subject: Re: non-invariant tsc and cputicker Message-ID: <201012031504.02532.jkim@FreeBSD.org> In-Reply-To: <4CF93395.3060601@freebsd.org> References: <4CF92852.20705@freebsd.org> <201012031305.53750.jkim@FreeBSD.org> <4CF93395.3060601@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201012031504.02532.jkim>