Re: [PATCH] x86: use cpufreq_quick_get() for /proc/cpuinfo "cpu MHz" again

[WANG Chao]

On 11/10/17 at 01:06P, Rafael J. Wysocki wrote:
> On Thursday, November 9, 2017 11:30:54 PM CET Rafael J. Wysocki wrote:
> > On Thu, Nov 9, 2017 at 5:06 PM, Rafael J. Wysocki
> > <rafael.j.wysocki@xxxxxxxxx> wrote:
> > > Hi Linus,
> > >
> > > On 11/9/2017 11:38 AM, WANG Chao wrote:
> > >>
> > >> Commit 941f5f0f6ef5 (x86: CPU: Fix up "cpu MHz" in /proc/cpuinfo) caused
> > >> a serious performance issue when reading from /proc/cpuinfo on system
> > >> with aperfmperf.
> > >>
> > >> For each cpu, arch_freq_get_on_cpu() sleeps 20ms to get its frequency.
> > >> On a system with 64 cpus, it takes 1.5s to finish running `cat
> > >> /proc/cpuinfo`, while it previously was done in 15ms.
> > >
> > > Honestly, I'm not sure what to do to address this ATM.
> > >
> > > The last requested frequency is only available in the non-HWP case, so it
> > > cannot be used universally.
> > 
> > OK, here's an idea.
> > 
> > c_start() can run aperfmperf_snapshot_khz() on all CPUs upfront (say
> > in parallel), then wait for a while (say 5 ms; the current 20 ms wait
> > is overkill) and then aperfmperf_snapshot_khz() can be run once on
> > each CPU in show_cpuinfo() without taking the "stale cache" threshold
> > into account.
> > 
> > I'm going to try that and see how far I can get with it.
> 
> Below is what I have.
> 
> I ended up using APERFMPERF_REFRESH_DELAY_MS for the delay in
> aperfmperf_snapshot_all(), because 5 ms tended to add too much
> variation to the results on my test box.
> 
> I think it may be reduced to 10 ms, though.
> 
> Chao, can you please try this one and report back?

Hi, Rafael

Thanks for the patch. But it doesn't work for me. lscpu takes 1.5s to
finish on a 64 cpus AMD box with aperfmperf.

You missed the fact that c_start() will also be called by c_next().

But I don't think the overall idea is good enough. I think /proc/cpuinfo
is too general for usespace too be delayed, no matter it's 10ms or 20ms.

My point is cpu MHz is best to use a cached value for quick access. If
people are looking for reliable and accurate cpu frequency,
/proc/cpuinfo is probably a bad idae.

What do you think?

WANG Chao

> 
> 
> ---
>  arch/x86/kernel/cpu/aperfmperf.c |   42 ++++++++++++++++++++++++++++-----------
>  arch/x86/kernel/cpu/cpu.h        |    4 +++
>  arch/x86/kernel/cpu/proc.c       |    5 +++-
>  3 files changed, 39 insertions(+), 12 deletions(-)
> 
> 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;
> @@ -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,15 +57,10 @@ 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)
> @@ -82,16 +77,41 @@ unsigned int arch_freq_get_on_cpu(int cp
>  	/* 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)
> +	if (time_delta < APERFMPERF_CACHE_THRESHOLD_MS)
>  		return khz;
>  
>  	smp_call_function_single(cpu, aperfmperf_snapshot_khz, NULL, 1);
>  	khz = per_cpu(samples.khz, cpu);
> -	if (khz)
> +	if (time_delta <= APERFMPERF_STALE_THRESHOLD_MS)
>  		return khz;
>  
> +	/* If the previous iteration was too long ago, take a new data point. */
>  	msleep(APERFMPERF_REFRESH_DELAY_MS);
>  	smp_call_function_single(cpu, aperfmperf_snapshot_khz, NULL, 1);
>  
>  	return per_cpu(samples.khz, cpu);
>  }
> +
> +void aperfmperf_snapshot_all(void)
> +{
> +	if (!cpu_khz)
> +		return;
> +
> +	if (!static_cpu_has(X86_FEATURE_APERFMPERF))
> +		return;
> +
> +	smp_call_function_many(cpu_online_mask, aperfmperf_snapshot_khz, NULL, 1);
> +	msleep(APERFMPERF_REFRESH_DELAY_MS);
> +}
> +
> +unsigned int aperfmperf_snapshot_cpu(int cpu)
> +{
> +	if (!cpu_khz)
> +		return 0;
> +
> +	if (!static_cpu_has(X86_FEATURE_APERFMPERF))
> +		return 0;
> +
> +	smp_call_function_single(cpu, aperfmperf_snapshot_khz, NULL, 1);
> +	return per_cpu(samples.khz, cpu);
> +}
> 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,8 @@ 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);
> +
> +extern unsigned int aperfmperf_snapshot_cpu(int cpu);
> +extern void aperfmperf_snapshot_all(void);
> +
>  #endif /* ARCH_X86_CPU_H */
> 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,7 +80,7 @@ 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 = arch_freq_get_on_cpu(cpu);
> +		unsigned int freq = aperfmperf_snapshot_cpu(cpu);
>  
>  		if (!freq)
>  			freq = cpufreq_quick_get(cpu);
> @@ -141,6 +143,7 @@ static int show_cpuinfo(struct seq_file
>  
>  static void *c_start(struct seq_file *m, loff_t *pos)
>  {
> +	aperfmperf_snapshot_all();
>  	*pos = cpumask_next(*pos - 1, cpu_online_mask);
>  	if ((*pos) < nr_cpu_ids)
>  		return &cpu_data(*pos);
>