| Message ID | 521BA068.8030003@semaphore.gr (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 27 August 2013 00:07, Stratos Karafotis <stratosk@semaphore.gr> wrote:
> 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/27/2013 08:57 AM, Viresh Kumar wrote: > On 27 August 2013 00:07, Stratos Karafotis <stratosk@semaphore.gr> wrote: >> 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; > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27 August 2013 21:16, Stratos Karafotis <stratosk@semaphore.gr> wrote: > 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.. > 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 :( -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/27/2013 07:07 PM, Viresh Kumar wrote: > On 27 August 2013 21:16, Stratos Karafotis <stratosk@semaphore.gr> wrote: >> 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. >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
27 August 2013 23:04, Stratos Karafotis <stratosk@semaphore.gr> wrote: > I'm sorry. Let me try to explain this better. Don't be :) > 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. :) Yes, your initial patch is fine. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27 August 2013 00:07, Stratos Karafotis <stratosk@semaphore.gr> wrote: > 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 <stratosk@semaphore.gr> > --- > drivers/cpufreq/cpufreq_conservative.c | 4 ---- > drivers/cpufreq/cpufreq_ondemand.c | 3 --- > 2 files changed, 7 deletions(-) Acked-by: Viresh Kumar <viresh.kumar@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c index 7f67a75..f62d822 100644 --- a/drivers/cpufreq/cpufreq_conservative.c +++ b/drivers/cpufreq/cpufreq_conservative.c @@ -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); diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c index 87f3305..32f26f6 100644 --- a/drivers/cpufreq/cpufreq_ondemand.c +++ b/drivers/cpufreq/cpufreq_ondemand.c @@ -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);
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 <stratosk@semaphore.gr> --- drivers/cpufreq/cpufreq_conservative.c | 4 ---- drivers/cpufreq/cpufreq_ondemand.c | 3 --- 2 files changed, 7 deletions(-)