On Thu, Sep 14, 2017 at 03:41:25PM +0200, Reyk Floeter wrote:
>
> On Fri, Aug 25, 2017 at 12:43:44PM +0200, Mike Belopuhov wrote:
> > On Fri, Aug 25, 2017 at 00:40 -0700, Mike Larkin wrote:
> > > On Thu, Aug 24, 2017 at 12:39:33PM +0800, Adam Steen wrote:
> > > > 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:
> > > > >>
> > > > >> 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.
> > > >
> > >
> > > No need, I think you've convinced me that it works :)
> >
> > But does it actually work on x230 as well? I'm surprised to learn
> > that you've observed TSC frequency change on Ivy Bridge. I was
> > under impression that everything since at least Sandy Bridge (x220)
> > has constant and invariant TSC as advertised.
> >
> > Adam, I've readjusted and simplified your diff a bit. The biggest
> > change is that we can select the reference tc based on it's quality
> > so there's no need to have acpitimer and acpihpet specific functions
> > and variables.
> >
> > There's one big thing missing here: increasing the timecounter
> > quality so that OS can pick it. Something like this:
> >
> > https://github.com/mbelop/src/commit/99d6ef3ae95bbd8ea93c27e0425eb65e5a3359a1
> >
> > I'd say we should try getting this in after 6.3 unlock unless there
> > are objections. Further cleanup and testing is welcome of course.
> >
> >
>
> I'd like to raise another point: as we figured out, the TSC frequency
> can be different than the CPU frequency on modern Intel CPUs. The
> const+invar TSC can even run faster than the CPU.
>
> For example, my X1 4th Gen currently boots up with the following speeds:
>
> cpu0: Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz, 2808.00 MHz
> ...
> cpu0: Enhanced SpeedStep 2808 MHz: speeds: 2701, 2700, 2600, 2500, 2300,
> 2100, 1900, 1800, 1600, 1400, 1300, 1100, 800, 700, 600, 400 MHz
>
> The 2.8GHz is actually the TSC speed, the fastest CPU speed is 2.7GHz.
>
> (I didn't figure out why ark.intel.com says max turbo speed is 3.4GHz,
> but the 2.7GHz is probably a vendor configuration from Lenovo based on
> the Laptop's cooling capabilities).
Turbo modes do not show as fixed clock speeds in p-states beyond
the 2701 in the above. To find the current cpu speed when in turbo
mode requires looking at various msrs (MSR_TURBO_RATIO_LIMIT,
MSR_PLATFORM_INFO etc) and running a performance counter for a period
of time from memory.
>
> But the MSR function returns a different value which is supposed to be
> the actual CPU speed (of core 0) after going out of power-saving mode.
> This caused problems on Skylake because the returned value can be very
> different to the TSC - using this value, the kernel failed to
> calibrated the TSC timecounter correctly. That's why I added the code
> that reads the TSC frequency from CPUID 0x15 and skips the
> MSR/RDTSC-based calibration, if CPUID 0x15 is supported and present.
>
> For example, without the CPUID method, I get the MSR-derived speed
> (depending on the on-boot power state that is configured in the BIOS):
>
> cpu0: Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz, 2494.75 MHz
>
> But here is my mistake: I actually thought that the MSR function is
> "wrong" on Skylake. But it is just a different thing. We have to
> differentiate if we want to obtain the TSC or the CPU frequency, but
> the CPU frequency might just be derived from the TSC on older CPUs
> that don't support the MSR.
>
> I attached a diff that illustrates the problem on the -current code,
> but the same could be adjusted for the TSC recalibration diff.
>
> I split the cpu_tsc_freq() function into two functions:
>
> 1. cpu_freq() tries to get the CPU frequency from the MSR, falls back
> to the TSC via cpu_tsc_freq(). This is used for cpuX dmesg printfs
> and for the global speedstep "cpuspeed" variable.
>
> 2. cpu_tsc_freq() tries to get the TSC frequency from CPUID 0x15 or
> falls back to the less accurate rdtsc() method. This is used for the
> (initial) TSC timecounter frequency.
>
> Thoughts?
>
> (I'm not asking for OKs on the diff)
>
> Reyk
>
> 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 -p -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 14 Sep 2017 12:49:47 -0000
> @@ -47,8 +47,9 @@
> #include <machine/cpufunc.h>
>
> void replacesmap(void);
> +u_int64_t cpu_freq(struct cpu_info *);
> +u_int64_t cpu_freq_ctr(struct cpu_info *);
> u_int64_t cpu_tsc_freq(struct cpu_info *);
> -u_int64_t cpu_tsc_freq_ctr(struct cpu_info *);
> #if NVMM > 0
> void cpu_check_vmm_cap(struct cpu_info *);
> #endif /* NVMM > 0 */
> @@ -388,12 +389,11 @@ via_update_sensor(void *args)
> #endif
>
> u_int64_t
> -cpu_tsc_freq_ctr(struct cpu_info *ci)
> +cpu_freq_ctr(struct cpu_info *ci)
> {
> u_int64_t count, last_count, msr;
>
> if ((ci->ci_flags & CPUF_CONST_TSC) == 0 ||
> - (ci->ci_flags & CPUF_INVAR_TSC) ||
> (cpu_perf_eax & CPUIDEAX_VERID) <= 1 ||
> CPUIDEDX_NUM_FC(cpu_perf_edx) <= 1)
> return (0);
> @@ -426,6 +426,23 @@ cpu_tsc_freq_ctr(struct cpu_info *ci)
> }
>
> u_int64_t
> +cpu_freq(struct cpu_info *ci)
> +{
> + u_int64_t count;
> +
> + /*
> + * Try to get the currently running CPU frequency. On modern CPUs,
> + * this does not have to match the TSC frequency.
> + */
> + count = cpu_freq_ctr(ci);
> + if (count != 0)
> + return (count);
> +
> + /* Fallback to the TSC frequency which was identical on old CPUs. */
> + return (cpu_tsc_freq(ci));
> +}
> +
> +u_int64_t
> cpu_tsc_freq(struct cpu_info *ci)
> {
> u_int64_t last_count, count;
> @@ -464,10 +481,6 @@ cpu_tsc_freq(struct cpu_info *ci)
> }
> }
>
> - count = cpu_tsc_freq_ctr(ci);
> - if (count != 0)
> - return (count);
> -
> last_count = rdtsc();
> delay(100000);
> count = rdtsc();
> @@ -489,6 +502,7 @@ identifycpu(struct cpu_info *ci)
> int i;
> char *brandstr_from, *brandstr_to;
> int skipspace;
> + uint64_t freq;
>
> CPUID(1, ci->ci_signature, val, dummy, ci->ci_feature_flags);
> CPUID(0x80000000, ci->ci_pnfeatset, dummy, dummy, dummy);
> @@ -568,18 +582,17 @@ identifycpu(struct cpu_info *ci)
> ci->ci_flags |= CPUF_INVAR_TSC;
> }
>
> - ci->ci_tsc_freq = cpu_tsc_freq(ci);
> -
> amd_cpu_cacheinfo(ci);
>
> printf("%s: %s", ci->ci_dev->dv_xname, mycpu_model);
>
> - if (ci->ci_tsc_freq != 0)
> - printf(", %llu.%02llu MHz", (ci->ci_tsc_freq + 4999) / 1000000,
> - ((ci->ci_tsc_freq + 4999) / 10000) % 100);
> + freq = cpu_freq(ci);
> + if (freq != 0)
> + printf(", %llu.%02llu MHz", (freq + 4999) / 1000000,
> + ((freq + 4999) / 10000) % 100);
>
> if (ci->ci_flags & CPUF_PRIMARY) {
> - cpuspeed = (ci->ci_tsc_freq + 4999) / 1000000;
> + cpuspeed = (freq + 4999) / 1000000;
> cpu_cpuspeed = cpu_amd64speed;
> }
>
> @@ -726,10 +739,10 @@ identifycpu(struct cpu_info *ci)
> if ((ci->ci_flags & CPUF_PRIMARY) &&
> (ci->ci_flags & CPUF_CONST_TSC) &&
> (ci->ci_flags & CPUF_INVAR_TSC)) {
> - printf("%s: TSC frequency %llu Hz\n",
> - ci->ci_dev->dv_xname, ci->ci_tsc_freq);
> - tsc_timecounter.tc_frequency = ci->ci_tsc_freq;
> + tsc_timecounter.tc_frequency = cpu_tsc_freq(ci);
> tc_init(&tsc_timecounter);
> + printf("%s: TSC frequency %llu Hz\n",
> + ci->ci_dev->dv_xname, tsc_timecounter.tc_frequency);
> }
>
> cpu_topology(ci);
> 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 -p -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 14 Sep 2017 12:49:47 -0000
> @@ -138,7 +138,6 @@ struct cpu_info {
> u_int32_t ci_family;
> u_int32_t ci_model;
> u_int32_t ci_cflushsz;
> - u_int64_t ci_tsc_freq;
>
> int ci_inatomic;
>
>
