Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1325484 > unrolled thread
| Started by | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| First post | 2016-02-03 15:10 +0100 |
| Last post | 2016-02-05 03:50 +0100 |
| Articles | 20 on this page of 23 — 7 participants |
Back to article view | Back to linux.kernel
[PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 15:10 +0100
[PATCH V2 4/7] Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT" Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 15:10 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Juri Lelli <juri.lelli@arm.com> - 2016-02-03 17:00 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 17:20 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Juri Lelli <juri.lelli@arm.com> - 2016-02-03 18:20 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 18:30 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Shilpa Bhat <shilpabhatppc@gmail.com> - 2016-02-04 00:40 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-04 01:00 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-04 07:00 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-04 12:10 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Saravana Kannan <skannan@codeaurora.org> - 2016-02-04 18:50 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Saravana Kannan <skannan@codeaurora.org> - 2016-02-04 18:50 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-04 19:20 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 03:50 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 05:00 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 10:50 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-08 03:20 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Saravana Kannan <skannan@codeaurora.org> - 2016-02-06 03:30 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-08 03:30 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-04 07:30 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-04 13:20 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> - 2016-02-04 22:00 +0100
Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 03:50 +0100
Page 1 of 2 [1] 2 Next page →
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-03 15:10 +0100 |
| Subject | [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups |
| Message-ID | <qY6i5-2YF-3@gated-at.bofh.it> |
Hi Rafael,
Here is the V2 with updated patches as suggested by you guys.
These are pushed here:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git cpufreq/governor-kobject
The first four patches are for 4.5, if possible and others you can keep
for 4.6.
V1->V2:
- Improved changelogs, thanks Rafael.
- Added new dbs_data->mutex to avoid concurrent updates to tunables.
- Moved kobj_type to common_dbs_data.
- Updated macros to static inline routines
- s/show/governor_show
- s/store/governor_store
- Improved comments
@Juri: More testing requested :)
Viresh Kumar (7):
cpufreq: governor: Treat min_sampling_rate as a governor-specific
tunable
cpufreq: governor: New sysfs show/store callbacks for governor
tunables
cpufreq: governor: Drop unused macros for creating governor tunable
attributes
Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT"
cpufreq: Merge cpufreq_offline_prepare/finish routines
cpufreq: Call __cpufreq_governor() with policy->rwsem held
cpufreq: Remove cpufreq_governor_lock
drivers/cpufreq/cpufreq.c | 93 +++++++++++++----------------
drivers/cpufreq/cpufreq_conservative.c | 79 +++++++++----------------
drivers/cpufreq/cpufreq_governor.c | 105 +++++++++++++++++++++++++--------
drivers/cpufreq/cpufreq_governor.h | 104 ++++++++------------------------
drivers/cpufreq/cpufreq_ondemand.c | 79 +++++++++----------------
include/linux/cpufreq.h | 4 --
6 files changed, 203 insertions(+), 261 deletions(-)
--
2.7.0.79.gdc08a19
[toc] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-03 15:10 +0100 |
| Subject | [PATCH V2 4/7] Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT" |
| Message-ID | <qY6i8-2YF-35@gated-at.bofh.it> |
| In reply to | #1325484 |
Earlier, when the struct freq-attr was used to represent governor
attributes, the standard cpufreq show/store sysfs attribute callbacks
were applied to the governor tunable attributes and they always acquire
the policy->rwsem lock before carrying out the operation. That could
have resulted in an ABBA deadlock if governor tunable attributes are
removed under policy->rwsem while one of them is being accessed
concurrently (if sysfs attributes removal wins the race, it will wait
for the access to complete with policy->rwsem held while the attribute
callback will block on policy->rwsem indefinitely).
We attempted to address this issue by dropping policy->rwsem around
governor tunable attributes removal (that is, around invocations of the
->governor callback with the event arg equal to CPUFREQ_GOV_POLICY_EXIT)
in cpufreq_set_policy(), but that opened up race conditions that had not
been possible with policy->rwsem held all the time.
The previous commit, "cpufreq: governor: New sysfs show/store callbacks
for governor tunables", fixed the original ABBA deadlock by adding new
governor specific show/store callbacks.
We don't have to drop rwsem around invocations of governor event
CPUFREQ_GOV_POLICY_EXIT anymore, and original fix can be reverted now.
Fixes: 955ef4833574 ("cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT")
Reported-by: Juri Lelli <juri.lelli@arm.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq.c | 5 -----
include/linux/cpufreq.h | 4 ----
2 files changed, 9 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e979ec78b695..5f7e24567e0e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2155,10 +2155,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
return ret;
}
- up_write(&policy->rwsem);
ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
- down_write(&policy->rwsem);
-
if (ret) {
pr_err("%s: Failed to Exit Governor: %s (%d)\n",
__func__, old_gov->name, ret);
@@ -2174,9 +2171,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
if (!ret)
goto out;
- up_write(&policy->rwsem);
__cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
- down_write(&policy->rwsem);
}
/* new governor failed, so re-start old one */
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 88a4215125bc..79b87cebaa9c 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -100,10 +100,6 @@ struct cpufreq_policy {
* - Any routine that will write to the policy structure and/or may take away
* the policy altogether (eg. CPU hotplug), will hold this lock in write
* mode before doing so.
- *
- * Additional rules:
- * - Lock should not be held across
- * __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
*/
struct rw_semaphore rwsem;
--
2.7.0.79.gdc08a19
[toc] | [prev] | [next] | [standalone]
| From | Juri Lelli <juri.lelli@arm.com> |
|---|---|
| Date | 2016-02-03 17:00 +0100 |
| Message-ID | <qY80y-3RT-9@gated-at.bofh.it> |
| In reply to | #1325484 |
Hi Viresh,
On 03/02/16 19:32, Viresh Kumar wrote:
> Hi Rafael,
>
> Here is the V2 with updated patches as suggested by you guys.
>
> These are pushed here:
> git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git cpufreq/governor-kobject
>
> The first four patches are for 4.5, if possible and others you can keep
> for 4.6.
>
> V1->V2:
> - Improved changelogs, thanks Rafael.
> - Added new dbs_data->mutex to avoid concurrent updates to tunables.
> - Moved kobj_type to common_dbs_data.
> - Updated macros to static inline routines
> - s/show/governor_show
> - s/store/governor_store
> - Improved comments
>
> @Juri: More testing requested :)
>
Ouch, I've just got this executing -f basic on Juno. :(
It happens with the hotplug_1_by_1 test.
[ 1086.531252] IRQ1 no longer affine to CPU1
[ 1086.531495] CPU1: shutdown
[ 1086.538199] psci: CPU1 killed.
[ 1086.583396]
[ 1086.584881] ======================================================
[ 1086.590999] [ INFO: possible circular locking dependency detected ]
[ 1086.597205] 4.5.0-rc2+ #37 Not tainted
[ 1086.600914] -------------------------------------------------------
[ 1086.607118] runme.sh/1052 is trying to acquire lock:
[ 1086.612031] (sb_writers#7){.+.+.+}, at: [<ffffffc000249500>] __sb_start_write+0xcc/0xe0
[ 1086.620090]
[ 1086.620090] but task is already holding lock:
[ 1086.625865] (&policy->rwsem){+++++.}, at: [<ffffffc0005c8ee4>] cpufreq_offline+0x7c/0x278
[ 1086.634081]
[ 1086.634081] which lock already depends on the new lock.
[ 1086.634081]
[ 1086.642180]
[ 1086.642180] the existing dependency chain (in reverse order) is:
[ 1086.649589]
-> #1 (&policy->rwsem){+++++.}:
[ 1086.653929] [<ffffffc00011d9a4>] check_prev_add+0x670/0x754
[ 1086.660060] [<ffffffc00011e1ac>] validate_chain.isra.36+0x724/0xa0c
[ 1086.666876] [<ffffffc00011f904>] __lock_acquire+0x4e4/0xba0
[ 1086.673001] [<ffffffc000120b58>] lock_release+0x244/0x570
[ 1086.678955] [<ffffffc0007351d0>] __mutex_unlock_slowpath+0xa0/0x18c
[ 1086.685771] [<ffffffc0007352dc>] mutex_unlock+0x20/0x2c
[ 1086.691553] [<ffffffc0002ccd24>] kernfs_fop_write+0xb0/0x194
[ 1086.697768] [<ffffffc00024478c>] __vfs_write+0x48/0x104
[ 1086.703550] [<ffffffc0002457a4>] vfs_write+0x98/0x198
[ 1086.709161] [<ffffffc0002465e4>] SyS_write+0x54/0xb0
[ 1086.714684] [<ffffffc000085d30>] el0_svc_naked+0x24/0x28
[ 1086.720555]
-> #0 (sb_writers#7){.+.+.+}:
[ 1086.724730] [<ffffffc00011c574>] print_circular_bug+0x80/0x2e4
[ 1086.731116] [<ffffffc00011d470>] check_prev_add+0x13c/0x754
[ 1086.737243] [<ffffffc00011e1ac>] validate_chain.isra.36+0x724/0xa0c
[ 1086.744059] [<ffffffc00011f904>] __lock_acquire+0x4e4/0xba0
[ 1086.750184] [<ffffffc0001207f4>] lock_acquire+0xe4/0x204
[ 1086.756052] [<ffffffc000118da0>] percpu_down_read+0x50/0xe4
[ 1086.762180] [<ffffffc000249500>] __sb_start_write+0xcc/0xe0
[ 1086.768306] [<ffffffc00026ae90>] mnt_want_write+0x28/0x54
[ 1086.774263] [<ffffffc0002555f8>] do_last+0x660/0xcb8
[ 1086.779788] [<ffffffc000255cdc>] path_openat+0x8c/0x2b0
[ 1086.785570] [<ffffffc000256fbc>] do_filp_open+0x78/0xf0
[ 1086.791353] [<ffffffc000244058>] do_sys_open+0x150/0x214
[ 1086.797222] [<ffffffc0002441a0>] SyS_openat+0x3c/0x48
[ 1086.802831] [<ffffffc000085d30>] el0_svc_naked+0x24/0x28
[ 1086.808700]
[ 1086.808700] other info that might help us debug this:
[ 1086.808700]
[ 1086.816627] Possible unsafe locking scenario:
[ 1086.816627]
[ 1086.822488] CPU0 CPU1
[ 1086.826971] ---- ----
[ 1086.831453] lock(&policy->rwsem);
[ 1086.834918] lock(sb_writers#7);
[ 1086.840713] lock(&policy->rwsem);
[ 1086.846671] lock(sb_writers#7);
[ 1086.849972]
[ 1086.849972] *** DEADLOCK ***
[ 1086.849972]
[ 1086.855836] 1 lock held by runme.sh/1052:
[ 1086.859802] #0: (&policy->rwsem){+++++.}, at: [<ffffffc0005c8ee4>] cpufreq_offline+0x7c/0x278
[ 1086.868453]
[ 1086.868453] stack backtrace:
[ 1086.872769] CPU: 5 PID: 1052 Comm: runme.sh Not tainted 4.5.0-rc2+ #37
[ 1086.879229] Hardware name: ARM Juno development board (r2) (DT)
[ 1086.885089] Call trace:
[ 1086.887511] [<ffffffc00008a788>] dump_backtrace+0x0/0x1f4
[ 1086.892858] [<ffffffc00008a99c>] show_stack+0x20/0x28
[ 1086.897861] [<ffffffc00041a380>] dump_stack+0x84/0xc0
[ 1086.902863] [<ffffffc00011c6c8>] print_circular_bug+0x1d4/0x2e4
[ 1086.908725] [<ffffffc00011d470>] check_prev_add+0x13c/0x754
[ 1086.914244] [<ffffffc00011e1ac>] validate_chain.isra.36+0x724/0xa0c
[ 1086.920448] [<ffffffc00011f904>] __lock_acquire+0x4e4/0xba0
[ 1086.925965] [<ffffffc0001207f4>] lock_acquire+0xe4/0x204
[ 1086.931224] [<ffffffc000118da0>] percpu_down_read+0x50/0xe4
[ 1086.936742] [<ffffffc000249500>] __sb_start_write+0xcc/0xe0
[ 1086.942260] [<ffffffc00026ae90>] mnt_want_write+0x28/0x54
[ 1086.947605] [<ffffffc0002555f8>] do_last+0x660/0xcb8
[ 1086.952520] [<ffffffc000255cdc>] path_openat+0x8c/0x2b0
[ 1086.957693] [<ffffffc000256fbc>] do_filp_open+0x78/0xf0
[ 1086.962865] [<ffffffc000244058>] do_sys_open+0x150/0x214
[ 1086.968123] [<ffffffc0002441a0>] SyS_openat+0x3c/0x48
[ 1086.973124] [<ffffffc000085d30>] el0_svc_naked+0x24/0x28
[ 1087.019315] Detected PIPT I-cache on CPU1
[ 1087.019373] CPU1: Booted secondary processor [410fd080]
Best,
- Juri
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-03 17:20 +0100 |
| Message-ID | <qY8jU-4ft-13@gated-at.bofh.it> |
| In reply to | #1325560 |
On 03-02-16, 15:54, Juri Lelli wrote:
> Ouch, I've just got this executing -f basic on Juno. :(
> It happens with the hotplug_1_by_1 test.
>
>
> [ 1086.531252] IRQ1 no longer affine to CPU1
> [ 1086.531495] CPU1: shutdown
> [ 1086.538199] psci: CPU1 killed.
> [ 1086.583396]
> [ 1086.584881] ======================================================
> [ 1086.590999] [ INFO: possible circular locking dependency detected ]
> [ 1086.597205] 4.5.0-rc2+ #37 Not tainted
> [ 1086.600914] -------------------------------------------------------
> [ 1086.607118] runme.sh/1052 is trying to acquire lock:
> [ 1086.612031] (sb_writers#7){.+.+.+}, at: [<ffffffc000249500>] __sb_start_write+0xcc/0xe0
> [ 1086.620090]
> [ 1086.620090] but task is already holding lock:
> [ 1086.625865] (&policy->rwsem){+++++.}, at: [<ffffffc0005c8ee4>] cpufreq_offline+0x7c/0x278
> [ 1086.634081]
> [ 1086.634081] which lock already depends on the new lock.
> [ 1086.634081]
> [ 1086.642180]
> [ 1086.642180] the existing dependency chain (in reverse order) is:
> [ 1086.649589]
> -> #1 (&policy->rwsem){+++++.}:
> [ 1086.653929] [<ffffffc00011d9a4>] check_prev_add+0x670/0x754
> [ 1086.660060] [<ffffffc00011e1ac>] validate_chain.isra.36+0x724/0xa0c
> [ 1086.666876] [<ffffffc00011f904>] __lock_acquire+0x4e4/0xba0
> [ 1086.673001] [<ffffffc000120b58>] lock_release+0x244/0x570
> [ 1086.678955] [<ffffffc0007351d0>] __mutex_unlock_slowpath+0xa0/0x18c
> [ 1086.685771] [<ffffffc0007352dc>] mutex_unlock+0x20/0x2c
> [ 1086.691553] [<ffffffc0002ccd24>] kernfs_fop_write+0xb0/0x194
> [ 1086.697768] [<ffffffc00024478c>] __vfs_write+0x48/0x104
> [ 1086.703550] [<ffffffc0002457a4>] vfs_write+0x98/0x198
> [ 1086.709161] [<ffffffc0002465e4>] SyS_write+0x54/0xb0
> [ 1086.714684] [<ffffffc000085d30>] el0_svc_naked+0x24/0x28
> [ 1086.720555]
> -> #0 (sb_writers#7){.+.+.+}:
> [ 1086.724730] [<ffffffc00011c574>] print_circular_bug+0x80/0x2e4
> [ 1086.731116] [<ffffffc00011d470>] check_prev_add+0x13c/0x754
> [ 1086.737243] [<ffffffc00011e1ac>] validate_chain.isra.36+0x724/0xa0c
> [ 1086.744059] [<ffffffc00011f904>] __lock_acquire+0x4e4/0xba0
> [ 1086.750184] [<ffffffc0001207f4>] lock_acquire+0xe4/0x204
> [ 1086.756052] [<ffffffc000118da0>] percpu_down_read+0x50/0xe4
> [ 1086.762180] [<ffffffc000249500>] __sb_start_write+0xcc/0xe0
> [ 1086.768306] [<ffffffc00026ae90>] mnt_want_write+0x28/0x54
> [ 1086.774263] [<ffffffc0002555f8>] do_last+0x660/0xcb8
> [ 1086.779788] [<ffffffc000255cdc>] path_openat+0x8c/0x2b0
> [ 1086.785570] [<ffffffc000256fbc>] do_filp_open+0x78/0xf0
> [ 1086.791353] [<ffffffc000244058>] do_sys_open+0x150/0x214
> [ 1086.797222] [<ffffffc0002441a0>] SyS_openat+0x3c/0x48
> [ 1086.802831] [<ffffffc000085d30>] el0_svc_naked+0x24/0x28
> [ 1086.808700]
> [ 1086.808700] other info that might help us debug this:
> [ 1086.808700]
> [ 1086.816627] Possible unsafe locking scenario:
> [ 1086.816627]
> [ 1086.822488] CPU0 CPU1
> [ 1086.826971] ---- ----
> [ 1086.831453] lock(&policy->rwsem);
> [ 1086.834918] lock(sb_writers#7);
> [ 1086.840713] lock(&policy->rwsem);
> [ 1086.846671] lock(sb_writers#7);
> [ 1086.849972]
> [ 1086.849972] *** DEADLOCK ***
> [ 1086.849972]
> [ 1086.855836] 1 lock held by runme.sh/1052:
> [ 1086.859802] #0: (&policy->rwsem){+++++.}, at: [<ffffffc0005c8ee4>] cpufreq_offline+0x7c/0x278
> [ 1086.868453]
> [ 1086.868453] stack backtrace:
> [ 1086.872769] CPU: 5 PID: 1052 Comm: runme.sh Not tainted 4.5.0-rc2+ #37
> [ 1086.879229] Hardware name: ARM Juno development board (r2) (DT)
> [ 1086.885089] Call trace:
> [ 1086.887511] [<ffffffc00008a788>] dump_backtrace+0x0/0x1f4
> [ 1086.892858] [<ffffffc00008a99c>] show_stack+0x20/0x28
> [ 1086.897861] [<ffffffc00041a380>] dump_stack+0x84/0xc0
> [ 1086.902863] [<ffffffc00011c6c8>] print_circular_bug+0x1d4/0x2e4
> [ 1086.908725] [<ffffffc00011d470>] check_prev_add+0x13c/0x754
> [ 1086.914244] [<ffffffc00011e1ac>] validate_chain.isra.36+0x724/0xa0c
> [ 1086.920448] [<ffffffc00011f904>] __lock_acquire+0x4e4/0xba0
> [ 1086.925965] [<ffffffc0001207f4>] lock_acquire+0xe4/0x204
> [ 1086.931224] [<ffffffc000118da0>] percpu_down_read+0x50/0xe4
> [ 1086.936742] [<ffffffc000249500>] __sb_start_write+0xcc/0xe0
> [ 1086.942260] [<ffffffc00026ae90>] mnt_want_write+0x28/0x54
> [ 1086.947605] [<ffffffc0002555f8>] do_last+0x660/0xcb8
> [ 1086.952520] [<ffffffc000255cdc>] path_openat+0x8c/0x2b0
> [ 1086.957693] [<ffffffc000256fbc>] do_filp_open+0x78/0xf0
> [ 1086.962865] [<ffffffc000244058>] do_sys_open+0x150/0x214
> [ 1086.968123] [<ffffffc0002441a0>] SyS_openat+0x3c/0x48
> [ 1086.973124] [<ffffffc000085d30>] el0_svc_naked+0x24/0x28
> [ 1087.019315] Detected PIPT I-cache on CPU1
> [ 1087.019373] CPU1: Booted secondary processor [410fd080]
Urg..
I failed to understand it for now though. Please test only the first 4
patches and leave the bottom three. AFAICT, this is caused by the 6th
patch.
The first 4 are important for 4.5 and must be tested soonish.
--
viresh
[toc] | [prev] | [next] | [standalone]
| From | Juri Lelli <juri.lelli@arm.com> |
|---|---|
| Date | 2016-02-03 18:20 +0100 |
| Message-ID | <qY9fZ-4Sx-21@gated-at.bofh.it> |
| In reply to | #1325598 |
On 03/02/16 21:40, Viresh Kumar wrote: > On 03-02-16, 15:54, Juri Lelli wrote: > > Ouch, I've just got this executing -f basic on Juno. :( > > It happens with the hotplug_1_by_1 test. > > [...] > > Urg.. > > I failed to understand it for now though. Please test only the first 4 > patches and leave the bottom three. AFAICT, this is caused by the 6th > patch. > > The first 4 are important for 4.5 and must be tested soonish. > First 4 look ok from a testing viewpoint. Best, - Juri
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-02-03 18:30 +0100 |
| Message-ID | <qY9pF-4WX-21@gated-at.bofh.it> |
| In reply to | #1325711 |
On Wed, Feb 3, 2016 at 6:20 PM, Juri Lelli <juri.lelli@arm.com> wrote: > On 03/02/16 21:40, Viresh Kumar wrote: >> On 03-02-16, 15:54, Juri Lelli wrote: >> > Ouch, I've just got this executing -f basic on Juno. :( >> > It happens with the hotplug_1_by_1 test. >> > > > [...] > >> >> Urg.. >> >> I failed to understand it for now though. Please test only the first 4 >> patches and leave the bottom three. AFAICT, this is caused by the 6th >> patch. >> >> The first 4 are important for 4.5 and must be tested soonish. >> > > First 4 look ok from a testing viewpoint. Good, thanks for the confirmation! I'm going to apply them and they will go to Linus next week. Thanks, Rafael
[toc] | [prev] | [next] | [standalone]
| From | Shilpa Bhat <shilpabhatppc@gmail.com> |
|---|---|
| Date | 2016-02-04 00:40 +0100 |
| Message-ID | <qYfbK-uu-73@gated-at.bofh.it> |
| In reply to | #1325724 |
Hi,
On 02/03/2016 10:50 PM, Rafael J. Wysocki wrote:
> On Wed, Feb 3, 2016 at 6:20 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>> On 03/02/16 21:40, Viresh Kumar wrote:
>>> On 03-02-16, 15:54, Juri Lelli wrote:
>>>> Ouch, I've just got this executing -f basic on Juno. :(
>>>> It happens with the hotplug_1_by_1 test.
>>>>
>>
>> [...]
>>
>>>
>>> Urg..
>>>
>>> I failed to understand it for now though. Please test only the first 4
>>> patches and leave the bottom three. AFAICT, this is caused by the 6th
>>> patch.
>>>
>>> The first 4 are important for 4.5 and must be tested soonish.
>>>
>>
>> First 4 look ok from a testing viewpoint.
>
> Good, thanks for the confirmation!
>
> I'm going to apply them and they will go to Linus next week.
>
> Thanks,
> Rafael
Sorry for the delayed report. But I see the below backtrace on Power8 box. It
has 4 chips with 128 cpus.
I see the below trace with the first four patches on running tests
from Viresh's testcase.
'./runme.sh -f basic'
hit this trace at 'shuffle_governors_for_all_cpus' test.
[ 906.762045] ======================================================
[ 906.762114] [ INFO: possible circular locking dependency detected ]
[ 906.762172] 4.5.0-rc2-sgb+ #96 Not tainted
[ 906.762207] -------------------------------------------------------
[ 906.762263] runme.sh/2840 is trying to acquire lock:
[ 906.762309] (s_active#91){++++.+}, at: [<c000000000407db8>]
kernfs_remove+0x48/0x70
[ 906.762419]
but task is already holding lock:
[ 906.762476] (od_dbs_cdata.mutex){+.+.+.}, at: [<c000000000ad7594>]
cpufreq_governor_dbs+0x64/0x7e0
[ 906.762592]
which lock already depends on the new lock.
[ 906.762659]
the existing dependency chain (in reverse order) is:
[ 906.762727]
-> #2 (od_dbs_cdata.mutex){+.+.+.}:
[ 906.762807] [<c000000000d485b0>] mutex_lock_nested+0x90/0x590
[ 906.762877] [<c000000000ad57f8>] update_sampling_rate+0x88/0x1c0
[ 906.762946] [<c000000000ad5990>] store_sampling_rate+0x60/0xa0
[ 906.763013] [<c000000000ad6af0>] governor_store+0x80/0xc0
[ 906.763070] [<c00000000040a8a4>] sysfs_kf_write+0x94/0xc0
[ 906.763128] [<c0000000004094a8>] kernfs_fop_write+0x188/0x1f0
[ 906.763196] [<c000000000347b8c>] __vfs_write+0x6c/0x180
[ 906.763254] [<c0000000003490a0>] vfs_write+0xc0/0x200
[ 906.763311] [<c00000000034a3cc>] SyS_write+0x6c/0x110
[ 906.763369] [<c00000000000926c>] system_call+0x38/0xd0
[ 906.763427]
-> #1 (&dbs_data->mutex){+.+...}:
[ 906.763495] [<c000000000d485b0>] mutex_lock_nested+0x90/0x590
[ 906.763563] [<c000000000ad6ac0>] governor_store+0x50/0xc0
[ 906.763620] [<c00000000040a8a4>] sysfs_kf_write+0x94/0xc0
[ 906.763677] [<c0000000004094a8>] kernfs_fop_write+0x188/0x1f0
[ 906.763745] [<c000000000347b8c>] __vfs_write+0x6c/0x180
[ 906.763801] [<c0000000003490a0>] vfs_write+0xc0/0x200
[ 906.763859] [<c00000000034a3cc>] SyS_write+0x6c/0x110
[ 906.763916] [<c00000000000926c>] system_call+0x38/0xd0
[ 906.763973]
-> #0 (s_active#91){++++.+}:
[ 906.764052] [<c00000000015f318>] lock_acquire+0xd8/0x1a0
[ 906.764111] [<c0000000004065f4>] __kernfs_remove+0x344/0x410
[ 906.764179] [<c000000000407db8>] kernfs_remove+0x48/0x70
[ 906.764236] [<c00000000040b868>] sysfs_remove_dir+0x78/0xd0
[ 906.764304] [<c0000000005eccec>] kobject_del+0x2c/0x80
[ 906.764362] [<c0000000005ec9e8>] kobject_release+0xa8/0x250
[ 906.764430] [<c000000000ad7c28>] cpufreq_governor_dbs+0x6f8/0x7e0
[ 906.764497] [<c000000000ad4bdc>] od_cpufreq_governor_dbs+0x3c/0x60
[ 906.764567] [<c000000000acf830>] __cpufreq_governor+0x1d0/0x390
[ 906.764634] [<c000000000ad0750>] cpufreq_set_policy+0x3b0/0x450
[ 906.764703] [<c000000000ad12cc>] store_scaling_governor+0x8c/0xf0
[ 906.764771] [<c000000000aced34>] store+0xb4/0x110
[ 906.764828] [<c00000000040a8a4>] sysfs_kf_write+0x94/0xc0
[ 906.764885] [<c0000000004094a8>] kernfs_fop_write+0x188/0x1f0
[ 906.764952] [<c000000000347b8c>] __vfs_write+0x6c/0x180
[ 906.765048] [<c0000000003490a0>] vfs_write+0xc0/0x200
[ 906.765160] [<c00000000034a3cc>] SyS_write+0x6c/0x110
[ 906.765272] [<c00000000000926c>] system_call+0x38/0xd0
[ 906.765384]
other info that might help us debug this:
[ 906.765522] Chain exists of:
s_active#91 --> &dbs_data->mutex --> od_dbs_cdata.mutex
[ 906.765768] Possible unsafe locking scenario:
[ 906.765880] CPU0 CPU1
[ 906.765969] ---- ----
[ 906.766058] lock(od_dbs_cdata.mutex);
[ 906.766170] lock(&dbs_data->mutex);
[ 906.766304] lock(od_dbs_cdata.mutex);
[ 906.766461] lock(s_active#91);
[ 906.766572]
*** DEADLOCK ***
[ 906.766686] 6 locks held by runme.sh/2840:
[ 906.766756] #0: (sb_writers#6){.+.+.+}, at: [<c00000000034cf10>]
__sb_start_write+0x120/0x150
[ 906.767002] #1: (&of->mutex){+.+.+.}, at: [<c00000000040939c>]
kernfs_fop_write+0x7c/0x1f0
[ 906.767225] #2: (s_active#82){.+.+.+}, at: [<c0000000004093a8>]
kernfs_fop_write+0x88/0x1f0
[ 906.767471] #3: (cpu_hotplug.lock){++++++}, at: [<c0000000000e06d8>]
get_online_cpus+0x48/0xc0
[ 906.767676] #4: (&policy->rwsem){+++++.}, at: [<c000000000aced04>]
store+0x84/0x110
[ 906.767878] #5: (od_dbs_cdata.mutex){+.+.+.}, at: [<c000000000ad7594>]
cpufreq_governor_dbs+0x64/0x7e0
[ 906.768124]
stack backtrace:
[ 906.768215] CPU: 0 PID: 2840 Comm: runme.sh Not tainted 4.5.0-rc2-sgb+ #96
[ 906.768329] Call Trace:
[ 906.768375] [c000007fe3126ec0] [c000000000d56530] dump_stack+0x90/0xbc
(unreliable)
[ 906.768536] [c000007fe3126ef0] [c00000000015884c]
print_circular_bug+0x28c/0x3e0
[ 906.768696] [c000007fe3126f90] [c00000000015ed88]
__lock_acquire+0x2278/0x22d0
[ 906.768853] [c000007fe3127120] [c00000000015f318] lock_acquire+0xd8/0x1a0
[ 906.768987] [c000007fe31271e0] [c0000000004065f4] __kernfs_remove+0x344/0x410
[ 906.769121] [c000007fe31272e0] [c000000000407db8] kernfs_remove+0x48/0x70
[ 906.769256] [c000007fe3127310] [c00000000040b868] sysfs_remove_dir+0x78/0xd0
[ 906.769394] [c000007fe3127350] [c0000000005eccec] kobject_del+0x2c/0x80
[ 906.769528] [c000007fe3127380] [c0000000005ec9e8] kobject_release+0xa8/0x250
[ 906.769607] [c000007fe3127410] [c000000000ad7c28]
cpufreq_governor_dbs+0x6f8/0x7e0
[ 906.769687] [c000007fe31274c0] [c000000000ad4bdc]
od_cpufreq_governor_dbs+0x3c/0x60
[ 906.769766] [c000007fe3127500] [c000000000acf830]
__cpufreq_governor+0x1d0/0x390
[ 906.769845] [c000007fe3127580] [c000000000ad0750]
cpufreq_set_policy+0x3b0/0x450
[ 906.769924] [c000007fe3127610] [c000000000ad12cc]
store_scaling_governor+0x8c/0xf0
[ 906.770003] [c000007fe3127c10] [c000000000aced34] store+0xb4/0x110
[ 906.770071] [c000007fe3127c60] [c00000000040a8a4] sysfs_kf_write+0x94/0xc0
[ 906.770139] [c000007fe3127ca0] [c0000000004094a8]
kernfs_fop_write+0x188/0x1f0
[ 906.770221] [c000007fe3127cf0] [c000000000347b8c] __vfs_write+0x6c/0x180
[ 906.770290] [c000007fe3127d90] [c0000000003490a0] vfs_write+0xc0/0x200
[ 906.770358] [c000007fe3127de0] [c00000000034a3cc] SyS_write+0x6c/0x110
[ 906.770426] [c000007fe3127e30] [c00000000000926c] system_call+0x38/0xd0
Thanks and Regards,
Shilpa
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-02-04 01:00 +0100 |
| Message-ID | <qYfv4-Cf-19@gated-at.bofh.it> |
| In reply to | #1326233 |
On Thu, Feb 4, 2016 at 12:31 AM, Shilpa Bhat <shilpabhatppc@gmail.com> wrote:
> Hi,
>
> On 02/03/2016 10:50 PM, Rafael J. Wysocki wrote:
>> On Wed, Feb 3, 2016 at 6:20 PM, Juri Lelli <juri.lelli@arm.com> wrote:
>>> On 03/02/16 21:40, Viresh Kumar wrote:
>>>> On 03-02-16, 15:54, Juri Lelli wrote:
>>>>> Ouch, I've just got this executing -f basic on Juno. :(
>>>>> It happens with the hotplug_1_by_1 test.
>>>>>
>>>
>>> [...]
>>>
>>>>
>>>> Urg..
>>>>
>>>> I failed to understand it for now though. Please test only the first 4
>>>> patches and leave the bottom three. AFAICT, this is caused by the 6th
>>>> patch.
>>>>
>>>> The first 4 are important for 4.5 and must be tested soonish.
>>>>
>>>
>>> First 4 look ok from a testing viewpoint.
>>
>> Good, thanks for the confirmation!
>>
>> I'm going to apply them and they will go to Linus next week.
>>
>> Thanks,
>> Rafael
>
> Sorry for the delayed report. But I see the below backtrace on Power8 box. It
> has 4 chips with 128 cpus.
Thanks for the report.
> I see the below trace with the first four patches on running tests
> from Viresh's testcase.
> './runme.sh -f basic'
> hit this trace at 'shuffle_governors_for_all_cpus' test.
>
> [ 906.762045] ======================================================
> [ 906.762114] [ INFO: possible circular locking dependency detected ]
> [ 906.762172] 4.5.0-rc2-sgb+ #96 Not tainted
> [ 906.762207] -------------------------------------------------------
> [ 906.762263] runme.sh/2840 is trying to acquire lock:
> [ 906.762309] (s_active#91){++++.+}, at: [<c000000000407db8>]
> kernfs_remove+0x48/0x70
> [ 906.762419]
> but task is already holding lock:
> [ 906.762476] (od_dbs_cdata.mutex){+.+.+.}, at: [<c000000000ad7594>]
> cpufreq_governor_dbs+0x64/0x7e0
> [ 906.762592]
> which lock already depends on the new lock.
>
> [ 906.762659]
> the existing dependency chain (in reverse order) is:
> [ 906.762727]
> -> #2 (od_dbs_cdata.mutex){+.+.+.}:
> [ 906.762807] [<c000000000d485b0>] mutex_lock_nested+0x90/0x590
> [ 906.762877] [<c000000000ad57f8>] update_sampling_rate+0x88/0x1c0
> [ 906.762946] [<c000000000ad5990>] store_sampling_rate+0x60/0xa0
> [ 906.763013] [<c000000000ad6af0>] governor_store+0x80/0xc0
> [ 906.763070] [<c00000000040a8a4>] sysfs_kf_write+0x94/0xc0
> [ 906.763128] [<c0000000004094a8>] kernfs_fop_write+0x188/0x1f0
> [ 906.763196] [<c000000000347b8c>] __vfs_write+0x6c/0x180
> [ 906.763254] [<c0000000003490a0>] vfs_write+0xc0/0x200
> [ 906.763311] [<c00000000034a3cc>] SyS_write+0x6c/0x110
> [ 906.763369] [<c00000000000926c>] system_call+0x38/0xd0
> [ 906.763427]
> -> #1 (&dbs_data->mutex){+.+...}:
> [ 906.763495] [<c000000000d485b0>] mutex_lock_nested+0x90/0x590
> [ 906.763563] [<c000000000ad6ac0>] governor_store+0x50/0xc0
> [ 906.763620] [<c00000000040a8a4>] sysfs_kf_write+0x94/0xc0
> [ 906.763677] [<c0000000004094a8>] kernfs_fop_write+0x188/0x1f0
> [ 906.763745] [<c000000000347b8c>] __vfs_write+0x6c/0x180
> [ 906.763801] [<c0000000003490a0>] vfs_write+0xc0/0x200
> [ 906.763859] [<c00000000034a3cc>] SyS_write+0x6c/0x110
> [ 906.763916] [<c00000000000926c>] system_call+0x38/0xd0
> [ 906.763973]
> -> #0 (s_active#91){++++.+}:
> [ 906.764052] [<c00000000015f318>] lock_acquire+0xd8/0x1a0
> [ 906.764111] [<c0000000004065f4>] __kernfs_remove+0x344/0x410
> [ 906.764179] [<c000000000407db8>] kernfs_remove+0x48/0x70
> [ 906.764236] [<c00000000040b868>] sysfs_remove_dir+0x78/0xd0
> [ 906.764304] [<c0000000005eccec>] kobject_del+0x2c/0x80
> [ 906.764362] [<c0000000005ec9e8>] kobject_release+0xa8/0x250
> [ 906.764430] [<c000000000ad7c28>] cpufreq_governor_dbs+0x6f8/0x7e0
> [ 906.764497] [<c000000000ad4bdc>] od_cpufreq_governor_dbs+0x3c/0x60
> [ 906.764567] [<c000000000acf830>] __cpufreq_governor+0x1d0/0x390
> [ 906.764634] [<c000000000ad0750>] cpufreq_set_policy+0x3b0/0x450
> [ 906.764703] [<c000000000ad12cc>] store_scaling_governor+0x8c/0xf0
> [ 906.764771] [<c000000000aced34>] store+0xb4/0x110
> [ 906.764828] [<c00000000040a8a4>] sysfs_kf_write+0x94/0xc0
> [ 906.764885] [<c0000000004094a8>] kernfs_fop_write+0x188/0x1f0
> [ 906.764952] [<c000000000347b8c>] __vfs_write+0x6c/0x180
> [ 906.765048] [<c0000000003490a0>] vfs_write+0xc0/0x200
> [ 906.765160] [<c00000000034a3cc>] SyS_write+0x6c/0x110
> [ 906.765272] [<c00000000000926c>] system_call+0x38/0xd0
> [ 906.765384]
> other info that might help us debug this:
>
> [ 906.765522] Chain exists of:
> s_active#91 --> &dbs_data->mutex --> od_dbs_cdata.mutex
>
> [ 906.765768] Possible unsafe locking scenario:
>
> [ 906.765880] CPU0 CPU1
> [ 906.765969] ---- ----
> [ 906.766058] lock(od_dbs_cdata.mutex);
> [ 906.766170] lock(&dbs_data->mutex);
> [ 906.766304] lock(od_dbs_cdata.mutex);
> [ 906.766461] lock(s_active#91);
> [ 906.766572]
> *** DEADLOCK ***
This is exactly right. We've avoided one deadlock only to trip into
another one.
This happens because update_sampling_rate() acquires
od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by
cpufreq_governor_dbs().
Worse yet, a deadlock can still happen without (the new)
dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if
update_sampling_rate() runs in parallel with
cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins
the race.
It looks like we need to drop the governor mutex before putting the
kobject in cpufreq_governor_exit().
Thanks,
Rafael
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-04 07:00 +0100 |
| Message-ID | <qYl7u-4t9-31@gated-at.bofh.it> |
| In reply to | #1326276 |
On 04-02-16, 00:50, Rafael J. Wysocki wrote:
> On Thu, Feb 4, 2016 at 12:31 AM, Shilpa Bhat <shilpabhatppc@gmail.com> wrote:
> > Sorry for the delayed report. But I see the below backtrace on Power8 box. It
> > has 4 chips with 128 cpus.
Honestly, I wasn't expecting you to test this stuff, but I really
appreciate you doing that.
Thanks a lot ..
> > [ 906.765768] Possible unsafe locking scenario:
> >
> > [ 906.765880] CPU0 CPU1
> > [ 906.765969] ---- ----
This race scenario is perhaps incomplete and difficult to understand
without below lines:
Governor's EXIT Update sampling rate from sysfs
lock(s_active#91);
> > [ 906.766058] lock(od_dbs_cdata.mutex);
> > [ 906.766170] lock(&dbs_data->mutex);
> > [ 906.766304] lock(od_dbs_cdata.mutex);
> > [ 906.766461] lock(s_active#91);
> > [ 906.766572]
> > *** DEADLOCK ***
>
> This is exactly right. We've avoided one deadlock only to trip into
> another one.
As we discussed on IRC, we haven't introduced this deadlock with the
current series. But this is what Juri has reported some days back,
while he tested linus/master on TC2.
> This happens because update_sampling_rate() acquires
> od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by
> cpufreq_governor_dbs().
>
> Worse yet, a deadlock can still happen without (the new)
> dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if
> update_sampling_rate() runs in parallel with
> cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins
> the race.
>
> It looks like we need to drop the governor mutex before putting the
> kobject in cpufreq_governor_exit().
That wouldn't be trivial to implement as we discussed.
Okay, here is a proposal for the current series and the series's you
have post Rafael:
- Firstly, I would like to clarify that I don't have any issues with
rebasing on top of your series, it should be easy enough.
- One thing is for sure that nothing from these 3 series's is getting
merged in 4.5, as we aren't fixing the real issue Shilpa/Juril have
reported.
- I think the first 4 patches here are just fine and don't need any
updates. They actually do the right thing and makes code so much
cleaner.
- So, can we apply the first 4 patches (which you have already
applied to bleeding-edge) now and do all work on top of that ?
Again, I can rebase if you merge your patches first, no issues at all
:)
--
viresh
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-04 12:10 +0100 |
| Message-ID | <qYpXs-7NG-23@gated-at.bofh.it> |
| In reply to | #1326276 |
On 04-02-16, 00:50, Rafael J. Wysocki wrote:
> This is exactly right. We've avoided one deadlock only to trip into
> another one.
>
> This happens because update_sampling_rate() acquires
> od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by
> cpufreq_governor_dbs().
>
> Worse yet, a deadlock can still happen without (the new)
> dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if
> update_sampling_rate() runs in parallel with
> cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins
> the race.
>
> It looks like we need to drop the governor mutex before putting the
> kobject in cpufreq_governor_exit().
I have tried to explore all possible ways of fixing this, and every
other way looked to be racy in some way.
Does anyone else have a better idea (untested):
-------------------------8<-------------------------
Subject: [PATCH] cpufreq: ondemand: Shoot update_sampling_rate with a separate
work
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq_governor.h | 2 ++
drivers/cpufreq/cpufreq_ondemand.c | 39 +++++++++++++++++++++++++++++---------
2 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 7bed63e14e7d..97e604356b20 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -141,6 +141,8 @@ struct od_dbs_tuners {
unsigned int powersave_bias;
unsigned int io_is_busy;
unsigned int min_sampling_rate;
+ struct work_struct work;
+ struct dbs_data *dbs_data;
};
struct cs_dbs_tuners {
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 82ed490f7de0..93ad7a226aee 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -242,20 +242,27 @@ static struct common_dbs_data od_dbs_cdata;
* reducing the sampling rate, we need to make the new value effective
* immediately.
*/
-static void update_sampling_rate(struct dbs_data *dbs_data,
- unsigned int new_rate)
+static void update_sampling_rate(struct work_struct *work)
{
- struct od_dbs_tuners *od_tuners = dbs_data->tuners;
+ struct od_dbs_tuners *od_tuners = container_of(work, struct
+ od_dbs_tuners, work);
+ unsigned int new_rate = od_tuners->sampling_rate;
+ struct dbs_data *dbs_data = od_tuners->dbs_data;
struct cpumask cpumask;
int cpu;
- od_tuners->sampling_rate = new_rate = max(new_rate,
- od_tuners->min_sampling_rate);
-
/*
* Lock governor so that governor start/stop can't execute in parallel.
+ *
+ * We can't do a regular mutex_lock() here, as that may deadlock against
+ * another thread performing CPUFREQ_GOV_POLICY_EXIT event on the
+ * governor, which might have already taken od_dbs_cdata.mutex and is
+ * waiting for this work to finish.
*/
- mutex_lock(&od_dbs_cdata.mutex);
+ if (!mutex_trylock(&od_dbs_cdata.mutex)) {
+ queue_work(system_wq, &od_tuners->work);
+ return;
+ }
cpumask_copy(&cpumask, cpu_online_mask);
@@ -311,13 +318,22 @@ static void update_sampling_rate(struct dbs_data *dbs_data,
static ssize_t store_sampling_rate(struct dbs_data *dbs_data, const char *buf,
size_t count)
{
+ struct od_dbs_tuners *od_tuners = dbs_data->tuners;
unsigned int input;
int ret;
ret = sscanf(buf, "%u", &input);
if (ret != 1)
return -EINVAL;
- update_sampling_rate(dbs_data, input);
+ od_tuners->sampling_rate = max(input, od_tuners->min_sampling_rate);
+
+ /*
+ * update_sampling_rate() requires to hold od_dbs_cdata.mutex, but we
+ * can't take that from this thread, otherwise it results in ABBA
+ * lockdep between s_active and od_dbs_cdata.mutex locks.
+ */
+ queue_work(system_wq, &od_tuners->work);
+
return count;
}
@@ -501,6 +517,8 @@ static int od_init(struct dbs_data *dbs_data, bool notify)
tuners->ignore_nice_load = 0;
tuners->powersave_bias = default_powersave_bias;
tuners->io_is_busy = should_io_be_busy();
+ INIT_WORK(&tuners->work, update_sampling_rate);
+ tuners->dbs_data = dbs_data;
dbs_data->tuners = tuners;
return 0;
@@ -508,7 +526,10 @@ static int od_init(struct dbs_data *dbs_data, bool notify)
static void od_exit(struct dbs_data *dbs_data, bool notify)
{
- kfree(dbs_data->tuners);
+ struct od_dbs_tuners *tuners = dbs_data->tuners;
+
+ cancel_work_sync(&tuners->work);
+ kfree(tuners);
}
define_get_cpu_dbs_routines(od_cpu_dbs_info);
[toc] | [prev] | [next] | [standalone]
| From | Saravana Kannan <skannan@codeaurora.org> |
|---|---|
| Date | 2016-02-04 18:50 +0100 |
| Message-ID | <qYwcx-5aH-7@gated-at.bofh.it> |
| In reply to | #1326673 |
On 02/04/2016 03:09 AM, Viresh Kumar wrote:
> On 04-02-16, 00:50, Rafael J. Wysocki wrote:
>> This is exactly right. We've avoided one deadlock only to trip into
>> another one.
>>
>> This happens because update_sampling_rate() acquires
>> od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by
>> cpufreq_governor_dbs().
>>
>> Worse yet, a deadlock can still happen without (the new)
>> dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if
>> update_sampling_rate() runs in parallel with
>> cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins
>> the race.
>>
>> It looks like we need to drop the governor mutex before putting the
>> kobject in cpufreq_governor_exit().
>
> I have tried to explore all possible ways of fixing this, and every
> other way looked to be racy in some way.
>
> Does anyone else have a better idea (untested):
>
> -------------------------8<-------------------------
>
> Subject: [PATCH] cpufreq: ondemand: Shoot update_sampling_rate with a separate
> work
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/cpufreq/cpufreq_governor.h | 2 ++
> drivers/cpufreq/cpufreq_ondemand.c | 39 +++++++++++++++++++++++++++++---------
> 2 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index 7bed63e14e7d..97e604356b20 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -141,6 +141,8 @@ struct od_dbs_tuners {
> unsigned int powersave_bias;
> unsigned int io_is_busy;
> unsigned int min_sampling_rate;
> + struct work_struct work;
> + struct dbs_data *dbs_data;
> };
>
> struct cs_dbs_tuners {
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 82ed490f7de0..93ad7a226aee 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -242,20 +242,27 @@ static struct common_dbs_data od_dbs_cdata;
> * reducing the sampling rate, we need to make the new value effective
> * immediately.
> */
> -static void update_sampling_rate(struct dbs_data *dbs_data,
> - unsigned int new_rate)
> +static void update_sampling_rate(struct work_struct *work)
> {
> - struct od_dbs_tuners *od_tuners = dbs_data->tuners;
> + struct od_dbs_tuners *od_tuners = container_of(work, struct
> + od_dbs_tuners, work);
> + unsigned int new_rate = od_tuners->sampling_rate;
> + struct dbs_data *dbs_data = od_tuners->dbs_data;
> struct cpumask cpumask;
> int cpu;
>
> - od_tuners->sampling_rate = new_rate = max(new_rate,
> - od_tuners->min_sampling_rate);
> -
> /*
> * Lock governor so that governor start/stop can't execute in parallel.
> + *
> + * We can't do a regular mutex_lock() here, as that may deadlock against
> + * another thread performing CPUFREQ_GOV_POLICY_EXIT event on the
> + * governor, which might have already taken od_dbs_cdata.mutex and is
> + * waiting for this work to finish.
> */
> - mutex_lock(&od_dbs_cdata.mutex);
> + if (!mutex_trylock(&od_dbs_cdata.mutex)) {
> + queue_work(system_wq, &od_tuners->work);
> + return;
> + }
>
> cpumask_copy(&cpumask, cpu_online_mask);
>
> @@ -311,13 +318,22 @@ static void update_sampling_rate(struct dbs_data *dbs_data,
> static ssize_t store_sampling_rate(struct dbs_data *dbs_data, const char *buf,
> size_t count)
> {
> + struct od_dbs_tuners *od_tuners = dbs_data->tuners;
> unsigned int input;
> int ret;
> ret = sscanf(buf, "%u", &input);
> if (ret != 1)
> return -EINVAL;
>
> - update_sampling_rate(dbs_data, input);
> + od_tuners->sampling_rate = max(input, od_tuners->min_sampling_rate);
> +
> + /*
> + * update_sampling_rate() requires to hold od_dbs_cdata.mutex, but we
> + * can't take that from this thread, otherwise it results in ABBA
> + * lockdep between s_active and od_dbs_cdata.mutex locks.
> + */
> + queue_work(system_wq, &od_tuners->work);
> +
> return count;
> }
>
> @@ -501,6 +517,8 @@ static int od_init(struct dbs_data *dbs_data, bool notify)
> tuners->ignore_nice_load = 0;
> tuners->powersave_bias = default_powersave_bias;
> tuners->io_is_busy = should_io_be_busy();
> + INIT_WORK(&tuners->work, update_sampling_rate);
> + tuners->dbs_data = dbs_data;
>
> dbs_data->tuners = tuners;
> return 0;
> @@ -508,7 +526,10 @@ static int od_init(struct dbs_data *dbs_data, bool notify)
>
> static void od_exit(struct dbs_data *dbs_data, bool notify)
> {
> - kfree(dbs_data->tuners);
> + struct od_dbs_tuners *tuners = dbs_data->tuners;
> +
> + cancel_work_sync(&tuners->work);
> + kfree(tuners);
> }
>
> define_get_cpu_dbs_routines(od_cpu_dbs_info);
>
No no no no! Let's not open up this can of worms of queuing up the work
to handle a write to a sysfs file. It *MIGHT* work for this specific
tunable (I haven't bothered to analyze), but this makes it impossible to
return a useful/proper error value.
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Saravana Kannan <skannan@codeaurora.org> |
|---|---|
| Date | 2016-02-04 18:50 +0100 |
| Message-ID | <qYwcy-5aH-25@gated-at.bofh.it> |
| In reply to | #1327070 |
On 02/04/2016 09:43 AM, Saravana Kannan wrote:
> On 02/04/2016 03:09 AM, Viresh Kumar wrote:
>> On 04-02-16, 00:50, Rafael J. Wysocki wrote:
>>> This is exactly right. We've avoided one deadlock only to trip into
>>> another one.
>>>
>>> This happens because update_sampling_rate() acquires
>>> od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by
>>> cpufreq_governor_dbs().
>>>
>>> Worse yet, a deadlock can still happen without (the new)
>>> dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if
>>> update_sampling_rate() runs in parallel with
>>> cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins
>>> the race.
>>>
>>> It looks like we need to drop the governor mutex before putting the
>>> kobject in cpufreq_governor_exit().
>>
>> I have tried to explore all possible ways of fixing this, and every
>> other way looked to be racy in some way.
>>
>> Does anyone else have a better idea (untested):
>>
>> -------------------------8<-------------------------
>>
>> Subject: [PATCH] cpufreq: ondemand: Shoot update_sampling_rate with a
>> separate
>> work
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>> drivers/cpufreq/cpufreq_governor.h | 2 ++
>> drivers/cpufreq/cpufreq_ondemand.c | 39
>> +++++++++++++++++++++++++++++---------
>> 2 files changed, 32 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq_governor.h
>> b/drivers/cpufreq/cpufreq_governor.h
>> index 7bed63e14e7d..97e604356b20 100644
>> --- a/drivers/cpufreq/cpufreq_governor.h
>> +++ b/drivers/cpufreq/cpufreq_governor.h
>> @@ -141,6 +141,8 @@ struct od_dbs_tuners {
>> unsigned int powersave_bias;
>> unsigned int io_is_busy;
>> unsigned int min_sampling_rate;
>> + struct work_struct work;
>> + struct dbs_data *dbs_data;
>> };
>>
>> struct cs_dbs_tuners {
>> diff --git a/drivers/cpufreq/cpufreq_ondemand.c
>> b/drivers/cpufreq/cpufreq_ondemand.c
>> index 82ed490f7de0..93ad7a226aee 100644
>> --- a/drivers/cpufreq/cpufreq_ondemand.c
>> +++ b/drivers/cpufreq/cpufreq_ondemand.c
>> @@ -242,20 +242,27 @@ static struct common_dbs_data od_dbs_cdata;
>> * reducing the sampling rate, we need to make the new value effective
>> * immediately.
>> */
>> -static void update_sampling_rate(struct dbs_data *dbs_data,
>> - unsigned int new_rate)
>> +static void update_sampling_rate(struct work_struct *work)
>> {
>> - struct od_dbs_tuners *od_tuners = dbs_data->tuners;
>> + struct od_dbs_tuners *od_tuners = container_of(work, struct
>> + od_dbs_tuners, work);
>> + unsigned int new_rate = od_tuners->sampling_rate;
>> + struct dbs_data *dbs_data = od_tuners->dbs_data;
>> struct cpumask cpumask;
>> int cpu;
>>
>> - od_tuners->sampling_rate = new_rate = max(new_rate,
>> - od_tuners->min_sampling_rate);
>> -
>> /*
>> * Lock governor so that governor start/stop can't execute in
>> parallel.
>> + *
>> + * We can't do a regular mutex_lock() here, as that may deadlock
>> against
>> + * another thread performing CPUFREQ_GOV_POLICY_EXIT event on the
>> + * governor, which might have already taken od_dbs_cdata.mutex
>> and is
>> + * waiting for this work to finish.
>> */
>> - mutex_lock(&od_dbs_cdata.mutex);
>> + if (!mutex_trylock(&od_dbs_cdata.mutex)) {
>> + queue_work(system_wq, &od_tuners->work);
>> + return;
>> + }
>>
>> cpumask_copy(&cpumask, cpu_online_mask);
>>
>> @@ -311,13 +318,22 @@ static void update_sampling_rate(struct dbs_data
>> *dbs_data,
>> static ssize_t store_sampling_rate(struct dbs_data *dbs_data, const
>> char *buf,
>> size_t count)
>> {
>> + struct od_dbs_tuners *od_tuners = dbs_data->tuners;
>> unsigned int input;
>> int ret;
>> ret = sscanf(buf, "%u", &input);
>> if (ret != 1)
>> return -EINVAL;
>>
>> - update_sampling_rate(dbs_data, input);
>> + od_tuners->sampling_rate = max(input, od_tuners->min_sampling_rate);
>> +
>> + /*
>> + * update_sampling_rate() requires to hold od_dbs_cdata.mutex,
>> but we
>> + * can't take that from this thread, otherwise it results in ABBA
>> + * lockdep between s_active and od_dbs_cdata.mutex locks.
>> + */
>> + queue_work(system_wq, &od_tuners->work);
>> +
>> return count;
>> }
>>
>> @@ -501,6 +517,8 @@ static int od_init(struct dbs_data *dbs_data, bool
>> notify)
>> tuners->ignore_nice_load = 0;
>> tuners->powersave_bias = default_powersave_bias;
>> tuners->io_is_busy = should_io_be_busy();
>> + INIT_WORK(&tuners->work, update_sampling_rate);
>> + tuners->dbs_data = dbs_data;
>>
>> dbs_data->tuners = tuners;
>> return 0;
>> @@ -508,7 +526,10 @@ static int od_init(struct dbs_data *dbs_data,
>> bool notify)
>>
>> static void od_exit(struct dbs_data *dbs_data, bool notify)
>> {
>> - kfree(dbs_data->tuners);
>> + struct od_dbs_tuners *tuners = dbs_data->tuners;
>> +
>> + cancel_work_sync(&tuners->work);
>> + kfree(tuners);
>> }
>>
>> define_get_cpu_dbs_routines(od_cpu_dbs_info);
>>
>
> No no no no! Let's not open up this can of worms of queuing up the work
> to handle a write to a sysfs file. It *MIGHT* work for this specific
> tunable (I haven't bothered to analyze), but this makes it impossible to
> return a useful/proper error value.
Sent too soon. Not only that, but it can also cause the writes to the
sysfs files to get processed in a different order and I don't know what
other issues/races THAT will open up.
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Date | 2016-02-04 19:20 +0100 |
| Message-ID | <qYwFA-5Am-5@gated-at.bofh.it> |
| In reply to | #1327075 |
On Thu, Feb 4, 2016 at 6:44 PM, Saravana Kannan <skannan@codeaurora.org> wrote: > On 02/04/2016 09:43 AM, Saravana Kannan wrote: >> >> On 02/04/2016 03:09 AM, Viresh Kumar wrote: >>> >>> On 04-02-16, 00:50, Rafael J. Wysocki wrote: >>>> >>>> This is exactly right. We've avoided one deadlock only to trip into >>>> another one. >>>> >>>> This happens because update_sampling_rate() acquires >>>> od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by >>>> cpufreq_governor_dbs(). >>>> >>>> Worse yet, a deadlock can still happen without (the new) >>>> dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if >>>> update_sampling_rate() runs in parallel with >>>> cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins >>>> the race. >>>> >>>> It looks like we need to drop the governor mutex before putting the >>>> kobject in cpufreq_governor_exit(). >>> [cut] >> >> No no no no! Let's not open up this can of worms of queuing up the work >> to handle a write to a sysfs file. It *MIGHT* work for this specific >> tunable (I haven't bothered to analyze), but this makes it impossible to >> return a useful/proper error value. > > > Sent too soon. Not only that, but it can also cause the writes to the sysfs > files to get processed in a different order and I don't know what other > issues/races THAT will open up. Well, I don't like this too. I actually do have an idea about how to fix these deadlocks, but it is on top of my cleanup series. I'll write more about it later today. Thanks, Rafael
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-05 03:50 +0100 |
| Message-ID | <qYED8-2lp-27@gated-at.bofh.it> |
| In reply to | #1327089 |
On 04-02-16, 19:18, Rafael J. Wysocki wrote: > On Thu, Feb 4, 2016 at 6:44 PM, Saravana Kannan <skannan@codeaurora.org> wrote: > > On 02/04/2016 09:43 AM, Saravana Kannan wrote: > >> No no no no! Let's not open up this can of worms of queuing up the work > >> to handle a write to a sysfs file. It *MIGHT* work for this specific > >> tunable (I haven't bothered to analyze), but this makes it impossible to > >> return a useful/proper error value. > > > > > > Sent too soon. Not only that, but it can also cause the writes to the sysfs > > files to get processed in a different order and I don't know what other > > issues/races THAT will open up. > > Well, I don't like this too. I expected similar responses only, so no surprises for me :) Though there are few things I would like to tell here: - There wouldn't be any race for updating the file, as that is done directly from store_sampling_rate(). It updates the *real* file we wanted to. - What's offloaded to the work-handler is something very special about ondemand governor and sampling rate. The same is not done for conservative governor as well, don't know why though. - After updating the sampling rate, we assess if we need to reschedule the timers/workqueue to a different time for better efficiency. I don't think there can be a race there and it can be safely done in a work.. > I actually do have an idea about how to fix these deadlocks, but it is > on top of my cleanup series. I would like to hear that, if you can, to save your time. I have tried so many different ways of fixing this yesterday and found issue everywhere :( -- viresh
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-05 05:00 +0100 |
| Message-ID | <qYFIS-3fA-9@gated-at.bofh.it> |
| In reply to | #1327089 |
On Thursday, February 04, 2016 07:18:32 PM Rafael J. Wysocki wrote: > On Thu, Feb 4, 2016 at 6:44 PM, Saravana Kannan <skannan@codeaurora.org> wrote: > > On 02/04/2016 09:43 AM, Saravana Kannan wrote: > >> > >> On 02/04/2016 03:09 AM, Viresh Kumar wrote: > >>> > >>> On 04-02-16, 00:50, Rafael J. Wysocki wrote: > >>>> > >>>> This is exactly right. We've avoided one deadlock only to trip into > >>>> another one. > >>>> > >>>> This happens because update_sampling_rate() acquires > >>>> od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by > >>>> cpufreq_governor_dbs(). > >>>> > >>>> Worse yet, a deadlock can still happen without (the new) > >>>> dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if > >>>> update_sampling_rate() runs in parallel with > >>>> cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins > >>>> the race. > >>>> > >>>> It looks like we need to drop the governor mutex before putting the > >>>> kobject in cpufreq_governor_exit(). > >>> > > [cut] > > >> > >> No no no no! Let's not open up this can of worms of queuing up the work > >> to handle a write to a sysfs file. It *MIGHT* work for this specific > >> tunable (I haven't bothered to analyze), but this makes it impossible to > >> return a useful/proper error value. > > > > > > Sent too soon. Not only that, but it can also cause the writes to the sysfs > > files to get processed in a different order and I don't know what other > > issues/races THAT will open up. > > Well, I don't like this too. > > I actually do have an idea about how to fix these deadlocks, but it is > on top of my cleanup series. > > I'll write more about it later today. Having actually posted that series again after cleaning it up I can say what I'm thinking about hopefully without confusing anyone too much. So please bear in mind that I'm going to refer to this series below: http://marc.info/?l=linux-pm&m=145463901630950&w=4 Also this is more of a brain dump rather than actual design description, so there may be holes etc in it. Please let me know if you can see any. The problem at hand is that policy->rwsem needs to be held around *all* operations in cpufreq_set_policy(). In particular, it cannot be dropped around invocations of __cpufreq_governor() with the event arg equal to _EXIT as that leads to interesting races. Unfortunately, we know that holding policy->rwsem in those places leads to a deadlock with governor sysfs attributes removal in cpufreq_governor_exit(). Viresh attempted to fix this by avoiding to acquire policy->rwsem for governor attributes access (as holding it is not necessary for them in principle). That was a nice try, but it turned out to be insufficient because of another deadlock scenario uncovered by it. Namely, since the ondemand governor's update_sampling_rate() acquires the governor mutex (called dbs_data_mutex after my patches mentioned above), it may deadlock with exactly the same piece of code in cpufreq_governor_exit() in almost exactly the same way. To avoid that other deadlock, we'd either need to drop dbs_data_mutex from update_sampling_rate(), or drop it for the removal of the governor sysfs attributes in cpufreq_governor_exit(). I don't think the former is an option at least at this point, so it looks like we pretty much have to do the latter. With that in mind, I'd start with the changes made by Viresh (maybe without the first patch which really isn't essential here). That is, introduce a separate kobject type for the governor attributes kobject and register that in cpufreq_governor_init(). The show/store callbacks for that kobject type won't acquire policy->rwsem so the first deadlock will be avoided. But in addition to that, I'd drop dbs_data_mutex before the removal of governor sysfs attributes. That actually happens in two places, in cpufreq_governor_exit() and in the error path of cpufreq_governor_init(). To that end, I'd move the locking from cpufreq_governor_dbs() to the functions called by it. That should be readily doable and they can do all of the necessary checks themselves. cpufreq_governor_dbs() would become a pure mux then, but that's not such a big deal. With that, cpufreq_governor_exit() may just drop the lock before it does the final kobject_put(). The danger here is that the sysfs show/store callbacks of the governor attributes kobject may see invalid dbs_data for a while, after the lock has been dropped and before the kobject is deleted. That may be addressed by checking, for example, the presence of the dbs_data's "tuners" pointer in those callbacks. If it is NULL, they can simply return -EAGAIN or similar. Now, that means, though, that they need to acquire the same lock as cpufreq_governor_exit(), or they may see things go away while they are running. The simplest approach here would be to take dbs_data_mutex in them too, although that's a bit of a sledgehammer. It might be better to have a per-policy lock in struct policy_dbs_info for that, for example, but then the governor attribute sysfs callbacks would need to get that object instead of dbs_data. On the flip side, it might be possible to migrate update_sampling_rate() to that lock too. And maybe we can get rid of dbs_data_mutex even, who knows? Thanks, Rafael
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-05 10:50 +0100 |
| Message-ID | <qYLbA-6TD-29@gated-at.bofh.it> |
| In reply to | #1327469 |
On 05-02-16, 04:54, Rafael J. Wysocki wrote:
> Having actually posted that series again after cleaning it up I can say
> what I'm thinking about hopefully without confusing anyone too much. So
> please bear in mind that I'm going to refer to this series below:
>
> http://marc.info/?l=linux-pm&m=145463901630950&w=4
>
> Also this is more of a brain dump rather than actual design description,
> so there may be holes etc in it. Please let me know if you can see any.
>
> The problem at hand is that policy->rwsem needs to be held around *all*
> operations in cpufreq_set_policy(). In particular, it cannot be dropped
> around invocations of __cpufreq_governor() with the event arg equal to
> _EXIT as that leads to interesting races.
>
> Unfortunately, we know that holding policy->rwsem in those places leads
> to a deadlock with governor sysfs attributes removal in cpufreq_governor_exit().
>
> Viresh attempted to fix this by avoiding to acquire policy->rwsem for governor
> attributes access (as holding it is not necessary for them in principle). That
> was a nice try, but it turned out to be insufficient because of another deadlock
> scenario uncovered by it.
Not really.
The other deadlock wasn't uncovered by it, its just that Shilpa tested
directly after my patches and reported the issue. Later yesterday, she
was hitting the exactly same issue on pm/linux-next as well (i.e.
without my patches). And ofcourse Juri has also reported the same
issue on linux-next few days back.
> Namely, since the ondemand governor's update_sampling_rate()
> acquires the governor mutex (called dbs_data_mutex after my patches mentioned
> above), it may deadlock with exactly the same piece of code in cpufreq_governor_exit()
> in almost exactly the same way.
Right.
> To avoid that other deadlock, we'd either need to drop dbs_data_mutex from
> update_sampling_rate(),
And my so called 'ugly' 8th patch tried to do just that :)
But as I also mentioned in reply to the update-util patchset of yours,
its possible somewhat.
> or drop it for the removal of the governor sysfs
> attributes in cpufreq_governor_exit(). I don't think the former is an option
> at least at this point, so it looks like we pretty much have to do the latter.
>
> With that in mind, I'd start with the changes made by Viresh (maybe without the
> first patch which really isn't essential here).
That was just to cleanup the macro mess a bit, nothing more. Over
that, I think the first 7 patches can be picked as it is without any
changes. Ofcourse they are required to be rebased over your 13
patches, if those are going in first :)
> That is, introduce a separate
> kobject type for the governor attributes kobject and register that in
> cpufreq_governor_init(). The show/store callbacks for that kobject type won't
> acquire policy->rwsem so the first deadlock will be avoided.
>
> But in addition to that, I'd drop dbs_data_mutex before the removal of governor
> sysfs attributes. That actually happens in two places, in cpufreq_governor_exit()
> and in the error path of cpufreq_governor_init().
>
> To that end, I'd move the locking from cpufreq_governor_dbs() to the functions
> called by it. That should be readily doable and they can do all of the
> necessary checks themselves. cpufreq_governor_dbs() would become a pure mux then,
> but that's not such a big deal.
>
> With that, cpufreq_governor_exit() may just drop the lock before it does the
> final kobject_put(). The danger here is that the sysfs show/store callbacks of
> the governor attributes kobject may see invalid dbs_data for a while, after the
> lock has been dropped and before the kobject is deleted. That may be addressed
> by checking, for example, the presence of the dbs_data's "tuners" pointer in those
> callbacks. If it is NULL, they can simply return -EAGAIN or similar.
So you mean something like this (consider only !governor_per_policy
case with ondemand governor for now):
exit()
{
lock-dbs_data_mutex;
...
dbs_data->tuners = NULL; //so that sysfs files can return early
dbs_governor->gdbs_data = NULL; //For !governor_per_policy case
unlock-dbs_data_mutex;
/*
* Problem: Who is stopping us to set ondemand as governor for
* another policy, which can try create a kobject which will
* try to create sysfs directory at the same path ?
*
* Though another field in dbs_governor can be used to fix this
* I think, which needs to block the other INIT operation.
*/
kobject_put(dbs_data->kobj); //This should wait for all sysfs operations to end.
kfree(dbs_data);
}
And the sysfs operations show/store need to take dbs_data_mutex() for
their entire operations.
??
--
viresh
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-08 03:20 +0100 |
| Message-ID | <qZJAK-6SN-11@gated-at.bofh.it> |
| In reply to | #1327579 |
On Friday, February 05, 2016 03:19:25 PM Viresh Kumar wrote:
> On 05-02-16, 04:54, Rafael J. Wysocki wrote:
> > Having actually posted that series again after cleaning it up I can say
> > what I'm thinking about hopefully without confusing anyone too much. So
> > please bear in mind that I'm going to refer to this series below:
> >
> > http://marc.info/?l=linux-pm&m=145463901630950&w=4
> >
> > Also this is more of a brain dump rather than actual design description,
> > so there may be holes etc in it. Please let me know if you can see any.
> >
> > The problem at hand is that policy->rwsem needs to be held around *all*
> > operations in cpufreq_set_policy(). In particular, it cannot be dropped
> > around invocations of __cpufreq_governor() with the event arg equal to
> > _EXIT as that leads to interesting races.
> >
> > Unfortunately, we know that holding policy->rwsem in those places leads
> > to a deadlock with governor sysfs attributes removal in cpufreq_governor_exit().
> >
> > Viresh attempted to fix this by avoiding to acquire policy->rwsem for governor
> > attributes access (as holding it is not necessary for them in principle). That
> > was a nice try, but it turned out to be insufficient because of another deadlock
> > scenario uncovered by it.
>
> Not really.
>
> The other deadlock wasn't uncovered by it, its just that Shilpa tested
> directly after my patches and reported the issue. Later yesterday, she
> was hitting the exactly same issue on pm/linux-next as well (i.e.
> without my patches). And ofcourse Juri has also reported the same
> issue on linux-next few days back.
OK, fair enough.
> > Namely, since the ondemand governor's update_sampling_rate()
> > acquires the governor mutex (called dbs_data_mutex after my patches mentioned
> > above), it may deadlock with exactly the same piece of code in cpufreq_governor_exit()
> > in almost exactly the same way.
>
> Right.
>
> > To avoid that other deadlock, we'd either need to drop dbs_data_mutex from
> > update_sampling_rate(),
>
> And my so called 'ugly' 8th patch tried to do just that :)
>
> But as I also mentioned in reply to the update-util patchset of yours,
> its possible somewhat.
Yes, it should be possible and not even too difficult.
> > or drop it for the removal of the governor sysfs
> > attributes in cpufreq_governor_exit(). I don't think the former is an option
> > at least at this point, so it looks like we pretty much have to do the latter.
> >
> > With that in mind, I'd start with the changes made by Viresh (maybe without the
> > first patch which really isn't essential here).
>
> That was just to cleanup the macro mess a bit, nothing more. Over
> that, I think the first 7 patches can be picked as it is without any
> changes. Ofcourse they are required to be rebased over your 13
> patches, if those are going in first :)
Yes, please rebase.
Also please skip the first one that was moving min_sampling_rate around,
at least for now.
As I said, we may be moving other attributes in the opposite direction,
so two sets of macros may be necessary anyway.
> > That is, introduce a separate
> > kobject type for the governor attributes kobject and register that in
> > cpufreq_governor_init(). The show/store callbacks for that kobject type won't
> > acquire policy->rwsem so the first deadlock will be avoided.
> >
> > But in addition to that, I'd drop dbs_data_mutex before the removal of governor
> > sysfs attributes. That actually happens in two places, in cpufreq_governor_exit()
> > and in the error path of cpufreq_governor_init().
> >
> > To that end, I'd move the locking from cpufreq_governor_dbs() to the functions
> > called by it. That should be readily doable and they can do all of the
> > necessary checks themselves. cpufreq_governor_dbs() would become a pure mux then,
> > but that's not such a big deal.
> >
> > With that, cpufreq_governor_exit() may just drop the lock before it does the
> > final kobject_put(). The danger here is that the sysfs show/store callbacks of
> > the governor attributes kobject may see invalid dbs_data for a while, after the
> > lock has been dropped and before the kobject is deleted. That may be addressed
> > by checking, for example, the presence of the dbs_data's "tuners" pointer in those
> > callbacks. If it is NULL, they can simply return -EAGAIN or similar.
>
> So you mean something like this (consider only !governor_per_policy
> case with ondemand governor for now):
>
> exit()
> {
> lock-dbs_data_mutex;
> ...
> dbs_data->tuners = NULL; //so that sysfs files can return early
> dbs_governor->gdbs_data = NULL; //For !governor_per_policy case
> unlock-dbs_data_mutex;
>
> /*
> * Problem: Who is stopping us to set ondemand as governor for
> * another policy, which can try create a kobject which will
> * try to create sysfs directory at the same path ?
> *
> * Though another field in dbs_governor can be used to fix this
> * I think, which needs to block the other INIT operation.
> */
>
> kobject_put(dbs_data->kobj); //This should wait for all sysfs operations to end.
>
> kfree(dbs_data);
> }
>
> And the sysfs operations show/store need to take dbs_data_mutex() for
> their entire operations.
>
> ??
Yes, roughly.
But it shouldn't be necessary after all, because dropping the mutex from
update_sampling_rate() looks easier than I thought previously.
Thanks,
Rafael
[toc] | [prev] | [next] | [standalone]
| From | Saravana Kannan <skannan@codeaurora.org> |
|---|---|
| Date | 2016-02-06 03:30 +0100 |
| Message-ID | <qZ0Nk-E3-5@gated-at.bofh.it> |
| In reply to | #1327469 |
On 02/04/2016 07:54 PM, Rafael J. Wysocki wrote:
> On Thursday, February 04, 2016 07:18:32 PM Rafael J. Wysocki wrote:
>> On Thu, Feb 4, 2016 at 6:44 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
>>> On 02/04/2016 09:43 AM, Saravana Kannan wrote:
>>>>
>>>> On 02/04/2016 03:09 AM, Viresh Kumar wrote:
>>>>>
>>>>> On 04-02-16, 00:50, Rafael J. Wysocki wrote:
>>>>>>
>>>>>> This is exactly right. We've avoided one deadlock only to trip into
>>>>>> another one.
>>>>>>
>>>>>> This happens because update_sampling_rate() acquires
>>>>>> od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by
>>>>>> cpufreq_governor_dbs().
>>>>>>
>>>>>> Worse yet, a deadlock can still happen without (the new)
>>>>>> dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if
>>>>>> update_sampling_rate() runs in parallel with
>>>>>> cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins
>>>>>> the race.
>>>>>>
>>>>>> It looks like we need to drop the governor mutex before putting the
>>>>>> kobject in cpufreq_governor_exit().
>>>>>
>>
>> [cut]
>>
>>>>
>>>> No no no no! Let's not open up this can of worms of queuing up the work
>>>> to handle a write to a sysfs file. It *MIGHT* work for this specific
>>>> tunable (I haven't bothered to analyze), but this makes it impossible to
>>>> return a useful/proper error value.
>>>
>>>
>>> Sent too soon. Not only that, but it can also cause the writes to the sysfs
>>> files to get processed in a different order and I don't know what other
>>> issues/races THAT will open up.
>>
>> Well, I don't like this too.
>>
>> I actually do have an idea about how to fix these deadlocks, but it is
>> on top of my cleanup series.
>>
>> I'll write more about it later today.
>
> Having actually posted that series again after cleaning it up I can say
> what I'm thinking about hopefully without confusing anyone too much. So
> please bear in mind that I'm going to refer to this series below:
>
> http://marc.info/?l=linux-pm&m=145463901630950&w=4
>
> Also this is more of a brain dump rather than actual design description,
> so there may be holes etc in it. Please let me know if you can see any.
>
> The problem at hand is that policy->rwsem needs to be held around *all*
> operations in cpufreq_set_policy(). In particular, it cannot be dropped
> around invocations of __cpufreq_governor() with the event arg equal to
> _EXIT as that leads to interesting races.
>
> Unfortunately, we know that holding policy->rwsem in those places leads
> to a deadlock with governor sysfs attributes removal in cpufreq_governor_exit().
>
> Viresh attempted to fix this by avoiding to acquire policy->rwsem for governor
> attributes access (as holding it is not necessary for them in principle). That
> was a nice try, but it turned out to be insufficient because of another deadlock
> scenario uncovered by it. Namely, since the ondemand governor's update_sampling_rate()
> acquires the governor mutex (called dbs_data_mutex after my patches mentioned
> above), it may deadlock with exactly the same piece of code in cpufreq_governor_exit()
> in almost exactly the same way.
>
> To avoid that other deadlock, we'd either need to drop dbs_data_mutex from
> update_sampling_rate(), or drop it for the removal of the governor sysfs
> attributes in cpufreq_governor_exit(). I don't think the former is an option
> at least at this point, so it looks like we pretty much have to do the latter.
>
> With that in mind, I'd start with the changes made by Viresh (maybe without the
> first patch which really isn't essential here). That is, introduce a separate
> kobject type for the governor attributes kobject and register that in
> cpufreq_governor_init(). The show/store callbacks for that kobject type won't
> acquire policy->rwsem so the first deadlock will be avoided.
>
> But in addition to that, I'd drop dbs_data_mutex before the removal of governor
> sysfs attributes. That actually happens in two places, in cpufreq_governor_exit()
> and in the error path of cpufreq_governor_init().
>
> To that end, I'd move the locking from cpufreq_governor_dbs() to the functions
> called by it. That should be readily doable and they can do all of the
> necessary checks themselves. cpufreq_governor_dbs() would become a pure mux then,
> but that's not such a big deal.
>
> With that, cpufreq_governor_exit() may just drop the lock before it does the
> final kobject_put(). The danger here is that the sysfs show/store callbacks of
> the governor attributes kobject may see invalid dbs_data for a while, after the
> lock has been dropped and before the kobject is deleted. That may be addressed
> by checking, for example, the presence of the dbs_data's "tuners" pointer in those
> callbacks. If it is NULL, they can simply return -EAGAIN or similar.
>
> Now, that means, though, that they need to acquire the same lock as
> cpufreq_governor_exit(), or they may see things go away while they are running.
> The simplest approach here would be to take dbs_data_mutex in them too, although
> that's a bit of a sledgehammer. It might be better to have a per-policy lock
> in struct policy_dbs_info for that, for example, but then the governor attribute
> sysfs callbacks would need to get that object instead of dbs_data.
>
> On the flip side, it might be possible to migrate update_sampling_rate() to
> that lock too. And maybe we can get rid of dbs_data_mutex even, who knows?
I'm glad you've analyzed it this far. So, the rest of my comments will
be easier to understand.
I'm going to go back to my point of NOT doing the sysfs add/remove
inside the governor at all (that includes cpufreq_governor.c) and doing
it in cpufreq.c. That suggestion was confusing to explain/understand
before when we were using policy rwsem inside the show/store ops for the
governor attributes. Now that has been removed, my suggestion would be
even easier/cleaner to implement/understand and you don't have to worry
about ANY races in the governor.
I'll just talk about the have_governor_per_policy() case. It can be
easily extended to the global case.
In cpufreq_governor.c:
cpufreq_governor_init(...)
{
...
/* NOT kobject_init_and_add */
kobject_init();
/* New field */
policy->gov_kobj = &dbs_data->kobj);
...
}
In cpufreq.c:
__cpufreq_governor(...)
{
if (event == POLICY_EXIT) {
kobject_put(policy->gov_kobj);
}
ret = policy->governor->governor(policy, event);
if (event == POLICY_INIT) {
kobj_add(policy->gov_kobj, policy->kobj, policy->governor->name);
}
}
This guarantees that there can be no races of the governor specific data
structures going away while being accessed from sysfs because the first
thing we do once we decide to "kill" a governor is to remove the sysfs
files and the accesses to governor data (and flush out all on going
accesses) and THEN ask the governor to exit.
Thoughts?
Thanks,
Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-02-08 03:30 +0100 |
| Message-ID | <qZJKq-6We-5@gated-at.bofh.it> |
| In reply to | #1328194 |
On Friday, February 05, 2016 06:22:35 PM Saravana Kannan wrote:
> On 02/04/2016 07:54 PM, Rafael J. Wysocki wrote:
> > On Thursday, February 04, 2016 07:18:32 PM Rafael J. Wysocki wrote:
> >> On Thu, Feb 4, 2016 at 6:44 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> >>> On 02/04/2016 09:43 AM, Saravana Kannan wrote:
> >>>>
> >>>> On 02/04/2016 03:09 AM, Viresh Kumar wrote:
> >>>>>
> >>>>> On 04-02-16, 00:50, Rafael J. Wysocki wrote:
> >>>>>>
> >>>>>> This is exactly right. We've avoided one deadlock only to trip into
> >>>>>> another one.
> >>>>>>
> >>>>>> This happens because update_sampling_rate() acquires
> >>>>>> od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by
> >>>>>> cpufreq_governor_dbs().
> >>>>>>
> >>>>>> Worse yet, a deadlock can still happen without (the new)
> >>>>>> dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if
> >>>>>> update_sampling_rate() runs in parallel with
> >>>>>> cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins
> >>>>>> the race.
> >>>>>>
> >>>>>> It looks like we need to drop the governor mutex before putting the
> >>>>>> kobject in cpufreq_governor_exit().
> >>>>>
> >>
> >> [cut]
> >>
> >>>>
> >>>> No no no no! Let's not open up this can of worms of queuing up the work
> >>>> to handle a write to a sysfs file. It *MIGHT* work for this specific
> >>>> tunable (I haven't bothered to analyze), but this makes it impossible to
> >>>> return a useful/proper error value.
> >>>
> >>>
> >>> Sent too soon. Not only that, but it can also cause the writes to the sysfs
> >>> files to get processed in a different order and I don't know what other
> >>> issues/races THAT will open up.
> >>
> >> Well, I don't like this too.
> >>
> >> I actually do have an idea about how to fix these deadlocks, but it is
> >> on top of my cleanup series.
> >>
> >> I'll write more about it later today.
> >
> > Having actually posted that series again after cleaning it up I can say
> > what I'm thinking about hopefully without confusing anyone too much. So
> > please bear in mind that I'm going to refer to this series below:
> >
> > http://marc.info/?l=linux-pm&m=145463901630950&w=4
> >
> > Also this is more of a brain dump rather than actual design description,
> > so there may be holes etc in it. Please let me know if you can see any.
> >
> > The problem at hand is that policy->rwsem needs to be held around *all*
> > operations in cpufreq_set_policy(). In particular, it cannot be dropped
> > around invocations of __cpufreq_governor() with the event arg equal to
> > _EXIT as that leads to interesting races.
> >
> > Unfortunately, we know that holding policy->rwsem in those places leads
> > to a deadlock with governor sysfs attributes removal in cpufreq_governor_exit().
> >
> > Viresh attempted to fix this by avoiding to acquire policy->rwsem for governor
> > attributes access (as holding it is not necessary for them in principle). That
> > was a nice try, but it turned out to be insufficient because of another deadlock
> > scenario uncovered by it. Namely, since the ondemand governor's update_sampling_rate()
> > acquires the governor mutex (called dbs_data_mutex after my patches mentioned
> > above), it may deadlock with exactly the same piece of code in cpufreq_governor_exit()
> > in almost exactly the same way.
> >
> > To avoid that other deadlock, we'd either need to drop dbs_data_mutex from
> > update_sampling_rate(), or drop it for the removal of the governor sysfs
> > attributes in cpufreq_governor_exit(). I don't think the former is an option
> > at least at this point, so it looks like we pretty much have to do the latter.
> >
> > With that in mind, I'd start with the changes made by Viresh (maybe without the
> > first patch which really isn't essential here). That is, introduce a separate
> > kobject type for the governor attributes kobject and register that in
> > cpufreq_governor_init(). The show/store callbacks for that kobject type won't
> > acquire policy->rwsem so the first deadlock will be avoided.
> >
> > But in addition to that, I'd drop dbs_data_mutex before the removal of governor
> > sysfs attributes. That actually happens in two places, in cpufreq_governor_exit()
> > and in the error path of cpufreq_governor_init().
> >
> > To that end, I'd move the locking from cpufreq_governor_dbs() to the functions
> > called by it. That should be readily doable and they can do all of the
> > necessary checks themselves. cpufreq_governor_dbs() would become a pure mux then,
> > but that's not such a big deal.
> >
> > With that, cpufreq_governor_exit() may just drop the lock before it does the
> > final kobject_put(). The danger here is that the sysfs show/store callbacks of
> > the governor attributes kobject may see invalid dbs_data for a while, after the
> > lock has been dropped and before the kobject is deleted. That may be addressed
> > by checking, for example, the presence of the dbs_data's "tuners" pointer in those
> > callbacks. If it is NULL, they can simply return -EAGAIN or similar.
> >
> > Now, that means, though, that they need to acquire the same lock as
> > cpufreq_governor_exit(), or they may see things go away while they are running.
> > The simplest approach here would be to take dbs_data_mutex in them too, although
> > that's a bit of a sledgehammer. It might be better to have a per-policy lock
> > in struct policy_dbs_info for that, for example, but then the governor attribute
> > sysfs callbacks would need to get that object instead of dbs_data.
> >
> > On the flip side, it might be possible to migrate update_sampling_rate() to
> > that lock too. And maybe we can get rid of dbs_data_mutex even, who knows?
>
> I'm glad you've analyzed it this far. So, the rest of my comments will
> be easier to understand.
>
> I'm going to go back to my point of NOT doing the sysfs add/remove
> inside the governor at all (that includes cpufreq_governor.c) and doing
> it in cpufreq.c. That suggestion was confusing to explain/understand
> before when we were using policy rwsem inside the show/store ops for the
> governor attributes. Now that has been removed, my suggestion would be
> even easier/cleaner to implement/understand and you don't have to worry
> about ANY races in the governor.
>
> I'll just talk about the have_governor_per_policy() case. It can be
> easily extended to the global case.
>
> In cpufreq_governor.c:
> cpufreq_governor_init(...)
> {
> ...
> /* NOT kobject_init_and_add */
> kobject_init();
> /* New field */
> policy->gov_kobj = &dbs_data->kobj);
> ...
> }
>
> In cpufreq.c:
> __cpufreq_governor(...)
> {
>
> if (event == POLICY_EXIT) {
> kobject_put(policy->gov_kobj);
> }
> ret = policy->governor->governor(policy, event);
> if (event == POLICY_INIT) {
> kobj_add(policy->gov_kobj, policy->kobj, policy->governor->name);
> }
> }
>
> This guarantees that there can be no races of the governor specific data
> structures going away while being accessed from sysfs because the first
> thing we do once we decide to "kill" a governor is to remove the sysfs
> files and the accesses to governor data (and flush out all on going
> accesses) and THEN ask the governor to exit.
>
> Thoughts?
The core would then have to rely on the governor code to populate the gov_kobj
field correctly which doesn't look really straightforward to me. It is better
if each code layer arranges the data structures it is going to use by itself.
Besides, ondemand and conservative are the only governors that use the governor
kobject at all, so I'm not sure if that really belongs to the core.
Thanks,
Rafael
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-02-04 07:30 +0100 |
| Message-ID | <qYlAt-4SU-3@gated-at.bofh.it> |
| In reply to | #1325598 |
On 03-02-16, 21:40, Viresh Kumar wrote:
> On 03-02-16, 15:54, Juri Lelli wrote:
> > Ouch, I've just got this executing -f basic on Juno. :(
> > It happens with the hotplug_1_by_1 test.
> >
> >
> > [ 1086.531252] IRQ1 no longer affine to CPU1
> > [ 1086.531495] CPU1: shutdown
> > [ 1086.538199] psci: CPU1 killed.
> > [ 1086.583396]
> > [ 1086.584881] ======================================================
> > [ 1086.590999] [ INFO: possible circular locking dependency detected ]
> > [ 1086.597205] 4.5.0-rc2+ #37 Not tainted
> > [ 1086.600914] -------------------------------------------------------
> > [ 1086.607118] runme.sh/1052 is trying to acquire lock:
> > [ 1086.612031] (sb_writers#7){.+.+.+}, at: [<ffffffc000249500>] __sb_start_write+0xcc/0xe0
> > [ 1086.620090]
> > [ 1086.620090] but task is already holding lock:
> > [ 1086.625865] (&policy->rwsem){+++++.}, at: [<ffffffc0005c8ee4>] cpufreq_offline+0x7c/0x278
> > [ 1086.634081]
> > [ 1086.634081] which lock already depends on the new lock.
> > [ 1086.634081]
> > [ 1086.642180]
> > [ 1086.642180] the existing dependency chain (in reverse order) is:
> > [ 1086.649589]
> > -> #1 (&policy->rwsem){+++++.}:
> > [ 1086.653929] [<ffffffc00011d9a4>] check_prev_add+0x670/0x754
> > [ 1086.660060] [<ffffffc00011e1ac>] validate_chain.isra.36+0x724/0xa0c
> > [ 1086.666876] [<ffffffc00011f904>] __lock_acquire+0x4e4/0xba0
> > [ 1086.673001] [<ffffffc000120b58>] lock_release+0x244/0x570
> > [ 1086.678955] [<ffffffc0007351d0>] __mutex_unlock_slowpath+0xa0/0x18c
> > [ 1086.685771] [<ffffffc0007352dc>] mutex_unlock+0x20/0x2c
> > [ 1086.691553] [<ffffffc0002ccd24>] kernfs_fop_write+0xb0/0x194
> > [ 1086.697768] [<ffffffc00024478c>] __vfs_write+0x48/0x104
> > [ 1086.703550] [<ffffffc0002457a4>] vfs_write+0x98/0x198
> > [ 1086.709161] [<ffffffc0002465e4>] SyS_write+0x54/0xb0
> > [ 1086.714684] [<ffffffc000085d30>] el0_svc_naked+0x24/0x28
> > [ 1086.720555]
> > -> #0 (sb_writers#7){.+.+.+}:
> > [ 1086.724730] [<ffffffc00011c574>] print_circular_bug+0x80/0x2e4
> > [ 1086.731116] [<ffffffc00011d470>] check_prev_add+0x13c/0x754
> > [ 1086.737243] [<ffffffc00011e1ac>] validate_chain.isra.36+0x724/0xa0c
> > [ 1086.744059] [<ffffffc00011f904>] __lock_acquire+0x4e4/0xba0
> > [ 1086.750184] [<ffffffc0001207f4>] lock_acquire+0xe4/0x204
> > [ 1086.756052] [<ffffffc000118da0>] percpu_down_read+0x50/0xe4
> > [ 1086.762180] [<ffffffc000249500>] __sb_start_write+0xcc/0xe0
> > [ 1086.768306] [<ffffffc00026ae90>] mnt_want_write+0x28/0x54
> > [ 1086.774263] [<ffffffc0002555f8>] do_last+0x660/0xcb8
> > [ 1086.779788] [<ffffffc000255cdc>] path_openat+0x8c/0x2b0
> > [ 1086.785570] [<ffffffc000256fbc>] do_filp_open+0x78/0xf0
> > [ 1086.791353] [<ffffffc000244058>] do_sys_open+0x150/0x214
> > [ 1086.797222] [<ffffffc0002441a0>] SyS_openat+0x3c/0x48
> > [ 1086.802831] [<ffffffc000085d30>] el0_svc_naked+0x24/0x28
> > [ 1086.808700]
> > [ 1086.808700] other info that might help us debug this:
> > [ 1086.808700]
> > [ 1086.816627] Possible unsafe locking scenario:
> > [ 1086.816627]
> > [ 1086.822488] CPU0 CPU1
> > [ 1086.826971] ---- ----
> > [ 1086.831453] lock(&policy->rwsem);
> > [ 1086.834918] lock(sb_writers#7);
> > [ 1086.840713] lock(&policy->rwsem);
> > [ 1086.846671] lock(sb_writers#7);
> > [ 1086.849972]
> > [ 1086.849972] *** DEADLOCK ***
> > [ 1086.849972]
> > [ 1086.855836] 1 lock held by runme.sh/1052:
> > [ 1086.859802] #0: (&policy->rwsem){+++++.}, at: [<ffffffc0005c8ee4>] cpufreq_offline+0x7c/0x278
> > [ 1086.868453]
> > [ 1086.868453] stack backtrace:
> > [ 1086.872769] CPU: 5 PID: 1052 Comm: runme.sh Not tainted 4.5.0-rc2+ #37
> > [ 1086.879229] Hardware name: ARM Juno development board (r2) (DT)
> > [ 1086.885089] Call trace:
> > [ 1086.887511] [<ffffffc00008a788>] dump_backtrace+0x0/0x1f4
> > [ 1086.892858] [<ffffffc00008a99c>] show_stack+0x20/0x28
> > [ 1086.897861] [<ffffffc00041a380>] dump_stack+0x84/0xc0
> > [ 1086.902863] [<ffffffc00011c6c8>] print_circular_bug+0x1d4/0x2e4
> > [ 1086.908725] [<ffffffc00011d470>] check_prev_add+0x13c/0x754
> > [ 1086.914244] [<ffffffc00011e1ac>] validate_chain.isra.36+0x724/0xa0c
> > [ 1086.920448] [<ffffffc00011f904>] __lock_acquire+0x4e4/0xba0
> > [ 1086.925965] [<ffffffc0001207f4>] lock_acquire+0xe4/0x204
> > [ 1086.931224] [<ffffffc000118da0>] percpu_down_read+0x50/0xe4
> > [ 1086.936742] [<ffffffc000249500>] __sb_start_write+0xcc/0xe0
> > [ 1086.942260] [<ffffffc00026ae90>] mnt_want_write+0x28/0x54
> > [ 1086.947605] [<ffffffc0002555f8>] do_last+0x660/0xcb8
> > [ 1086.952520] [<ffffffc000255cdc>] path_openat+0x8c/0x2b0
> > [ 1086.957693] [<ffffffc000256fbc>] do_filp_open+0x78/0xf0
> > [ 1086.962865] [<ffffffc000244058>] do_sys_open+0x150/0x214
> > [ 1086.968123] [<ffffffc0002441a0>] SyS_openat+0x3c/0x48
> > [ 1086.973124] [<ffffffc000085d30>] el0_svc_naked+0x24/0x28
> > [ 1087.019315] Detected PIPT I-cache on CPU1
> > [ 1087.019373] CPU1: Booted secondary processor [410fd080]
>
> Urg..
Urg square :(
> I failed to understand it for now though. Please test only the first 4
> patches and leave the bottom three. AFAICT, this is caused by the 6th
> patch.
From the code I still failed to understand this since sometime back
and I something just caught my eyes and the 6th patch needs this
fixup:
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 7bc8a5ed97e5..ac3348ecde7b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1351,7 +1351,7 @@ static void cpufreq_offline(unsigned int cpu)
pr_err("%s: Failed to start governor\n", __func__);
}
- return;
+ goto unlock;
}
if (cpufreq_driver->stop_cpu)
@@ -1373,6 +1373,8 @@ static void cpufreq_offline(unsigned int cpu)
cpufreq_driver->exit(policy);
policy->freq_table = NULL;
}
+
+unlock:
up_write(&policy->rwsem);
}
I tried the basic tests using './runme' and they aren't reporting the
same lockdep now. And yes, your lockdep occurred on my exynos board as
well :)
I have re-pushed my patches again to the same branch. All 7 look fine
to me now :)
--
viresh
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web