On Wed, 2013-08-07 at 18:52 +0530, Viresh Kumar wrote:
While getting support for In-Kernel-Switcher in big LITTLE cpufreq driver we introduced cpu_last_req_freq per-cpu variable that stores the last frequency requested for a cpu. It was important for IKS as CPUs in the same cluster can have separate struct cpufreq_policy associated with them and so cpufreq core may try to set different frequencies on both CPUs of same cluster.
But for non-IKS solution or MP we don't need to cache last requested frequency. Currently there is a bug in code where if cpufreq_driver->get() is called for any cpu other than policy->cpu, we are returning 0, because we set cpu_last_req_freq only for policy->cpu and not for others.
Do you know what the observable symptoms of this are?
As this is a fix which needs to get into LSK I've added the linaro-kernel list to the cc so it has more public visibility.
Tixy
This problem could have been fixed by setting cpu_last_req_freq for all CPUs in policy->cpus, but that wouldn't be the best solution. Purpose of cpufreq_driver->get() is to get exact frequency from the hardware instead of returning cached frequency that was last requested as that is already present with the core.
Hence, this patch forces only IKS to use cpu_last_req_freq and not MP. We will get the frequency from hardware when ->get() is called for MP.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/arm_big_little.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c index f589b6c..c8de71f 100644 --- a/drivers/cpufreq/arm_big_little.c +++ b/drivers/cpufreq/arm_big_little.c @@ -91,9 +91,14 @@ static unsigned int clk_get_cpu_rate(unsigned int cpu) static unsigned int bL_cpufreq_get_rate(unsigned int cpu) {
- pr_debug("%s: freq: %d\n", __func__, per_cpu(cpu_last_req_freq, cpu));
- if (is_bL_switching_enabled()) {
pr_debug("%s: freq: %d\n", __func__, per_cpu(cpu_last_req_freq,
cpu));
- return per_cpu(cpu_last_req_freq, cpu);
return per_cpu(cpu_last_req_freq, cpu);
- } else {
return clk_get_cpu_rate(cpu);
- }
} static unsigned int @@ -104,11 +109,11 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate) mutex_lock(&cluster_lock[new_cluster]);
- prev_rate = per_cpu(cpu_last_req_freq, cpu);
- per_cpu(cpu_last_req_freq, cpu) = rate;
- per_cpu(physical_cluster, cpu) = new_cluster;
- if (is_bL_switching_enabled()) {
prev_rate = per_cpu(cpu_last_req_freq, cpu);
per_cpu(cpu_last_req_freq, cpu) = rate;
per_cpu(physical_cluster, cpu) = new_cluster;
- new_rate = find_cluster_maxfreq(new_cluster); new_rate = ACTUAL_FREQ(new_cluster, new_rate); } else {
@@ -122,8 +127,10 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate) if (WARN_ON(ret)) { pr_err("clk_set_rate failed: %d, new cluster: %d\n", ret, new_cluster);
per_cpu(cpu_last_req_freq, cpu) = prev_rate;
per_cpu(physical_cluster, cpu) = old_cluster;
if (is_bL_switching_enabled()) {
per_cpu(cpu_last_req_freq, cpu) = prev_rate;
per_cpu(physical_cluster, cpu) = old_cluster;
}
mutex_unlock(&cluster_lock[new_cluster]); @@ -461,7 +468,9 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy) policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL; policy->cur = clk_get_cpu_rate(policy->cpu);
- per_cpu(cpu_last_req_freq, policy->cpu) = policy->cur;
- if (is_bL_switching_enabled())
per_cpu(cpu_last_req_freq, policy->cpu) = policy->cur;
dev_info(cpu_dev, "%s: CPU %d initialized\n", __func__, policy->cpu); return 0;
On 8 August 2013 13:09, Jon Medhurst (Tixy) tixy@linaro.org wrote:
On Wed, 2013-08-07 at 18:52 +0530, Viresh Kumar wrote:
While getting support for In-Kernel-Switcher in big LITTLE cpufreq driver we introduced cpu_last_req_freq per-cpu variable that stores the last frequency requested for a cpu. It was important for IKS as CPUs in the same cluster can have separate struct cpufreq_policy associated with them and so cpufreq core may try to set different frequencies on both CPUs of same cluster.
But for non-IKS solution or MP we don't need to cache last requested frequency. Currently there is a bug in code where if cpufreq_driver->get() is called for any cpu other than policy->cpu, we are returning 0, because we set cpu_last_req_freq only for policy->cpu and not for others.
Do you know what the observable symptoms of this are?
Ok.. I skipped the history behind this commit earlier.. Lets have it now:
I was playing around with cpufreq stuff some time back, on tc2 using ondemand governor. I got stuck on a bug for sometime where taking cpu4 down and bringing it back crashed my system..
Then I spent some time trying to understand what's going wrong and found above to be the culprit.
Sequence of events is like this: - offline cpu4 (2nd A15, 1st is still there) and online it back. -> cpufreq.c: cpufreq_cpu_callback(CPU_ONLINE) -> cpufreq_update_policy(4) -> cpufreq_driver->get(4) and zero is returned due to mentioned issue -> cpufreq_out_of_sync() is called and it sets policy->cur = 0..
-> cpufreq_governor.c: cpufreq_governor_dbs(CPUFREQ_GOV_START) -> It checks below and returns error if (!policy->cur) return -EINVAL;
-> Caller of cpufreq_governor_dbs(CPUFREQ_GOV_START) wasn't checking return values of this function (Though I fixed it now in a separate thread).. and called cpufreq_governor_dbs(CPUFREQ_GOV_LIMITS) -> cpufreq_governor_dbs(CPUFREQ_GOV_LIMITS) tried to access: cpu_cdbs->cur_policy->cur and cpu_cdbs->cur_policy was set to NULL earlier when CPUFREQ_GOV_STOP was called while removing cpu.
And so the crash.
As this is a fix which needs to get into LSK I've added the linaro-kernel list to the cc so it has more public visibility.
Thanks. Adding Broonie directly as well so that he doesn't miss it :)
On Thu, 2013-08-08 at 08:39 +0100, Jon Medhurst (Tixy) wrote:
On Wed, 2013-08-07 at 18:52 +0530, Viresh Kumar wrote:
While getting support for In-Kernel-Switcher in big LITTLE cpufreq driver we introduced cpu_last_req_freq per-cpu variable that stores the last frequency requested for a cpu. It was important for IKS as CPUs in the same cluster can have separate struct cpufreq_policy associated with them and so cpufreq core may try to set different frequencies on both CPUs of same cluster.
But for non-IKS solution or MP we don't need to cache last requested frequency. Currently there is a bug in code where if cpufreq_driver->get() is called for any cpu other than policy->cpu, we are returning 0, because we set cpu_last_req_freq only for policy->cpu and not for others.
Do you know what the observable symptoms of this are?
As this is a fix which needs to get into LSK I've added the linaro-kernel list to the cc so it has more public visibility.
Tixy
This problem could have been fixed by setting cpu_last_req_freq for all CPUs in policy->cpus, but that wouldn't be the best solution. Purpose of cpufreq_driver->get() is to get exact frequency from the hardware instead of returning cached frequency that was last requested as that is already present with the core.
Hence, this patch forces only IKS to use cpu_last_req_freq and not MP. We will get the frequency from hardware when ->get() is called for MP.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/arm_big_little.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c index f589b6c..c8de71f 100644 --- a/drivers/cpufreq/arm_big_little.c +++ b/drivers/cpufreq/arm_big_little.c @@ -91,9 +91,14 @@ static unsigned int clk_get_cpu_rate(unsigned int cpu) static unsigned int bL_cpufreq_get_rate(unsigned int cpu) {
- pr_debug("%s: freq: %d\n", __func__, per_cpu(cpu_last_req_freq, cpu));
- if (is_bL_switching_enabled()) {
pr_debug("%s: freq: %d\n", __func__, per_cpu(cpu_last_req_freq,
cpu));
- return per_cpu(cpu_last_req_freq, cpu);
return per_cpu(cpu_last_req_freq, cpu);
- } else {
return clk_get_cpu_rate(cpu);
- }
} static unsigned int @@ -104,11 +109,11 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate) mutex_lock(&cluster_lock[new_cluster]);
- prev_rate = per_cpu(cpu_last_req_freq, cpu);
- per_cpu(cpu_last_req_freq, cpu) = rate;
- per_cpu(physical_cluster, cpu) = new_cluster;
- if (is_bL_switching_enabled()) {
prev_rate = per_cpu(cpu_last_req_freq, cpu);
per_cpu(cpu_last_req_freq, cpu) = rate;
per_cpu(physical_cluster, cpu) = new_cluster;
- new_rate = find_cluster_maxfreq(new_cluster); new_rate = ACTUAL_FREQ(new_cluster, new_rate); } else {
@@ -122,8 +127,10 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate) if (WARN_ON(ret)) { pr_err("clk_set_rate failed: %d, new cluster: %d\n", ret, new_cluster);
per_cpu(cpu_last_req_freq, cpu) = prev_rate;
per_cpu(physical_cluster, cpu) = old_cluster;
if (is_bL_switching_enabled()) {
per_cpu(cpu_last_req_freq, cpu) = prev_rate;
This causes the warnings below.
drivers/cpufreq/arm_big_little.c: In function 'bL_cpufreq_set_target': drivers/cpufreq/arm_big_little.c:131:36: warning: 'prev_rate' may be used uninitialized in this function [-Wmaybe-uninitialized] drivers/cpufreq/arm_big_little.c:107:16: note: 'prev_rate' was declared here
How about we add
bool bLs = is_bL_switching_enabled();
and use "if (bLs)" rather than "if (is_bL_switching_enabled())"?
per_cpu(physical_cluster, cpu) = old_cluster;
}
mutex_unlock(&cluster_lock[new_cluster]); @@ -461,7 +468,9 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy) policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL; policy->cur = clk_get_cpu_rate(policy->cpu);
- per_cpu(cpu_last_req_freq, policy->cpu) = policy->cur;
- if (is_bL_switching_enabled())
per_cpu(cpu_last_req_freq, policy->cpu) = policy->cur;
dev_info(cpu_dev, "%s: CPU %d initialized\n", __func__, policy->cpu); return 0;
On 8 August 2013 15:31, Jon Medhurst (Tixy) tixy@linaro.org wrote:
This causes the warnings below.
drivers/cpufreq/arm_big_little.c: In function 'bL_cpufreq_set_target': drivers/cpufreq/arm_big_little.c:131:36: warning: 'prev_rate' may be used uninitialized in this function [-Wmaybe-uninitialized] drivers/cpufreq/arm_big_little.c:107:16: note: 'prev_rate' was declared here
Looks like an obvious warning but I haven't seen it when I compiled this code :(
Probably something wrong at my end..
Can you initialize with zero in my patch? I don't want to send a patch just for this change. Being too lazy :)
How about we add
bool bLs = is_bL_switching_enabled();
and use "if (bLs)" rather than "if (is_bL_switching_enabled())"?
Probably current name is self descriptive and the new name will impact on readability..
On Thu, 2013-08-08 at 15:36 +0530, Viresh Kumar wrote:
On 8 August 2013 15:31, Jon Medhurst (Tixy) tixy@linaro.org wrote:
This causes the warnings below.
drivers/cpufreq/arm_big_little.c: In function 'bL_cpufreq_set_target': drivers/cpufreq/arm_big_little.c:131:36: warning: 'prev_rate' may be used uninitialized in this function [-Wmaybe-uninitialized] drivers/cpufreq/arm_big_little.c:107:16: note: 'prev_rate' was declared here
Looks like an obvious warning but I haven't seen it when I compiled this code :(
Probably something wrong at my end..
Can you initialize with zero in my patch? I don't want to send a patch just for this change. Being too lazy :)
How about we add
bool bLs = is_bL_switching_enabled();
and use "if (bLs)" rather than "if (is_bL_switching_enabled())"?
Probably current name is self descriptive and the new name will impact on readability..
I was proposing that as a fix for the "may be used uninitialized" warning. :-) If we use a local variable then the compiler knows both if clauses are always executed or not as a pair and so knows the variable isn't used uninitialised. Seems the safest fix to me as it lets the compiler warn us about any dodgy use of that variable that may creep in in the future.
On 8 August 2013 15:44, Jon Medhurst (Tixy) tixy@linaro.org wrote:
I was proposing that as a fix for the "may be used uninitialized" warning. :-) If we use a local variable then the compiler knows both if clauses are always executed or not as a pair and so knows the variable isn't used uninitialised. Seems the safest fix to me as it lets the compiler warn us about any dodgy use of that variable that may creep in in the future.
Hmm... because currently it is based on calling a function and its outcome isn't constant..
If compiler is smart enough to suppress this warning with the code you just suggested, I wouldn't have any issues with it..
You want the patch from me now? Or you can take care of it?
On Thu, 2013-08-08 at 15:49 +0530, Viresh Kumar wrote:
On 8 August 2013 15:44, Jon Medhurst (Tixy) tixy@linaro.org wrote:
I was proposing that as a fix for the "may be used uninitialized" warning. :-) If we use a local variable then the compiler knows both if clauses are always executed or not as a pair and so knows the variable isn't used uninitialised. Seems the safest fix to me as it lets the compiler warn us about any dodgy use of that variable that may creep in in the future.
Hmm... because currently it is based on calling a function and its outcome isn't constant..
Yes, though actually it's just a global variable.
If compiler is smart enough to suppress this warning with the code you just suggested, I wouldn't have any issues with it..
Do you think I would have proposed it without testing it ;-)
You want the patch from me now? Or you can take care of it?
I'm happy to fold in that change to the original patch, I'll follow up to this reply with the end result.
On 8 August 2013 16:01, Jon Medhurst (Tixy) tixy@linaro.org wrote:
If compiler is smart enough to suppress this warning with the code you just suggested, I wouldn't have any issues with it..
Do you think I would have proposed it without testing it ;-)
No.. But I actually knew about it today only.. and wasn't sure until today that compilers do it.. Wasn't actually doubting the way you work :)
You want the patch from me now? Or you can take care of it?
I'm happy to fold in that change to the original patch. I'll follow up to this reply with the end result.
ok.
From: Viresh Kumar viresh.kumar@linaro.org
While getting support for In-Kernel-Switcher in big LITTLE cpufreq driver we introduced cpu_last_req_freq per-cpu variable that stores the last frequency requested for a cpu. It was important for IKS as CPUs in the same cluster can have separate struct cpufreq_policy associated with them and so cpufreq core may try to set different frequencies on both CPUs of same cluster.
But for non-IKS solution or MP we don't need to cache last requested frequency. Currently there is a bug in code where if cpufreq_driver->get() is called for any cpu other than policy->cpu, we are returning 0, because we set cpu_last_req_freq only for policy->cpu and not for others.
This problem could have been fixed by setting cpu_last_req_freq for all CPUs in policy->cpus, but that wouldn't be the best solution. Purpose of cpufreq_driver->get() is to get exact frequency from the hardware instead of returning cached frequency that was last requested as that is already present with the core.
Hence, this patch forces only IKS to use cpu_last_req_freq and not MP. We will get the frequency from hardware when ->get() is called for MP.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Signed-off-by: Jon Medhurst tixy@linaro.org --- drivers/cpufreq/arm_big_little.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c index 7c2be81..076f25a 100644 --- a/drivers/cpufreq/arm_big_little.c +++ b/drivers/cpufreq/arm_big_little.c @@ -91,9 +91,14 @@ static unsigned int clk_get_cpu_rate(unsigned int cpu)
static unsigned int bL_cpufreq_get_rate(unsigned int cpu) { - pr_debug("%s: freq: %d\n", __func__, per_cpu(cpu_last_req_freq, cpu)); + if (is_bL_switching_enabled()) { + pr_debug("%s: freq: %d\n", __func__, per_cpu(cpu_last_req_freq, + cpu));
- return per_cpu(cpu_last_req_freq, cpu); + return per_cpu(cpu_last_req_freq, cpu); + } else { + return clk_get_cpu_rate(cpu); + } }
static unsigned int @@ -101,14 +106,15 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate) { u32 new_rate, prev_rate; int ret; + bool bLs = is_bL_switching_enabled();
mutex_lock(&cluster_lock[new_cluster]);
- prev_rate = per_cpu(cpu_last_req_freq, cpu); - per_cpu(cpu_last_req_freq, cpu) = rate; - per_cpu(physical_cluster, cpu) = new_cluster; + if (bLs) { + prev_rate = per_cpu(cpu_last_req_freq, cpu); + per_cpu(cpu_last_req_freq, cpu) = rate; + per_cpu(physical_cluster, cpu) = new_cluster;
- if (is_bL_switching_enabled()) { new_rate = find_cluster_maxfreq(new_cluster); new_rate = ACTUAL_FREQ(new_cluster, new_rate); } else { @@ -122,8 +128,10 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate) if (WARN_ON(ret)) { pr_err("clk_set_rate failed: %d, new cluster: %d\n", ret, new_cluster); - per_cpu(cpu_last_req_freq, cpu) = prev_rate; - per_cpu(physical_cluster, cpu) = old_cluster; + if (bLs) { + per_cpu(cpu_last_req_freq, cpu) = prev_rate; + per_cpu(physical_cluster, cpu) = old_cluster; + }
mutex_unlock(&cluster_lock[new_cluster]);
@@ -463,7 +471,9 @@ static int bL_cpufreq_init(struct cpufreq_policy *policy) policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
policy->cur = clk_get_cpu_rate(policy->cpu); - per_cpu(cpu_last_req_freq, policy->cpu) = policy->cur; + + if (is_bL_switching_enabled()) + per_cpu(cpu_last_req_freq, policy->cpu) = policy->cur;
dev_info(cpu_dev, "%s: CPU %d initialized\n", __func__, policy->cpu); return 0;
On 8 August 2013 16:11, Jon Medhurst (Tixy) tixy@linaro.org wrote:
From: Viresh Kumar viresh.kumar@linaro.org
While getting support for In-Kernel-Switcher in big LITTLE cpufreq driver we introduced cpu_last_req_freq per-cpu variable that stores the last frequency requested for a cpu. It was important for IKS as CPUs in the same cluster can have separate struct cpufreq_policy associated with them and so cpufreq core may try to set different frequencies on both CPUs of same cluster.
But for non-IKS solution or MP we don't need to cache last requested frequency. Currently there is a bug in code where if cpufreq_driver->get() is called for any cpu other than policy->cpu, we are returning 0, because we set cpu_last_req_freq only for policy->cpu and not for others.
This problem could have been fixed by setting cpu_last_req_freq for all CPUs in policy->cpus, but that wouldn't be the best solution. Purpose of cpufreq_driver->get() is to get exact frequency from the hardware instead of returning cached frequency that was last requested as that is already present with the core.
Hence, this patch forces only IKS to use cpu_last_req_freq and not MP. We will get the frequency from hardware when ->get() is called for MP.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org Signed-off-by: Jon Medhurst tixy@linaro.org
drivers/cpufreq/arm_big_little.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)
Looks good. Thanks.
linaro-kernel@lists.linaro.org