* [PATCH v2] timekeeping: Allow runtime PM from change_clocksource()
@ 2021-02-11 13:43 Niklas Söderlund
2021-03-03 11:04 ` Wolfram Sang
2021-03-29 14:47 ` [tip: timers/core] " tip-bot2 for Niklas Söderlund
0 siblings, 2 replies; 3+ messages in thread
From: Niklas Söderlund @ 2021-02-11 13:43 UTC (permalink / raw)
To: John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel
Cc: linux-renesas-soc, Niklas Söderlund
The struct clocksource callbacks enable() and disable() are described as
a way to allow clock sources to enter a power save mode [1]. But using
runtime PM from these callbacks triggers a cyclic lockdep warning when
switching clock source using change_clocksource().
This change allows the new clocksource to be enabled() and the old one
to be disabled() without holding the timekeeper_lock. This solution is
modeled on timekeeping_resume() and timekeeping_suspend() where the
struct clocksource resume() and suspend() callbacks are called without
holding the timekeeper_lock.
Triggering and log of the deadlock warning,
# echo e60f0000.timer > /sys/devices/system/clocksource/clocksource0/current_clocksource
[ 118.081095] ======================================================
[ 118.087455] WARNING: possible circular locking dependency detected
[ 118.093821] 5.10.0-rc6-arm64-renesas-00002-ga0cf8106e584 #37 Not tainted
[ 118.100712] ------------------------------------------------------
[ 118.107069] migration/0/11 is trying to acquire lock:
[ 118.112269] ffff0000403ed220 (&dev->power.lock){-...}-{2:2}, at: __pm_runtime_resume+0x40/0x74
[ 118.121172]
[ 118.121172] but task is already holding lock:
[ 118.127170] ffff8000113c8f88 (tk_core.seq.seqcount){----}-{0:0}, at: multi_cpu_stop+0xa4/0x190
[ 118.136061]
[ 118.136061] which lock already depends on the new lock.
[ 118.136061]
[ 118.144461]
[ 118.144461] the existing dependency chain (in reverse order) is:
[ 118.152149]
[ 118.152149] -> #2 (tk_core.seq.seqcount){----}-{0:0}:
[ 118.158900] lock_acquire.part.0+0x120/0x330
[ 118.163834] lock_acquire+0x64/0x80
[ 118.167971] seqcount_lockdep_reader_access.constprop.0+0x74/0x100
[ 118.174865] ktime_get+0x28/0xa0
[ 118.178729] hrtimer_start_range_ns+0x210/0x2dc
[ 118.183934] generic_sched_clock_init+0x70/0x88
[ 118.189134] sched_clock_init+0x40/0x64
[ 118.193622] start_kernel+0x494/0x524
[ 118.197926]
[ 118.197926] -> #1 (hrtimer_bases.lock){-.-.}-{2:2}:
[ 118.204491] lock_acquire.part.0+0x120/0x330
[ 118.209424] lock_acquire+0x64/0x80
[ 118.213557] _raw_spin_lock_irqsave+0x7c/0xc4
[ 118.218579] hrtimer_start_range_ns+0x68/0x2dc
[ 118.223690] rpm_suspend+0x308/0x5dc
[ 118.227909] rpm_idle+0xc4/0x2a4
[ 118.231771] pm_runtime_work+0x98/0xc0
[ 118.236171] process_one_work+0x294/0x6f0
[ 118.240836] worker_thread+0x70/0x45c
[ 118.245143] kthread+0x154/0x160
[ 118.249007] ret_from_fork+0x10/0x20
[ 118.253222]
[ 118.253222] -> #0 (&dev->power.lock){-...}-{2:2}:
[ 118.259607] check_noncircular+0x128/0x140
[ 118.268774] __lock_acquire+0x13b0/0x204c
[ 118.277780] lock_acquire.part.0+0x120/0x330
[ 118.287001] lock_acquire+0x64/0x80
[ 118.295375] _raw_spin_lock_irqsave+0x7c/0xc4
[ 118.304623] __pm_runtime_resume+0x40/0x74
[ 118.313644] sh_cmt_start+0x1c4/0x260
[ 118.322275] sh_cmt_clocksource_enable+0x28/0x50
[ 118.331891] change_clocksource+0x9c/0x160
[ 118.340910] multi_cpu_stop+0xa4/0x190
[ 118.349522] cpu_stopper_thread+0x90/0x154
[ 118.358429] smpboot_thread_fn+0x244/0x270
[ 118.367265] kthread+0x154/0x160
[ 118.375158] ret_from_fork+0x10/0x20
[ 118.383284]
[ 118.383284] other info that might help us debug this:
[ 118.383284]
[ 118.402810] Chain exists of:
[ 118.402810] &dev->power.lock --> hrtimer_bases.lock --> tk_core.seq.seqcount
[ 118.402810]
[ 118.425597] Possible unsafe locking scenario:
[ 118.425597]
[ 118.439130] CPU0 CPU1
[ 118.447413] ---- ----
[ 118.455641] lock(tk_core.seq.seqcount);
[ 118.463335] lock(hrtimer_bases.lock);
[ 118.473507] lock(tk_core.seq.seqcount);
[ 118.483787] lock(&dev->power.lock);
[ 118.491120]
[ 118.491120] *** DEADLOCK ***
[ 118.491120]
[ 118.507666] 2 locks held by migration/0/11:
[ 118.515424] #0: ffff8000113c9278 (timekeeper_lock){-.-.}-{2:2}, at: change_clocksource+0x2c/0x160
[ 118.528257] #1: ffff8000113c8f88 (tk_core.seq.seqcount){----}-{0:0}, at: multi_cpu_stop+0xa4/0x190
[ 118.541248]
[ 118.541248] stack backtrace:
[ 118.553226] CPU: 0 PID: 11 Comm: migration/0 Not tainted 5.10.0-rc6-arm64-renesas-00002-ga0cf8106e584 #37
[ 118.566923] Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
[ 118.579051] Call trace:
[ 118.585649] dump_backtrace+0x0/0x190
[ 118.593505] show_stack+0x14/0x30
[ 118.601001] dump_stack+0xe8/0x130
[ 118.608567] print_circular_bug+0x1f0/0x200
[ 118.616930] check_noncircular+0x128/0x140
[ 118.625231] __lock_acquire+0x13b0/0x204c
[ 118.633451] lock_acquire.part.0+0x120/0x330
[ 118.641958] lock_acquire+0x64/0x80
[ 118.649630] _raw_spin_lock_irqsave+0x7c/0xc4
[ 118.658186] __pm_runtime_resume+0x40/0x74
[ 118.666483] sh_cmt_start+0x1c4/0x260
[ 118.674327] sh_cmt_clocksource_enable+0x28/0x50
[ 118.683170] change_clocksource+0x9c/0x160
[ 118.691460] multi_cpu_stop+0xa4/0x190
[ 118.699384] cpu_stopper_thread+0x90/0x154
[ 118.707672] smpboot_thread_fn+0x244/0x270
[ 118.715961] kthread+0x154/0x160
[ 118.723360] ret_from_fork+0x10/0x20
[ 118.731465] clocksource: Switched to clocksource e60f0000.timer
1. commit 4614e6adafa2c5e6 ("clocksource: add enable() and disable() callbacks")
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
kernel/time/timekeeping.c | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 6aee5768c86ff7db..26ef4c9ef5c57081 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1427,35 +1427,45 @@ static void __timekeeping_set_tai_offset(struct timekeeper *tk, s32 tai_offset)
static int change_clocksource(void *data)
{
struct timekeeper *tk = &tk_core.timekeeper;
- struct clocksource *new, *old;
+ struct clocksource *new, *old = NULL;
unsigned long flags;
+ bool change = false;
new = (struct clocksource *) data;
- raw_spin_lock_irqsave(&timekeeper_lock, flags);
- write_seqcount_begin(&tk_core.seq);
-
- timekeeping_forward_now(tk);
/*
* If the cs is in module, get a module reference. Succeeds
* for built-in code (owner == NULL) as well.
*/
if (try_module_get(new->owner)) {
- if (!new->enable || new->enable(new) == 0) {
- old = tk->tkr_mono.clock;
- tk_setup_internals(tk, new);
- if (old->disable)
- old->disable(old);
- module_put(old->owner);
- } else {
+ if (!new->enable || new->enable(new) == 0)
+ change = true;
+ else
module_put(new->owner);
- }
}
+
+ raw_spin_lock_irqsave(&timekeeper_lock, flags);
+ write_seqcount_begin(&tk_core.seq);
+
+ timekeeping_forward_now(tk);
+
+ if (change) {
+ old = tk->tkr_mono.clock;
+ tk_setup_internals(tk, new);
+ }
+
timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);
write_seqcount_end(&tk_core.seq);
raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+ if (old) {
+ if (old->disable)
+ old->disable(old);
+
+ module_put(old->owner);
+ }
+
return 0;
}
--
2.30.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] timekeeping: Allow runtime PM from change_clocksource()
2021-02-11 13:43 [PATCH v2] timekeeping: Allow runtime PM from change_clocksource() Niklas Söderlund
@ 2021-03-03 11:04 ` Wolfram Sang
2021-03-29 14:47 ` [tip: timers/core] " tip-bot2 for Niklas Söderlund
1 sibling, 0 replies; 3+ messages in thread
From: Wolfram Sang @ 2021-03-03 11:04 UTC (permalink / raw)
To: Niklas Söderlund
Cc: John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel,
linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]
On Thu, Feb 11, 2021 at 02:43:18PM +0100, Niklas Söderlund wrote:
> The struct clocksource callbacks enable() and disable() are described as
> a way to allow clock sources to enter a power save mode [1]. But using
> runtime PM from these callbacks triggers a cyclic lockdep warning when
> switching clock source using change_clocksource().
>
> This change allows the new clocksource to be enabled() and the old one
> to be disabled() without holding the timekeeper_lock. This solution is
> modeled on timekeeping_resume() and timekeeping_suspend() where the
> struct clocksource resume() and suspend() callbacks are called without
> holding the timekeeper_lock.
>
> Triggering and log of the deadlock warning,
>
> # echo e60f0000.timer > /sys/devices/system/clocksource/clocksource0/current_clocksource
...
I had the same issue when running 'clocksource-switch' from the Kernel
selftests with a Renesas Falcon board using a R-Car V3U. This patch
fixed the deadlock warning, so:
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tip: timers/core] timekeeping: Allow runtime PM from change_clocksource()
2021-02-11 13:43 [PATCH v2] timekeeping: Allow runtime PM from change_clocksource() Niklas Söderlund
2021-03-03 11:04 ` Wolfram Sang
@ 2021-03-29 14:47 ` tip-bot2 for Niklas Söderlund
1 sibling, 0 replies; 3+ messages in thread
From: tip-bot2 for Niklas Söderlund @ 2021-03-29 14:47 UTC (permalink / raw)
To: linux-tip-commits
Cc: niklas.soderlund+renesas, Thomas Gleixner, Wolfram Sang, x86,
linux-kernel
The following commit has been merged into the timers/core branch of tip:
Commit-ID: d4c7c28806616809e3baa0b7cd8c665516b2726d
Gitweb: https://git.kernel.org/tip/d4c7c28806616809e3baa0b7cd8c665516b2726d
Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
AuthorDate: Thu, 11 Feb 2021 14:43:18 +01:00
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 29 Mar 2021 16:41:59 +02:00
timekeeping: Allow runtime PM from change_clocksource()
The struct clocksource callbacks enable() and disable() are described as a
way to allow clock sources to enter a power save mode. See commit
4614e6adafa2 ("clocksource: add enable() and disable() callbacks")
But using runtime PM from these callbacks triggers a cyclic lockdep warning when
switching clock source using change_clocksource().
# echo e60f0000.timer > /sys/devices/system/clocksource/clocksource0/current_clocksource
======================================================
WARNING: possible circular locking dependency detected
------------------------------------------------------
migration/0/11 is trying to acquire lock:
ffff0000403ed220 (&dev->power.lock){-...}-{2:2}, at: __pm_runtime_resume+0x40/0x74
but task is already holding lock:
ffff8000113c8f88 (tk_core.seq.seqcount){----}-{0:0}, at: multi_cpu_stop+0xa4/0x190
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (tk_core.seq.seqcount){----}-{0:0}:
ktime_get+0x28/0xa0
hrtimer_start_range_ns+0x210/0x2dc
generic_sched_clock_init+0x70/0x88
sched_clock_init+0x40/0x64
start_kernel+0x494/0x524
-> #1 (hrtimer_bases.lock){-.-.}-{2:2}:
hrtimer_start_range_ns+0x68/0x2dc
rpm_suspend+0x308/0x5dc
rpm_idle+0xc4/0x2a4
pm_runtime_work+0x98/0xc0
process_one_work+0x294/0x6f0
worker_thread+0x70/0x45c
kthread+0x154/0x160
ret_from_fork+0x10/0x20
-> #0 (&dev->power.lock){-...}-{2:2}:
_raw_spin_lock_irqsave+0x7c/0xc4
__pm_runtime_resume+0x40/0x74
sh_cmt_start+0x1c4/0x260
sh_cmt_clocksource_enable+0x28/0x50
change_clocksource+0x9c/0x160
multi_cpu_stop+0xa4/0x190
cpu_stopper_thread+0x90/0x154
smpboot_thread_fn+0x244/0x270
kthread+0x154/0x160
ret_from_fork+0x10/0x20
other info that might help us debug this:
Chain exists of:
&dev->power.lock --> hrtimer_bases.lock --> tk_core.seq.seqcount
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(tk_core.seq.seqcount);
lock(hrtimer_bases.lock);
lock(tk_core.seq.seqcount);
lock(&dev->power.lock);
*** DEADLOCK ***
2 locks held by migration/0/11:
#0: ffff8000113c9278 (timekeeper_lock){-.-.}-{2:2}, at: change_clocksource+0x2c/0x160
#1: ffff8000113c8f88 (tk_core.seq.seqcount){----}-{0:0}, at: multi_cpu_stop+0xa4/0x190
Rework change_clocksource() so it enables the new clocksource and disables
the old clocksource outside of the timekeeper_lock and seqcount write held
region. There is no requirement that these callbacks are invoked from the
lock held region.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Link: https://lore.kernel.org/r/20210211134318.323910-1-niklas.soderlund+renesas@ragnatech.se
---
kernel/time/timekeeping.c | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 77bafd8..81fe2a3 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1427,35 +1427,45 @@ static void __timekeeping_set_tai_offset(struct timekeeper *tk, s32 tai_offset)
static int change_clocksource(void *data)
{
struct timekeeper *tk = &tk_core.timekeeper;
- struct clocksource *new, *old;
+ struct clocksource *new, *old = NULL;
unsigned long flags;
+ bool change = false;
new = (struct clocksource *) data;
- raw_spin_lock_irqsave(&timekeeper_lock, flags);
- write_seqcount_begin(&tk_core.seq);
-
- timekeeping_forward_now(tk);
/*
* If the cs is in module, get a module reference. Succeeds
* for built-in code (owner == NULL) as well.
*/
if (try_module_get(new->owner)) {
- if (!new->enable || new->enable(new) == 0) {
- old = tk->tkr_mono.clock;
- tk_setup_internals(tk, new);
- if (old->disable)
- old->disable(old);
- module_put(old->owner);
- } else {
+ if (!new->enable || new->enable(new) == 0)
+ change = true;
+ else
module_put(new->owner);
- }
}
+
+ raw_spin_lock_irqsave(&timekeeper_lock, flags);
+ write_seqcount_begin(&tk_core.seq);
+
+ timekeeping_forward_now(tk);
+
+ if (change) {
+ old = tk->tkr_mono.clock;
+ tk_setup_internals(tk, new);
+ }
+
timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);
write_seqcount_end(&tk_core.seq);
raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+ if (old) {
+ if (old->disable)
+ old->disable(old);
+
+ module_put(old->owner);
+ }
+
return 0;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-29 14:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-11 13:43 [PATCH v2] timekeeping: Allow runtime PM from change_clocksource() Niklas Söderlund
2021-03-03 11:04 ` Wolfram Sang
2021-03-29 14:47 ` [tip: timers/core] " tip-bot2 for Niklas Söderlund
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).