Re: [PATCH] x86 / CPU: Always show current CPU frequency in /proc/cpuinfo

[Rafael J. Wysocki]

On Thursday, November 16, 2017 10:50:36 AM CET WANG Chao wrote:
> On 11/16/17 at 01:24P, Rafael J. Wysocki wrote:
> > On Wednesday, November 15, 2017 10:33:47 AM CET WANG Chao wrote:
> > > On 11/15/17 at 02:13P, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > > 
> > > > After commit 890da9cf0983 (Revert "x86: do not use cpufreq_quick_get()
> > > > for /proc/cpuinfo "cpu MHz"") the "cpu MHz" number in /proc/cpuinfo
> > > > on x86 can be either the nominal CPU frequency (which is constant)
> > > > or the frequency most recently requested by a scaling governor in
> > > > cpufreq, depending on the cpufreq configuration.  That is somewhat
> > > > inconsistent and is different from what it was before 4.13, so in
> > > > order to restore the previous behavior, make it report the current
> > > > CPU frequency like the scaling_cur_freq sysfs file in cpufreq.
> > > > 
> > > > To that end, modify the /proc/cpuinfo implementation on x86 to use
> > > > aperfmperf_snapshot_khz() to snapshot the APERF and MPERF feedback
> > > > registers, if available, and use their values to compute the CPU
> > > > frequency to be reported as "cpu MHz".
> > > > 
> > > > However, do that carefully enough to avoid accumulating delays that
> > > > lead to unacceptable access times for /proc/cpuinfo on systems with
> > > > many CPUs.  Run aperfmperf_snapshot_khz() once on all CPUs
> > > > asynchronously at the /proc/cpuinfo open time, add a single delay
> > > > upfront (if necessary) at that point and simply compute the current
> > > > frequency while running show_cpuinfo() for each individual CPU.
> > > 
> > > Hi, Rafael
> > > 
> > > I tested your patch. It's much faster.
> > > 
> > > But from what I got, calling aperfmperf_snapshot_khz() asynchronously
> > > with 10ms sleep takes much longer than calling aperfmperf_snapshot_khz()
> > > synchronously.
> > > 
> > > Here's my result on 64 CPUs:
> > > 
> > >  - async aperfmperf_snapshot_khz() w/ 10ms sleep:
> > > 
> > > # time cat /proc/cpuinfo > /dev/null
> > > real    0m0.014s
> > > user    0m0.000s
> > > sys     0m0.002s
> > > 
> > >  - sync aperfmperf_snapshot_khz() w/o any sleep:
> > > 
> > > # time cat /proc/cpuinfo > /dev/null
> > > real    0m0.002s
> > > user    0m0.000s
> > > sys     0m0.002s
> > 
> > Sure, but the delay is there, because without it the computed frequency
> > may be way off for at least one of the CPUs.
> 
> Thanks, I understand now. In this case, The 10ms delay turns out to be
> the interval of measuring aperf and mperf and computing their deltas.
> 
> Last question though, is 10ms best practice or can we make it shorter,
> say 5ms?

Experimentally, I found 5 ms to be slightly too short.  It all depends on
how accurate the numbers are expected to be, however, so there is some room
for adjustments.

Regardless, I'd prefer to start with 10 ms as that is what has been used in
intel_pstate for quite a long time at least and adjust later if need be.

Thanks,
Rafael