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

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

[Sai Gurrappadi]

Resending without the copyright headed appended..sorry!

-Sai

=======

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

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

[Viresh Kumar]

On 28-04-16, 11:06, Sai Gurrappadi wrote:
> Resending without the copyright headed appended..sorry!
> 
> -Sai
> 
> =======

:)

For your information, this isn't the right place to add text which shouldn't be
committed.

> 
> 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>
> ---

Rather do it here and git am will automatically drop that while applying the
patch.

>  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

-- 
viresh

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

[Sai Gurrappadi]

On 04/28/2016 08:49 PM, Viresh Kumar wrote:
> On 28-04-16, 11:06, Sai Gurrappadi wrote:
>> Resending without the copyright headed appended..sorry!
>>
>> -Sai
>>
>> =======
> 
> :)
> 
> For your information, this isn't the right place to add text which shouldn't be
> committed.

[..]

>>
>> Signed-off-by: Sai Gurrappadi <sgurrappadi@nvidia.com>
>> ---
> 
> Rather do it here and git am will automatically drop that while applying the
> patch.

Good to know. Thanks!

-Sai

> 
>>  drivers/cpufreq/cpufreq_userspace.c | 20 ++++++++++++++++----