[PATCH 2/3 linux-next] cpufreq: conservative: Fix the logic in frequency decrease checking

[PATCH 2/3 linux-next] cpufreq: conservative: Fix the logic in frequency decrease checking

[Stratos Karafotis]

When we evaluate the CPU load for frequency decrease we have to compare
the load against down_threshold. There is no need to subtract 10 points
from down_threshold.

Instead, we have to use the default down_threshold or user's selection
unmodified.

Signed-off-by: Stratos Karafotis <stratosk@semaphore.gr>
---
 drivers/cpufreq/cpufreq_conservative.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 1e3be56..08be431 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -92,12 +92,8 @@ static void cs_check_cpu(int cpu, unsigned int load)
 		return;
 	dbs_info->down_skip = 0;
 
-	/*
-	 * The optimal frequency is the frequency that is the lowest that can
-	 * support the current CPU usage without triggering the up policy. To be
-	 * safe, we focus 10 points under the threshold.
-	 */
-	if (load < (cs_tuners.down_threshold - 10)) {
+	/* Check for frequency decrease */
+	if (load < cs_tuners.down_threshold) {
 		freq_target = (cs_tuners.freq_step * policy->max) / 100;
 
 		dbs_info->requested_freq -= freq_target;
-- 
1.8.1.4

Re: [PATCH 2/3 linux-next] cpufreq: conservative: Fix the logic in frequency decrease checking

[Viresh Kumar]

On 6 March 2013 06:06, Stratos Karafotis <stratosk@semaphore.gr> wrote:
> When we evaluate the CPU load for frequency decrease we have to compare
> the load against down_threshold. There is no need to subtract 10 points
> from down_threshold.
>
> Instead, we have to use the default down_threshold or user's selection
> unmodified.
>
> Signed-off-by: Stratos Karafotis <stratosk@semaphore.gr>
> ---
>  drivers/cpufreq/cpufreq_conservative.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCH 2/3 linux-next] cpufreq: conservative: Fix the logic in frequency decrease checking

[David C Niemi]

The "10" sounds like an attempt to add some hysteresis to the up/down decisionmaking.  If you take it out, you should make sure you don't get into situations where you're continually switching rapidly between two frequencies.  (In the ondemand governor some care was also taken to avoid the cost of doing a CPU idleness evaluation counting towards the CPU looking busy enough to upshift; I am not familiar enough with Conservative to know whether that is a problem for it too).

DCN

On 03/05/13 17:06, Stratos Karafotis wrote:
> When we evaluate the CPU load for frequency decrease we have to compare
> the load against down_threshold. There is no need to subtract 10 points
> from down_threshold.
>
> Instead, we have to use the default down_threshold or user's selection
> unmodified.
>
> Signed-off-by: Stratos Karafotis <stratosk@semaphore.gr>
> ---
>  drivers/cpufreq/cpufreq_conservative.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index 1e3be56..08be431 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -92,12 +92,8 @@ static void cs_check_cpu(int cpu, unsigned int load)
>  		return;
>  	dbs_info->down_skip = 0;
>  
> -	/*
> -	 * The optimal frequency is the frequency that is the lowest that can
> -	 * support the current CPU usage without triggering the up policy. To be
> -	 * safe, we focus 10 points under the threshold.
> -	 */
> -	if (load < (cs_tuners.down_threshold - 10)) {
> +	/* Check for frequency decrease */
> +	if (load < cs_tuners.down_threshold) {
>  		freq_target = (cs_tuners.freq_step * policy->max) / 100;
>  
>  		dbs_info->requested_freq -= freq_target;

Re: [PATCH 2/3 linux-next] cpufreq: conservative: Fix the logic in frequency decrease checking

[Stratos Karafotis]

On 03/06/2013 09:35 PM, David C Niemi wrote:
> The "10" sounds like an attempt to add some hysteresis to the up/down decisionmaking.  If you take it out, you should make sure you don't get into situations where you're continually switching rapidly between two frequencies.  (In the ondemand governor some care was also taken to avoid the cost of doing a CPU idleness evaluation counting towards the CPU looking busy enough to upshift; I am not familiar enough with Conservative to know whether that is a problem for it too).
> 
> DCN

This is true for ondemand but, as you know, there is a separate tunable 
(down_threshold) in conservative with default value 20.
It's independent from up_threshold (default 80), so I believe there is no
need to add a hysteresis.
Also, if we subtract 10 from down_threshold, we change user's decision
about this threshold. For example, if user sets down_threshold to 25, wants
this value to 25 not to 15.

Checking the initial commit of conservative governor, we can see that it
was not use hysteresis factor. This was added later (by mistake in my opinion)
as an attempt to make conservative to function similar to ondemand.