NACK/Cmnt: [SRU J][PATCH 1/1] cpufreq: ACPI: Defer setting boost MSRs

[Stefan Bader]

On 22.03.23 22:39, Michael Reed wrote:
> From: Stuart Hayes <stuart.w.hayes at gmail.com>
> 
> 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.
> 
> BugLink: https://bugs.launchpad.net/bugs/2008527
> 
> Signed-off-by: Stuart Hayes <stuart.w.hayes at gmail.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
> (cherry picked from commit 13fdbc8b8da6a2325cad3359c9a70504b0ff2f93)
> Signed-off-by: Michael Reed <Michael.Reed at canonical.com>
> ---

Rejected for the following reasons:

This is a patch from 6.2. While this might be ok for Lunar once it moves 
to that version, I can see no reason why this should not also apply and 
get applied to Kinetic. Even more so since desktop users that installed 
a fresh 22.04 will be on the HWE stream which is now a Kinetic kernel.

-Stefan
>   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 28467d83c745..8b5c84c9dc1e 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -530,15 +530,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)
>   {
>   	/*
> @@ -892,6 +883,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:
> @@ -911,6 +904,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);
> @@ -967,25 +961,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;
> @@ -1027,7 +1005,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;
>   }
> @@ -1036,8 +1013,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();

-- 
- Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xE8675DEECBEECEA3.asc
Type: application/pgp-keys
Size: 44613 bytes
Desc: OpenPGP public key
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20230323/4a398ad1/attachment-0001.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20230323/4a398ad1/attachment-0001.sig>