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: <20221102195957.82871-1-stuart.w.hayes@gmail.com>
In-Reply-To: <20221102195957.82871-1-stuart.w.hayes@gmail.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 3 Nov 2022 19:19:47 +0100
Message-ID: <CAJZ5v0iM28y2YSWOv81VCB9vqh2xwJcz36wnR7PujDehvrkN-Q@mail.gmail.com>
Subject: Re: [PATCH] cpufreq: acpi: Defer setting boost MSRs
To:     Stuart Hayes <stuart.w.hayes@gmail.com>
Cc:     "Rafael J . Wysocki" <rafael@kernel.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Kyle Meyer <kyle.meyer@hpe.com>, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Nov 2, 2022 at 9:01 PM Stuart Hayes <stuart.w.hayes@gmail.com> wrote:
>
> When acpi-cpufreq is loaded, boost is enabled on every CPU (by setting an
> MSR) before the driver is registered with cpufreq.  This can be very time
> consuming, because it is done with a CPU hotplug startup callback, and
> cpuhp_setup_state() schedules the callback (cpufreq_boost_online()) to run
> on each CPU one at a time, waiting for each to run before calling the next.
>
> If cpufreq_register_driver() fails--if, for example, there are no ACPI
> P-states present--this is wasted time.
>
> Since cpufreq already sets up a CPU hotplug startup callback if and when
> acpi-cpufreq is registered, set the boost MSRs in acpi_cpufreq_cpu_init(),
> which is called by the cpufreq cpuhp callback.  This allows acpi-cpufreq to
> exit quickly if it is loaded but not needed.
>
> On one system with 192 CPUs, this patch speeds up boot by about 30 seconds.
>
> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 31 +++----------------------------
>  1 file changed, 3 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 1bb2b90ebb21..cb167263de72 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -535,15 +535,6 @@ static void free_acpi_perf_data(void)
>         free_percpu(acpi_perf_data);
>  }
>
> -static int cpufreq_boost_online(unsigned int cpu)
> -{
> -       /*
> -        * On the CPU_UP path we simply keep the boost-disable flag
> -        * in sync with the current global state.
> -        */
> -       return boost_set_msr(acpi_cpufreq_driver.boost_enabled);
> -}
> -
>  static int cpufreq_boost_down_prep(unsigned int cpu)
>  {
>         /*
> @@ -897,6 +888,8 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
>         if (perf->states[0].core_frequency * 1000 != freq_table[0].frequency)
>                 pr_warn(FW_WARN "P-state 0 is not max freq\n");
>
> +       set_boost(policy, acpi_cpufreq_driver.boost_enabled);
> +
>         return result;
>
>  err_unreg:
> @@ -916,6 +909,7 @@ static int acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>
>         pr_debug("%s\n", __func__);
>
> +       cpufreq_boost_down_prep(policy->cpu);
>         policy->fast_switch_possible = false;
>         policy->driver_data = NULL;
>         acpi_processor_unregister_performance(data->acpi_perf_cpu);
> @@ -972,25 +966,9 @@ static void __init acpi_cpufreq_boost_init(void)
>         acpi_cpufreq_driver.set_boost = set_boost;
>         acpi_cpufreq_driver.boost_enabled = boost_state(0);
>
> -       /*
> -        * This calls the online callback on all online cpu and forces all
> -        * MSRs to the same value.
> -        */
> -       ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "cpufreq/acpi:online",
> -                               cpufreq_boost_online, cpufreq_boost_down_prep);
> -       if (ret < 0) {
> -               pr_err("acpi_cpufreq: failed to register hotplug callbacks\n");
> -               return;
> -       }
>         acpi_cpufreq_online = ret;
>  }
>
> -static void acpi_cpufreq_boost_exit(void)
> -{
> -       if (acpi_cpufreq_online > 0)
> -               cpuhp_remove_state_nocalls(acpi_cpufreq_online);
> -}
> -
>  static int __init acpi_cpufreq_init(void)
>  {
>         int ret;
> @@ -1032,7 +1010,6 @@ static int __init acpi_cpufreq_init(void)
>         ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>         if (ret) {
>                 free_acpi_perf_data();
> -               acpi_cpufreq_boost_exit();
>         }
>         return ret;
>  }
> @@ -1041,8 +1018,6 @@ static void __exit acpi_cpufreq_exit(void)
>  {
>         pr_debug("%s\n", __func__);
>
> -       acpi_cpufreq_boost_exit();
> -
>         cpufreq_unregister_driver(&acpi_cpufreq_driver);
>
>         free_acpi_perf_data();
> --

Applied as 6.2 material, thanks!