cpufreq: governors: Remove duplicate check of target freq in supported range

PATCH In review Stratos Karafotis 2 participants 7 messages Aug 26, 2013 – Aug 28, 2013 CPU frequency scaling framework

Share & tools

Stratos Karafotis

author

2013-08-26 18:37 UTC #0 lore raw

PATCH 1/1 cpufreq: governors: Remove duplicate check of target freq in supported range

Function __cpufreq_driver_target checks if target_freq is within policy->min and policy->max range. generic_powersave_bias_target also checks if target_freq is valid through cpufreq_frequency_table_target call. So, drop the unnecessary duplicate check in *_check_cpu functions.

Signed-off-by Stratos Karafotis <[email protected]>

---
 drivers/cpufreq/cpufreq_conservative.c | 4 ----
 drivers/cpufreq/cpufreq_ondemand.c     | 3 ---
 2 files changed, 7 deletions(-)

drivers/cpufreq/cpufreq_conservative.c +0 -4

index 7f67a75..f62d822 100644
@@ -67,8 +67,6 @@ static void cs_check_cpu(int cpu, unsigned int load)
 			return;
 
 		dbs_info->requested_freq += get_freq_target(cs_tuners, policy);
-		if (dbs_info->requested_freq > policy->max)
-			dbs_info->requested_freq = policy->max;
 
 		__cpufreq_driver_target(policy, dbs_info->requested_freq,
 			CPUFREQ_RELATION_H);

@@ -89,8 +87,6 @@ static void cs_check_cpu(int cpu, unsigned int load)
 			return;
 
 		dbs_info->requested_freq -= get_freq_target(cs_tuners, policy);
-		if (dbs_info->requested_freq < policy->min)
-			dbs_info->requested_freq = policy->min;
 
 		__cpufreq_driver_target(policy, dbs_info->requested_freq,
 				CPUFREQ_RELATION_L);

drivers/cpufreq/cpufreq_ondemand.c +0 -3

index 87f3305..32f26f6 100644
@@ -177,9 +177,6 @@ static void od_check_cpu(int cpu, unsigned int load)
 		/* No longer fully busy, reset rate_mult */
 		dbs_info->rate_mult = 1;
 
-		if (freq_next < policy->min)
-			freq_next = policy->min;
-
 		if (!od_tuners->powersave_bias) {
 			__cpufreq_driver_target(policy, freq_next,
 					CPUFREQ_RELATION_L);

Signature · 1 line

1.8.3.1

Viresh Kumar

maintainer

2013-08-27 05:57 UTC #1 lore raw

Quoted 1 line · Stratos Karafotis

 drivers/cpufreq/cpufreq_conservative.c | 4 ----

Get rid of few more checks..

/* if we are already at full speed then break out early */ if (dbs_info->requested_freq == policy->max) return;

/*
* if we cannot reduce the frequency anymore, break out early */
if (policy->cur == policy->min)
return;

Stratos Karafotis

author

2013-08-27 15:46 UTC #2 lore raw

Quoted 10 lines · Viresh Kumar

Quoted 1 line · Stratos Karafotis
  drivers/cpufreq/cpufreq_conservative.c | 4 ----
Get rid of few more checks..
/* if we are already at full speed then break out early */ if (dbs_info->requested_freq == policy->max) return;

Quoted 6 lines

/*
* if we cannot reduce the frequency anymore, break out early */
if (policy->cur == policy->min)
return;

I think we should keep these checks because:

1) They shorten the execution code (there is no unnecessary call of __cpufreq_driver_target)
2) In case my patch will be accepted, we need them to avoid continuously increase of dbs_info->requested_freq.With my patch the requested_freq can temporarily overcome policy->min and policy->max. __cpufreq_driver_target will select the correct frequency (within policy->min and policy->max). Then, dbs_cpufreq_notifier will adjust requested_freq.

I hope the logic in 2) to be acceptable.

Thanks,
Stratos

Viresh Kumar

maintainer

2013-08-27 16:07 UTC #3 lore raw

Quoted 4 lines · Stratos Karafotis

I think we should keep these checks because:
1) They shorten the execution code (there is no unnecessary call of __cpufreq_driver_target)

I don't really count this one.. This is how code is present everywhere in kernel.. These checks are present in routines and callers don't need to take care of them..

Quoted 5 lines

2) In case my patch will be accepted, we need them to avoid continuously increase of dbs_info->requested_freq.With my patch the requested_freq can temporarily overcome policy->min and policy->max. __cpufreq_driver_target will select the correct frequency (within policy->min and policy->max). Then, dbs_cpufreq_notifier will adjust requested_freq.

Sorry, I couldn't understand what you meant here :(

Stratos Karafotis

author

2013-08-27 17:34 UTC #4 lore raw

Quoted 9 lines · Viresh Kumar

Quoted 4 lines · Stratos Karafotis
I think we should keep these checks because:
1) They shorten the execution code (there is no unnecessary call of __cpufreq_driver_target)
I don't really count this one.. This is how code is present everywhere in kernel.. These checks are present in routines and callers don't need to take care of them..

I mean that if we will get rid of the code you mentioned, we will have an extra call to function __cpufreq_driver_target in some cases.

Quoted 8 lines

Quoted 5 lines
2) In case my patch will be accepted, we need them to avoid continuously increase of dbs_info->requested_freq.With my patch the requested_freq can temporarily overcome policy->min and policy->max. __cpufreq_driver_target will select the correct frequency (within policy->min and policy->max). Then, dbs_cpufreq_notifier will adjust requested_freq.
Sorry, I couldn't understand what you meant here :(

I'm sorry. Let me try to explain this better.

With my patch, dbs_info->requested_freq will not be capped within
policy->min and policy->max in cs_check_cpu.
So, temporarily it may have a value greater than policy->max
or lower that policy->min.
When we call __cpufreq_driver_target, the correct frequency will be selected because __cpufreq_driver_target takes care to adjust the target frequency within policy range.
But, eventually, dbs_cpufreq_notifier will adjust dbs_info->requested
within policy range, if needed.

If we remove

	if (dbs_info->requested_freq == policy->max)
		return;
and
	if (policy->cur == policy->min)
		return;

request_freq will keep increasing or decreasing in each iteration and finally will overflow or underflow.

Consider, for example, that in a CPU with policy->max = 1000MHz the current frequency is 950MHz. With a constant load above up_threshold, the requested_freq in first iteration will be 1000MHz and __cpufreq_driver_target will select 1000MHz freq.

In second iteration, requested_freq will be 1050MHz, and __cpufreq_driver_target will select 1000MHz. dbs_cpufreq_notifier will adjust requested_freq back to 1000MHz.

In next iterations, dbs_cpufreq_notifier will not be called, so we need the above check (dbs_info->requested_freq == policy->max) to prevent requested_freq to grow arbitrary.

I hope my explanation was better now. :)

Thanks,
Stratos

Viresh Kumar

maintainer

2013-08-28 05:35 UTC #5 lore raw

Quoted 1 line · Stratos Karafotis

I'm sorry. Let me try to explain this better.

Don't be :)

Quoted 31 lines

With my patch, dbs_info->requested_freq will not be capped within policy->min and policy->max in cs_check_cpu.
So, temporarily it may have a value greater than policy->max
or lower that policy->min.
When we call __cpufreq_driver_target, the correct frequency will be selected because __cpufreq_driver_target takes care to adjust the
target frequency within policy range.
But, eventually, dbs_cpufreq_notifier will adjust dbs_info->requested within policy range, if needed.
If we remove
        if (dbs_info->requested_freq == policy->max)
                return;
and
        if (policy->cur == policy->min)
                return;
request_freq will keep increasing or decreasing in each iteration and finally will overflow or underflow.
Consider, for example, that in a CPU with policy->max = 1000MHz the current frequency is 950MHz. With a constant load above up_threshold, the requested_freq in first iteration will be 1000MHz and __cpufreq_driver_target will select 1000MHz freq.
In second iteration, requested_freq will be 1050MHz, and __cpufreq_driver_target will select 1000MHz. dbs_cpufreq_notifier will adjust requested_freq back to 1000MHz.

Quoted 5 lines

In next iterations, dbs_cpufreq_notifier will not be called, so we need the above check (dbs_info->requested_freq == policy->max) to prevent requested_freq to grow arbitrary.
I hope my explanation was better now. :)

Yes, your initial patch is fine.

Viresh Kumar

maintainer Acked-by on v1 1/1

2013-08-28 05:35 UTC #6 lore raw

In reply to Stratos Karafotis

Quoted 10 lines · Stratos Karafotis

Function __cpufreq_driver_target checks if target_freq is within policy->min and policy->max range. generic_powersave_bias_target also checks if target_freq is valid through cpufreq_frequency_table_target call. So, drop the unnecessary duplicate check in *_check_cpu functions.
Signed-off-by Stratos Karafotis <[email protected]>
---
 drivers/cpufreq/cpufreq_conservative.c | 4 ----
 drivers/cpufreq/cpufreq_ondemand.c     | 3 ---
 2 files changed, 7 deletions(-)

Acked-by Viresh Kumar <[email protected]>