Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220415133356.179706384@linutronix.de> <20220415161206.536733494@linutronix.de>
In-Reply-To: <20220415161206.536733494@linutronix.de>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 19 Apr 2022 18:04:04 +0200
Message-ID: <CAJZ5v0h07hXW3WuBq673yk1HOuQLSw7-hJTntX331FDZRh1tDA@mail.gmail.com>
Subject: Re: [patch 03/10] x86/aperfmperf: Separate AP/BP frequency invariance init
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Apr 15, 2022 at 9:19 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> This code is convoluted and because it can be invoked post init via the
> ACPI/CPPC code, all of the initialization functionality is built in instead
> of being part of init text and init data.
>
> As a first step create separate calls for the boot and the application
> processors.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/topology.h  |   12 +++++-------
>  arch/x86/kernel/acpi/cppc.c      |    3 ++-
>  arch/x86/kernel/cpu/aperfmperf.c |   23 +++++++++++------------
>  arch/x86/kernel/smpboot.c        |    4 ++--
>  4 files changed, 20 insertions(+), 22 deletions(-)
>
> --- a/arch/x86/include/asm/topology.h
> +++ b/arch/x86/include/asm/topology.h
> @@ -216,14 +216,12 @@ extern void arch_scale_freq_tick(void);
>  #define arch_scale_freq_tick arch_scale_freq_tick
>
>  extern void arch_set_max_freq_ratio(bool turbo_disabled);
> -void init_freq_invariance(bool secondary, bool cppc_ready);
> +extern void bp_init_freq_invariance(bool cppc_ready);
> +extern void ap_init_freq_invariance(void);
>  #else
> -static inline void arch_set_max_freq_ratio(bool turbo_disabled)
> -{
> -}
> -static inline void init_freq_invariance(bool secondary, bool cppc_ready)
> -{
> -}
> +static inline void arch_set_max_freq_ratio(bool turbo_disabled) { }
> +static inline void bp_init_freq_invariance(bool cppc_ready) { }
> +static inline void ap_init_freq_invariance(void) { }
>  #endif
>
>  #ifdef CONFIG_ACPI_CPPC_LIB
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -96,7 +96,8 @@ void init_freq_invariance_cppc(void)
>
>         mutex_lock(&freq_invariance_lock);
>
> -       init_freq_invariance(secondary, true);
> +       if (!secondary)
> +               bp_init_freq_invariance(true);
>         secondary = true;
>
>         mutex_unlock(&freq_invariance_lock);
> --- a/arch/x86/kernel/cpu/aperfmperf.c
> +++ b/arch/x86/kernel/cpu/aperfmperf.c
> @@ -428,31 +428,24 @@ static void register_freq_invariance_sys
>  static inline void register_freq_invariance_syscore_ops(void) {}
>  #endif
>
> -void init_freq_invariance(bool secondary, bool cppc_ready)
> +void bp_init_freq_invariance(bool cppc_ready)
>  {
> -       bool ret = false;
> +       bool ret;
>
> -       if (!boot_cpu_has(X86_FEATURE_APERFMPERF))
> +       if (!cpu_feature_enabled(X86_FEATURE_APERFMPERF))
>                 return;
>
> -       if (secondary) {
> -               if (static_branch_likely(&arch_scale_freq_key)) {
> -                       init_counter_refs();
> -               }
> -               return;
> -       }
> +       init_counter_refs();
>
>         if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
>                 ret = intel_set_max_freq_ratio();
>         else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
> -               if (!cppc_ready) {
> +               if (!cppc_ready)
>                         return;
> -               }
>                 ret = amd_set_max_freq_ratio(&arch_turbo_freq_ratio);
>         }
>
>         if (ret) {
> -               init_counter_refs();
>                 static_branch_enable(&arch_scale_freq_key);
>                 register_freq_invariance_syscore_ops();
>                 pr_info("Estimated ratio of average max frequency by base frequency (times 1024): %llu\n", arch_max_freq_ratio);
> @@ -461,6 +454,12 @@ void init_freq_invariance(bool secondary
>         }
>  }
>
> +void ap_init_freq_invariance(void)
> +{
> +       if (cpu_feature_enabled(X86_FEATURE_APERFMPERF))
> +               init_counter_refs();

This doesn't check arch_scale_freq_key now which may be a good thing
to mention in the changelog.

I don't see anything questionable in the patch, though, so

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

> +}
> +
>  static void disable_freq_invariance_workfn(struct work_struct *work)
>  {
>         static_branch_disable(&arch_scale_freq_key);
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -186,7 +186,7 @@ static void smp_callin(void)
>          */
>         set_cpu_sibling_map(raw_smp_processor_id());
>
> -       init_freq_invariance(true, false);
> +       ap_init_freq_invariance();
>
>         /*
>          * Get our bogomips.
> @@ -1396,7 +1396,7 @@ void __init native_smp_prepare_cpus(unsi
>  {
>         smp_prepare_cpus_common();
>
> -       init_freq_invariance(false, false);
> +       bp_init_freq_invariance(false);
>         smp_sanity_check();
>
>         switch (apic_intr_mode) {
>