Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1310257 > unrolled thread
| Started by | Juri Lelli <juri.lelli@arm.com> |
|---|---|
| First post | 2016-01-15 17:40 +0100 |
| Last post | 2016-01-20 11:20 +0100 |
| Articles | 8 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [RFC PATCH 15/19] cpufreq: remove useless usage of cpufreq_governor_mutex in __cpufreq_governor Juri Lelli <juri.lelli@arm.com> - 2016-01-15 17:40 +0100
Re: [RFC PATCH 15/19] cpufreq: remove useless usage of cpufreq_governor_mutex in __cpufreq_governor Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-18 07:00 +0100
Re: [RFC PATCH 15/19] cpufreq: remove useless usage of cpufreq_governor_mutex in __cpufreq_governor Juri Lelli <juri.lelli@arm.com> - 2016-01-19 17:50 +0100
Re: [RFC PATCH 15/19] cpufreq: remove useless usage of cpufreq_governor_mutex in __cpufreq_governor Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-20 08:30 +0100
Re: [RFC PATCH 15/19] cpufreq: remove useless usage of cpufreq_governor_mutex in __cpufreq_governor Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-20 11:20 +0100
Re: [RFC PATCH 15/19] cpufreq: remove useless usage of cpufreq_governor_mutex in __cpufreq_governor Juri Lelli <juri.lelli@arm.com> - 2016-01-20 11:30 +0100
Re: [RFC PATCH 15/19] cpufreq: remove useless usage of cpufreq_governor_mutex in __cpufreq_governor Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-20 11:40 +0100
Re: [RFC PATCH 15/19] cpufreq: remove useless usage of cpufreq_governor_mutex in __cpufreq_governor Juri Lelli <juri.lelli@arm.com> - 2016-01-20 11:20 +0100
| From | Juri Lelli <juri.lelli@arm.com> |
|---|---|
| Date | 2016-01-15 17:40 +0100 |
| Subject | Re: [RFC PATCH 15/19] cpufreq: remove useless usage of cpufreq_governor_mutex in __cpufreq_governor |
| Message-ID | <qRfzP-gk-3@gated-at.bofh.it> |
Hi,
On 12/01/16 16:36, Viresh Kumar wrote:
> On 11-01-16, 17:35, Juri Lelli wrote:
> > Commit 6f1e4efd882e ("cpufreq: Fix timer/workqueue corruption by
> > protecting reading governor_enabled") made policy->governor_enabled
> > guarded by cpufreq_governor_mutex in __cpufreq_governor. Now that
> > holding of policy->rwsem is asserted in __cpufreq_governor,
> > cpufreq_governor_mutex is overkilling.
>
> I am sure that is going to break it. Try that x86, somehow I don't get
> it on my exynos boards.
>
But governor_enabled seems to not be checked anymore outside cpufreq.c
(see also 01/19), as it was in the commit you are referring to. Now that
users of this should be holding policy->rwsem, so that should suffice
for protecting governor_enabled, as governor_enabled is only changed
inside here.
I run some test on a x86 box I setup and didn't see anything related to
this. I'll wait to get the first 0-day report anyway.
> > - mutex_lock(&cpufreq_governor_mutex);
> > if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
> > || (!policy->governor_enabled
> > && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {
> > - mutex_unlock(&cpufreq_governor_mutex);
> > return -EBUSY;
> > }
>
> Actually the above checks should also be removed as the governors are
> responsible for maintaining their state machines. But
> userspace/powersave/performance don't have that support yet and so
> these checks save them from going into undefined states.
>
> Over that, above and below checks are incomplete..
>
You mean we need an additional patch that extends the checks performed?
Thanks,
- Juri
> > @@ -2006,8 +2004,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> > else if (event == CPUFREQ_GOV_START)
> > policy->governor_enabled = true;
> >
> > - mutex_unlock(&cpufreq_governor_mutex);
> > -
> > ret = policy->governor->governor(policy, event);
> >
> > if (!ret) {
> > @@ -2017,12 +2013,10 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
> > policy->governor->initialized--;
> > } else {
> > /* Restore original values */
> > - mutex_lock(&cpufreq_governor_mutex);
> > if (event == CPUFREQ_GOV_STOP)
> > policy->governor_enabled = true;
> > else if (event == CPUFREQ_GOV_START)
> > policy->governor_enabled = false;
> > - mutex_unlock(&cpufreq_governor_mutex);
> > }
> >
> > if (((event == CPUFREQ_GOV_POLICY_INIT) && ret) ||
> > --
> > 2.2.2
>
> --
> viresh
>
[toc] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-01-18 07:00 +0100 |
| Message-ID | <qSb17-4JX-3@gated-at.bofh.it> |
| In reply to | #1310257 |
On 15-01-16, 16:30, Juri Lelli wrote:
> But governor_enabled seems to not be checked anymore outside cpufreq.c
> (see also 01/19), as it was in the commit you are referring to.
Okay, I must have told you this earlier but anyway ..
governor_enabled was introduced long back to keep governor state
changes serialized. Because of the complex cases we had in hand
(governor-per-policy or system wide governors, etc.), it failed to do
so. Though simple races were avoided with it, complex ones still came
back to haunt us.
We fixed that by managing state changes within ondemand and
conservative governors instead and that worked very well.
Then I wrote a patch to kill the stupid code around governor_enabled
thing, but I got into few races. Those races happened because of
userspace governor, which was getting into invalid states on some
extreme cases (These were caught using the test-suite I wrote and you
perhaps used it).
And I never came back to fix those corner cases ..
You can try that on ARM or x86 by running following command from my
test-suite (I remember that you are using it, right?):
./runme.sh -f sp1 or sp2 or sp3
Only one of sp1, sp2 or sp3 is required..
> Now that
> users of this should be holding policy->rwsem, so that should suffice
> for protecting governor_enabled, as governor_enabled is only changed
> inside here.
If we can get rid of the rwsem dropping problem, then yeah this can be
killed for sure.
> I run some test on a x86 box I setup and didn't see anything related to
> this. I'll wait to get the first 0-day report anyway.
Okay, so run the above test and make sure you have following enabled
in your configuration:
CONFIG_LOCKDEP_SUPPORT=y
CONFIG_DEBUG_RT_MUTEXES=y
CONFIG_DEBUG_PI_LIST=y
CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_MUTEXES=y
CONFIG_DEBUG_LOCK_ALLOC=y
CONFIG_PROVE_LOCKING=y
CONFIG_LOCKDEP=y
CONFIG_DEBUG_ATOMIC_SLEEP=y
> > > - mutex_lock(&cpufreq_governor_mutex);
> > > if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
> > > || (!policy->governor_enabled
> > > && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {
> > > - mutex_unlock(&cpufreq_governor_mutex);
> > > return -EBUSY;
> > > }
> >
> > Actually the above checks should also be removed as the governors are
> > responsible for maintaining their state machines. But
> > userspace/powersave/performance don't have that support yet and so
> > these checks save them from going into undefined states.
> >
> > Over that, above and below checks are incomplete..
> >
>
> You mean we need an additional patch that extends the checks performed?
Yeah, we need to add some state-management code in
userspace/powersave/performance governors as well.
--
viresh
[toc] | [prev] | [next] | [standalone]
| From | Juri Lelli <juri.lelli@arm.com> |
|---|---|
| Date | 2016-01-19 17:50 +0100 |
| Message-ID | <qSHDI-23d-21@gated-at.bofh.it> |
| In reply to | #1311327 |
On 18/01/16 11:20, Viresh Kumar wrote:
> On 15-01-16, 16:30, Juri Lelli wrote:
> > But governor_enabled seems to not be checked anymore outside cpufreq.c
> > (see also 01/19), as it was in the commit you are referring to.
>
> Okay, I must have told you this earlier but anyway ..
>
> governor_enabled was introduced long back to keep governor state
> changes serialized. Because of the complex cases we had in hand
> (governor-per-policy or system wide governors, etc.), it failed to do
> so. Though simple races were avoided with it, complex ones still came
> back to haunt us.
>
> We fixed that by managing state changes within ondemand and
> conservative governors instead and that worked very well.
>
> Then I wrote a patch to kill the stupid code around governor_enabled
> thing, but I got into few races. Those races happened because of
> userspace governor, which was getting into invalid states on some
> extreme cases (These were caught using the test-suite I wrote and you
> perhaps used it).
>
> And I never came back to fix those corner cases ..
>
OK, thanks for the explanation.
> You can try that on ARM or x86 by running following command from my
> test-suite (I remember that you are using it, right?):
>
Yep, I'm constantly running those on my boxes.
> ./runme.sh -f sp1 or sp2 or sp3
>
> Only one of sp1, sp2 or sp3 is required..
>
I'm actually hitting this running sp2, on linux-pm/linux-next :/.
======================================================
[ INFO: possible circular locking dependency detected ]
4.4.0+ #445 Not tainted
-------------------------------------------------------
trace.sh/1723 is trying to acquire lock:
(s_active#48){++++.+}, at: [<c01f78c8>] kernfs_remove_by_name_ns+0x4c/0x94
but task is already holding lock:
(od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (od_dbs_cdata.mutex){+.+.+.}:
[<c075b040>] mutex_lock_nested+0x7c/0x434
[<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4
[<c0017c10>] return_to_handler+0x0/0x18
-> #1 (&policy->rwsem){+++++.}:
[<c075ca8c>] down_read+0x58/0x94
[<c057c244>] show+0x30/0x60
[<c01f934c>] sysfs_kf_seq_show+0x90/0xfc
[<c01f7ad8>] kernfs_seq_show+0x34/0x38
[<c01a22ec>] seq_read+0x1e4/0x4e4
[<c01f8694>] kernfs_fop_read+0x120/0x1a0
[<c01794b4>] __vfs_read+0x3c/0xe0
[<c017a378>] vfs_read+0x98/0x104
[<c017a434>] SyS_read+0x50/0x90
[<c000fd40>] ret_fast_syscall+0x0/0x1c
-> #0 (s_active#48){++++.+}:
[<c008238c>] lock_acquire+0xd4/0x20c
[<c01f6ae4>] __kernfs_remove+0x288/0x328
[<c01f78c8>] kernfs_remove_by_name_ns+0x4c/0x94
[<c01fa024>] remove_files+0x44/0x88
[<c01fa5a4>] sysfs_remove_group+0x50/0xa4
[<c058285c>] cpufreq_governor_dbs+0x3f0/0x5d4
[<c0017c10>] return_to_handler+0x0/0x18
other info that might help us debug this:
Chain exists of:
s_active#48 --> &policy->rwsem --> od_dbs_cdata.mutex
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(od_dbs_cdata.mutex);
lock(&policy->rwsem);
lock(od_dbs_cdata.mutex);
lock(s_active#48);
*** DEADLOCK ***
5 locks held by trace.sh/1723:
#0: (sb_writers#6){.+.+.+}, at: [<c017beb8>] __sb_start_write+0xb4/0xc0
#1: (&of->mutex){+.+.+.}, at: [<c01f8418>] kernfs_fop_write+0x6c/0x1c8
#2: (s_active#35){.+.+.+}, at: [<c01f8420>] kernfs_fop_write+0x74/0x1c8
#3: (cpu_hotplug.lock){++++++}, at: [<c0029e6c>] get_online_cpus+0x48/0xb8
#4: (od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4
stack backtrace:
CPU: 2 PID: 1723 Comm: trace.sh Not tainted 4.4.0+ #445
Hardware name: ARM-Versatile Express
[<c001883c>] (unwind_backtrace) from [<c0013f50>] (show_stack+0x20/0x24)
[<c0013f50>] (show_stack) from [<c044ad90>] (dump_stack+0x80/0xb4)
[<c044ad90>] (dump_stack) from [<c0128edc>] (print_circular_bug+0x29c/0x2f0)
[<c0128edc>] (print_circular_bug) from [<c0081708>] (__lock_acquire+0x163c/0x1d74)
[<c0081708>] (__lock_acquire) from [<c008238c>] (lock_acquire+0xd4/0x20c)
[<c008238c>] (lock_acquire) from [<c01f6ae4>] (__kernfs_remove+0x288/0x328)
[<c01f6ae4>] (__kernfs_remove) from [<c01f78c8>] (kernfs_remove_by_name_ns+0x4c/0x94)
[<c01f78c8>] (kernfs_remove_by_name_ns) from [<c01fa024>] (remove_files+0x44/0x88)
[<c01fa024>] (remove_files) from [<c01fa5a4>] (sysfs_remove_group+0x50/0xa4)
[<c01fa5a4>] (sysfs_remove_group) from [<c058285c>] (cpufreq_governor_dbs+0x3f0/0x5d4)
[<c058285c>] (cpufreq_governor_dbs) from [<c0017c10>] (return_to_handler+0x0/0x18)
Now, I couldn't yet make sense of this, but it seems to be
triggered by setting ondemand, printing its attributes and then
switching to conservative (that's what sp2 does, right?). Also, s_active
seems to come into play only when lockdep is enabled. Are you seeing
this as well?
> > Now that
> > users of this should be holding policy->rwsem, so that should suffice
> > for protecting governor_enabled, as governor_enabled is only changed
> > inside here.
>
> If we can get rid of the rwsem dropping problem, then yeah this can be
> killed for sure.
>
OK.
> > I run some test on a x86 box I setup and didn't see anything related to
> > this. I'll wait to get the first 0-day report anyway.
>
0-day is setup. I didn't yet receive any major bad thing from it :).
> Okay, so run the above test and make sure you have following enabled
> in your configuration:
>
> CONFIG_LOCKDEP_SUPPORT=y
> CONFIG_DEBUG_RT_MUTEXES=y
> CONFIG_DEBUG_PI_LIST=y
> CONFIG_DEBUG_SPINLOCK=y
> CONFIG_DEBUG_MUTEXES=y
> CONFIG_DEBUG_LOCK_ALLOC=y
> CONFIG_PROVE_LOCKING=y
> CONFIG_LOCKDEP=y
> CONFIG_DEBUG_ATOMIC_SLEEP=y
>
Yep, that's what I normally use for developing.
Thanks,
- Juri
> > > > - mutex_lock(&cpufreq_governor_mutex);
> > > > if ((policy->governor_enabled && event == CPUFREQ_GOV_START)
> > > > || (!policy->governor_enabled
> > > > && (event == CPUFREQ_GOV_LIMITS || event == CPUFREQ_GOV_STOP))) {
> > > > - mutex_unlock(&cpufreq_governor_mutex);
> > > > return -EBUSY;
> > > > }
> > >
> > > Actually the above checks should also be removed as the governors are
> > > responsible for maintaining their state machines. But
> > > userspace/powersave/performance don't have that support yet and so
> > > these checks save them from going into undefined states.
> > >
> > > Over that, above and below checks are incomplete..
> > >
> >
> > You mean we need an additional patch that extends the checks performed?
>
> Yeah, we need to add some state-management code in
> userspace/powersave/performance governors as well.
>
> --
> viresh
>
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-01-20 08:30 +0100 |
| Message-ID | <qSVnk-3cB-7@gated-at.bofh.it> |
| In reply to | #1312380 |
On 19-01-16, 16:49, Juri Lelli wrote:
> I'm actually hitting this running sp2, on linux-pm/linux-next :/.
That's really bad .. Are you hitting this on Juno or x86 ?
And I am sure you would have hit that with your changes as well, but
now its on the currently merged patches :(
> ======================================================
> [ INFO: possible circular locking dependency detected ]
> 4.4.0+ #445 Not tainted
> -------------------------------------------------------
> trace.sh/1723 is trying to acquire lock:
> (s_active#48){++++.+}, at: [<c01f78c8>] kernfs_remove_by_name_ns+0x4c/0x94
>
> but task is already holding lock:
> (od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #2 (od_dbs_cdata.mutex){+.+.+.}:
> [<c075b040>] mutex_lock_nested+0x7c/0x434
> [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4
> [<c0017c10>] return_to_handler+0x0/0x18
>
> -> #1 (&policy->rwsem){+++++.}:
> [<c075ca8c>] down_read+0x58/0x94
> [<c057c244>] show+0x30/0x60
> [<c01f934c>] sysfs_kf_seq_show+0x90/0xfc
> [<c01f7ad8>] kernfs_seq_show+0x34/0x38
> [<c01a22ec>] seq_read+0x1e4/0x4e4
> [<c01f8694>] kernfs_fop_read+0x120/0x1a0
> [<c01794b4>] __vfs_read+0x3c/0xe0
> [<c017a378>] vfs_read+0x98/0x104
> [<c017a434>] SyS_read+0x50/0x90
> [<c000fd40>] ret_fast_syscall+0x0/0x1c
>
> -> #0 (s_active#48){++++.+}:
> [<c008238c>] lock_acquire+0xd4/0x20c
> [<c01f6ae4>] __kernfs_remove+0x288/0x328
> [<c01f78c8>] kernfs_remove_by_name_ns+0x4c/0x94
> [<c01fa024>] remove_files+0x44/0x88
> [<c01fa5a4>] sysfs_remove_group+0x50/0xa4
> [<c058285c>] cpufreq_governor_dbs+0x3f0/0x5d4
> [<c0017c10>] return_to_handler+0x0/0x18
>
> other info that might help us debug this:
>
> Chain exists of:
> s_active#48 --> &policy->rwsem --> od_dbs_cdata.mutex
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(od_dbs_cdata.mutex);
> lock(&policy->rwsem);
> lock(od_dbs_cdata.mutex);
> lock(s_active#48);
>
> *** DEADLOCK ***
>
> 5 locks held by trace.sh/1723:
> #0: (sb_writers#6){.+.+.+}, at: [<c017beb8>] __sb_start_write+0xb4/0xc0
> #1: (&of->mutex){+.+.+.}, at: [<c01f8418>] kernfs_fop_write+0x6c/0x1c8
> #2: (s_active#35){.+.+.+}, at: [<c01f8420>] kernfs_fop_write+0x74/0x1c8
> #3: (cpu_hotplug.lock){++++++}, at: [<c0029e6c>] get_online_cpus+0x48/0xb8
> #4: (od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4
>
> stack backtrace:
> CPU: 2 PID: 1723 Comm: trace.sh Not tainted 4.4.0+ #445
> Hardware name: ARM-Versatile Express
> [<c001883c>] (unwind_backtrace) from [<c0013f50>] (show_stack+0x20/0x24)
> [<c0013f50>] (show_stack) from [<c044ad90>] (dump_stack+0x80/0xb4)
> [<c044ad90>] (dump_stack) from [<c0128edc>] (print_circular_bug+0x29c/0x2f0)
> [<c0128edc>] (print_circular_bug) from [<c0081708>] (__lock_acquire+0x163c/0x1d74)
> [<c0081708>] (__lock_acquire) from [<c008238c>] (lock_acquire+0xd4/0x20c)
> [<c008238c>] (lock_acquire) from [<c01f6ae4>] (__kernfs_remove+0x288/0x328)
> [<c01f6ae4>] (__kernfs_remove) from [<c01f78c8>] (kernfs_remove_by_name_ns+0x4c/0x94)
> [<c01f78c8>] (kernfs_remove_by_name_ns) from [<c01fa024>] (remove_files+0x44/0x88)
> [<c01fa024>] (remove_files) from [<c01fa5a4>] (sysfs_remove_group+0x50/0xa4)
> [<c01fa5a4>] (sysfs_remove_group) from [<c058285c>] (cpufreq_governor_dbs+0x3f0/0x5d4)
> [<c058285c>] (cpufreq_governor_dbs) from [<c0017c10>] (return_to_handler+0x0/0x18)
>
> Now, I couldn't yet make sense of this, but it seems to be
> triggered by setting ondemand, printing its attributes and then
> switching to conservative (that's what sp2 does, right?). Also, s_active
> seems to come into play only when lockdep is enabled. Are you seeing
> this as well?
There is something about the platform you are running this on.. I
don't hit it most of the times in my exynos board (Dual A15), but x86
and powerpc guys used to report this all the time. I have tried with
both have-governor-per-policy and otherwise.
I have explained something similar in the earlier commits I pointed to
you, here is the commit log:
http://pastebin.com/JbEJBLzU
--
viresh
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-01-20 11:20 +0100 |
| Message-ID | <qSY1Q-4Y7-27@gated-at.bofh.it> |
| In reply to | #1312951 |
On 20-01-16, 10:17, Juri Lelli wrote: > On 20/01/16 12:59, Viresh Kumar wrote: > > On 19-01-16, 16:49, Juri Lelli wrote: > > > I'm actually hitting this running sp2, on linux-pm/linux-next :/. > > > > That's really bad .. Are you hitting this on Juno or x86 ? > > > > That's on TC2. I'll try to run the same on Juno and x86. Juno will be the same as that also set the per-policy-governor thing :) -- viresh
[toc] | [prev] | [next] | [standalone]
| From | Juri Lelli <juri.lelli@arm.com> |
|---|---|
| Date | 2016-01-20 11:30 +0100 |
| Message-ID | <qSYbw-51L-5@gated-at.bofh.it> |
| In reply to | #1313063 |
On 20/01/16 15:48, Viresh Kumar wrote: > On 20-01-16, 10:17, Juri Lelli wrote: > > On 20/01/16 12:59, Viresh Kumar wrote: > > > On 19-01-16, 16:49, Juri Lelli wrote: > > > > I'm actually hitting this running sp2, on linux-pm/linux-next :/. > > > > > > That's really bad .. Are you hitting this on Juno or x86 ? > > > > > > > That's on TC2. I'll try to run the same on Juno and x86. > > Juno will be the same as that also set the per-policy-governor thing > :) That is what I expect as well, but you never know ;).
[toc] | [prev] | [next] | [standalone]
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Date | 2016-01-20 11:40 +0100 |
| Message-ID | <qSYlc-56e-21@gated-at.bofh.it> |
| In reply to | #1313083 |
On 20-01-16, 10:27, Juri Lelli wrote: > That is what I expect as well, but you never know ;). Perhaps not. We are talking about policy->rwsem here being used while reading the values of governor's sysfs files. And that lock is only taken for single governor case. -- viresh
[toc] | [prev] | [next] | [standalone]
| From | Juri Lelli <juri.lelli@arm.com> |
|---|---|
| Date | 2016-01-20 11:20 +0100 |
| Message-ID | <qSY1Q-4Y7-29@gated-at.bofh.it> |
| In reply to | #1312951 |
On 20/01/16 12:59, Viresh Kumar wrote:
> On 19-01-16, 16:49, Juri Lelli wrote:
> > I'm actually hitting this running sp2, on linux-pm/linux-next :/.
>
> That's really bad .. Are you hitting this on Juno or x86 ?
>
That's on TC2. I'll try to run the same on Juno and x86.
> And I am sure you would have hit that with your changes as well, but
> now its on the currently merged patches :(
>
> > ======================================================
> > [ INFO: possible circular locking dependency detected ]
> > 4.4.0+ #445 Not tainted
> > -------------------------------------------------------
> > trace.sh/1723 is trying to acquire lock:
> > (s_active#48){++++.+}, at: [<c01f78c8>] kernfs_remove_by_name_ns+0x4c/0x94
> >
> > but task is already holding lock:
> > (od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4
> >
> > which lock already depends on the new lock.
> >
> >
> > the existing dependency chain (in reverse order) is:
> >
> > -> #2 (od_dbs_cdata.mutex){+.+.+.}:
> > [<c075b040>] mutex_lock_nested+0x7c/0x434
> > [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4
> > [<c0017c10>] return_to_handler+0x0/0x18
> >
> > -> #1 (&policy->rwsem){+++++.}:
> > [<c075ca8c>] down_read+0x58/0x94
> > [<c057c244>] show+0x30/0x60
> > [<c01f934c>] sysfs_kf_seq_show+0x90/0xfc
> > [<c01f7ad8>] kernfs_seq_show+0x34/0x38
> > [<c01a22ec>] seq_read+0x1e4/0x4e4
> > [<c01f8694>] kernfs_fop_read+0x120/0x1a0
> > [<c01794b4>] __vfs_read+0x3c/0xe0
> > [<c017a378>] vfs_read+0x98/0x104
> > [<c017a434>] SyS_read+0x50/0x90
> > [<c000fd40>] ret_fast_syscall+0x0/0x1c
> >
> > -> #0 (s_active#48){++++.+}:
> > [<c008238c>] lock_acquire+0xd4/0x20c
> > [<c01f6ae4>] __kernfs_remove+0x288/0x328
> > [<c01f78c8>] kernfs_remove_by_name_ns+0x4c/0x94
> > [<c01fa024>] remove_files+0x44/0x88
> > [<c01fa5a4>] sysfs_remove_group+0x50/0xa4
> > [<c058285c>] cpufreq_governor_dbs+0x3f0/0x5d4
> > [<c0017c10>] return_to_handler+0x0/0x18
> >
> > other info that might help us debug this:
> >
> > Chain exists of:
> > s_active#48 --> &policy->rwsem --> od_dbs_cdata.mutex
> >
> > Possible unsafe locking scenario:
> >
> > CPU0 CPU1
> > ---- ----
> > lock(od_dbs_cdata.mutex);
> > lock(&policy->rwsem);
> > lock(od_dbs_cdata.mutex);
> > lock(s_active#48);
> >
> > *** DEADLOCK ***
> >
> > 5 locks held by trace.sh/1723:
> > #0: (sb_writers#6){.+.+.+}, at: [<c017beb8>] __sb_start_write+0xb4/0xc0
> > #1: (&of->mutex){+.+.+.}, at: [<c01f8418>] kernfs_fop_write+0x6c/0x1c8
> > #2: (s_active#35){.+.+.+}, at: [<c01f8420>] kernfs_fop_write+0x74/0x1c8
> > #3: (cpu_hotplug.lock){++++++}, at: [<c0029e6c>] get_online_cpus+0x48/0xb8
> > #4: (od_dbs_cdata.mutex){+.+.+.}, at: [<c05824a0>] cpufreq_governor_dbs+0x34/0x5d4
> >
> > stack backtrace:
> > CPU: 2 PID: 1723 Comm: trace.sh Not tainted 4.4.0+ #445
> > Hardware name: ARM-Versatile Express
> > [<c001883c>] (unwind_backtrace) from [<c0013f50>] (show_stack+0x20/0x24)
> > [<c0013f50>] (show_stack) from [<c044ad90>] (dump_stack+0x80/0xb4)
> > [<c044ad90>] (dump_stack) from [<c0128edc>] (print_circular_bug+0x29c/0x2f0)
> > [<c0128edc>] (print_circular_bug) from [<c0081708>] (__lock_acquire+0x163c/0x1d74)
> > [<c0081708>] (__lock_acquire) from [<c008238c>] (lock_acquire+0xd4/0x20c)
> > [<c008238c>] (lock_acquire) from [<c01f6ae4>] (__kernfs_remove+0x288/0x328)
> > [<c01f6ae4>] (__kernfs_remove) from [<c01f78c8>] (kernfs_remove_by_name_ns+0x4c/0x94)
> > [<c01f78c8>] (kernfs_remove_by_name_ns) from [<c01fa024>] (remove_files+0x44/0x88)
> > [<c01fa024>] (remove_files) from [<c01fa5a4>] (sysfs_remove_group+0x50/0xa4)
> > [<c01fa5a4>] (sysfs_remove_group) from [<c058285c>] (cpufreq_governor_dbs+0x3f0/0x5d4)
> > [<c058285c>] (cpufreq_governor_dbs) from [<c0017c10>] (return_to_handler+0x0/0x18)
> >
> > Now, I couldn't yet make sense of this, but it seems to be
> > triggered by setting ondemand, printing its attributes and then
> > switching to conservative (that's what sp2 does, right?). Also, s_active
> > seems to come into play only when lockdep is enabled. Are you seeing
> > this as well?
>
> There is something about the platform you are running this on.. I
> don't hit it most of the times in my exynos board (Dual A15), but x86
> and powerpc guys used to report this all the time. I have tried with
> both have-governor-per-policy and otherwise.
>
> I have explained something similar in the earlier commits I pointed to
> you, here is the commit log:
>
> http://pastebin.com/JbEJBLzU
>
Yeah, saw that. I guess I have to stare at this thing more.
Thanks,
- Juri
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web