x86 / CPU: Always show current CPU frequency in /proc/cpuinfo
diff mbox

Message ID 50366815.86mgx8Isne@aspire.rjw.lan
State Mainlined
Delegated to: Rafael Wysocki
Headers show

Commit Message

Rafael J. Wysocki Nov. 15, 2017, 1:13 a.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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.

Also, to avoid slowing down /proc/cpuinfo accesses too much, reduce
the default delay between consecutive APERF and MPERF reads to 10 ms,
which should be sufficient to get large enough numbers for the
frequency computation in all cases.

Fixes: 890da9cf0983 (Revert "x86: do not use cpufreq_quick_get() for /proc/cpuinfo "cpu MHz"")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Resent with a changelog & tags.

I'm going to route it via the linux-pm tree.

---
 arch/x86/kernel/cpu/aperfmperf.c |   74 +++++++++++++++++++++++++++------------
 arch/x86/kernel/cpu/cpu.h        |    3 +
 arch/x86/kernel/cpu/proc.c       |    6 ++-
 fs/proc/cpuinfo.c                |    6 +++
 include/linux/cpufreq.h          |    1 
 5 files changed, 67 insertions(+), 23 deletions(-)

Comments

Thomas Gleixner Nov. 15, 2017, 8:47 a.m. UTC | #1
On Wed, 15 Nov 2017, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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.
> 
> Also, to avoid slowing down /proc/cpuinfo accesses too much, reduce
> the default delay between consecutive APERF and MPERF reads to 10 ms,
> which should be sufficient to get large enough numbers for the
> frequency computation in all cases.
> 
> Fixes: 890da9cf0983 (Revert "x86: do not use cpufreq_quick_get() for /proc/cpuinfo "cpu MHz"")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Thomas Gleixner <tglx@linutronix.de>
WANG Chao Nov. 15, 2017, 9:33 a.m. UTC | #2
On 11/15/17 at 02:13P, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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

Thanks,
WANG Chao

> 
> Also, to avoid slowing down /proc/cpuinfo accesses too much, reduce
> the default delay between consecutive APERF and MPERF reads to 10 ms,
> which should be sufficient to get large enough numbers for the
> frequency computation in all cases.
> 
> Fixes: 890da9cf0983 (Revert "x86: do not use cpufreq_quick_get() for /proc/cpuinfo "cpu MHz"")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Resent with a changelog & tags.
> 
> I'm going to route it via the linux-pm tree.
> 
> ---
>  arch/x86/kernel/cpu/aperfmperf.c |   74 +++++++++++++++++++++++++++------------
>  arch/x86/kernel/cpu/cpu.h        |    3 +
>  arch/x86/kernel/cpu/proc.c       |    6 ++-
>  fs/proc/cpuinfo.c                |    6 +++
>  include/linux/cpufreq.h          |    1 
>  5 files changed, 67 insertions(+), 23 deletions(-)
> 
> Index: linux-pm/arch/x86/kernel/cpu/proc.c
> ===================================================================
> --- linux-pm.orig/arch/x86/kernel/cpu/proc.c
> +++ linux-pm/arch/x86/kernel/cpu/proc.c
> @@ -5,6 +5,8 @@
>  #include <linux/seq_file.h>
>  #include <linux/cpufreq.h>
>  
> +#include "cpu.h"
> +
>  /*
>   *	Get CPU information for use by the procfs.
>   */
> @@ -78,9 +80,11 @@ static int show_cpuinfo(struct seq_file
>  		seq_printf(m, "microcode\t: 0x%x\n", c->microcode);
>  
>  	if (cpu_has(c, X86_FEATURE_TSC)) {
> -		unsigned int freq = cpufreq_quick_get(cpu);
> +		unsigned int freq = aperfmperf_get_khz(cpu);
>  
>  		if (!freq)
> +			freq = cpufreq_quick_get(cpu);
> +		if (!freq)
>  			freq = cpu_khz;
>  		seq_printf(m, "cpu MHz\t\t: %u.%03u\n",
>  			   freq / 1000, (freq % 1000));
> Index: linux-pm/arch/x86/kernel/cpu/aperfmperf.c
> ===================================================================
> --- linux-pm.orig/arch/x86/kernel/cpu/aperfmperf.c
> +++ linux-pm/arch/x86/kernel/cpu/aperfmperf.c
> @@ -14,6 +14,8 @@
>  #include <linux/percpu.h>
>  #include <linux/smp.h>
>  
> +#include "cpu.h"
> +
>  struct aperfmperf_sample {
>  	unsigned int	khz;
>  	ktime_t	time;
> @@ -24,7 +26,7 @@ struct aperfmperf_sample {
>  static DEFINE_PER_CPU(struct aperfmperf_sample, samples);
>  
>  #define APERFMPERF_CACHE_THRESHOLD_MS	10
> -#define APERFMPERF_REFRESH_DELAY_MS	20
> +#define APERFMPERF_REFRESH_DELAY_MS	10
>  #define APERFMPERF_STALE_THRESHOLD_MS	1000
>  
>  /*
> @@ -38,8 +40,6 @@ static void aperfmperf_snapshot_khz(void
>  	u64 aperf, aperf_delta;
>  	u64 mperf, mperf_delta;
>  	struct aperfmperf_sample *s = this_cpu_ptr(&samples);
> -	ktime_t now = ktime_get();
> -	s64 time_delta = ktime_ms_delta(now, s->time);
>  	unsigned long flags;
>  
>  	local_irq_save(flags);
> @@ -57,38 +57,68 @@ static void aperfmperf_snapshot_khz(void
>  	if (mperf_delta == 0)
>  		return;
>  
> -	s->time = now;
> +	s->time = ktime_get();
>  	s->aperf = aperf;
>  	s->mperf = mperf;
> -
> -	/* If the previous iteration was too long ago, discard it. */
> -	if (time_delta > APERFMPERF_STALE_THRESHOLD_MS)
> -		s->khz = 0;
> -	else
> -		s->khz = div64_u64((cpu_khz * aperf_delta), mperf_delta);
> +	s->khz = div64_u64((cpu_khz * aperf_delta), mperf_delta);
>  }
>  
> -unsigned int arch_freq_get_on_cpu(int cpu)
> +static bool aperfmperf_snapshot_cpu(int cpu, ktime_t now, bool wait)
>  {
> -	s64 time_delta;
> -	unsigned int khz;
> +	s64 time_delta = ktime_ms_delta(now, per_cpu(samples.time, cpu));
> +
> +	/* Don't bother re-computing within the cache threshold time. */
> +	if (time_delta < APERFMPERF_CACHE_THRESHOLD_MS)
> +		return true;
> +
> +	smp_call_function_single(cpu, aperfmperf_snapshot_khz, NULL, wait);
> +
> +	/* Return false if the previous iteration was too long ago. */
> +	return time_delta <= APERFMPERF_STALE_THRESHOLD_MS;
> +}
>  
> +unsigned int aperfmperf_get_khz(int cpu)
> +{
>  	if (!cpu_khz)
>  		return 0;
>  
>  	if (!static_cpu_has(X86_FEATURE_APERFMPERF))
>  		return 0;
>  
> -	/* Don't bother re-computing within the cache threshold time. */
> -	time_delta = ktime_ms_delta(ktime_get(), per_cpu(samples.time, cpu));
> -	khz = per_cpu(samples.khz, cpu);
> -	if (khz && time_delta < APERFMPERF_CACHE_THRESHOLD_MS)
> -		return khz;
> +	aperfmperf_snapshot_cpu(cpu, ktime_get(), true);
> +	return per_cpu(samples.khz, cpu);
> +}
>  
> -	smp_call_function_single(cpu, aperfmperf_snapshot_khz, NULL, 1);
> -	khz = per_cpu(samples.khz, cpu);
> -	if (khz)
> -		return khz;
> +void arch_freq_prepare_all(void)
> +{
> +	ktime_t now = ktime_get();
> +	bool wait = false;
> +	int cpu;
> +
> +	if (!cpu_khz)
> +		return;
> +
> +	if (!static_cpu_has(X86_FEATURE_APERFMPERF))
> +		return;
> +
> +	for_each_online_cpu(cpu)
> +		if (!aperfmperf_snapshot_cpu(cpu, now, false))
> +			wait = true;
> +
> +	if (wait)
> +		msleep(APERFMPERF_REFRESH_DELAY_MS);
> +}
> +
> +unsigned int arch_freq_get_on_cpu(int cpu)
> +{
> +	if (!cpu_khz)
> +		return 0;
> +
> +	if (!static_cpu_has(X86_FEATURE_APERFMPERF))
> +		return 0;
> +
> +	if (aperfmperf_snapshot_cpu(cpu, ktime_get(), true))
> +		return per_cpu(samples.khz, cpu);
>  
>  	msleep(APERFMPERF_REFRESH_DELAY_MS);
>  	smp_call_function_single(cpu, aperfmperf_snapshot_khz, NULL, 1);
> Index: linux-pm/arch/x86/kernel/cpu/cpu.h
> ===================================================================
> --- linux-pm.orig/arch/x86/kernel/cpu/cpu.h
> +++ linux-pm/arch/x86/kernel/cpu/cpu.h
> @@ -47,4 +47,7 @@ extern const struct cpu_dev *const __x86
>  
>  extern void get_cpu_cap(struct cpuinfo_x86 *c);
>  extern void cpu_detect_cache_sizes(struct cpuinfo_x86 *c);
> +
> +unsigned int aperfmperf_get_khz(int cpu);
> +
>  #endif /* ARCH_X86_CPU_H */
> Index: linux-pm/fs/proc/cpuinfo.c
> ===================================================================
> --- linux-pm.orig/fs/proc/cpuinfo.c
> +++ linux-pm/fs/proc/cpuinfo.c
> @@ -1,12 +1,18 @@
>  // SPDX-License-Identifier: GPL-2.0
> +#include <linux/cpufreq.h>
>  #include <linux/fs.h>
>  #include <linux/init.h>
>  #include <linux/proc_fs.h>
>  #include <linux/seq_file.h>
>  
> +__weak void arch_freq_prepare_all(void)
> +{
> +}
> +
>  extern const struct seq_operations cpuinfo_op;
>  static int cpuinfo_open(struct inode *inode, struct file *file)
>  {
> +	arch_freq_prepare_all();
>  	return seq_open(file, &cpuinfo_op);
>  }
>  
> Index: linux-pm/include/linux/cpufreq.h
> ===================================================================
> --- linux-pm.orig/include/linux/cpufreq.h
> +++ linux-pm/include/linux/cpufreq.h
> @@ -917,6 +917,7 @@ static inline bool policy_has_boost_freq
>  }
>  #endif
>  
> +extern void arch_freq_prepare_all(void);
>  extern unsigned int arch_freq_get_on_cpu(int cpu);
>  
>  extern void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
>
Rafael J. Wysocki Nov. 16, 2017, 12:24 a.m. UTC | #3
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@intel.com>
> > 
> > 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,
Rafael
WANG Chao Nov. 16, 2017, 9:50 a.m. UTC | #4
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@intel.com>
> > > 
> > > 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?

Thanks,
WANG Chao
Rafael J. Wysocki Nov. 16, 2017, 1:54 p.m. UTC | #5
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@intel.com>
> > > > 
> > > > 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
WANG Chao Nov. 17, 2017, 4:27 a.m. UTC | #6
On 11/16/17 at 02:54P, Rafael J. Wysocki wrote:
> 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@intel.com>
> > > > > 
> > > > > 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.

Sure. Thanks for your explanation.

This patch looks good to me.

Reviewed-by: WANG Chao <chao.wang@ucloud.cn>
Rafael J. Wysocki Nov. 17, 2017, 1:33 p.m. UTC | #7
On Friday, November 17, 2017 5:27:07 AM CET WANG Chao wrote:
> On 11/16/17 at 02:54P, Rafael J. Wysocki wrote:
> > 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@intel.com>
> > > > > > 
> > > > > > 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.
> 
> Sure. Thanks for your explanation.
> 
> This patch looks good to me.
> 
> Reviewed-by: WANG Chao <chao.wang@ucloud.cn>

Thanks for the reviews, much appreciated!

Patch
diff mbox

Index: linux-pm/arch/x86/kernel/cpu/proc.c
===================================================================
--- linux-pm.orig/arch/x86/kernel/cpu/proc.c
+++ linux-pm/arch/x86/kernel/cpu/proc.c
@@ -5,6 +5,8 @@ 
 #include <linux/seq_file.h>
 #include <linux/cpufreq.h>
 
+#include "cpu.h"
+
 /*
  *	Get CPU information for use by the procfs.
  */
@@ -78,9 +80,11 @@  static int show_cpuinfo(struct seq_file
 		seq_printf(m, "microcode\t: 0x%x\n", c->microcode);
 
 	if (cpu_has(c, X86_FEATURE_TSC)) {
-		unsigned int freq = cpufreq_quick_get(cpu);
+		unsigned int freq = aperfmperf_get_khz(cpu);
 
 		if (!freq)
+			freq = cpufreq_quick_get(cpu);
+		if (!freq)
 			freq = cpu_khz;
 		seq_printf(m, "cpu MHz\t\t: %u.%03u\n",
 			   freq / 1000, (freq % 1000));
Index: linux-pm/arch/x86/kernel/cpu/aperfmperf.c
===================================================================
--- linux-pm.orig/arch/x86/kernel/cpu/aperfmperf.c
+++ linux-pm/arch/x86/kernel/cpu/aperfmperf.c
@@ -14,6 +14,8 @@ 
 #include <linux/percpu.h>
 #include <linux/smp.h>
 
+#include "cpu.h"
+
 struct aperfmperf_sample {
 	unsigned int	khz;
 	ktime_t	time;
@@ -24,7 +26,7 @@  struct aperfmperf_sample {
 static DEFINE_PER_CPU(struct aperfmperf_sample, samples);
 
 #define APERFMPERF_CACHE_THRESHOLD_MS	10
-#define APERFMPERF_REFRESH_DELAY_MS	20
+#define APERFMPERF_REFRESH_DELAY_MS	10
 #define APERFMPERF_STALE_THRESHOLD_MS	1000
 
 /*
@@ -38,8 +40,6 @@  static void aperfmperf_snapshot_khz(void
 	u64 aperf, aperf_delta;
 	u64 mperf, mperf_delta;
 	struct aperfmperf_sample *s = this_cpu_ptr(&samples);
-	ktime_t now = ktime_get();
-	s64 time_delta = ktime_ms_delta(now, s->time);
 	unsigned long flags;
 
 	local_irq_save(flags);
@@ -57,38 +57,68 @@  static void aperfmperf_snapshot_khz(void
 	if (mperf_delta == 0)
 		return;
 
-	s->time = now;
+	s->time = ktime_get();
 	s->aperf = aperf;
 	s->mperf = mperf;
-
-	/* If the previous iteration was too long ago, discard it. */
-	if (time_delta > APERFMPERF_STALE_THRESHOLD_MS)
-		s->khz = 0;
-	else
-		s->khz = div64_u64((cpu_khz * aperf_delta), mperf_delta);
+	s->khz = div64_u64((cpu_khz * aperf_delta), mperf_delta);
 }
 
-unsigned int arch_freq_get_on_cpu(int cpu)
+static bool aperfmperf_snapshot_cpu(int cpu, ktime_t now, bool wait)
 {
-	s64 time_delta;
-	unsigned int khz;
+	s64 time_delta = ktime_ms_delta(now, per_cpu(samples.time, cpu));
+
+	/* Don't bother re-computing within the cache threshold time. */
+	if (time_delta < APERFMPERF_CACHE_THRESHOLD_MS)
+		return true;
+
+	smp_call_function_single(cpu, aperfmperf_snapshot_khz, NULL, wait);
+
+	/* Return false if the previous iteration was too long ago. */
+	return time_delta <= APERFMPERF_STALE_THRESHOLD_MS;
+}
 
+unsigned int aperfmperf_get_khz(int cpu)
+{
 	if (!cpu_khz)
 		return 0;
 
 	if (!static_cpu_has(X86_FEATURE_APERFMPERF))
 		return 0;
 
-	/* Don't bother re-computing within the cache threshold time. */
-	time_delta = ktime_ms_delta(ktime_get(), per_cpu(samples.time, cpu));
-	khz = per_cpu(samples.khz, cpu);
-	if (khz && time_delta < APERFMPERF_CACHE_THRESHOLD_MS)
-		return khz;
+	aperfmperf_snapshot_cpu(cpu, ktime_get(), true);
+	return per_cpu(samples.khz, cpu);
+}
 
-	smp_call_function_single(cpu, aperfmperf_snapshot_khz, NULL, 1);
-	khz = per_cpu(samples.khz, cpu);
-	if (khz)
-		return khz;
+void arch_freq_prepare_all(void)
+{
+	ktime_t now = ktime_get();
+	bool wait = false;
+	int cpu;
+
+	if (!cpu_khz)
+		return;
+
+	if (!static_cpu_has(X86_FEATURE_APERFMPERF))
+		return;
+
+	for_each_online_cpu(cpu)
+		if (!aperfmperf_snapshot_cpu(cpu, now, false))
+			wait = true;
+
+	if (wait)
+		msleep(APERFMPERF_REFRESH_DELAY_MS);
+}
+
+unsigned int arch_freq_get_on_cpu(int cpu)
+{
+	if (!cpu_khz)
+		return 0;
+
+	if (!static_cpu_has(X86_FEATURE_APERFMPERF))
+		return 0;
+
+	if (aperfmperf_snapshot_cpu(cpu, ktime_get(), true))
+		return per_cpu(samples.khz, cpu);
 
 	msleep(APERFMPERF_REFRESH_DELAY_MS);
 	smp_call_function_single(cpu, aperfmperf_snapshot_khz, NULL, 1);
Index: linux-pm/arch/x86/kernel/cpu/cpu.h
===================================================================
--- linux-pm.orig/arch/x86/kernel/cpu/cpu.h
+++ linux-pm/arch/x86/kernel/cpu/cpu.h
@@ -47,4 +47,7 @@  extern const struct cpu_dev *const __x86
 
 extern void get_cpu_cap(struct cpuinfo_x86 *c);
 extern void cpu_detect_cache_sizes(struct cpuinfo_x86 *c);
+
+unsigned int aperfmperf_get_khz(int cpu);
+
 #endif /* ARCH_X86_CPU_H */
Index: linux-pm/fs/proc/cpuinfo.c
===================================================================
--- linux-pm.orig/fs/proc/cpuinfo.c
+++ linux-pm/fs/proc/cpuinfo.c
@@ -1,12 +1,18 @@ 
 // SPDX-License-Identifier: GPL-2.0
+#include <linux/cpufreq.h>
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 
+__weak void arch_freq_prepare_all(void)
+{
+}
+
 extern const struct seq_operations cpuinfo_op;
 static int cpuinfo_open(struct inode *inode, struct file *file)
 {
+	arch_freq_prepare_all();
 	return seq_open(file, &cpuinfo_op);
 }
 
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -917,6 +917,7 @@  static inline bool policy_has_boost_freq
 }
 #endif
 
+extern void arch_freq_prepare_all(void);
 extern unsigned int arch_freq_get_on_cpu(int cpu);
 
 extern void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,