Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220415133356.179706384@linutronix.de> <20220415161206.875029458@linutronix.de>
In-Reply-To: <20220415161206.875029458@linutronix.de>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 19 Apr 2022 18:35:37 +0200
Message-ID: <CAJZ5v0jcS8p1aKfW=W05-+Zo=MytwyxOCCO+Fq-WfNhkW7804w@mail.gmail.com>
Subject: Re: [patch 09/10] x86/aperfmperf: Replace aperfmperf_get_khz()
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Eric Dumazet <eric.dumazet@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>