Re: [patch 09/10] x86/aperfmperf: Replace aperfmperf_get_khz()

Rafael J. Wysocki
2022-04-19T16:35:56Z

On Fri, Apr 15, 2022 at 9:20 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> The frequency invariance infrastructure provides the APERF/MPERF samples
> already. Utilize them for the cpu frequency display in /proc/cpuinfo.
>
> The sample is considered valid for 20ms. So for idle or isolated NOHZ full
> CPUs the function returns 0, which is matching the previous behaviour.
>
> This gets rid of the mass IPIs and a delay of 20ms for stabilizing observed
> by Eric when reading /proc/cpuinfo.
>
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

All fine IMV, one minor nit below.

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
> arch/x86/kernel/cpu/aperfmperf.c | 77 +++++++++++++++++----------------------
> fs/proc/cpuinfo.c | 6 ---
> include/linux/cpufreq.h | 1
> 3 files changed, 35 insertions(+), 49 deletions(-)
>
> --- a/arch/x86/kernel/cpu/aperfmperf.c
> +++ b/arch/x86/kernel/cpu/aperfmperf.c
> @@ -100,49 +100,6 @@ static bool aperfmperf_snapshot_cpu(int
> return time_delta <= APERFMPERF_STALE_THRESHOLD_MS;
> }
>
> -unsigned int aperfmperf_get_khz(int cpu)
> -{
> - if (!cpu_khz)
> - return 0;
> -
> - if (!boot_cpu_has(X86_FEATURE_APERFMPERF))
> - return 0;
> -
> - if (!housekeeping_cpu(cpu, HK_TYPE_MISC))
> - return 0;
> -
> - if (rcu_is_idle_cpu(cpu))
> - return 0; /* Idle CPUs are completely uninteresting. */
> -
> - aperfmperf_snapshot_cpu(cpu, ktime_get(), true);
> - return per_cpu(samples.khz, cpu);
> -}
> -
> -void arch_freq_prepare_all(void)
> -{
> - ktime_t now = ktime_get();
> - bool wait = false;
> - int cpu;
> -
> - if (!cpu_khz)
> - return;
> -
> - if (!boot_cpu_has(X86_FEATURE_APERFMPERF))
> - return;
> -
> - for_each_online_cpu(cpu) {
> - if (!housekeeping_cpu(cpu, HK_TYPE_MISC))
> - continue;
> - if (rcu_is_idle_cpu(cpu))
> - continue; /* Idle CPUs are completely uninteresting. */
> - 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)
> {
> struct aperfmperf_sample *s = per_cpu_ptr(&samples, cpu);
> @@ -529,6 +486,40 @@ void arch_scale_freq_tick(void)
> scale_freq_tick(acnt, mcnt);
> }
>
> +/*
> + * Discard samples older than the define maximum sample age of 20ms. There
> + * is no point in sending IPIs in such a case. If the scheduler tick was
> + * not running then the CPU is either idle or isolated.
> + */
> +#define MAX_SAMPLE_AGE ((unsigned long)HZ / 50)
> +
> +unsigned int aperfmperf_get_khz(int cpu)
> +{
> + struct aperfmperf *s = per_cpu_ptr(&cpu_samples, cpu);
> + unsigned long last;
> + unsigned int seq;
> + u64 acnt, mcnt;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_APERFMPERF))
> + return 0;
> +
> + do {
> + seq = raw_read_seqcount_begin(&s->seq);
> + last = s->last_update;
> + acnt = s->acnt;
> + mcnt = s->mcnt;
> + } while (read_seqcount_retry(&s->seq, seq));
> +
> + /*
> + * Bail on invalid count and when the last update was too long ago,
> + * which covers idle and NOHZ full CPUs.
> + */
> + if (!mcnt || (jiffies - last) > MAX_SAMPLE_AGE)

The inner parens are not needed here.

> + return 0;
> +
> + return div64_u64((cpu_khz * acnt), mcnt);
> +}
> +
> static int __init bp_init_aperfmperf(void)
> {
> if (!cpu_feature_enabled(X86_FEATURE_APERFMPERF))
> --- a/fs/proc/cpuinfo.c
> +++ b/fs/proc/cpuinfo.c
> @@ -5,14 +5,10 @@
> #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);
> }
>
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -1199,7 +1199,6 @@ static inline void sched_cpufreq_governo
> struct cpufreq_governor *old_gov) { }
> #endif
>
> -extern void arch_freq_prepare_all(void);
> extern unsigned int arch_freq_get_on_cpu(int cpu);
>
> #ifndef arch_set_freq_scale
>