[RFC/PATCH] cpufreq: Fix GOV_LIMITS handling for the userspace governor

[RFC/PATCH] cpufreq: Fix GOV_LIMITS handling for the userspace governor

[Sai Gurrappadi]

Currently, the userspace governor only updates frequency on GOV_LIMITS
if policy->cur falls outside policy->{min/max}. However, it is also
necessary to update current frequency on GOV_LIMITS to match the user
requested value if it can be achieved within the new policy->{max/min}.

This was previously the behaviour in the governor until commit d1922f0
("cpufreq: Simplify userspace governor") which incorrectly assumed that
policy->cur == user requested frequency via scaling_setspeed. This won't
be true if the user requested frequency falls outside policy->{min/max}.
Ex: a temporary thermal cap throttled the user requested frequency.

Fix this by doing a partial revert of commit d1922f0 to bring back the
per-cpu cpu_set_freq variable that stores the user requested frequency.
The governor will then try to achieve this request on every GOV_LIMITS.

Signed-off-by: Sai Gurrappadi <sgurrappadi@nvidia.com>
---
 drivers/cpufreq/cpufreq_userspace.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_userspace.c
b/drivers/cpufreq/cpufreq_userspace.c
index 4d16f45..ae03ba7 100644
--- a/drivers/cpufreq/cpufreq_userspace.c
+++ b/drivers/cpufreq/cpufreq_userspace.c
@@ -18,6 +18,8 @@
 #include <linux/module.h>
 #include <linux/mutex.h>

+static DEFINE_PER_CPU(unsigned int, cpu_set_freq); /* CPU freq desired by
+							userspace */
 static DEFINE_PER_CPU(unsigned int, cpu_is_managed);
 static DEFINE_MUTEX(userspace_mutex);

@@ -38,6 +40,8 @@ static int cpufreq_set(struct cpufreq_policy *policy,
unsigned int freq)
 	if (!per_cpu(cpu_is_managed, policy->cpu))
 		goto err;

+	per_cpu(cpu_set_freq, policy->cpu) = freq;
+
 	ret = __cpufreq_driver_target(policy, freq, CPUFREQ_RELATION_L);
  err:
 	mutex_unlock(&userspace_mutex);
@@ -62,6 +66,7 @@ static int cpufreq_governor_userspace(struct cpufreq_policy
*policy,

 		mutex_lock(&userspace_mutex);
 		per_cpu(cpu_is_managed, cpu) = 1;
+		per_cpu(cpu_set_freq, cpu) = policy->cur;
 		mutex_unlock(&userspace_mutex);
 		break;
 	case CPUFREQ_GOV_STOP:
@@ -69,20 +74,27 @@ static int cpufreq_governor_userspace(struct
cpufreq_policy *policy,

 		mutex_lock(&userspace_mutex);
 		per_cpu(cpu_is_managed, cpu) = 0;
+		per_cpu(cpu_set_freq, cpu) = 0;
 		mutex_unlock(&userspace_mutex);
 		break;
 	case CPUFREQ_GOV_LIMITS:
 		mutex_lock(&userspace_mutex);
-		pr_debug("limit event for cpu %u: %u - %u kHz, currently %u kHz\n",
+		pr_debug("limit event for cpu %u: %u - %u kHz, "
+			"currently %u kHz, last set to %u kHz\n",
 			cpu, policy->min, policy->max,
-			policy->cur);
+			policy->cur, per_cpu(cpu_set_freq, cpu));

-		if (policy->max < policy->cur)
+		if (policy->max < per_cpu(cpu_set_freq, cpu)) {
 			__cpufreq_driver_target(policy, policy->max,
 						CPUFREQ_RELATION_H);
-		else if (policy->min > policy->cur)
+		} else if (policy->min > per_cpu(cpu_set_freq, cpu)) {
 			__cpufreq_driver_target(policy, policy->min,
 						CPUFREQ_RELATION_L);
+		} else {
+			__cpufreq_driver_target(policy,
+						per_cpu(cpu_set_freq, cpu),
+						CPUFREQ_RELATION_L);
+		}
 		mutex_unlock(&userspace_mutex);
 		break;
 	}
-- 
2.1.4

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [RFC/PATCH] cpufreq: Fix GOV_LIMITS handling for the userspace governor

[Viresh Kumar]

On 28-04-16, 11:00, Sai Gurrappadi wrote:
> Currently, the userspace governor only updates frequency on GOV_LIMITS
> if policy->cur falls outside policy->{min/max}. However, it is also
> necessary to update current frequency on GOV_LIMITS to match the user
> requested value if it can be achieved within the new policy->{max/min}.
> 
> This was previously the behaviour in the governor until commit d1922f0
> ("cpufreq: Simplify userspace governor") which incorrectly assumed that
> policy->cur == user requested frequency via scaling_setspeed. This won't

Urg...

> be true if the user requested frequency falls outside policy->{min/max}.
> Ex: a temporary thermal cap throttled the user requested frequency.
> 
> Fix this by doing a partial revert of commit d1922f0 to bring back the
> per-cpu cpu_set_freq variable that stores the user requested frequency.
> The governor will then try to achieve this request on every GOV_LIMITS.
> 

Add this here (just before your sob)

Fixes: d1922f02562f ("cpufreq: Simplify userspace governor")

> Signed-off-by: Sai Gurrappadi <sgurrappadi@nvidia.com>
> ---
>  drivers/cpufreq/cpufreq_userspace.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_userspace.c
> b/drivers/cpufreq/cpufreq_userspace.c
> index 4d16f45..ae03ba7 100644
> --- a/drivers/cpufreq/cpufreq_userspace.c
> +++ b/drivers/cpufreq/cpufreq_userspace.c
> @@ -18,6 +18,8 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> 
> +static DEFINE_PER_CPU(unsigned int, cpu_set_freq); /* CPU freq desired by
> +							userspace */

We don't have to be this bad now. The policy structure has a governor_data
field, please use that for storing any policy specific values.

>  static DEFINE_PER_CPU(unsigned int, cpu_is_managed);
>  static DEFINE_MUTEX(userspace_mutex);
> 
> @@ -38,6 +40,8 @@ static int cpufreq_set(struct cpufreq_policy *policy,
> unsigned int freq)
>  	if (!per_cpu(cpu_is_managed, policy->cpu))
>  		goto err;
> 
> +	per_cpu(cpu_set_freq, policy->cpu) = freq;
> +
>  	ret = __cpufreq_driver_target(policy, freq, CPUFREQ_RELATION_L);
>   err:
>  	mutex_unlock(&userspace_mutex);
> @@ -62,6 +66,7 @@ static int cpufreq_governor_userspace(struct cpufreq_policy
> *policy,
> 
>  		mutex_lock(&userspace_mutex);
>  		per_cpu(cpu_is_managed, cpu) = 1;
> +		per_cpu(cpu_set_freq, cpu) = policy->cur;
>  		mutex_unlock(&userspace_mutex);
>  		break;
>  	case CPUFREQ_GOV_STOP:
> @@ -69,20 +74,27 @@ static int cpufreq_governor_userspace(struct
> cpufreq_policy *policy,
> 
>  		mutex_lock(&userspace_mutex);
>  		per_cpu(cpu_is_managed, cpu) = 0;
> +		per_cpu(cpu_set_freq, cpu) = 0;
>  		mutex_unlock(&userspace_mutex);
>  		break;
>  	case CPUFREQ_GOV_LIMITS:
>  		mutex_lock(&userspace_mutex);
> -		pr_debug("limit event for cpu %u: %u - %u kHz, currently %u kHz\n",
> +		pr_debug("limit event for cpu %u: %u - %u kHz, "
> +			"currently %u kHz, last set to %u kHz\n",

*Never* break print messages into multiple lines, let them cross 80 columns.

>  			cpu, policy->min, policy->max,
> -			policy->cur);
> +			policy->cur, per_cpu(cpu_set_freq, cpu));
> 
> -		if (policy->max < policy->cur)
> +		if (policy->max < per_cpu(cpu_set_freq, cpu)) {
>  			__cpufreq_driver_target(policy, policy->max,
>  						CPUFREQ_RELATION_H);
> -		else if (policy->min > policy->cur)
> +		} else if (policy->min > per_cpu(cpu_set_freq, cpu)) {
>  			__cpufreq_driver_target(policy, policy->min,
>  						CPUFREQ_RELATION_L);
> +		} else {
> +			__cpufreq_driver_target(policy,
> +						per_cpu(cpu_set_freq, cpu),
> +						CPUFREQ_RELATION_L);
> +		}
>  		mutex_unlock(&userspace_mutex);
>  		break;
>  	}

Good catch :)

-- 
viresh