* [PATCH v2] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled
@ 2014-01-02 3:14 jiel
2014-01-02 5:34 ` Viresh Kumar
2014-01-02 23:26 ` Dmitry Torokhov
0 siblings, 2 replies; 4+ messages in thread
From: jiel @ 2014-01-02 3:14 UTC (permalink / raw)
To: rjw, viresh.kumar; +Cc: cpufreq, linux-pm, linux-kernel, Jane Li
From: Jane Li <jiel@marvell.com>
When a CPU is hot removed we'll cancel all the delayed work items via
gov_cancel_work(). Sometimes the delayed work function determines that
it should adjust the delay for all other CPUs that the policy is
managing. If this scenario occurs, the canceling CPU will cancel its own
work but queue up the other CPUs works to run.
Commit 3617f2(cpufreq: Fix timer/workqueue corruption due to double
queueing) has tried to fix this, but reading governor_enabled is not
protected by cpufreq_governor_lock. Even though od_dbs_timer() checks
governor_enabled before gov_queue_work(), this scenario may occur. For
example:
CPU0 CPU1
---- ----
cpu_down()
... <work runs>
__cpufreq_remove_dev() od_dbs_timer()
__cpufreq_governor() policy->governor_enabled
policy->governor_enabled = false;
cpufreq_governor_dbs()
case CPUFREQ_GOV_STOP:
gov_cancel_work(dbs_data, policy);
cpu0 work is canceled
timer is canceled
cpu1 work is canceled
<waits for cpu1>
gov_queue_work(*, *, true);
cpu0 work queued
cpu1 work queued
cpu2 work queued
...
cpu1 work is canceled
cpu2 work is canceled
...
At the end of the GOV_STOP case cpu0 still has a work queued to
run although the code is expecting all of the works to be
canceled. __cpufreq_remove_dev() will then proceed to
re-initialize all the other CPUs works except for the CPU that is
going down. The CPUFREQ_GOV_START case in cpufreq_governor_dbs()
will trample over the queued work and debugobjects will spit out
a warning:
WARNING: at lib/debugobjects.c:260 debug_print_object+0x94/0xbc()
ODEBUG: init active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x14
Modules linked in:
CPU: 1 PID: 1205 Comm: sh Tainted: G W 3.10.0 #200
[<c01144f0>] (unwind_backtrace+0x0/0xf8) from [<c0111d98>] (show_stack+0x10/0x14)
[<c0111d98>] (show_stack+0x10/0x14) from [<c01272cc>] (warn_slowpath_common+0x4c/0x68)
[<c01272cc>] (warn_slowpath_common+0x4c/0x68) from [<c012737c>] (warn_slowpath_fmt+0x30/0x40)
[<c012737c>] (warn_slowpath_fmt+0x30/0x40) from [<c034c640>] (debug_print_object+0x94/0xbc)
[<c034c640>] (debug_print_object+0x94/0xbc) from [<c034c7f8>] (__debug_object_init+0xc8/0x3c0)
[<c034c7f8>] (__debug_object_init+0xc8/0x3c0) from [<c01360e0>] (init_timer_key+0x20/0x104)
[<c01360e0>] (init_timer_key+0x20/0x104) from [<c04872ac>] (cpufreq_governor_dbs+0x1dc/0x68c)
[<c04872ac>] (cpufreq_governor_dbs+0x1dc/0x68c) from [<c04833a8>] (__cpufreq_governor+0x80/0x1b0)
[<c04833a8>] (__cpufreq_governor+0x80/0x1b0) from [<c0483704>] (__cpufreq_remove_dev.isra.12+0x22c/0x380)
[<c0483704>] (__cpufreq_remove_dev.isra.12+0x22c/0x380) from [<c0692f38>] (cpufreq_cpu_callback+0x48/0x5c)
[<c0692f38>] (cpufreq_cpu_callback+0x48/0x5c) from [<c014fb40>] (notifier_call_chain+0x44/0x84)
[<c014fb40>] (notifier_call_chain+0x44/0x84) from [<c012ae44>] (__cpu_notify+0x2c/0x48)
[<c012ae44>] (__cpu_notify+0x2c/0x48) from [<c068dd40>] (_cpu_down+0x80/0x258)
[<c068dd40>] (_cpu_down+0x80/0x258) from [<c068df40>] (cpu_down+0x28/0x3c)
[<c068df40>] (cpu_down+0x28/0x3c) from [<c068e4c0>] (store_online+0x30/0x74)
[<c068e4c0>] (store_online+0x30/0x74) from [<c03a7308>] (dev_attr_store+0x18/0x24)
[<c03a7308>] (dev_attr_store+0x18/0x24) from [<c0256fe0>] (sysfs_write_file+0x100/0x180)
[<c0256fe0>] (sysfs_write_file+0x100/0x180) from [<c01fec9c>] (vfs_write+0xbc/0x184)
[<c01fec9c>] (vfs_write+0xbc/0x184) from [<c01ff034>] (SyS_write+0x40/0x68)
[<c01ff034>] (SyS_write+0x40/0x68) from [<c010e200>] (ret_fast_syscall+0x0/0x48)
In gov_queue_work(), lock cpufreq_governor_lock before gov_queue_work,
and unlock it after __gov_queue_work(). In this way, governor_enabled
is guaranteed not changed in gov_queue_work().
Signed-off-by: Jane Li <jiel@marvell.com>
---
drivers/cpufreq/cpufreq.c | 2 +-
drivers/cpufreq/cpufreq_governor.c | 8 +++++++-
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 16d7b4a..185c9f5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -39,7 +39,7 @@ static struct cpufreq_driver *cpufreq_driver;
static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data_fallback);
static DEFINE_RWLOCK(cpufreq_driver_lock);
-static DEFINE_MUTEX(cpufreq_governor_lock);
+DEFINE_MUTEX(cpufreq_governor_lock);
static LIST_HEAD(cpufreq_policy_list);
#ifdef CONFIG_HOTPLUG_CPU
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index e6be635..485ee0d 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -22,6 +22,8 @@
#include "cpufreq_governor.h"
+extern struct mutex cpufreq_governor_lock;
+
static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
{
if (have_governor_per_policy())
@@ -119,8 +121,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
{
int i;
- if (!policy->governor_enabled)
+ mutex_lock(&cpufreq_governor_lock);
+ if (!policy->governor_enabled) {
+ mutex_unlock(&cpufreq_governor_lock);
return;
+ }
if (!all_cpus) {
/*
@@ -135,6 +140,7 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
for_each_cpu(i, policy->cpus)
__gov_queue_work(i, dbs_data, delay);
}
+ mutex_unlock(&cpufreq_governor_lock);
}
EXPORT_SYMBOL_GPL(gov_queue_work);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled
2014-01-02 3:14 [PATCH v2] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled jiel
@ 2014-01-02 5:34 ` Viresh Kumar
2014-01-02 23:26 ` Dmitry Torokhov
1 sibling, 0 replies; 4+ messages in thread
From: Viresh Kumar @ 2014-01-02 5:34 UTC (permalink / raw)
To: Jane Li
Cc: Rafael J. Wysocki, cpufreq@vger.kernel.org,
linux-pm@vger.kernel.org, Linux Kernel Mailing List
On 2 January 2014 08:44, <jiel@marvell.com> wrote:
> From: Jane Li <jiel@marvell.com>
>
> When a CPU is hot removed we'll cancel all the delayed work items via
> gov_cancel_work(). Sometimes the delayed work function determines that
> it should adjust the delay for all other CPUs that the policy is
> managing. If this scenario occurs, the canceling CPU will cancel its own
> work but queue up the other CPUs works to run.
>
> Commit 3617f2(cpufreq: Fix timer/workqueue corruption due to double
> queueing) has tried to fix this, but reading governor_enabled is not
> protected by cpufreq_governor_lock. Even though od_dbs_timer() checks
> governor_enabled before gov_queue_work(), this scenario may occur. For
> example:
>
> CPU0 CPU1
> ---- ----
> cpu_down()
> ... <work runs>
> __cpufreq_remove_dev() od_dbs_timer()
> __cpufreq_governor() policy->governor_enabled
> policy->governor_enabled = false;
> cpufreq_governor_dbs()
> case CPUFREQ_GOV_STOP:
> gov_cancel_work(dbs_data, policy);
> cpu0 work is canceled
> timer is canceled
> cpu1 work is canceled
> <waits for cpu1>
> gov_queue_work(*, *, true);
> cpu0 work queued
> cpu1 work queued
> cpu2 work queued
> ...
> cpu1 work is canceled
> cpu2 work is canceled
> ...
>
> At the end of the GOV_STOP case cpu0 still has a work queued to
> run although the code is expecting all of the works to be
> canceled. __cpufreq_remove_dev() will then proceed to
> re-initialize all the other CPUs works except for the CPU that is
> going down. The CPUFREQ_GOV_START case in cpufreq_governor_dbs()
> will trample over the queued work and debugobjects will spit out
> a warning:
>
> WARNING: at lib/debugobjects.c:260 debug_print_object+0x94/0xbc()
> ODEBUG: init active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x14
> Modules linked in:
> CPU: 1 PID: 1205 Comm: sh Tainted: G W 3.10.0 #200
> [<c01144f0>] (unwind_backtrace+0x0/0xf8) from [<c0111d98>] (show_stack+0x10/0x14)
> [<c0111d98>] (show_stack+0x10/0x14) from [<c01272cc>] (warn_slowpath_common+0x4c/0x68)
> [<c01272cc>] (warn_slowpath_common+0x4c/0x68) from [<c012737c>] (warn_slowpath_fmt+0x30/0x40)
> [<c012737c>] (warn_slowpath_fmt+0x30/0x40) from [<c034c640>] (debug_print_object+0x94/0xbc)
> [<c034c640>] (debug_print_object+0x94/0xbc) from [<c034c7f8>] (__debug_object_init+0xc8/0x3c0)
> [<c034c7f8>] (__debug_object_init+0xc8/0x3c0) from [<c01360e0>] (init_timer_key+0x20/0x104)
> [<c01360e0>] (init_timer_key+0x20/0x104) from [<c04872ac>] (cpufreq_governor_dbs+0x1dc/0x68c)
> [<c04872ac>] (cpufreq_governor_dbs+0x1dc/0x68c) from [<c04833a8>] (__cpufreq_governor+0x80/0x1b0)
> [<c04833a8>] (__cpufreq_governor+0x80/0x1b0) from [<c0483704>] (__cpufreq_remove_dev.isra.12+0x22c/0x380)
> [<c0483704>] (__cpufreq_remove_dev.isra.12+0x22c/0x380) from [<c0692f38>] (cpufreq_cpu_callback+0x48/0x5c)
> [<c0692f38>] (cpufreq_cpu_callback+0x48/0x5c) from [<c014fb40>] (notifier_call_chain+0x44/0x84)
> [<c014fb40>] (notifier_call_chain+0x44/0x84) from [<c012ae44>] (__cpu_notify+0x2c/0x48)
> [<c012ae44>] (__cpu_notify+0x2c/0x48) from [<c068dd40>] (_cpu_down+0x80/0x258)
> [<c068dd40>] (_cpu_down+0x80/0x258) from [<c068df40>] (cpu_down+0x28/0x3c)
> [<c068df40>] (cpu_down+0x28/0x3c) from [<c068e4c0>] (store_online+0x30/0x74)
> [<c068e4c0>] (store_online+0x30/0x74) from [<c03a7308>] (dev_attr_store+0x18/0x24)
> [<c03a7308>] (dev_attr_store+0x18/0x24) from [<c0256fe0>] (sysfs_write_file+0x100/0x180)
> [<c0256fe0>] (sysfs_write_file+0x100/0x180) from [<c01fec9c>] (vfs_write+0xbc/0x184)
> [<c01fec9c>] (vfs_write+0xbc/0x184) from [<c01ff034>] (SyS_write+0x40/0x68)
> [<c01ff034>] (SyS_write+0x40/0x68) from [<c010e200>] (ret_fast_syscall+0x0/0x48)
>
> In gov_queue_work(), lock cpufreq_governor_lock before gov_queue_work,
> and unlock it after __gov_queue_work(). In this way, governor_enabled
> is guaranteed not changed in gov_queue_work().
>
> Signed-off-by: Jane Li <jiel@marvell.com>
> ---
> drivers/cpufreq/cpufreq.c | 2 +-
> drivers/cpufreq/cpufreq_governor.c | 8 +++++++-
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 16d7b4a..185c9f5 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -39,7 +39,7 @@ static struct cpufreq_driver *cpufreq_driver;
> static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data_fallback);
> static DEFINE_RWLOCK(cpufreq_driver_lock);
> -static DEFINE_MUTEX(cpufreq_governor_lock);
> +DEFINE_MUTEX(cpufreq_governor_lock);
> static LIST_HEAD(cpufreq_policy_list);
>
> #ifdef CONFIG_HOTPLUG_CPU
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index e6be635..485ee0d 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -22,6 +22,8 @@
>
> #include "cpufreq_governor.h"
>
> +extern struct mutex cpufreq_governor_lock;
> +
> static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
> {
> if (have_governor_per_policy())
> @@ -119,8 +121,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
> {
> int i;
>
> - if (!policy->governor_enabled)
> + mutex_lock(&cpufreq_governor_lock);
> + if (!policy->governor_enabled) {
> + mutex_unlock(&cpufreq_governor_lock);
> return;
> + }
>
> if (!all_cpus) {
> /*
> @@ -135,6 +140,7 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
> for_each_cpu(i, policy->cpus)
> __gov_queue_work(i, dbs_data, delay);
> }
> + mutex_unlock(&cpufreq_governor_lock);
> }
> EXPORT_SYMBOL_GPL(gov_queue_work);
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled
2014-01-02 3:14 [PATCH v2] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled jiel
2014-01-02 5:34 ` Viresh Kumar
@ 2014-01-02 23:26 ` Dmitry Torokhov
2014-01-03 6:44 ` Jane Li
1 sibling, 1 reply; 4+ messages in thread
From: Dmitry Torokhov @ 2014-01-02 23:26 UTC (permalink / raw)
To: jiel; +Cc: rjw, viresh.kumar, cpufreq, linux-pm, linux-kernel
Hi Jane:
On Thu, Jan 02, 2014 at 11:14:42AM +0800, jiel@marvell.com wrote:
> @@ -119,8 +121,11 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
> {
> int i;
>
> - if (!policy->governor_enabled)
> + mutex_lock(&cpufreq_governor_lock);
> + if (!policy->governor_enabled) {
> + mutex_unlock(&cpufreq_governor_lock);
> return;
> + }
>
> if (!all_cpus) {
> /*
> @@ -135,6 +140,7 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
> for_each_cpu(i, policy->cpus)
> __gov_queue_work(i, dbs_data, delay);
> }
> + mutex_unlock(&cpufreq_governor_lock);
Unlocking in different branches is not the best practice IMO, I'd
recommend doing:
mutex_lock(&cpufreq_governor_lock);
if (!policy->governor_enabled)
goto out_unlock;
...
out_unlock:
mutex_unlock(&cpufreq_governor_lock);
Thanks!
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled
2014-01-02 23:26 ` Dmitry Torokhov
@ 2014-01-03 6:44 ` Jane Li
0 siblings, 0 replies; 4+ messages in thread
From: Jane Li @ 2014-01-03 6:44 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: rjw@rjwysocki.net, viresh.kumar@linaro.org,
cpufreq@vger.kernel.org, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org
On 01/03/2014 07:26 AM, Dmitry Torokhov wrote
> Unlocking in different branches is not the best practice IMO, I'd
> recommend doing:
>
> mutex_lock(&cpufreq_governor_lock);
>
> if (!policy->governor_enabled)
> goto out_unlock;
>
> ...
>
> out_unlock:
> mutex_unlock(&cpufreq_governor_lock);
>
> Thanks!
>
OK. I have pushed PATCH v3. Please review again.
Besides, I use checkpatch.pl to check this patch, and find there is warning. PATCH v3 also move cpufreq_governor_lock declaration to cpufreq.h.
WARNING: externs should be avoided in .c files
#106: FILE: drivers/cpufreq/cpufreq_governor.c:25:
+extern struct mutex cpufreq_governor_lock;
Thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-01-03 6:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-02 3:14 [PATCH v2] cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled jiel
2014-01-02 5:34 ` Viresh Kumar
2014-01-02 23:26 ` Dmitry Torokhov
2014-01-03 6:44 ` Jane Li
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).