diff mbox series

[v3] cpufreq/cppc: Remove the desired_perf compare when set target

Message ID OS3P286MB249076187B3497D1EDD70988B1F32@OS3P286MB2490.JPNP286.PROD.OUTLOOK.COM
State New
Headers show
Series [v3] cpufreq/cppc: Remove the desired_perf compare when set target | expand

Commit Message

Riwen Lu May 30, 2024, 11:08 a.m. UTC 
From: Riwen Lu <luriwen@kylinos.cn>

There is a case that desired_perf is exactly the same with the old perf,
but the actual current freq is not.

This happened in S3 while the cpufreq governor is set to powersave.
During cpufreq resume process, the booting CPU's new_freq obtained via
.get() is the highest frequency, while the policy->cur and
cpu->perf_ctrls.desired_perf are in the lowest level(powersave
governor). Causing the warning: "CPU frequency out of sync:", and set
policy->cur to new_freq. Then the governor->limits() calls
cppc_cpufreq_set_target() to configures the CPU frequency and returns
directly because the desired_perf converted from target_freq is the
same with cpu->perf_ctrls.desired_perf and both are the lowest_perf.
Since target_freq and policy->cur have been compared in
__cpufreq_driver_target(), there's no need to compare desired_perf
and cpu->perf_ctrls.desired_perf again in cppc_cpufreq_set_target()
to ensure that the CPU frequency is properly configured.

Signed-off-by: Riwen Lu <luriwen@kylinos.cn>

---
v1 -> v2:
 - Update commit message and email.
v2 -> v3:
 - Update patch subject and commit message.
 - Remove the desired_perf compare logic.
---
 drivers/cpufreq/cppc_cpufreq.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Viresh Kumar June 6, 2024, 9:07 a.m. UTC | #1 
Ionela, Beata,

On 30-05-24, 19:08, Riwen Lu wrote:
> From: Riwen Lu <luriwen@kylinos.cn>
> 
> There is a case that desired_perf is exactly the same with the old perf,
> but the actual current freq is not.
> 
> This happened in S3 while the cpufreq governor is set to powersave.
> During cpufreq resume process, the booting CPU's new_freq obtained via
> .get() is the highest frequency, while the policy->cur and
> cpu->perf_ctrls.desired_perf are in the lowest level(powersave
> governor). Causing the warning: "CPU frequency out of sync:", and set
> policy->cur to new_freq. Then the governor->limits() calls
> cppc_cpufreq_set_target() to configures the CPU frequency and returns
> directly because the desired_perf converted from target_freq is the
> same with cpu->perf_ctrls.desired_perf and both are the lowest_perf.
> Since target_freq and policy->cur have been compared in
> __cpufreq_driver_target(), there's no need to compare desired_perf
> and cpu->perf_ctrls.desired_perf again in cppc_cpufreq_set_target()
> to ensure that the CPU frequency is properly configured.
> 
> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> 
> ---
> v1 -> v2:
>  - Update commit message and email.
> v2 -> v3:
>  - Update patch subject and commit message.
>  - Remove the desired_perf compare logic.
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 15f1d41920a3..337cece61ab5 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -295,9 +295,6 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>  	int ret = 0;
>  
>  	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
> -	/* Return if it is exactly the same perf */
> -	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
> -		return ret;
>  
>  	cpu_data->perf_ctrls.desired_perf = desired_perf;
>  	freqs.old = policy->cur;

Any objections to this change ?
Riwen Lu June 12, 2024, 2:52 a.m. UTC | #2 
在 2024/6/11 17:10, Viresh Kumar 写道:
> On 11-06-24, 09:54, Ionela Voinescu wrote:
>> It's alright with me.
> 
> Great.
> 
>> Some "nits":
>>   - the "desired_perf" local variable could be removed in this case.
> 
> Riwen, please fix this and resend.

I think it's no need to remove the "desired_perf" local variable here, 
the code is seems more readable.
> 
>>   - [note] while this change helps, we'd still need policy->cur to always
>>     have the latest request value (see details at [1]) for this check to
>>     be made obsolete by the comparison between target_freq and policy->cur,
>>     as mentioned in the commit message. But this is/can be a separate
>>     matter.
>>
>>     [1] https://lore.kernel.org/lkml/ZmB1qKucR5fXk100@arm.com/
> 
> Yeah, lets discuss that in the other thread only.
>
Viresh Kumar June 12, 2024, 6:24 a.m. UTC | #3 
On 12-06-24, 10:52, Riwen Lu wrote:
> I think it's no need to remove the "desired_perf" local variable here, the
> code is seems more readable.

There is no need of a temporary variable and I don't think it increases
readability at all. Moreover, same needs to be done in
cppc_cpufreq_fast_switch() too.
Viresh Kumar June 12, 2024, 6:26 a.m. UTC | #4 
On 12-06-24, 11:54, Viresh Kumar wrote:
> On 12-06-24, 10:52, Riwen Lu wrote:
> > I think it's no need to remove the "desired_perf" local variable here, the
> > code is seems more readable.
> 
> There is no need of a temporary variable and I don't think it increases
> readability at all. Moreover, same needs to be done in
> cppc_cpufreq_fast_switch() too.

Also please don't send the new patch in-reply-to the earlier one. Send
a fresh thread.

Thanks.
Pierre Gondois June 12, 2024, 9:08 a.m. UTC | #5 
Hello Riwen,

This function seems to be the only cpufreq function saving and comparing the
requested frequency with the last requested frequency. This seems to be more the
task of the cpufreq framework than the cpufreq driver.

So FYIW, the patch looks good to me.

On 5/30/24 13:08, Riwen Lu wrote:
> From: Riwen Lu <luriwen@kylinos.cn>
> 
> There is a case that desired_perf is exactly the same with the old perf,
> but the actual current freq is not.
> 
> This happened in S3 while the cpufreq governor is set to powersave.
> During cpufreq resume process, the booting CPU's new_freq obtained via
> .get() is the highest frequency, while the policy->cur and
> cpu->perf_ctrls.desired_perf are in the lowest level(powersave
> governor). Causing the warning: "CPU frequency out of sync:", and set
> policy->cur to new_freq.

(new paragraph)

Then the governor->limits() calls
> cppc_cpufreq_set_target() to configures the CPU frequency and returns
> directly because the desired_perf converted from target_freq is the
> same with cpu->perf_ctrls.desired_perf and both are the lowest_perf.

(new paragraph)

> Since target_freq and policy->cur have been compared in
> __cpufreq_driver_target(), there's no need to compare desired_perf
> and cpu->perf_ctrls.desired_perf again in cppc_cpufreq_set_target()
> to ensure that the CPU frequency is properly configured.

NIT:
Would it be possible to make distinct paragraphs ?

> 
> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> 
> ---
> v1 -> v2:
>   - Update commit message and email.
> v2 -> v3:
>   - Update patch subject and commit message.
>   - Remove the desired_perf compare logic.
> ---
>   drivers/cpufreq/cppc_cpufreq.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 15f1d41920a3..337cece61ab5 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -295,9 +295,6 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>   	int ret = 0;
>   
>   	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
> -	/* Return if it is exactly the same perf */
> -	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
> -		return ret;
>   
>   	cpu_data->perf_ctrls.desired_perf = desired_perf;
>   	freqs.old = policy->cur;
Riwen Lu June 14, 2024, 12:12 p.m. UTC | #6 
在 2024/6/12 14:24, Viresh Kumar 写道:
> On 12-06-24, 10:52, Riwen Lu wrote:
>> I think it's no need to remove the "desired_perf" local variable here, the
>> code is seems more readable.
> 
> There is no need of a temporary variable and I don't think it increases
> readability at all. Moreover, same needs to be done in
> cppc_cpufreq_fast_switch() too.
> 
OK. I'll push a patch that remove the desired_perf temporary in 
cppc_cpufreq_fast_switch().
diff mbox series

Patch

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 15f1d41920a3..337cece61ab5 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -295,9 +295,6 @@  static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
 	int ret = 0;
 
 	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
-	/* Return if it is exactly the same perf */
-	if (desired_perf == cpu_data->perf_ctrls.desired_perf)
-		return ret;
 
 	cpu_data->perf_ctrls.desired_perf = desired_perf;
 	freqs.old = policy->cur;