On Thu, Aug 24, 2017 at 2:35 AM, Mike Larkin <mlar...@azathoth.net> wrote: > On Wed, Aug 23, 2017 at 09:29:15PM +0800, Adam Steen wrote: >> On Thu, Aug 17, 2017 at 12:19:28PM +0800, Adam Steen wrote: >> > On Tue, Aug 8, 2017 at 10:12 PM, Mike Belopuhov <m...@belopuhov.com> wrote: >> > > On Tue, Aug 08, 2017 at 08:18 +0800, Adam Steen wrote: >> > >> On Mon, Jul 31, 2017 at 3:58 PM, Mike Belopuhov <m...@belopuhov.com> >> > >> wrote: >> > >> > On Mon, Jul 31, 2017 at 09:48 +0800, Adam Steen wrote: >> > >> >> Ted Unangst wrote: >> > >> >> > we don't currently export this info, but we could add some >> > >> >> > sysctls. there's >> > >> >> > some cpufeatures stuff there, but generally stuff isn't exported >> > >> >> > until >> > >> >> > somebody finds a use for it... it shouldn't be too hard to add >> > >> >> > something to >> > >> >> > amd64/machdep.c sysctl if you're interested. >> > >> >> >> > >> >> I am interested, as i need the info, i will look into it and >> > >> >> hopefully >> > >> >> come back with a patch. >> > >> > >> > >> > This is a bad idea because TSC as the time source is only usable >> > >> > by OpenBSD on Skylake and Kaby Lake CPUs since they encode the TSC >> > >> > frequency in the CPUID. All older CPUs have their TSCs measured >> > >> > against the PIT. Currently the measurement done by the kernel isn't >> > >> > very precise and if TSC is selected as a timecounter, the machine >> > >> > would be gaining time on a pace that cannot be corrected by our NTP >> > >> > daemon. (IIRC, about an hour a day on my Haswell running with NTP). >> > >> > >> > >> > To be able to use TSC as a timecounter source on OpenBSD or Solo5 >> > >> > you'd have to improve the in-kernel measurement of the TSC frequency >> > >> > first. I've tried to perform 10 measurements and take an average and >> > >> > it does improve accuracy, however I believe we need to poach another >> > >> > bit from Linux and re-calibrate TSC via HPET: >> > >> > >> > >> > >> > >> > http://elixir.free-electrons.com/linux/v4.12.4/source/arch/x86/kernel/tsc.c#L409 >> > >> > >> > >> > I think this is the most sane thing we can do. Here's a complete >> > >> > procedure that Linux kernel undertakes: >> > >> > >> > >> > >> > >> > http://elixir.free-electrons.com/linux/v4.12.4/source/arch/x86/kernel/tsc.c#L751 >> > >> > >> > >> > Regards, >> > >> > Mike >> > >> >> > >> Hi Mike/All >> > >> >> > >> I would like to improve the accuracy of TSC frequency calibration as >> > >> Mike B. describes above. >> > >> >> > >> I initially thought the calibration would take place at line 470 of >> > >> amd64/identcpu.c >> > >> (https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/amd64/amd64/identcpu.c?annotate=1.87) >> > >> >> > > >> > > Indeed, it cannot happen there simply because you don't know at >> > > that point whether or not HPET actually exists. >> > > >> > >> But I looked into using the acpihpet directly but it is never exposed >> > >> outside of acpihpet.c. >> > >> >> > > >> > > And it shouldn't be. >> > > >> > >> Could someone point me to were if would be appropriate to complete >> > >> this calibration and how to use the acpihpet? >> > > >> > > The way I envision this is a multi-step approach: >> > > >> > > 1) TSC frequency is approximated with the PIT (possibly performing >> > > multiple measurements and averaging them out; also keep in mind that >> > > doing it 8 times means you can shift the sum right by 3 instead of >> > > using actual integer division). This is what should happen around >> > > the line 470 of identcpu.c >> > > >> > > 2) A function can be provided by identcpu.c to further adjust the >> > > TSC frequency once acpitimer(4) (this is a PM timer) and acpihpet(4) >> > > (or any other timer for that matter) are attached. >> > > >> > > 3) Once acpitimer(4) or acpihpet(4) or any other timecounter source >> > > are attached and are verified to be operating correctly, they can >> > > perform TSC re-calibration and update the TSC frequency with their >> > > measurements. The idea here is that the function (or functions) that >> > > facilitate this must abstract enough logic so that you don't have to >> > > duplicate it in the acpitimer or acpihpet themselves. >> > > >> > >> (Will it need to be >> > >> exposed like i8254_delay/delay_func/delay in machdep.c and cpu.h) >> > >> >> > > >> > > No it won't. >> > > >> > >> Lastly should the calibration be done using both delay(i8254 pit) and >> > >> hpet timers similar to Linux described above or just using the hpet? >> > >> >> > > >> > > Well, that's what I was arguing for. As I said in my initial mail >> > > on misc (not quoted here), the TSC must be calibrated using separate >> > > known clocks sources. >> > >> > Hi Mike >> > >> > Please see the below diff to improve the accuracy of the TSC >> > frequency. It is model after the linux calibration you linked to >> > earlier. https://marc.info/?l=openbsd-misc&m=150148792804747&w=2 >> > >> > I feel like i don't know enough about the kernel internals, the >> > consistency of the results across reboots are not as close as i would >> > have liked, i feel the call to do the actual calibration should be >> > later in the boot cycle, when things have calmed down a little, but >> > couldn't figure out the best way of doing this. >> > >> > please bear with me i haven't been programming c for long, but the >> > only way to get things done is to do it your self. >> > >> > Cheers >> > Adam >> >> <SNIP> >> >> Thank you Mike on the feedback on the last patch, please see the diff >> below, update with your input and style(9) >> >> I have continued to use tsc as my timecounter and /var/db/ntpd.driff >> has stayed under 10. >> >> cat /var/db/ntpd.drift >> 6.665 >> >> ntpctl -s all >> 4/4 peers valid, constraint offset -1s, clock synced, stratum 3 >> >> peer >> wt tl st next poll offset delay jitter >> 144.48.166.166 from pool pool.ntp.org >> 1 10 2 4s 32s -3.159ms 87.723ms 10.389ms >> 13.55.50.68 from pool pool.ntp.org >> 1 10 3 11s 32s -3.433ms 86.053ms 18.095ms >> 14.202.204.182 from pool pool.ntp.org >> 1 10 1 14s 32s 1.486ms 86.545ms 16.483ms >> 27.124.125.250 from pool pool.ntp.org >> * 1 10 2 12s 30s -10.275ms 54.156ms 70.389ms >> >> Cheers >> Adam > > IIRC you have an x220, right? > > If so, could you try letting the clock run for a bit (while using tsc > timecounter selection) after apm -L to drop the speed? (make sure > apm shows that it dropped). > > Even though my x230 supposedly has a constant/invar TSC (according to > cpuid), the TSC drops from 2.5GHz to 1.2GHz when apm -L runs, which > causes time to run too slowly when tsc is selected there. > > -ml >
Yes, x220 (bios: LENOVO version "8DET69WW (1.39 )" date 07/18/2013) (cpu: Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz, 2491.91 MHz) I took some measurements to before starting the test. note: the laptop has been up for a few days with apm -A set via rc.conf.local and sysctl kern.timecounter.hardware as tsc via sysctl.conf and mostly idle. cat /var/db/ntpd.drift 6.459 apm -v Battery state: high, 100% remaining, unknown life estimate A/C adapter state: connected Performance adjustment mode: auto (800 MHz) 6 hours ago i ran apm -L, verified it was running slowly (800 MHz), and got the following results The clock appears correct (comparing to other computers) apm -v Battery state: high, 100% remaining, unknown life estimate A/C adapter state: connected Performance adjustment mode: manual (800 MHz) cat /var/db/ntpd.drift 6.385 ntpctl -s all 4/4 peers valid, constraint offset 0s, clock synced, stratum 4 peer wt tl st next poll offset delay jitter 203.23.237.200 from pool pool.ntp.org 1 10 2 153s 1505s -25.546ms 73.450ms 2.644ms 203.114.73.24 from pool pool.ntp.org 1 10 2 253s 1560s -1.042ms 75.133ms 0.752ms 192.189.54.33 from pool pool.ntp.org * 1 10 2 204s 1558s 31.644ms 70.910ms 3.388ms 54.252.165.245 from pool pool.ntp.org 1 10 2 238s 1518s 0.146ms 73.005ms 2.025ms I will leave the laptop in lower power mode over the weekend and see what happens. > >> Index: sys/arch/amd64/amd64/acpi_machdep.c >> =================================================================== >> RCS file: /cvs/src/sys/arch/amd64/amd64/acpi_machdep.c,v >> retrieving revision 1.78 >> diff -u -p -u -r1.78 acpi_machdep.c >> --- sys/arch/amd64/amd64/acpi_machdep.c 27 Mar 2017 18:32:53 -0000 >> 1.78 >> +++ sys/arch/amd64/amd64/acpi_machdep.c 23 Aug 2017 11:43:25 -0000 >> @@ -69,6 +69,17 @@ extern int acpi_savecpu(void) __returns_ >> >> u_int8_t *acpi_scan(struct acpi_mem_map *, paddr_t, size_t); >> >> +#define RECALIBRATE_MAX_RETRIES 5 >> +#define RECALIBRATE_SMI_THRESHOLD 50000 >> +#define RECALIBRATE_DELAY_THRESHOLD 20 >> +struct timecounter *acpitimer_recalibrate_timecounter; >> +struct timecounter *acpihpet_recalibrate_timecounter; >> +uint64_t acpi_calculate_timecounter_delay(struct timecounter *, uint64_t, >> + uint64_t); >> +uint64_t acpi_calculate_tsc_frequency(uint64_t, uint64_t, int); >> +uint64_t acpi_calibrate_tsc_frequency(); >> +int acpi_get_tsc_and_timecount(struct timecounter *, uint64_t *, uint64_t >> *); >> + >> int >> acpi_map(paddr_t pa, size_t len, struct acpi_mem_map *handle) >> { >> @@ -438,6 +449,123 @@ acpi_resume_cpu(struct acpi_softc *sc) >> /* Re-initialise memory range handling on BSP */ >> if (mem_range_softc.mr_op != NULL) >> mem_range_softc.mr_op->initAP(&mem_range_softc); >> +} >> + >> +int >> +acpi_get_tsc_and_timecount(struct timecounter *tc, uint64_t *tsc, >> + uint64_t *count) >> +{ >> + uint64_t n, tsc1, tsc2; >> + int i; >> + >> + for (i = 0; i < RECALIBRATE_MAX_RETRIES; i++) { >> + tsc1 = rdtsc(); >> + n = (tc->tc_get_timecount(tc) & tc->tc_counter_mask); >> + tsc2 = rdtsc(); >> + >> + if ((tsc2 - tsc1) < RECALIBRATE_SMI_THRESHOLD) { >> + *count = n; >> + *tsc = tsc2; >> + return (0); >> + } >> + } >> + return 1; >> +} >> + >> +uint64_t >> +acpi_calculate_timecounter_delay(struct timecounter *tc, uint64_t count1, >> + uint64_t count2) >> +{ >> + uint64_t delta; >> + >> + if (count2 < count1) >> + count2 += tc->tc_counter_mask; >> + >> + delta = (count2 - count1); >> + return delta * 1000000 / tc->tc_frequency; >> +} >> + >> +uint64_t >> +acpi_calculate_tsc_frequency(uint64_t tsc1, uint64_t tsc2, int usec) >> +{ >> + uint64_t delta; >> + >> + delta = (tsc2 - tsc1); >> + return delta * 1000000 / usec; >> +} >> + >> +void >> +acpitimer_recalibrate_timecounter_init(struct timecounter *tc) >> +{ >> + acpitimer_recalibrate_timecounter = tc; >> + cpu_adjust_tsc_frequency(&acpi_calibrate_tsc_frequency); >> +} >> + >> +void >> +acpihpet_recalibrate_timecounter_init(struct timecounter *tc) >> +{ >> + acpihpet_recalibrate_timecounter = tc; >> + cpu_adjust_tsc_frequency(&acpi_calibrate_tsc_frequency); >> +} >> + >> +uint64_t >> +acpi_calibrate_tsc_frequency() >> +{ >> + struct timecounter *tc; >> + uint64_t count1, count2, frequency, min_freq, tsc1, tsc2; >> + u_long ef; >> + int delay_usec, i, err1, err2, usec; >> + >> + if (acpihpet_recalibrate_timecounter != NULL) { >> + tc = acpihpet_recalibrate_timecounter; >> + } else if (acpitimer_recalibrate_timecounter != NULL) { >> + tc = acpitimer_recalibrate_timecounter; >> + } else { >> + printf("Failed to recalibrate timecounter \"tsc\""); >> + printf(" No timcounter set"); >> + return 0; >> + } >> + >> + /* warmup the timers */ >> + for (i =0; i < 3; i++) { >> + (void)tc->tc_get_timecount(tc); >> + (void)rdtsc(); >> + } >> + >> + min_freq = ULLONG_MAX; >> + >> + delay_usec = 100000; >> + for (i = 0; i < 3; i++) { >> + ef = read_rflags(); >> + disable_intr(); >> + >> + err1 = acpi_get_tsc_and_timecount(tc, &tsc1, &count1); >> + delay(delay_usec); >> + err2 = acpi_get_tsc_and_timecount(tc, &tsc2, &count2); >> + >> + write_rflags(ef); >> + >> + if (err1 || err2) >> + continue; >> + >> + usec = acpi_calculate_timecounter_delay(tc, count1, count2); >> + >> + if (usec < (delay_usec - RECALIBRATE_DELAY_THRESHOLD)) >> + continue; >> + >> + if (usec > (delay_usec + RECALIBRATE_DELAY_THRESHOLD)) >> + continue; >> + >> + frequency = acpi_calculate_tsc_frequency(tsc1, tsc2, usec); >> + >> + min_freq = ulmin(min_freq, frequency); >> + } >> + >> + if (min_freq == 0) { >> + printf("Failed to recalibrate timecounter \"tsc\""); >> + printf(" with timecounter \"%s\"\n", tc->tc_name); >> + } >> + return min_freq; >> } >> >> #ifdef MULTIPROCESSOR >> Index: sys/arch/amd64/amd64/identcpu.c >> =================================================================== >> RCS file: /cvs/src/sys/arch/amd64/amd64/identcpu.c,v >> retrieving revision 1.87 >> diff -u -p -u -r1.87 identcpu.c >> --- sys/arch/amd64/amd64/identcpu.c 20 Jun 2017 05:34:41 -0000 1.87 >> +++ sys/arch/amd64/amd64/identcpu.c 23 Aug 2017 11:43:26 -0000 >> @@ -63,6 +63,10 @@ struct timecounter tsc_timecounter = { >> tsc_get_timecount, NULL, ~0u, 0, "tsc", -1000, NULL >> }; >> >> +u_int64_t (*amd64_tsc_get_frequency)(); >> +u_int64_t amd64_tsc_freq; >> +int amd64_tsc_requires_calibration; >> +int amd64_has_invariant_tsc; >> int amd64_has_xcrypt; >> #ifdef CRYPTO >> int amd64_has_pclmul; >> @@ -428,8 +432,9 @@ cpu_tsc_freq_ctr(struct cpu_info *ci) >> u_int64_t >> cpu_tsc_freq(struct cpu_info *ci) >> { >> - u_int64_t last_count, count; >> + u_int64_t last_count, count, sum; >> uint32_t eax, ebx, khz, dummy; >> + int i; >> >> if (!strcmp(cpu_vendor, "GenuineIntel") && >> cpuid_level >= 0x15) { >> @@ -468,11 +473,21 @@ cpu_tsc_freq(struct cpu_info *ci) >> if (count != 0) >> return (count); >> >> - last_count = rdtsc(); >> - delay(100000); >> - count = rdtsc(); >> + if (amd64_tsc_get_frequency != NULL) >> + return (*amd64_tsc_get_frequency)(); >> >> - return ((count - last_count) * 10); >> + if (ci->ci_flags & CPUF_PRIMARY) >> + amd64_tsc_requires_calibration = 1; >> + >> + sum = 0; >> + for (i=0; i < 8; i++) { >> + last_count = rdtsc(); >> + delay(100000); >> + count = rdtsc(); >> + sum += (count - last_count); >> + } >> + >> + return ((sum >> 3) * 10); >> } >> >> u_int >> @@ -482,6 +497,22 @@ tsc_get_timecount(struct timecounter *tc >> } >> >> void >> +cpu_adjust_tsc_frequency(u_int64_t (*get_frequency)()) >> +{ >> + if (amd64_tsc_requires_calibration) { >> + amd64_tsc_freq = (*get_frequency)(); >> + tsc_timecounter.tc_frequency = amd64_tsc_freq; >> + >> + printf("Timecounter \"%s\" recalibrated", >> + tsc_timecounter.tc_name); >> + printf("with frequency %llu Hz\n", >> + tsc_timecounter.tc_frequency); >> + } else { >> + amd64_tsc_get_frequency = get_frequency; >> + } >> +} >> + >> +void >> identifycpu(struct cpu_info *ci) >> { >> u_int32_t dummy, val; >> @@ -566,9 +597,13 @@ identifycpu(struct cpu_info *ci) >> /* Check if it's an invariant TSC */ >> if (cpu_apmi_edx & CPUIDEDX_ITSC) >> ci->ci_flags |= CPUF_INVAR_TSC; >> + >> + amd64_has_invariant_tsc = (ci->ci_flags & CPUF_INVAR_TSC) != 0; >> } >> >> ci->ci_tsc_freq = cpu_tsc_freq(ci); >> + if (ci->ci_flags & CPUF_PRIMARY) >> + amd64_tsc_freq = ci->ci_tsc_freq; >> >> amd_cpu_cacheinfo(ci); >> >> Index: sys/arch/amd64/amd64/machdep.c >> =================================================================== >> RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v >> retrieving revision 1.231 >> diff -u -p -u -r1.231 machdep.c >> --- sys/arch/amd64/amd64/machdep.c 12 Jul 2017 06:26:32 -0000 1.231 >> +++ sys/arch/amd64/amd64/machdep.c 23 Aug 2017 11:43:26 -0000 >> @@ -425,6 +425,8 @@ int >> cpu_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void >> *newp, >> size_t newlen, struct proc *p) >> { >> + extern u_int64_t amd64_tsc_freq; >> + extern int amd64_has_invariant_tsc; >> extern int amd64_has_xcrypt; >> dev_t consdev; >> dev_t dev; >> @@ -496,6 +498,11 @@ cpu_sysctl(int *name, u_int namelen, voi >> pckbc_release_console(); >> return (error); >> #endif >> + case CPU_TSCFREQ: >> + return (sysctl_rdquad(oldp, oldlenp, newp, amd64_tsc_freq)); >> + case CPU_INVARIANTTSC: >> + return (sysctl_rdint(oldp, oldlenp, newp, >> + amd64_has_invariant_tsc)); >> default: >> return (EOPNOTSUPP); >> } >> Index: sys/arch/amd64/include/cpu.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/amd64/include/cpu.h,v >> retrieving revision 1.114 >> diff -u -p -u -r1.114 cpu.h >> --- sys/arch/amd64/include/cpu.h 11 Aug 2017 20:19:14 -0000 1.114 >> +++ sys/arch/amd64/include/cpu.h 23 Aug 2017 11:43:28 -0000 >> @@ -427,7 +427,9 @@ void mp_setperf_init(void); >> #define CPU_XCRYPT 12 /* supports VIA xcrypt in userland */ >> #define CPU_LIDACTION 14 /* action caused by lid close >> */ >> #define CPU_FORCEUKBD 15 /* Force ukbd(4) as console >> keyboard */ >> -#define CPU_MAXID 16 /* number of valid machdep ids */ >> +#define CPU_TSCFREQ 16 /* tsc frequency */ >> +#define CPU_INVARIANTTSC 17 /* has invariant tsc */ >> +#define CPU_MAXID 18 /* number of valid machdep ids */ >> >> #define CTL_MACHDEP_NAMES { \ >> { 0, 0 }, \ >> @@ -446,6 +448,8 @@ void mp_setperf_init(void); >> { 0, 0 }, \ >> { "lidaction", CTLTYPE_INT }, \ >> { "forceukbd", CTLTYPE_INT }, \ >> + { "tscfreq", CTLTYPE_QUAD }, \ >> + { "invarianttsc", CTLTYPE_INT }, \ >> } >> >> /* >> Index: sys/arch/amd64/include/cpuvar.h >> =================================================================== >> RCS file: /cvs/src/sys/arch/amd64/include/cpuvar.h,v >> retrieving revision 1.8 >> diff -u -p -u -r1.8 cpuvar.h >> --- sys/arch/amd64/include/cpuvar.h 28 Jul 2016 21:57:57 -0000 1.8 >> +++ sys/arch/amd64/include/cpuvar.h 23 Aug 2017 11:43:28 -0000 >> @@ -96,5 +96,6 @@ void x86_ipi_init(int); >> void identifycpu(struct cpu_info *); >> void cpu_init(struct cpu_info *); >> void cpu_init_first(void); >> +void cpu_adjust_tsc_frequency(uint64_t (*)()); >> >> #endif >> Index: sys/dev/acpi/acpihpet.c >> =================================================================== >> RCS file: /cvs/src/sys/dev/acpi/acpihpet.c,v >> retrieving revision 1.21 >> diff -u -p -u -r1.21 acpihpet.c >> --- sys/dev/acpi/acpihpet.c 6 Oct 2015 20:49:32 -0000 1.21 >> +++ sys/dev/acpi/acpihpet.c 23 Aug 2017 11:43:30 -0000 >> @@ -265,6 +265,7 @@ acpihpet_attach(struct device *parent, s >> hpet_timecounter.tc_name = sc->sc_dev.dv_xname; >> tc_init(&hpet_timecounter); >> acpihpet_attached++; >> + acpihpet_recalibrate_timecounter_init(&hpet_timecounter); >> } >> >> u_int >> Index: sys/dev/acpi/acpitimer.c >> =================================================================== >> RCS file: /cvs/src/sys/dev/acpi/acpitimer.c,v >> retrieving revision 1.11 >> diff -u -p -u -r1.11 acpitimer.c >> --- sys/dev/acpi/acpitimer.c 14 Mar 2015 03:38:46 -0000 1.11 >> +++ sys/dev/acpi/acpitimer.c 23 Aug 2017 11:43:30 -0000 >> @@ -96,6 +96,7 @@ acpitimerattach(struct device *parent, s >> acpi_timecounter.tc_priv = sc; >> acpi_timecounter.tc_name = sc->sc_dev.dv_xname; >> tc_init(&acpi_timecounter); >> + acpitimer_recalibrate_timecounter_init(&acpi_timecounter); >> } >> >> >> Index: sys/dev/acpi/acpivar.h >> =================================================================== >> RCS file: /cvs/src/sys/dev/acpi/acpivar.h,v >> retrieving revision 1.88 >> diff -u -p -u -r1.88 acpivar.h >> --- sys/dev/acpi/acpivar.h 17 Aug 2017 05:16:27 -0000 1.88 >> +++ sys/dev/acpi/acpivar.h 23 Aug 2017 11:43:30 -0000 >> @@ -25,6 +25,7 @@ >> >> #include <sys/timeout.h> >> #include <sys/rwlock.h> >> +#include <sys/timetc.h> >> #include <machine/biosvar.h> >> >> #include "acpipwrres.h" >> @@ -324,6 +325,9 @@ void acpi_resume_cpu(struct acpi_softc >> void acpi_resume_mp(void); >> void acpi_sleep_walk(struct acpi_softc *, int); >> >> +void acpihpet_recalibrate_timecounter_init(struct timecounter *); >> +void acpitimer_recalibrate_timecounter_init(struct timecounter *); >> +uint64_t acpi_calibrate_tsc_frequency(); >> >> #define ACPI_IOREAD 0 >> #define ACPI_IOWRITE 1 >