> Date: Mon, 5 Aug 2019 16:58:27 +0300 > From: Paul Irofti <[email protected]> > > On Fri, Aug 02, 2019 at 01:29:37PM +0300, Paul Irofti wrote: > > On Mon, Jul 01, 2019 at 10:32:51AM +0200, Mark Kettenis wrote: > > > > Date: Thu, 27 Jun 2019 15:08:00 +0300 > > > > From: Paul Irofti <[email protected]> > > > > > > > > Hi, > > > > > > > > Here is an initial diff, adapted from NetBSD, that synchronizes TSC > > > > clocks across cores. > > > > > > > > CPU0 is the reference clock and all others are skewed. During CPU > > > > initialization the clocks synchronize by keeping a registry of each CPU > > > > clock skewness and adapting the TSC read routine accordingly. > > > > > > > > I choose this implementation over what FreeBSD is doing (which is just > > > > copying Linux really), because it is clean and elegant. > > > > > > > > I would love to hear reports from machines that were broken by this. > > > > Mine, which never exhibited the problem in the first place, run just > > > > fine with the following diff. In fact I am writting this message on one > > > > such machine. > > > > > > > > Also constructive comments are more than welcomed! > > > > > > > > Notes: > > > > > > > > - cpu_counter_serializing() could probably have a better name > > > > (tsc _read for example) > > > > - the PAUSE instruction is probably not needed > > > > - acpi(4) suspend and resume bits are left out on purpose, but should > > > > be trivial to add once the current diff settles > > > > > > > > Paul Irofti > > > > > > I don't think we want to introduce a <machine/tsc.h> header file. > > > > > > The code suffers from some NetBSD-isms, so that'll need to be fixed. > > > I pointed some of them out below. > > > > > > Also, how accurate is your skew detection? What skew is detected on a > > > machine that (supposedly) has the TSCs in sync? The result will be > > > that you actually slightly desync the counters on different CPUs. > > > > > > I think Linux uses the TSC_ADJUST MSR and compares its value across > > > cores. If the skew is small and the TSC_ADJUST values are the same > > > across cores it skips the TSC adjustments. > > > > Hi, > > > > Here is an updated diff with a few bugs eliminated from the previous and > > with most of the concerns I got in private and from Mark fixed. > > > > I will do the TSC_ADJUST_MSR dance in another iteration if the current > > incarnation turns out to be correct for machines suffering from TSCs not > > in sync. > > > > The thing I am mostly worried about now is in the following sum > > > > uint > > tsc_get_timecount(struct timecounter *tc) > > { > > return rdtsc() + curcpu()->cpu_cc_skew; > > } > > > > can one term be executed on one CPU and the other on another? Is there a > > way to protect this from happening other than locking? > > > > I see NetBSD is checking for a change in the number of context switches > > of the current process. > > > > My plan is to have a fix in the tree before 6.6 is released, so I would > > love to hear your thoughts and reports on this. > > > > Thanks, > > Paul > > Hi, > > Here is a third version of the TSC diff that also take into > consideration the suspend-resume path which was ignored by the previous > thus rendering resume broken.
Also... > Index: arch/amd64/amd64/tsc.c > =================================================================== > RCS file: /cvs/src/sys/arch/amd64/amd64/tsc.c,v > retrieving revision 1.11 > diff -u -p -u -p -r1.11 tsc.c > --- arch/amd64/amd64/tsc.c 6 Jun 2019 19:43:35 -0000 1.11 > +++ arch/amd64/amd64/tsc.c 5 Aug 2019 13:54:34 -0000 > @@ -1,8 +1,10 @@ > /* $OpenBSD: tsc.c,v 1.11 2019/06/06 19:43:35 kettenis Exp $ */ > /* > + * Copyright (c) 2008 The NetBSD Foundation, Inc. > * Copyright (c) 2016,2017 Reyk Floeter <[email protected]> > * Copyright (c) 2017 Adam Steen <[email protected]> > * Copyright (c) 2017 Mike Belopuhov <[email protected]> > + * Copyright (c) 2019 Paul Irofti <[email protected]> > * > * Permission to use, copy, modify, and distribute this software for any > * purpose with or without fee is hereby granted, provided that the above > @@ -20,6 +22,7 @@ > #include <sys/param.h> > #include <sys/systm.h> > #include <sys/timetc.h> > +#include <sys/atomic.h> > > #include <machine/cpu.h> > #include <machine/cpufunc.h> > @@ -33,6 +36,13 @@ int tsc_recalibrate; > uint64_t tsc_frequency; > int tsc_is_invariant; > > +int64_t tsc_drift_max = 250; /* max cycles */ > +int64_t tsc_drift_observed; > +bool tsc_good; > + > +volatile int64_t tsc_sync_val; > +volatile struct cpu_info *tsc_sync_cpu; > + > uint tsc_get_timecount(struct timecounter *tc); > > struct timecounter tsc_timecounter = { > @@ -172,10 +182,8 @@ calibrate_tsc_freq(void) > return; > tsc_frequency = freq; > tsc_timecounter.tc_frequency = freq; > -#ifndef MULTIPROCESSOR > if (tsc_is_invariant) > tsc_timecounter.tc_quality = 2000; > -#endif > } > > void > @@ -194,26 +202,25 @@ cpu_recalibrate_tsc(struct timecounter * > uint > tsc_get_timecount(struct timecounter *tc) > { > - return rdtsc(); > + return rdtsc() + curcpu()->cpu_cc_skew; > } > > void > tsc_timecounter_init(struct cpu_info *ci, uint64_t cpufreq) > { > - if (!(ci->ci_flags & CPUF_PRIMARY) || > - !(ci->ci_flags & CPUF_CONST_TSC) || > + if (!(ci->ci_flags & CPUF_CONST_TSC) || > !(ci->ci_flags & CPUF_INVAR_TSC)) > return; > > tsc_frequency = tsc_freq_cpuid(ci); > tsc_is_invariant = 1; > > + tsc_good = (rdmsr(MSR_TSC) != 0 || rdmsr(MSR_TSC) != 0); > + > /* Newer CPUs don't require recalibration */ > if (tsc_frequency > 0) { > tsc_timecounter.tc_frequency = tsc_frequency; > -#ifndef MULTIPROCESSOR > tsc_timecounter.tc_quality = 2000; > -#endif > } else { > tsc_recalibrate = 1; > tsc_frequency = cpufreq; > @@ -221,5 +228,112 @@ tsc_timecounter_init(struct cpu_info *ci > calibrate_tsc_freq(); > } > > - tc_init(&tsc_timecounter); > + if (tsc_drift_observed > tsc_drift_max) { > + printf("ERROR: %lld cycle TSC drift observed\n", > + (long long)tsc_drift_observed); > + tsc_timecounter.tc_quality = -100; > + tsc_is_invariant = 0; > + } How is this ever going to knock out the tsc timecounter on a system with less than 10 cores?
