Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440555 > unrolled thread

[patch 61/66] timers: Convert to hotplug state machine

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.

Contents

  [patch 61/66] timers: Convert to hotplug state machine Anna-Maria Gleixner <anna-maria@linutronix.de> - 2016-07-11 14:40 +0200
    Re: [patch 61/66] timers: Convert to hotplug state machine Jon Hunter <jonathanh@nvidia.com> - 2016-07-25 17:00 +0200
      Re: [patch 61/66] timers: Convert to hotplug state machine rcochran@linutronix.de - 2016-07-25 17:40 +0200
        Re: [patch 61/66] timers: Convert to hotplug state machine rcochran@linutronix.de - 2016-07-25 22:50 +0200

#1440555 — [patch 61/66] timers: Convert to hotplug state machine

From: Richard Cochran <rcochran@linutronix.de>

When tearing down, call timers_dead_cpu before notify_dead.
There is a hidden dependency between:

- timers
- Block multiqueue
- rcutree

If timers_dead_cpu() comes later than blk_mq_queue_reinit_notify()
that latter function causes a RCU stall.

Signed-off-by: Richard Cochran <rcochran@linutronix.de>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>

---
 include/linux/cpuhotplug.h |    1 +
 include/linux/timer.h      |    6 ++++++
 kernel/cpu.c               |    5 +++++
 kernel/time/timer.c        |   25 ++-----------------------
 4 files changed, 14 insertions(+), 23 deletions(-)

--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -18,6 +18,7 @@ enum cpuhp_state {
 	CPUHP_RCUTREE_PREP,
 	CPUHP_POWER_NUMA_PREPARE,
 	CPUHP_HRTIMERS_PREPARE,
+	CPUHP_TIMERS_DEAD,
 	CPUHP_NOTIFY_PREPARE,
 	CPUHP_BRINGUP_CPU,
 	CPUHP_AP_IDLE_DEAD,
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -259,4 +259,10 @@ unsigned long __round_jiffies_up_relativ
 unsigned long round_jiffies_up(unsigned long j);
 unsigned long round_jiffies_up_relative(unsigned long j);
 
+#ifdef CONFIG_HOTPLUG_CPU
+int timers_dead_cpu(unsigned int cpu);
+#else
+#define timers_dead_cpu NULL
+#endif
+
 #endif
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1194,6 +1194,11 @@ static struct cpuhp_step cpuhp_bp_states
 		.startup = hrtimers_prepare_cpu,
 		.teardown = hrtimers_dead_cpu,
 	},
+	[CPUHP_TIMERS_DEAD] = {
+		.name = "timers dead",
+		.startup = NULL,
+		.teardown = timers_dead_cpu,
+	},
 	/*
 	 * Preparatory and dead notifiers. Will be replaced once the notifiers
 	 * are converted to states.
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1577,7 +1577,7 @@ static void migrate_timer_list(struct tv
 	}
 }
 
-static void migrate_timers(int cpu)
+int timers_dead_cpu(unsigned int cpu)
 {
 	struct tvec_base *old_base;
 	struct tvec_base *new_base;
@@ -1610,29 +1610,9 @@ static void migrate_timers(int cpu)
 	spin_unlock(&old_base->lock);
 	spin_unlock_irq(&new_base->lock);
 	put_cpu_ptr(&tvec_bases);
+	return 0;
 }
 
-static int timer_cpu_notify(struct notifier_block *self,
-				unsigned long action, void *hcpu)
-{
-	switch (action) {
-	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
-		migrate_timers((long)hcpu);
-		break;
-	default:
-		break;
-	}
-
-	return NOTIFY_OK;
-}
-
-static inline void timer_register_cpu_notifier(void)
-{
-	cpu_notifier(timer_cpu_notify, 0);
-}
-#else
-static inline void timer_register_cpu_notifier(void) { }
 #endif /* CONFIG_HOTPLUG_CPU */
 
 static void __init init_timer_cpu(int cpu)
@@ -1658,7 +1638,6 @@ void __init init_timers(void)
 {
 	init_timer_cpus();
 	init_timer_stats();
-	timer_register_cpu_notifier();
 	open_softirq(TIMER_SOFTIRQ, run_timer_softirq);
 }
 

[toc] | [next] | [standalone]

#1449558

Hi Richard,

On 11/07/16 13:29, Anna-Maria Gleixner wrote:
> From: Richard Cochran <rcochran@linutronix.de>
> 
> When tearing down, call timers_dead_cpu before notify_dead.
> There is a hidden dependency between:
>
> - timers
> - Block multiqueue
> - rcutree
>
> If timers_dead_cpu() comes later than blk_mq_queue_reinit_notify()
> that latter function causes a RCU stall.

After this change is applied I am seeing RCU stalls during suspend
on Tegra. I guess I am hitting the case mentioned above? How should
this be avoided?

[    5.321824] PM: Syncing filesystems ... done.
[    5.349746] Freezing user space processes ... (elapsed 0.001 seconds) done.
[    5.358122] Double checking all user space processes after OOM killer disable... (elapsed 0.000 seconds) 
[    5.367817] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
[    5.376746] Suspending console(s) (use no_console_suspend to debug)
[    5.427213] PM: suspend of devices complete after 42.812 msecs
[    5.429909] PM: late suspend of devices complete after 2.680 msecs
[    5.431968] PM: noirq suspend of devices complete after 2.049 msecs
[    5.431973] Disabling non-boot CPUs ...
[    5.432861] CPU1: shutdown
[    5.467806] CPU2: shutdown
[    5.506925] IRQ17 no longer affine to CPU3
[    5.507294] CPU3: shutdown
[   26.509992] INFO: rcu_sched detected stalls on CPUs/tasks:
[   26.510005]  3-O.N: (0 ticks this GP) idle=e13/140000000000000/0 softirq=86/86 fqs=0 
[   26.510016]  (detected by 0, t=4202 jiffies, g=-225, c=-226, q=23)
[   26.510020] Task dump for CPU 3:
[   26.510033] swapper/3       R running      0     0      1 0x00000000
[   26.510063] [<c0b79fac>] (__schedule) from [<c033b808>] (tegra_cpu_die+0x30/0x48)
[   26.510080] [<c033b808>] (tegra_cpu_die) from [<c030dd4c>] (arch_cpu_idle_dead+0x44/0x88)
[   26.510094] [<c030dd4c>] (arch_cpu_idle_dead) from [<c03794bc>] (cpu_startup_entry+0x1c0/0x220)
[   26.510106] [<c03794bc>] (cpu_startup_entry) from [<80301c2c>] (0x80301c2c)
[   26.510116] rcu_sched kthread starved for 4202 jiffies! g4294967071 c4294967070 f0x0 RCU_GP_WAIT_FQS(3) ->state=0x1
[   26.510128] rcu_sched       S c0b79fac     0     7      2 0x00000000
[   26.510139] [<c0b79fac>] (__schedule) from [<c0b7a434>] (schedule+0x38/0x9c)
[   26.510152] [<c0b7a434>] (schedule) from [<c0b7cf3c>] (schedule_timeout+0x158/0x21c)
[   26.510166] [<c0b7cf3c>] (schedule_timeout) from [<c03922e0>] (rcu_gp_kthread+0x414/0x99c)
[   26.510179] [<c03922e0>] (rcu_gp_kthread) from [<c035cdb8>] (kthread+0xd8/0xf4)
[   26.510191] [<c035cdb8>] (kthread) from [<c0307fb8>] (ret_from_fork+0x14/0x3c)
[   26.531238] Enabling non-boot CPUs ...
[   26.546568] CPU1 is up
[   26.566858] CPU2 is up
[   26.587169] CPU3 is up
[   26.588470] PM: noirq resume of devices complete after 1.290 msecs
[   26.591329] PM: early resume of devices complete after 2.574 msecs
[   26.696785] PM: resume of devices complete after 105.439 msecs
[   26.876814] Restarting tasks ... done.

Interestingly I am only seeing the above when using the ARM
multi_v7_defconfig kernel configuration and not with the tegra_defconfig.
One key difference between these is that the multi_v7_defconfig does not
have CONFIG_PREEMPT enabled. Initial testing shows enabling CONFIG_PREEMPT
for multi_v7_defconfig makes the problem go away.

Cheers
Jon

[toc] | [prev] | [next] | [standalone]

#1449594

On Mon, Jul 25, 2016 at 03:56:48PM +0100, Jon Hunter wrote:
> > There is a hidden dependency between:
> >
> > - timers
> > - Block multiqueue
> > - rcutree
> >
> > If timers_dead_cpu() comes later than blk_mq_queue_reinit_notify()
> > that latter function causes a RCU stall.
> 
> After this change is applied I am seeing RCU stalls during suspend
> on Tegra. I guess I am hitting the case mentioned above? How should
> this be avoided?

The problem that I had found was a hidden dependency.  When I
initially placed the timers callback into the new HP state list, that
caused a stall because the dependency was broken.  The old code worked
by luck, based on the order of the notifier registrations.  The new
code makes the old implicit ordering explicit, so it should work just
as well as before (famous last words).

> Interestingly I am only seeing the above when using the ARM
> multi_v7_defconfig kernel configuration and not with the tegra_defconfig.
> One key difference between these is that the multi_v7_defconfig does not
> have CONFIG_PREEMPT enabled. Initial testing shows enabling CONFIG_PREEMPT
> for multi_v7_defconfig makes the problem go away.

Just to be sure, this problem didn't exist before the HP rework, that
is, suspend worked fine with and without CONFIG_PREEMPT, right?

I see if I can find a tegra system to test with...

Thanks,
Richard

[toc] | [prev] | [next] | [standalone]

#1449790

On Mon, Jul 25, 2016 at 05:35:43PM +0200, rcochran@linutronix.de wrote:
> I see if I can find a tegra system to test with...

I tried the tip:smp/hotplug branch under kvm x86_64, and I didn't see
any problems with suspend or hibernate, even with CONFIG_PREEMPT_NONE.
I'll see if I can get my hands on a tegra system tomorrow.  Until
then, two questions:

1. The issue appears with suspend (echo mem > /sys/power/state), as
   opposed to hibernate, right?

2. CONFIG_PREEMPT_NONE is good, but CONFIG_PREEMPT is bad, right?

Thanks,
Richard

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web