Skip site navigation (1)Skip section navigation (2)
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>