Re: non-invariant tsc and cputicker

On Friday 03 December 2010 06:47 pm, Andriy Gapon wrote:
> on 03/12/2010 22:03 Jung-uk Kim said the following:
> > 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.
>
> I fail to see how this corrects the calculations (cpu tick
> accumulation) in !invariant_tsc case.

Sorry, I interpreted your problem backwards. :-(

> >> 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. :-(
>
> I don't think that init_TSC() is called earlier than the code that
> probes CPU features.  After all, presence of TSC is another CPU
> feature.

If my understanding is correct, your patch uses the dummy timecounter 
until a real timecounter is chosen.  When a real timecounter is set, 
tc_cpu_ticks() changes the frequency naturally.  How are you going to 
solve this problem?  What should we do when a user set a new 
timecounter hardware via "sysctl kern.timecounter.hardware"?  I don't 
think it is any better than current code.  Am I missing 
something? :-(

Jung-uk Kim