__cpufreq_governor() returns with -EBUSY when governor is already stopped and we try to stop it again, but when it is stopped we must not allow calls to CPUFREQ_GOV_LIMITS event as well.
This patch adds this check in __cpufreq_governor().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Hi Rafael,
Its better if we can get these in for 3.11, otherwise we need to get them in the stable tree..
Anyway, we will get these in 3.10 stable tree but that requires us to identify few more patches that will go with these. I will do that later.
This must fix the issues reported by Stephen.
Tested on my thinkpad over your linux-next branch.
drivers/cpufreq/cpufreq.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 5c75e31..f320a20 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1692,8 +1692,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->cpu, event);
mutex_lock(&cpufreq_governor_lock); - if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) || - (policy->governor_enabled && (event == CPUFREQ_GOV_START))) { + if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) || + (!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) || + (event == CPUFREQ_GOV_STOP)))) { mutex_unlock(&cpufreq_governor_lock); return -EBUSY; }
We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy.
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 7 ++++++- include/linux/cpufreq.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f320a20..4d5723db 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->cpu, event);
mutex_lock(&cpufreq_governor_lock); - if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) || + if (policy->governor_busy || + (policy->governor_enabled && (event == CPUFREQ_GOV_START)) || (!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) || (event == CPUFREQ_GOV_STOP)))) { mutex_unlock(&cpufreq_governor_lock); return -EBUSY; }
+ policy->governor_busy = true; if (event == CPUFREQ_GOV_STOP) policy->governor_enabled = false; else if (event == CPUFREQ_GOV_START) @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret)) module_put(policy->governor->owner);
+ mutex_lock(&cpufreq_governor_lock); + policy->governor_busy = false; + mutex_unlock(&cpufreq_governor_lock); return ret; }
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index d568f39..cca885d 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -76,6 +76,7 @@ struct cpufreq_policy { struct cpufreq_governor *governor; /* see below */ void *governor_data; bool governor_enabled; /* governor start/stop flag */ + bool governor_busy;
struct work_struct update; /* if update_policy() needs to be * called, but you're in IRQ context */
On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy.
Care to explain here why it must be serialized?
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 7 ++++++- include/linux/cpufreq.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f320a20..4d5723db 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->cpu, event); mutex_lock(&cpufreq_governor_lock);
- if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
- if (policy->governor_busy ||
(policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
Again, broken white space, but I can fix it up.
(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) || (event == CPUFREQ_GOV_STOP)))) { mutex_unlock(&cpufreq_governor_lock); return -EBUSY;}
- policy->governor_busy = true; if (event == CPUFREQ_GOV_STOP) policy->governor_enabled = false; else if (event == CPUFREQ_GOV_START)
@@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret)) module_put(policy->governor->owner);
- mutex_lock(&cpufreq_governor_lock);
- policy->governor_busy = false;
- mutex_unlock(&cpufreq_governor_lock); return ret;
} diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index d568f39..cca885d 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -76,6 +76,7 @@ struct cpufreq_policy { struct cpufreq_governor *governor; /* see below */ void *governor_data; bool governor_enabled; /* governor start/stop flag */
- bool governor_busy;
struct work_struct update; /* if update_policy() needs to be * called, but you're in IRQ context */
On 1 September 2013 18:58, Rafael J. Wysocki rjw@sisk.pl wrote:
On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy.
Care to explain here why it must be serialized?
Yes, added that to the attached patches (Added reported-by too):
commit dc51771506b113b998c49c3be2db0fb88bb92153 Author: Viresh Kumar viresh.kumar@linaro.org Date: Sat Aug 31 17:48:23 2013 +0530
cpufreq: serialize calls to __cpufreq_governor()
We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy. Otherwise we can see some unpredictable events.
For example, consider following scenario:
__cpufreq_remove_dev() __cpufreq_governor(policy, CPUFREQ_GOV_STOP); policy->governor->governor(policy, CPUFREQ_GOV_STOP); cpufreq_governor_dbs() case CPUFREQ_GOV_STOP: mutex_destroy(&cpu_cdbs->timer_mutex) cpu_cdbs->cur_policy = NULL; <PREEMPT> store() __cpufreq_set_policy() __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); policy->governor->governor(policy, CPUFREQ_GOV_LIMITS); case CPUFREQ_GOV_LIMITS: mutex_lock(&cpu_cdbs->timer_mutex); <-- Warning (destroyed mutex) if (policy->max < cpu_cdbs->cur_policy->cur) <- cur_policy == NULL
And so store() will eventually result in a crash cur_policy is already NULL;
Lets introduce another variable which would guarantee serialization here.
Reported-by: Stephen Boyd sboyd@codeaurora.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 7 ++++++- include/linux/cpufreq.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f320a20..4d5723db 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->cpu, event);
mutex_lock(&cpufreq_governor_lock);
if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
if (policy->governor_busy ||(policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||Again, broken white space, but I can fix it up.
Sorry, what exactly.. Sorry couldn't understand it :(
On Sunday, September 01, 2013 09:30:49 PM Viresh Kumar wrote:
On 1 September 2013 18:58, Rafael J. Wysocki rjw@sisk.pl wrote:
On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy.
Care to explain here why it must be serialized?
Yes, added that to the attached patches (Added reported-by too):
commit dc51771506b113b998c49c3be2db0fb88bb92153 Author: Viresh Kumar viresh.kumar@linaro.org Date: Sat Aug 31 17:48:23 2013 +0530
cpufreq: serialize calls to __cpufreq_governor() We can't take a big lock around __cpufreq_governor() as thiscauses recursive locking for some cases. But calls to this routine must be serialized for every policy. Otherwise we can see some unpredictable events.
For example, consider following scenario: __cpufreq_remove_dev() __cpufreq_governor(policy, CPUFREQ_GOV_STOP); policy->governor->governor(policy, CPUFREQ_GOV_STOP); cpufreq_governor_dbs() case CPUFREQ_GOV_STOP: mutex_destroy(&cpu_cdbs->timer_mutex) cpu_cdbs->cur_policy = NULL; <PREEMPT> store() __cpufreq_set_policy() __cpufreq_governor(policy, CPUFREQ_GOV_LIMITS); policy->governor->governor(policy, CPUFREQ_GOV_LIMITS); case CPUFREQ_GOV_LIMITS: mutex_lock(&cpu_cdbs->timer_mutex); <-- Warning (destroyed mutex) if (policy->max < cpu_cdbs->cur_policy->cur) <- cur_policy == NULL And so store() will eventually result in a crash cur_policy is already NULL; Lets introduce another variable which would guarantee serialization here.Reported-by: Stephen Boyd sboyd@codeaurora.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 7 ++++++- include/linux/cpufreq.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f320a20..4d5723db 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->cpu, event);
mutex_lock(&cpufreq_governor_lock);
if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
if (policy->governor_busy ||(policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||Again, broken white space, but I can fix it up.
Sorry, what exactly.. Sorry couldn't understand it :(
The second tab is one too many, I usually write such things like this:
if (policy->governor_busy || (policy->governor_enabled && event == CPUFREQ_GOV_START) || ...
Then it is much easier to distinguish the conditional code from the condition itself.
On 2 September 2013 01:57, Rafael J. Wysocki rjw@sisk.pl wrote:
The second tab is one too many, I usually write such things like this:
if (policy->governor_busy || (policy->governor_enabled && event == CPUFREQ_GOV_START) || ...Then it is much easier to distinguish the conditional code from the condition itself.
I see... I tried to do it this way but got confused again :) You fix it this time and I will understand it from that :)
-- viresh
On Sunday, September 01, 2013 09:30:49 PM Viresh Kumar wrote:
On 1 September 2013 18:58, Rafael J. Wysocki rjw@sisk.pl wrote:
On Sunday, September 01, 2013 10:56:02 AM Viresh Kumar wrote:
We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy.
Care to explain here why it must be serialized?
Yes, added that to the attached patches (Added reported-by too):
OK, the attached patches queued up for 3.12.
Thanks, Rafael
On 09/01/2013 10:56 AM, Viresh Kumar wrote:
We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy.
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 7 ++++++- include/linux/cpufreq.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f320a20..4d5723db 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->cpu, event);
mutex_lock(&cpufreq_governor_lock);
- if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
if (policy->governor_busy ||
(policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) || (event == CPUFREQ_GOV_STOP)))) { mutex_unlock(&cpufreq_governor_lock); return -EBUSY; }
policy->governor_busy = true; if (event == CPUFREQ_GOV_STOP) policy->governor_enabled = false; else if (event == CPUFREQ_GOV_START)
@@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret)) module_put(policy->governor->owner);
- mutex_lock(&cpufreq_governor_lock);
- policy->governor_busy = false;
- mutex_unlock(&cpufreq_governor_lock); return ret;
}
This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it.
So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the store_*() thread will wait until the entire CPU offline operation is completed. After that, if it continues, it will get -EBUSY, due to patch [1], since policy->governor_enabled will be set to false.
[1]. https://patchwork.kernel.org/patch/2852463/
So here is the (completely untested) fix that I propose, as a replacement to this patch [2/2]:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 5c75e31..71c4fb2 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -440,17 +440,24 @@ static ssize_t store_##file_name \ unsigned int ret; \ struct cpufreq_policy new_policy; \ \ + get_online_cpus(); \ ret = cpufreq_get_policy(&new_policy, policy->cpu); \ - if (ret) \ - return -EINVAL; \ + if (ret) { \ + ret = -EINVAL; \ + goto out; \ + } \ \ ret = sscanf(buf, "%u", &new_policy.object); \ - if (ret != 1) \ - return -EINVAL; \ + if (ret != 1) { \ + ret = -EINVAL; \ + goto out; \ + } \ \ ret = __cpufreq_set_policy(policy, &new_policy); \ policy->user_policy.object = policy->object; \ \ +out: \ + put_online_cpus(); \ return ret ? ret : count; \ }
@@ -494,17 +501,22 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy, char str_governor[16]; struct cpufreq_policy new_policy;
+ get_online_cpus(); ret = cpufreq_get_policy(&new_policy, policy->cpu); if (ret) - return ret; + goto out;
ret = sscanf(buf, "%15s", str_governor); - if (ret != 1) - return -EINVAL; + if (ret != 1) { + ret = -EINVAL; + goto out; + }
if (cpufreq_parse_governor(str_governor, &new_policy.policy, - &new_policy.governor)) - return -EINVAL; + &new_policy.governor)) { + ret = -EINVAL; + goto out; + }
/* * Do not use cpufreq_set_policy here or the user_policy.max @@ -515,6 +527,9 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy, policy->user_policy.policy = policy->policy; policy->user_policy.governor = policy->governor;
+out: + put_online_cpus(); + if (ret) return ret; else
Regards, Srivatsa S. Bhat
On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
On 09/01/2013 10:56 AM, Viresh Kumar wrote:
We can't take a big lock around __cpufreq_governor() as this causes recursive locking for some cases. But calls to this routine must be serialized for every policy.
Lets introduce another variable which would guarantee serialization here.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 7 ++++++- include/linux/cpufreq.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f320a20..4d5723db 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->cpu, event);
mutex_lock(&cpufreq_governor_lock);
- if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
if (policy->governor_busy ||
(policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) || (event == CPUFREQ_GOV_STOP)))) { mutex_unlock(&cpufreq_governor_lock); return -EBUSY; }
policy->governor_busy = true; if (event == CPUFREQ_GOV_STOP) policy->governor_enabled = false; else if (event == CPUFREQ_GOV_START)
@@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret)) module_put(policy->governor->owner);
- mutex_lock(&cpufreq_governor_lock);
- policy->governor_busy = false;
- mutex_unlock(&cpufreq_governor_lock); return ret;
}
This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it.
Yeah, I overlooked that.
So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the store_*() thread will wait until the entire CPU offline operation is completed. After that, if it continues, it will get -EBUSY, due to patch [1], since policy->governor_enabled will be set to false.
[1]. https://patchwork.kernel.org/patch/2852463/
So here is the (completely untested) fix that I propose, as a replacement to this patch [2/2]:
That looks reasonable to me.
Viresh?
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 5c75e31..71c4fb2 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -440,17 +440,24 @@ static ssize_t store_##file_name \ unsigned int ret; \ struct cpufreq_policy new_policy; \ \
- get_online_cpus(); \ ret = cpufreq_get_policy(&new_policy, policy->cpu); \
- if (ret) \
return -EINVAL; \
- if (ret) { \
ret = -EINVAL; \goto out; \- } \ \ ret = sscanf(buf, "%u", &new_policy.object); \
- if (ret != 1) \
return -EINVAL; \
- if (ret != 1) { \
ret = -EINVAL; \goto out; \- } \ \ ret = __cpufreq_set_policy(policy, &new_policy); \ policy->user_policy.object = policy->object; \ \
+out: \
- put_online_cpus(); \ return ret ? ret : count; \
} @@ -494,17 +501,22 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy, char str_governor[16]; struct cpufreq_policy new_policy;
- get_online_cpus(); ret = cpufreq_get_policy(&new_policy, policy->cpu); if (ret)
return ret;
goto out;ret = sscanf(buf, "%15s", str_governor);
- if (ret != 1)
return -EINVAL;
- if (ret != 1) {
ret = -EINVAL;goto out;- }
if (cpufreq_parse_governor(str_governor, &new_policy.policy,
&new_policy.governor))return -EINVAL;
&new_policy.governor)) {ret = -EINVAL;goto out;- }
/* * Do not use cpufreq_set_policy here or the user_policy.max @@ -515,6 +527,9 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy, policy->user_policy.policy = policy->policy; policy->user_policy.governor = policy->governor; +out:
- put_online_cpus();
- if (ret) return ret; else
Regards, Srivatsa S. Bhat
On 4 September 2013 01:10, Rafael J. Wysocki rjw@sisk.pl wrote:
On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it.
Yeah, I overlooked that.
As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked this unread as there were other important topics to close)..
And he had some other code in mind and these synchronization problems aren't there with my patch at all (as per him too)..
Rafael, probably both me and Srivatsa are missing something that you understood, can you please share what problem you see here with my patch?
And yes, even with Srivatsa's patchset I found a problem: Two threads, one changing governor from ondemand->conservative and other one changing min/max freq..
First one will try to STOP governor and other one will try to change limits of gov. Suppose 2nd one gets to ->governor() and after this first one stops the governor. Now the first one tries to access lock and crashes..
On IRC, Srivatsa agreed about the problem..
So, probably the first thing to do is to get this patch back, i.e. revert of Srivatsa's patch:
commit 56d07db274b7b15ca38b60ea4a762d40de093000 Author: Srivatsa S. Bhat srivatsa.bhat@linux.vnet.ibm.com Date: Sat Sep 7 01:23:55 2013 +0530
cpufreq: Remove temporary fix for race between CPU hotplug and sysfs-writes
Srivatsa also asked if we can get a big lock around call to ->governor() which would create recursive locks on a call to EXIT event (as we have seen it earlier..)..
But he believes he can pull it off and will try this later.. So, for now we can revert the above patch :)
-- viresh
On Friday, September 13, 2013 05:14:26 PM Viresh Kumar wrote:
On 4 September 2013 01:10, Rafael J. Wysocki rjw@sisk.pl wrote:
On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it.
Yeah, I overlooked that.
As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked this unread as there were other important topics to close)..
And he had some other code in mind and these synchronization problems aren't there with my patch at all (as per him too)..
Rafael, probably both me and Srivatsa are missing something that you understood, can you please share what problem you see here with my patch?
And yes, even with Srivatsa's patchset I found a problem: Two threads, one changing governor from ondemand->conservative and other one changing min/max freq..
First one will try to STOP governor and other one will try to change limits of gov. Suppose 2nd one gets to ->governor() and after this first one stops the governor. Now the first one tries to access lock and crashes..
On IRC, Srivatsa agreed about the problem..
So, probably the first thing to do is to get this patch back, i.e. revert of Srivatsa's patch:
commit 56d07db274b7b15ca38b60ea4a762d40de093000 Author: Srivatsa S. Bhat srivatsa.bhat@linux.vnet.ibm.com Date: Sat Sep 7 01:23:55 2013 +0530
cpufreq: Remove temporary fix for race between CPU hotplug and sysfs-writesSrivatsa also asked if we can get a big lock around call to ->governor() which would create recursive locks on a call to EXIT event (as we have seen it earlier..)..
But he believes he can pull it off and will try this later.. So, for now we can revert the above patch :)
OK, I'll think about that, but not today and probably not within the next few days, because I'm heading to New Orleans in several hours and really have other stuff to take care of now. Sorry about that.
Thanks, Rafael
On 14 September 2013 05:18, Rafael J. Wysocki rjw@sisk.pl wrote:
OK, I'll think about that, but not today and probably not within the next few days, because I'm heading to New Orleans in several hours and really have other stuff to take care of now. Sorry about that.
No issues..
On 09/13/2013 05:14 PM, Viresh Kumar wrote:
On 4 September 2013 01:10, Rafael J. Wysocki rjw@sisk.pl wrote:
On Tuesday, September 03, 2013 06:50:05 PM Srivatsa S. Bhat wrote:
This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it.
Yeah, I overlooked that.
As a background, I had a IRC chat with Srivatsa on this mail.. (I have marked this unread as there were other important topics to close)..
And he had some other code in mind and these synchronization problems aren't there with my patch at all (as per him too)..
Rafael, probably both me and Srivatsa are missing something that you understood, can you please share what problem you see here with my patch?
And yes, even with Srivatsa's patchset I found a problem: Two threads, one changing governor from ondemand->conservative and other one changing min/max freq..
First one will try to STOP governor and other one will try to change limits of gov. Suppose 2nd one gets to ->governor() and after this first one stops the governor. Now the first one tries to access lock and crashes..
On IRC, Srivatsa agreed about the problem..
Actually, we had another round of discussions and decided that there is no problem here. The reason is, the top-level store() routine takes the rwsem of that policy for write; so only one store() can go at a time. So the race between changing the governor and changing the min/max freq won't happen because only one of them can execute at a given time. In fact, by extension, *any* two store()s in cpufreq shouldn't race with one another.
So the conclusion is that the mainline code is fine in this aspect. Nothing needs to be reverted.
That said, there are still certain low-priority/less-visible synchronization issues to be fixed in cpufreq, and we'll take them up later, once the dust is settled.
Regards, Srivatsa S. Bhat
Back finally and I see lots of mails over cpufreq stuff.. :)
On 3 September 2013 18:50, Srivatsa S. Bhat srivatsa.bhat@linux.vnet.ibm.com wrote:
This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it.
How exactly? My brain is still on vacations :)
Anyway, this was one of the problem that I tried to solve with my patch. But there can be other race conditions where things can go wrong and so that patch may still be useful.
Call to __cpufreq_governor() must be serialized I believe.
On 09/10/2013 12:22 PM, Viresh Kumar wrote:
Back finally and I see lots of mails over cpufreq stuff.. :)
On 3 September 2013 18:50, Srivatsa S. Bhat srivatsa.bhat@linux.vnet.ibm.com wrote:
This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it.
How exactly? My brain is still on vacations :)
The race window is not limited to __cpufreq_governor() alone. It just starts there, but doesn't end there; it extends beyond that point. That's the main problem. And that's why serializing __cpufreq_governor() won't completely solve the issue.
CPU_DOWN:
__cpufreq_governor(STOP) ----+ ... | ... | RACE ... | WINDOW ... | sysfs teardown ----+
In the STOP call, we destroy the timer mutex and set cur_policy = NULL. So the incorrect access to timer mutex can occur from that point until we finally unlink or teardown the sysfs files and prevent userspace from writing to those files. Thus, this window involves stuff other than the call to __cpufreq_governor() as well. So serializing __cpufreq_governor() alone is not enough.
Anyway, this was one of the problem that I tried to solve with my patch. But there can be other race conditions where things can go wrong and so that patch may still be useful.
I agree, but see below (its the "may be useful" part that I'm not convinced about).
Call to __cpufreq_governor() must be serialized I believe.
Sure, its a good idea to serialize __cpufreq_governor(). However, we need specific justification for that. Just a hunch that something will go wrong is not enough, IMHO. We have to *prove* that something is indeed wrong, and only then add appropriate synchronization to fix it.
Besides, even the commit message appeared a bit odd. It mentions something about problems with recursive calls. What cases are those? When do we end up recursively calling __cpufreq_governor()? And hence, why can't we use plain locks and must instead use yet another quick-and-dirty variable to protect that function? [Using variables like that is bad from a lockdep perspective as well (apart from other things) : lockdep can't catch any resulting locking issues.]
Other related questions that are worth answering would be: why should we add the synchronization *inside* __cpufreq_governor()? Why not add it at its call-sites?
IOW, I'm all in for fixing problems, just that I'd be comfortable with the changes only when we completely understand what problem we are fixing and why that patch is the right fix for that problem.
Regards, Srivatsa S. Bhat
On 1 September 2013 10:56, Viresh Kumar viresh.kumar@linaro.org wrote:
Its better if we can get these in for 3.11, otherwise we need to get them in the stable tree..
They can't get into 3.11 due to two reasons.. - I haven't tested them over 3.11 but linux-next/master. - Few more dependency patches might also be required for 3.11..
So get them into 3.12-rc1 if they look okay to you :)
On Sunday, September 01, 2013 11:56:09 AM Viresh Kumar wrote:
On 1 September 2013 10:56, Viresh Kumar viresh.kumar@linaro.org wrote:
Its better if we can get these in for 3.11, otherwise we need to get them in the stable tree..
They can't get into 3.11 due to two reasons..
- I haven't tested them over 3.11 but linux-next/master.
- Few more dependency patches might also be required for 3.11..
So get them into 3.12-rc1 if they look okay to you :)
I'm going to do that, but please add explanation to the [2/2] changelog.
Thanks!
On Sunday, September 01, 2013 10:56:01 AM Viresh Kumar wrote:
__cpufreq_governor() returns with -EBUSY when governor is already stopped and we try to stop it again, but when it is stopped we must not allow calls to CPUFREQ_GOV_LIMITS event as well.
This patch adds this check in __cpufreq_governor().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hi Rafael,
Its better if we can get these in for 3.11, otherwise we need to get them in the stable tree..
There's no way they could go into 3.11 or even 3.12 without speding time in linux-next. I'll queue them up for the second part of the 3.12 merge window, unless there is 3.11-rc8 (which I doubt will happen).
Anyway, we will get these in 3.10 stable tree but that requires us to identify few more patches that will go with these. I will do that later.
This must fix the issues reported by Stephen.
Tested on my thinkpad over your linux-next branch.
drivers/cpufreq/cpufreq.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 5c75e31..f320a20 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1692,8 +1692,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->cpu, event); mutex_lock(&cpufreq_governor_lock);
- if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) ||
(policy->governor_enabled && (event == CPUFREQ_GOV_START))) {
- if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) ||
(!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) ||(event == CPUFREQ_GOV_STOP)))) {
Broken white space, but never mind.
mutex_unlock(&cpufreq_governor_lock); return -EBUSY;}
Thanks, Rafael
linaro-kernel@lists.linaro.org
-
Rafael J. Wysocki -
Srivatsa S. Bhat -
Viresh Kumar