* [PATCH v4] cpufreq: fix governor start/stop race condition @ 2013-06-12 9:09 Xiaoguang Chen 2013-06-12 9:31 ` Viresh Kumar 0 siblings, 1 reply; 6+ messages in thread From: Xiaoguang Chen @ 2013-06-12 9:09 UTC (permalink / raw) To: viresh.kumar, rjw Cc: cpufreq, linux-pm, linux-kernel, njiang1, zjwu, ylmao, chenxg.marvell, Xiaoguang Chen cpufreq governor stop and start should be kept in sequence. If not, there will be unexpected behavior, for example: we have 4 cpus and policy->cpu=cpu0, cpu1/2/3 are linked to cpu0. the normal sequence is as below: 1) Current governor is userspace, one application tries to set governor to ondemand. it will call __cpufreq_set_policy in which it will stop userspace governor and then start ondemand governor. 2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will call cpufreq_add_policy_cpu. on which it first stops userspace governor, and then starts userspace governor. Now if the sequence of above two cases interleaves, it becames below sequence: 1) application stops userspace governor 2) hotplug stops userspace governor 3) application starts ondemand governor 4) hotplug starts a governor in step 4, hotplug is supposed to start userspace governor, but now the governor has been changed by application to ondemand, so hotplug starts ondemand governor again !!!! The solution is: do not allow stop one policy's governor multi-times Governor stop should only do once for one policy, after it is stopped, no other governor stop should be executed. also add one mutext to protect __cpufreq_governor so governor operation can be kept in sequence. Signed-off-by: Xiaoguang Chen <chenxg@marvell.com> --- drivers/cpufreq/cpufreq.c | 28 +++++++++++++++++++++++----- include/linux/cpufreq.h | 1 + 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2d53f47..6c10cf0 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -46,6 +46,7 @@ static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor); #endif static DEFINE_RWLOCK(cpufreq_driver_lock); +static DEFINE_MUTEX(cpufreq_governor_lock); /* * cpu_policy_rwsem is a per CPU reader-writer semaphore designed to cure @@ -896,6 +897,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) goto module_out; } + + policy = kzalloc(sizeof(struct cpufreq_policy), GFP_KERNEL); if (!policy) goto nomem_out; @@ -1541,13 +1544,14 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, #else struct cpufreq_governor *gov = NULL; #endif - + mutex_lock(&cpufreq_governor_lock); if (policy->governor->max_transition_latency && policy->cpuinfo.transition_latency > policy->governor->max_transition_latency) { - if (!gov) + if (!gov) { + mutex_unlock(&cpufreq_governor_lock); return -EINVAL; - else { + } else { printk(KERN_WARNING "%s governor failed, too long" " transition latency of HW, fallback" " to %s governor\n", @@ -1557,11 +1561,19 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, } } - if (!try_module_get(policy->governor->owner)) + if (!try_module_get(policy->governor->owner)) { + mutex_unlock(&cpufreq_governor_lock); return -EINVAL; - + } pr_debug("__cpufreq_governor for CPU %u, event %u\n", policy->cpu, event); + + if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) || + (policy->governor_enabled && (event == CPUFREQ_GOV_START))) { + mutex_unlock(&cpufreq_governor_lock); + return -EBUSY; + } + ret = policy->governor->governor(policy, event); if (!ret) { @@ -1569,6 +1581,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->governor->initialized++; else if (event == CPUFREQ_GOV_POLICY_EXIT) policy->governor->initialized--; + else if (event == CPUFREQ_GOV_STOP) + policy->governor_enabled = 0; + else if (event == CPUFREQ_GOV_START) + policy->governor_enabled = 1; } /* we keep one module reference alive for @@ -1578,6 +1594,8 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, if ((event == CPUFREQ_GOV_STOP) && !ret) module_put(policy->governor->owner); + mutex_unlock(&cpufreq_governor_lock); + return ret; } diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 037d36a..c12db73 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -107,6 +107,7 @@ struct cpufreq_policy { unsigned int policy; /* see above */ struct cpufreq_governor *governor; /* see below */ void *governor_data; + int governor_enabled; /* governor start/stop flag */ struct work_struct update; /* if update_policy() needs to be * called, but you're in IRQ context */ -- 1.8.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] cpufreq: fix governor start/stop race condition 2013-06-12 9:09 [PATCH v4] cpufreq: fix governor start/stop race condition Xiaoguang Chen @ 2013-06-12 9:31 ` Viresh Kumar 2013-06-13 5:40 ` Xiaoguang Chen 0 siblings, 1 reply; 6+ messages in thread From: Viresh Kumar @ 2013-06-12 9:31 UTC (permalink / raw) To: Xiaoguang Chen Cc: rjw, cpufreq, linux-pm, linux-kernel, njiang1, zjwu, ylmao, chenxg.marvell On 12 June 2013 14:39, Xiaoguang Chen <chenxg@marvell.com> wrote: > ret = policy->governor->governor(policy, event); We again reached to the same problem. We shouldn't call this between taking locks, otherwise recursive locks problems would be seen again. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] cpufreq: fix governor start/stop race condition 2013-06-12 9:31 ` Viresh Kumar @ 2013-06-13 5:40 ` Xiaoguang Chen 2013-06-13 5:52 ` Viresh Kumar 0 siblings, 1 reply; 6+ messages in thread From: Xiaoguang Chen @ 2013-06-13 5:40 UTC (permalink / raw) To: Viresh Kumar Cc: Xiaoguang Chen, Rafael J. Wysocki, cpufreq, linux-pm, linux-kernel, njiang1, zjwu, ylmao 2013/6/12 Viresh Kumar <viresh.kumar@linaro.org>: > On 12 June 2013 14:39, Xiaoguang Chen <chenxg@marvell.com> wrote: > >> ret = policy->governor->governor(policy, event); > > We again reached to the same problem. We shouldn't call > this between taking locks, otherwise recursive locks problems > would be seen again. But this is not the same lock as the deadlock case, it is a new lock, and only used in this function. no other functions use this lock. I don't know how can we get dead lock again? Thanks Xiaoguang ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] cpufreq: fix governor start/stop race condition 2013-06-13 5:40 ` Xiaoguang Chen @ 2013-06-13 5:52 ` Viresh Kumar 2013-06-13 7:19 ` Xiaoguang Chen 0 siblings, 1 reply; 6+ messages in thread From: Viresh Kumar @ 2013-06-13 5:52 UTC (permalink / raw) To: Xiaoguang Chen Cc: Xiaoguang Chen, Rafael J. Wysocki, cpufreq, linux-pm, linux-kernel, njiang1, zjwu, ylmao [-- Attachment #1: Type: text/plain, Size: 3267 bytes --] On 13 June 2013 11:10, Xiaoguang Chen <chenxg.marvell@gmail.com> wrote: > 2013/6/12 Viresh Kumar <viresh.kumar@linaro.org>: >> On 12 June 2013 14:39, Xiaoguang Chen <chenxg@marvell.com> wrote: >> >>> ret = policy->governor->governor(policy, event); >> >> We again reached to the same problem. We shouldn't call >> this between taking locks, otherwise recursive locks problems >> would be seen again. > > But this is not the same lock as the deadlock case, it is a new lock, > and only used in this function. no other functions use this lock. > I don't know how can we get dead lock again? I believe I have seen the recursive lock issue with different locks but I am not sure. Anyway, I believe the implementation can be simpler than that. Check below patch (attached too): ------------x------------------x---------------- diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2d53f47..80b9798 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -46,6 +46,7 @@ static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor); #endif static DEFINE_RWLOCK(cpufreq_driver_lock); +static DEFINE_MUTEX(cpufreq_governor_lock); /* * cpu_policy_rwsem is a per CPU reader-writer semaphore designed to cure @@ -1541,7 +1542,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, #else struct cpufreq_governor *gov = NULL; #endif - if (policy->governor->max_transition_latency && policy->cpuinfo.transition_latency > policy->governor->max_transition_latency) { @@ -1562,6 +1562,21 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, pr_debug("__cpufreq_governor for CPU %u, event %u\n", policy->cpu, event); + + mutex_lock(&cpufreq_governor_lock); + if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) || + (policy->governor_enabled && (event == CPUFREQ_GOV_START))) { + mutex_unlock(&cpufreq_governor_lock); + return -EBUSY; + } + + if (event == CPUFREQ_GOV_STOP) + policy->governor_enabled = 0; + else if (event == CPUFREQ_GOV_START) + policy->governor_enabled = 1; + + mutex_unlock(&cpufreq_governor_lock); + ret = policy->governor->governor(policy, event); if (!ret) { @@ -1569,6 +1584,14 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->governor->initialized++; else if (event == CPUFREQ_GOV_POLICY_EXIT) policy->governor->initialized--; + } else { + /* Restore original values */ + mutex_lock(&cpufreq_governor_lock); + if (event == CPUFREQ_GOV_STOP) + policy->governor_enabled = 1; + else if (event == CPUFREQ_GOV_START) + policy->governor_enabled = 0; + mutex_unlock(&cpufreq_governor_lock); } /* we keep one module reference alive for diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 037d36a..c12db73 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -107,6 +107,7 @@ struct cpufreq_policy { unsigned int policy; /* see above */ struct cpufreq_governor *governor; /* see below */ void *governor_data; + int governor_enabled; /* governor start/stop flag */ struct work_struct update; /* if update_policy() needs to be * called, but you're in IRQ context */ [-- Attachment #2: 0001-cpufreq-fix-governor-start-stop-race-condition.patch --] [-- Type: application/octet-stream, Size: 4321 bytes --] From badb38ce4742c752e573d2884390467ea29866c9 Mon Sep 17 00:00:00 2001 Message-Id: <badb38ce4742c752e573d2884390467ea29866c9.1371102669.git.viresh.kumar@linaro.org> From: Xiaoguang Chen <chenxg@marvell.com> Date: Wed, 12 Jun 2013 17:09:49 +0800 Subject: [PATCH] cpufreq: fix governor start/stop race condition cpufreq governor stop and start should be kept in sequence. If not, there will be unexpected behavior, for example: we have 4 cpus and policy->cpu=cpu0, cpu1/2/3 are linked to cpu0. the normal sequence is as below: 1) Current governor is userspace, one application tries to set governor to ondemand. it will call __cpufreq_set_policy in which it will stop userspace governor and then start ondemand governor. 2) Current governor is userspace, now cpu0 hotplugs in cpu3, it will call cpufreq_add_policy_cpu. on which it first stops userspace governor, and then starts userspace governor. Now if the sequence of above two cases interleaves, it becames below sequence: 1) application stops userspace governor 2) hotplug stops userspace governor 3) application starts ondemand governor 4) hotplug starts a governor in step 4, hotplug is supposed to start userspace governor, but now the governor has been changed by application to ondemand, so hotplug starts ondemand governor again !!!! The solution is: do not allow stop one policy's governor multi-times Governor stop should only do once for one policy, after it is stopped, no other governor stop should be executed. also add one mutext to protect __cpufreq_governor so governor operation can be kept in sequence. Signed-off-by: Xiaoguang Chen <chenxg@marvell.com> --- drivers/cpufreq/cpufreq.c | 25 ++++++++++++++++++++++++- include/linux/cpufreq.h | 1 + 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 2d53f47..80b9798 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -46,6 +46,7 @@ static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor); #endif static DEFINE_RWLOCK(cpufreq_driver_lock); +static DEFINE_MUTEX(cpufreq_governor_lock); /* * cpu_policy_rwsem is a per CPU reader-writer semaphore designed to cure @@ -1541,7 +1542,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, #else struct cpufreq_governor *gov = NULL; #endif - if (policy->governor->max_transition_latency && policy->cpuinfo.transition_latency > policy->governor->max_transition_latency) { @@ -1562,6 +1562,21 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, pr_debug("__cpufreq_governor for CPU %u, event %u\n", policy->cpu, event); + + mutex_lock(&cpufreq_governor_lock); + if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) || + (policy->governor_enabled && (event == CPUFREQ_GOV_START))) { + mutex_unlock(&cpufreq_governor_lock); + return -EBUSY; + } + + if (event == CPUFREQ_GOV_STOP) + policy->governor_enabled = 0; + else if (event == CPUFREQ_GOV_START) + policy->governor_enabled = 1; + + mutex_unlock(&cpufreq_governor_lock); + ret = policy->governor->governor(policy, event); if (!ret) { @@ -1569,6 +1584,14 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, policy->governor->initialized++; else if (event == CPUFREQ_GOV_POLICY_EXIT) policy->governor->initialized--; + } else { + /* Restore original values */ + mutex_lock(&cpufreq_governor_lock); + if (event == CPUFREQ_GOV_STOP) + policy->governor_enabled = 1; + else if (event == CPUFREQ_GOV_START) + policy->governor_enabled = 0; + mutex_unlock(&cpufreq_governor_lock); } /* we keep one module reference alive for diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 037d36a..c12db73 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -107,6 +107,7 @@ struct cpufreq_policy { unsigned int policy; /* see above */ struct cpufreq_governor *governor; /* see below */ void *governor_data; + int governor_enabled; /* governor start/stop flag */ struct work_struct update; /* if update_policy() needs to be * called, but you're in IRQ context */ -- 1.7.12.rc2.18.g61b472e ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] cpufreq: fix governor start/stop race condition 2013-06-13 5:52 ` Viresh Kumar @ 2013-06-13 7:19 ` Xiaoguang Chen 2013-06-13 8:40 ` Viresh Kumar 0 siblings, 1 reply; 6+ messages in thread From: Xiaoguang Chen @ 2013-06-13 7:19 UTC (permalink / raw) To: Viresh Kumar Cc: Xiaoguang Chen, Rafael J. Wysocki, cpufreq, linux-pm, linux-kernel, njiang1, zjwu, ylmao 2013/6/13 Viresh Kumar <viresh.kumar@linaro.org>: > On 13 June 2013 11:10, Xiaoguang Chen <chenxg.marvell@gmail.com> wrote: >> 2013/6/12 Viresh Kumar <viresh.kumar@linaro.org>: >>> On 12 June 2013 14:39, Xiaoguang Chen <chenxg@marvell.com> wrote: >>> >>>> ret = policy->governor->governor(policy, event); >>> >>> We again reached to the same problem. We shouldn't call >>> this between taking locks, otherwise recursive locks problems >>> would be seen again. >> >> But this is not the same lock as the deadlock case, it is a new lock, >> and only used in this function. no other functions use this lock. >> I don't know how can we get dead lock again? > > I believe I have seen the recursive lock issue with different locks but > I am not sure. Anyway, I believe the implementation can be simpler than > that. > > Check below patch (attached too): > > ------------x------------------x---------------- > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 2d53f47..80b9798 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -46,6 +46,7 @@ static DEFINE_PER_CPU(struct cpufreq_policy *, > cpufreq_cpu_data); > static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor); > #endif > static DEFINE_RWLOCK(cpufreq_driver_lock); > +static DEFINE_MUTEX(cpufreq_governor_lock); > > /* > * cpu_policy_rwsem is a per CPU reader-writer semaphore designed to cure > @@ -1541,7 +1542,6 @@ static int __cpufreq_governor(struct > cpufreq_policy *policy, > #else > struct cpufreq_governor *gov = NULL; > #endif > - > if (policy->governor->max_transition_latency && > policy->cpuinfo.transition_latency > > policy->governor->max_transition_latency) { > @@ -1562,6 +1562,21 @@ static int __cpufreq_governor(struct > cpufreq_policy *policy, > > pr_debug("__cpufreq_governor for CPU %u, event %u\n", > policy->cpu, event); > + > + mutex_lock(&cpufreq_governor_lock); > + if ((!policy->governor_enabled && (event == CPUFREQ_GOV_STOP)) || > + (policy->governor_enabled && (event == CPUFREQ_GOV_START))) { > + mutex_unlock(&cpufreq_governor_lock); > + return -EBUSY; > + } > + > + if (event == CPUFREQ_GOV_STOP) > + policy->governor_enabled = 0; > + else if (event == CPUFREQ_GOV_START) > + policy->governor_enabled = 1; > + > + mutex_unlock(&cpufreq_governor_lock); > + > ret = policy->governor->governor(policy, event); > > if (!ret) { > @@ -1569,6 +1584,14 @@ static int __cpufreq_governor(struct > cpufreq_policy *policy, > policy->governor->initialized++; > else if (event == CPUFREQ_GOV_POLICY_EXIT) > policy->governor->initialized--; > + } else { > + /* Restore original values */ > + mutex_lock(&cpufreq_governor_lock); > + if (event == CPUFREQ_GOV_STOP) > + policy->governor_enabled = 1; > + else if (event == CPUFREQ_GOV_START) > + policy->governor_enabled = 0; > + mutex_unlock(&cpufreq_governor_lock); > } > > /* we keep one module reference alive for > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 037d36a..c12db73 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -107,6 +107,7 @@ struct cpufreq_policy { > unsigned int policy; /* see above */ > struct cpufreq_governor *governor; /* see below */ > void *governor_data; > + int governor_enabled; /* governor start/stop flag */ > > struct work_struct update; /* if update_policy() needs to be > * called, but you're in IRQ context */ Thanks So you add the return value checking, I was about to do it in another patch :) this patch is simpler than my previous patch, it is ok for me. Do I need to submit it again or it can be merged? Thanks Xiaoguang ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] cpufreq: fix governor start/stop race condition 2013-06-13 7:19 ` Xiaoguang Chen @ 2013-06-13 8:40 ` Viresh Kumar 0 siblings, 0 replies; 6+ messages in thread From: Viresh Kumar @ 2013-06-13 8:40 UTC (permalink / raw) To: Xiaoguang Chen Cc: Xiaoguang Chen, Rafael J. Wysocki, cpufreq, linux-pm, linux-kernel, njiang1, zjwu, ylmao On 13 June 2013 12:49, Xiaoguang Chen <chenxg.marvell@gmail.com> wrote: > So you add the return value checking, I was about to do it in another patch :) What? I couldn't related that statement to my code. > this patch is simpler than my previous patch, it is ok for me. > Do I need to submit it again or it can be merged? Yeah.. Send this patch as another version so that Rafael can easily pick it up. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-06-13 8:40 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-06-12 9:09 [PATCH v4] cpufreq: fix governor start/stop race condition Xiaoguang Chen 2013-06-12 9:31 ` Viresh Kumar 2013-06-13 5:40 ` Xiaoguang Chen 2013-06-13 5:52 ` Viresh Kumar 2013-06-13 7:19 ` Xiaoguang Chen 2013-06-13 8:40 ` Viresh Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).