cpufreq: conservative: Fix next frequency selection

Commit Message

Rafael J. Wysocki Oct. 12, 2016, 7:47 p.m. UTC

From: Rafael J. Wysocki <[e-mail]>

Commit d352cf47d93e (cpufreq: conservative: Do not use transition
notifications) overlooked the case when the "frequency step" used
by the conservative governor is small relative to the distances
between the available frequencies and broke the algorithm by
using policy->cur instead of the previously requested frequency
when computing the next one.

As a result, the governor may not be able to go outside of a narrow
range between two consecutive available frequencies.

Fix the problem by making the governor save the previously requested
frequency and select the next one relative that value (unless it is
out of range, in which case policy->cur will be used instead).

Fixes: d352cf47d93e (cpufreq: conservative: Do not use transition notifications)
Link: https://bugzilla.kernel.org/show_bug.cgi?id=177171
Reported-and-tested-by: Aleksey Rybalkin <[e-mail]>
Signed-off-by: Rafael J. Wysocki <[e-mail]>
---
 drivers/cpufreq/cpufreq_conservative.c |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Viresh Kumar Oct. 13, 2016, 6:26 a.m. UTC | #1

On 12-10-16, 21:47, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[e-mail]>
> 
> Commit d352cf47d93e (cpufreq: conservative: Do not use transition
> notifications) overlooked the case when the "frequency step" used
> by the conservative governor is small relative to the distances
> between the available frequencies and broke the algorithm by
> using policy->cur instead of the previously requested frequency
> when computing the next one.
> 
> As a result, the governor may not be able to go outside of a narrow
> range between two consecutive available frequencies.
> 
> Fix the problem by making the governor save the previously requested
> frequency and select the next one relative that value (unless it is
> out of range, in which case policy->cur will be used instead).
> 
> Fixes: d352cf47d93e (cpufreq: conservative: Do not use transition notifications)
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=177171
> Reported-and-tested-by: Aleksey Rybalkin <[e-mail]>
> Signed-off-by: Rafael J. Wysocki <[e-mail]>
> ---
>  drivers/cpufreq/cpufreq_conservative.c |   19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

Acked-by: Viresh Kumar <[e-mail]>

Patch

Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -17,6 +17,7 @@ 
 struct cs_policy_dbs_info {
 	struct policy_dbs_info policy_dbs;
 	unsigned int down_skip;
+	unsigned int requested_freq;
 };
 
 static inline struct cs_policy_dbs_info *to_dbs_info(struct policy_dbs_info *policy_dbs)
@@ -61,6 +62,7 @@  static unsigned int cs_dbs_timer(struct
 {
 	struct policy_dbs_info *policy_dbs = policy->governor_data;
 	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy_dbs);
+	unsigned int requested_freq = dbs_info->requested_freq;
 	struct dbs_data *dbs_data = policy_dbs->dbs_data;
 	struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
 	unsigned int load = dbs_update(policy);
@@ -72,10 +74,16 @@  static unsigned int cs_dbs_timer(struct
 	if (cs_tuners->freq_step == 0)
 		goto out;
 
+	/*
+	 * If requested_freq is out of range, it is likely that the limits
+	 * changed in the meantime, so fall back to current frequency in that
+	 * case.
+	 */
+	if (requested_freq > policy->max || requested_freq < policy->min)
+		requested_freq = policy->cur;
+
 	/* Check for frequency increase */
 	if (load > dbs_data->up_threshold) {
-		unsigned int requested_freq = policy->cur;
-
 		dbs_info->down_skip = 0;
 
 		/* if we are already at full speed then break out early */
@@ -83,8 +91,11 @@  static unsigned int cs_dbs_timer(struct
 			goto out;
 
 		requested_freq += get_freq_target(cs_tuners, policy);
+		if (requested_freq > policy->max)
+			requested_freq = policy->max;
 
 		__cpufreq_driver_target(policy, requested_freq, CPUFREQ_RELATION_H);
+		dbs_info->requested_freq = requested_freq;
 		goto out;
 	}
 
@@ -95,7 +106,7 @@  static unsigned int cs_dbs_timer(struct
 
 	/* Check for frequency decrease */
 	if (load < cs_tuners->down_threshold) {
-		unsigned int freq_target, requested_freq = policy->cur;
+		unsigned int freq_target;
 		/*
 		 * if we cannot reduce the frequency anymore, break out early
 		 */
@@ -109,6 +120,7 @@  static unsigned int cs_dbs_timer(struct
 			requested_freq = policy->min;
 
 		__cpufreq_driver_target(policy, requested_freq, CPUFREQ_RELATION_L);
+		dbs_info->requested_freq = requested_freq;
 	}
 
  out:
@@ -287,6 +299,7 @@  static void cs_start(struct cpufreq_poli
 	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy->governor_data);
 
 	dbs_info->down_skip = 0;
+	dbs_info->requested_freq = policy->cur;
 }
 
 static struct dbs_governor cs_governor = {