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


> 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

Reply via email to