Userspace governor has got more code than what it needs for its functioning.
Lets simplify it.
Over that it will fix issues in cpufreq_governor_userspace(), which isn't doing
right things in START/STOP. It is working per-cpu currently whereas it just
required to manage policy->cpu.
Reported-by: Xiaoguang Chen <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
@Rafael:
I don't know why this code was initially added. Please let me know if I am doing
something stupid.
Also, please apply it as a fix for 3.10 as it is broken recently in 3.9.
drivers/cpufreq/cpufreq_userspace.c | 108 ++++--------------------------------
1 file changed, 12 insertions(+), 96 deletions(-)
diff --git a/drivers/cpufreq/cpufreq_userspace.c b/drivers/cpufreq/cpufreq_userspace.c
index bbeb9c0..5dc77b7 100644
--- a/drivers/cpufreq/cpufreq_userspace.c
+++ b/drivers/cpufreq/cpufreq_userspace.c
@@ -13,55 +13,13 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/smp.h>
-#include <linux/init.h>
-#include <linux/spinlock.h>
-#include <linux/interrupt.h>
#include <linux/cpufreq.h>
-#include <linux/cpu.h>
-#include <linux/types.h>
-#include <linux/fs.h>
-#include <linux/sysfs.h>
+#include <linux/init.h>
+#include <linux/module.h>
#include <linux/mutex.h>
-/**
- * A few values needed by the userspace governor
- */
-static DEFINE_PER_CPU(unsigned int, cpu_max_freq);
-static DEFINE_PER_CPU(unsigned int, cpu_min_freq);
-static DEFINE_PER_CPU(unsigned int, cpu_cur_freq); /* current CPU freq */
-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);
-static int cpus_using_userspace_governor;
-
-/* keep track of frequency transitions */
-static int
-userspace_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
- void *data)
-{
- struct cpufreq_freqs *freq = data;
-
- if (!per_cpu(cpu_is_managed, freq->cpu))
- return 0;
-
- if (val == CPUFREQ_POSTCHANGE) {
- pr_debug("saving cpu_cur_freq of cpu %u to be %u kHz\n",
- freq->cpu, freq->new);
- per_cpu(cpu_cur_freq, freq->cpu) = freq->new;
- }
-
- return 0;
-}
-
-static struct notifier_block userspace_cpufreq_notifier_block = {
- .notifier_call = userspace_cpufreq_notifier
-};
-
/**
* cpufreq_set - set the CPU frequency
@@ -80,13 +38,6 @@ 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;
-
- if (freq < per_cpu(cpu_min_freq, policy->cpu))
- freq = per_cpu(cpu_min_freq, policy->cpu);
- if (freq > per_cpu(cpu_max_freq, policy->cpu))
- freq = per_cpu(cpu_max_freq, policy->cpu);
-
/*
* We're safe from concurrent calls to ->target() here
* as we hold the userspace_mutex lock. If we were calling
@@ -107,7 +58,7 @@ static int cpufreq_set(struct cpufreq_policy *policy, unsigned int freq)
static ssize_t show_speed(struct cpufreq_policy *policy, char *buf)
{
- return sprintf(buf, "%u\n", per_cpu(cpu_cur_freq, policy->cpu));
+ return sprintf(buf, "%u\n", policy->cur);
}
static int cpufreq_governor_userspace(struct cpufreq_policy *policy,
@@ -119,66 +70,31 @@ static int cpufreq_governor_userspace(struct cpufreq_policy *policy,
switch (event) {
case CPUFREQ_GOV_START:
BUG_ON(!policy->cur);
- mutex_lock(&userspace_mutex);
-
- if (cpus_using_userspace_governor == 0) {
- cpufreq_register_notifier(
- &userspace_cpufreq_notifier_block,
- CPUFREQ_TRANSITION_NOTIFIER);
- }
- cpus_using_userspace_governor++;
+ pr_debug("started managing cpu %u\n", cpu);
+ mutex_lock(&userspace_mutex);
per_cpu(cpu_is_managed, cpu) = 1;
- per_cpu(cpu_min_freq, cpu) = policy->min;
- per_cpu(cpu_max_freq, cpu) = policy->max;
- per_cpu(cpu_cur_freq, cpu) = policy->cur;
- per_cpu(cpu_set_freq, cpu) = policy->cur;
- pr_debug("managing cpu %u started "
- "(%u - %u kHz, currently %u kHz)\n",
- cpu,
- per_cpu(cpu_min_freq, cpu),
- per_cpu(cpu_max_freq, cpu),
- per_cpu(cpu_cur_freq, cpu));
-
mutex_unlock(&userspace_mutex);
break;
case CPUFREQ_GOV_STOP:
- mutex_lock(&userspace_mutex);
- cpus_using_userspace_governor--;
- if (cpus_using_userspace_governor == 0) {
- cpufreq_unregister_notifier(
- &userspace_cpufreq_notifier_block,
- CPUFREQ_TRANSITION_NOTIFIER);
- }
+ pr_debug("managing cpu %u stopped\n", cpu);
+ mutex_lock(&userspace_mutex);
per_cpu(cpu_is_managed, cpu) = 0;
- per_cpu(cpu_min_freq, cpu) = 0;
- per_cpu(cpu_max_freq, cpu) = 0;
- per_cpu(cpu_set_freq, cpu) = 0;
- pr_debug("managing cpu %u stopped\n", cpu);
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, last set to %u kHz\n",
+ pr_debug("limit event for cpu %u: %u - %u kHz, currently %u kHz\n",
cpu, policy->min, policy->max,
- per_cpu(cpu_cur_freq, cpu),
- per_cpu(cpu_set_freq, cpu));
- if (policy->max < per_cpu(cpu_set_freq, cpu)) {
+ policy->cur);
+
+ if (policy->max < policy->cur)
__cpufreq_driver_target(policy, policy->max,
CPUFREQ_RELATION_H);
- } else if (policy->min > per_cpu(cpu_set_freq, cpu)) {
+ else if (policy->min > policy->cur)
__cpufreq_driver_target(policy, policy->min,
CPUFREQ_RELATION_L);
- } else {
- __cpufreq_driver_target(policy,
- per_cpu(cpu_set_freq, cpu),
- CPUFREQ_RELATION_L);
- }
- per_cpu(cpu_min_freq, cpu) = policy->min;
- per_cpu(cpu_max_freq, cpu) = policy->max;
- per_cpu(cpu_cur_freq, cpu) = policy->cur;
mutex_unlock(&userspace_mutex);
break;
}
--
1.7.12.rc2.18.g61b472e
On Wednesday, June 05, 2013 02:34:33 PM Viresh Kumar wrote:
> Userspace governor has got more code than what it needs for its functioning.
> Lets simplify it.
OK
Why exactly is the code you're removing unnecessary?
> Over that it will fix issues in cpufreq_governor_userspace(), which isn't doing
> right things in START/STOP.
What exactly is the problem?
> It is working per-cpu currently whereas it just
> required to manage policy->cpu.
>
> Reported-by: Xiaoguang Chen <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> @Rafael:
>
> I don't know why this code was initially added. Please let me know if I am doing
> something stupid.
>
> Also, please apply it as a fix for 3.10 as it is broken recently in 3.9.
I'd love to, but I need answers to the above questions before I do that.
Thanks,
Rafael
> drivers/cpufreq/cpufreq_userspace.c | 108 ++++--------------------------------
> 1 file changed, 12 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_userspace.c b/drivers/cpufreq/cpufreq_userspace.c
> index bbeb9c0..5dc77b7 100644
> --- a/drivers/cpufreq/cpufreq_userspace.c
> +++ b/drivers/cpufreq/cpufreq_userspace.c
> @@ -13,55 +13,13 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> -#include <linux/kernel.h>
> -#include <linux/module.h>
> -#include <linux/smp.h>
> -#include <linux/init.h>
> -#include <linux/spinlock.h>
> -#include <linux/interrupt.h>
> #include <linux/cpufreq.h>
> -#include <linux/cpu.h>
> -#include <linux/types.h>
> -#include <linux/fs.h>
> -#include <linux/sysfs.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> #include <linux/mutex.h>
>
> -/**
> - * A few values needed by the userspace governor
> - */
> -static DEFINE_PER_CPU(unsigned int, cpu_max_freq);
> -static DEFINE_PER_CPU(unsigned int, cpu_min_freq);
> -static DEFINE_PER_CPU(unsigned int, cpu_cur_freq); /* current CPU freq */
> -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);
> -static int cpus_using_userspace_governor;
> -
> -/* keep track of frequency transitions */
> -static int
> -userspace_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
> - void *data)
> -{
> - struct cpufreq_freqs *freq = data;
> -
> - if (!per_cpu(cpu_is_managed, freq->cpu))
> - return 0;
> -
> - if (val == CPUFREQ_POSTCHANGE) {
> - pr_debug("saving cpu_cur_freq of cpu %u to be %u kHz\n",
> - freq->cpu, freq->new);
> - per_cpu(cpu_cur_freq, freq->cpu) = freq->new;
> - }
> -
> - return 0;
> -}
> -
> -static struct notifier_block userspace_cpufreq_notifier_block = {
> - .notifier_call = userspace_cpufreq_notifier
> -};
> -
>
> /**
> * cpufreq_set - set the CPU frequency
> @@ -80,13 +38,6 @@ 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;
> -
> - if (freq < per_cpu(cpu_min_freq, policy->cpu))
> - freq = per_cpu(cpu_min_freq, policy->cpu);
> - if (freq > per_cpu(cpu_max_freq, policy->cpu))
> - freq = per_cpu(cpu_max_freq, policy->cpu);
> -
> /*
> * We're safe from concurrent calls to ->target() here
> * as we hold the userspace_mutex lock. If we were calling
> @@ -107,7 +58,7 @@ static int cpufreq_set(struct cpufreq_policy *policy, unsigned int freq)
>
> static ssize_t show_speed(struct cpufreq_policy *policy, char *buf)
> {
> - return sprintf(buf, "%u\n", per_cpu(cpu_cur_freq, policy->cpu));
> + return sprintf(buf, "%u\n", policy->cur);
> }
>
> static int cpufreq_governor_userspace(struct cpufreq_policy *policy,
> @@ -119,66 +70,31 @@ static int cpufreq_governor_userspace(struct cpufreq_policy *policy,
> switch (event) {
> case CPUFREQ_GOV_START:
> BUG_ON(!policy->cur);
> - mutex_lock(&userspace_mutex);
> -
> - if (cpus_using_userspace_governor == 0) {
> - cpufreq_register_notifier(
> - &userspace_cpufreq_notifier_block,
> - CPUFREQ_TRANSITION_NOTIFIER);
> - }
> - cpus_using_userspace_governor++;
> + pr_debug("started managing cpu %u\n", cpu);
>
> + mutex_lock(&userspace_mutex);
> per_cpu(cpu_is_managed, cpu) = 1;
> - per_cpu(cpu_min_freq, cpu) = policy->min;
> - per_cpu(cpu_max_freq, cpu) = policy->max;
> - per_cpu(cpu_cur_freq, cpu) = policy->cur;
> - per_cpu(cpu_set_freq, cpu) = policy->cur;
> - pr_debug("managing cpu %u started "
> - "(%u - %u kHz, currently %u kHz)\n",
> - cpu,
> - per_cpu(cpu_min_freq, cpu),
> - per_cpu(cpu_max_freq, cpu),
> - per_cpu(cpu_cur_freq, cpu));
> -
> mutex_unlock(&userspace_mutex);
> break;
> case CPUFREQ_GOV_STOP:
> - mutex_lock(&userspace_mutex);
> - cpus_using_userspace_governor--;
> - if (cpus_using_userspace_governor == 0) {
> - cpufreq_unregister_notifier(
> - &userspace_cpufreq_notifier_block,
> - CPUFREQ_TRANSITION_NOTIFIER);
> - }
> + pr_debug("managing cpu %u stopped\n", cpu);
>
> + mutex_lock(&userspace_mutex);
> per_cpu(cpu_is_managed, cpu) = 0;
> - per_cpu(cpu_min_freq, cpu) = 0;
> - per_cpu(cpu_max_freq, cpu) = 0;
> - per_cpu(cpu_set_freq, cpu) = 0;
> - pr_debug("managing cpu %u stopped\n", cpu);
> 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, last set to %u kHz\n",
> + pr_debug("limit event for cpu %u: %u - %u kHz, currently %u kHz\n",
> cpu, policy->min, policy->max,
> - per_cpu(cpu_cur_freq, cpu),
> - per_cpu(cpu_set_freq, cpu));
> - if (policy->max < per_cpu(cpu_set_freq, cpu)) {
> + policy->cur);
> +
> + if (policy->max < policy->cur)
> __cpufreq_driver_target(policy, policy->max,
> CPUFREQ_RELATION_H);
> - } else if (policy->min > per_cpu(cpu_set_freq, cpu)) {
> + else if (policy->min > policy->cur)
> __cpufreq_driver_target(policy, policy->min,
> CPUFREQ_RELATION_L);
> - } else {
> - __cpufreq_driver_target(policy,
> - per_cpu(cpu_set_freq, cpu),
> - CPUFREQ_RELATION_L);
> - }
> - per_cpu(cpu_min_freq, cpu) = policy->min;
> - per_cpu(cpu_max_freq, cpu) = policy->max;
> - per_cpu(cpu_cur_freq, cpu) = policy->cur;
> mutex_unlock(&userspace_mutex);
> break;
> }
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 5 June 2013 18:07, Rafael J. Wysocki <[email protected]> wrote:
> On Wednesday, June 05, 2013 02:34:33 PM Viresh Kumar wrote:
>> Userspace governor has got more code than what it needs for its functioning.
>> Lets simplify it.
>
> OK
>
> Why exactly is the code you're removing unnecessary?
>
>> Over that it will fix issues in cpufreq_governor_userspace(), which isn't doing
>> right things in START/STOP.
>
> What exactly is the problem?
I sent a reply now to the problem reported by Xiaoguang as I don't feel
now there is a problem :(
>> It is working per-cpu currently whereas it just
>> required to manage policy->cpu.
>>
>> Reported-by: Xiaoguang Chen <[email protected]>
>> Signed-off-by: Viresh Kumar <[email protected]>
>> ---
>> @Rafael:
>>
>> I don't know why this code was initially added. Please let me know if I am doing
>> something stupid.
>>
>> Also, please apply it as a fix for 3.10 as it is broken recently in 3.9.
>
> I'd love to, but I need answers to the above questions before I do that.
To be honest with the amount of experience I have now, my log was
poor :(
I have used following log in the attached patch:
Subject: [PATCH] cpufreq: userspace: Simplify governor
Userspace governor has got more code than what it needs for its functioning.
Lets simplify it. Portions of code removed are:
- Extra header files which aren't required anymore (rearranged them as well)
- cpu_{max|min|cur|set}_freq, as they are always same as policy->{max|min|cur}
- userspace_cpufreq_notifier_block as we don't need to set cpu_cur_freq anymore
- cpus_using_userspace_governor as it was for notifier code
Signed-off-by: Viresh Kumar <[email protected]>
2013/6/5 Viresh Kumar <[email protected]>:
> On 5 June 2013 18:07, Rafael J. Wysocki <[email protected]> wrote:
>> On Wednesday, June 05, 2013 02:34:33 PM Viresh Kumar wrote:
>>> Userspace governor has got more code than what it needs for its functioning.
>>> Lets simplify it.
>>
>> OK
>>
>> Why exactly is the code you're removing unnecessary?
>>
>>> Over that it will fix issues in cpufreq_governor_userspace(), which isn't doing
>>> right things in START/STOP.
>>
>> What exactly is the problem?
>
> I sent a reply now to the problem reported by Xiaoguang as I don't feel
> now there is a problem :(
>
Yes, I think the problem will disappear since the related code is
deleted. the original code path will not be executed.
>>> It is working per-cpu currently whereas it just
>>> required to manage policy->cpu.
>>>
>>> Reported-by: Xiaoguang Chen <[email protected]>
>>> Signed-off-by: Viresh Kumar <[email protected]>
>>> ---
>>> @Rafael:
>>>
>>> I don't know why this code was initially added. Please let me know if I am doing
>>> something stupid.
>>>
>>> Also, please apply it as a fix for 3.10 as it is broken recently in 3.9.
>>
>> I'd love to, but I need answers to the above questions before I do that.
>
> To be honest with the amount of experience I have now, my log was
> poor :(
>
> I have used following log in the attached patch:
>
> Subject: [PATCH] cpufreq: userspace: Simplify governor
>
> Userspace governor has got more code than what it needs for its functioning.
> Lets simplify it. Portions of code removed are:
> - Extra header files which aren't required anymore (rearranged them as well)
> - cpu_{max|min|cur|set}_freq, as they are always same as policy->{max|min|cur}
> - userspace_cpufreq_notifier_block as we don't need to set cpu_cur_freq anymore
> - cpus_using_userspace_governor as it was for notifier code
>
> Signed-off-by: Viresh Kumar <[email protected]>
On 5 June 2013 18:38, Viresh Kumar <[email protected]> wrote:
> To be honest with the amount of experience I have now, my log was
> poor :(
>
> I have used following log in the attached patch:
>
> Subject: [PATCH] cpufreq: userspace: Simplify governor
>
> Userspace governor has got more code than what it needs for its functioning.
> Lets simplify it. Portions of code removed are:
> - Extra header files which aren't required anymore (rearranged them as well)
> - cpu_{max|min|cur|set}_freq, as they are always same as policy->{max|min|cur}
> - userspace_cpufreq_notifier_block as we don't need to set cpu_cur_freq anymore
> - cpus_using_userspace_governor as it was for notifier code
>
> Signed-off-by: Viresh Kumar <[email protected]>
Hi Rafael,
Are you happy with the log & description of this patch now?
On Tuesday, June 18, 2013 10:20:33 AM Viresh Kumar wrote:
> On 5 June 2013 18:38, Viresh Kumar <[email protected]> wrote:
> > To be honest with the amount of experience I have now, my log was
> > poor :(
> >
> > I have used following log in the attached patch:
> >
> > Subject: [PATCH] cpufreq: userspace: Simplify governor
> >
> > Userspace governor has got more code than what it needs for its functioning.
> > Lets simplify it. Portions of code removed are:
> > - Extra header files which aren't required anymore (rearranged them as well)
> > - cpu_{max|min|cur|set}_freq, as they are always same as policy->{max|min|cur}
> > - userspace_cpufreq_notifier_block as we don't need to set cpu_cur_freq anymore
> > - cpus_using_userspace_governor as it was for notifier code
> >
> > Signed-off-by: Viresh Kumar <[email protected]>
>
> Hi Rafael,
>
> Are you happy with the log & description of this patch now?
Yes, I am. I'm hoping to get to the pending patches later today.
Thanks,
Rafael