[patch 0/2] tic/broadcast: Plug a few corner cases which cause malfunction

[patch 0/2] tic/broadcast: Plug a few corner cases which cause malfunction

[Thomas Gleixner]

The following two patches address shortcomings in the tick broadcast
code, which were reported and analyzed by Sudeep Holla and Andriy
Gapon.

Thanks,

	tglx

[patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Thomas Gleixner]

[-- Attachment #1: broadcast.diff --]
[-- Type: text/plain, Size: 8176 bytes --]

If no broadcast device is installed and the cpu local timers stop in
deeper idle states, then there is currently nothing telling the idle
code that it should not go into deep idle states, so the timers stop
and nothing wakes up the cpus.

Make the broadcast_enter/exit() functions independent of the
configuration options and always check on enter:

- whether the cpu local device is affected by idle states
- whether a broadcast device is available

This covers all possible config combinations including
CONFIG_BROADCAST=n.

Reported-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Suzuki Poulose <Suzuki.Poulose@arm.com>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

---
 include/linux/tick.h         |    4 -
 kernel/time/tick-broadcast.c |   97 +++++++++++++++++++++++++------------------
 kernel/time/tick-common.c    |   21 +++++++++
 kernel/time/tick-sched.h     |   10 ++++
 4 files changed, 88 insertions(+), 44 deletions(-)

Index: tip/include/linux/tick.h
===================================================================
--- tip.orig/include/linux/tick.h
+++ tip/include/linux/tick.h
@@ -63,11 +63,7 @@ extern void tick_broadcast_control(enum
 static inline void tick_broadcast_control(enum tick_broadcast_mode mode) { }
 #endif /* BROADCAST */
 
-#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
 extern int tick_broadcast_oneshot_control(enum tick_broadcast_state state);
-#else
-static inline int tick_broadcast_oneshot_control(enum tick_broadcast_state state) { return 0; }
-#endif
 
 static inline void tick_broadcast_enable(void)
 {
Index: tip/kernel/time/tick-broadcast.c
===================================================================
--- tip.orig/kernel/time/tick-broadcast.c
+++ tip/kernel/time/tick-broadcast.c
@@ -359,7 +359,14 @@ void tick_broadcast_control(enum tick_br
 	case TICK_BROADCAST_ON:
 		cpumask_set_cpu(cpu, tick_broadcast_on);
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask)) {
-			if (tick_broadcast_device.mode ==
+			/*
+			 * Only shutdown the cpu local device, if the
+			 * broadcast device exists and is in periodic
+			 * mode. The latter check prevents a hickup
+			 * during the switch from periodic to oneshot
+			 * mode.
+			 */
+			if (bc != NULL && tick_broadcast_device.mode ==
 			    TICKDEV_MODE_PERIODIC)
 				clockevents_shutdown(dev);
 		}
@@ -662,46 +669,43 @@ static void broadcast_shutdown_local(str
 	clockevents_switch_state(dev, CLOCK_EVT_STATE_SHUTDOWN);
 }
 
-/**
- * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode
- * @state:	The target state (enter/exit)
- *
- * The system enters/leaves a state, where affected devices might stop
- * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups.
- *
- * Called with interrupts disabled, so clockevents_lock is not
- * required here because the local clock event device cannot go away
- * under us.
- */
-int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 {
 	struct clock_event_device *bc, *dev;
-	struct tick_device *td;
 	int cpu, ret = 0;
 	ktime_t now;
 
 	/*
-	 * Periodic mode does not care about the enter/exit of power
-	 * states
+	 * If there is no broadcast device, tell the caller not to go
+	 * into deep idle.
 	 */
-	if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
-		return 0;
-
-	/*
-	 * We are called with preemtion disabled from the depth of the
-	 * idle code, so we can't be moved away.
-	 */
-	td = this_cpu_ptr(&tick_cpu_device);
-	dev = td->evtdev;
+	if (!tick_broadcast_device.evtdev)
+		return -EBUSY;
 
-	if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
-		return 0;
+	dev = this_cpu_ptr(&tick_cpu_device)->evtdev;
 
 	raw_spin_lock(&tick_broadcast_lock);
 	bc = tick_broadcast_device.evtdev;
 	cpu = smp_processor_id();
 
 	if (state == TICK_BROADCAST_ENTER) {
+		/*
+		 * If the current CPU owns the hrtimer broadcast
+		 * mechanism, it cannot go deep idle and we do not add
+		 * the CPU to the broadcast mask. We don't have to go
+		 * through the EXIT path as the local timer is not
+		 * shutdown.
+		 */
+		ret = broadcast_needs_cpu(bc, cpu);
+
+		/*
+		 * If the hrtimer broadcast check tells us that the
+		 * cpu cannot go deep idle, or if the broadcast device
+		 * is in periodic mode, we just return.
+		 */
+		if (ret || tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
+			goto out;
+
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_oneshot_mask)) {
 			WARN_ON_ONCE(cpumask_test_cpu(cpu, tick_broadcast_pending_mask));
 			broadcast_shutdown_local(bc, dev);
@@ -711,22 +715,28 @@ int tick_broadcast_oneshot_control(enum
 			 * if the cpu local event is earlier than the
 			 * broadcast event. If the current CPU is in
 			 * the force mask, then we are going to be
-			 * woken by the IPI right away.
+			 * woken by the IPI right away; we return
+			 * busy, so the CPU does not try to go deep
+			 * idle.
 			 */
-			if (!cpumask_test_cpu(cpu, tick_broadcast_force_mask) &&
-			    dev->next_event.tv64 < bc->next_event.tv64)
+			if (cpumask_test_cpu(cpu, tick_broadcast_force_mask)) {
+				ret = -EBUSY;
+			} else if (dev->next_event.tv64 < bc->next_event.tv64) {
 				tick_broadcast_set_event(bc, cpu, dev->next_event);
+				/*
+				 * In case of hrtimer broadcasts the
+				 * programming might have moved the
+				 * timer to this cpu. If yes, remove
+				 * us from the broadcast mask and
+				 * return busy.
+				 */
+				ret = broadcast_needs_cpu(bc, cpu);
+				if (ret) {
+					cpumask_clear_cpu(cpu,
+						tick_broadcast_oneshot_mask);
+				}
+			}
 		}
-		/*
-		 * If the current CPU owns the hrtimer broadcast
-		 * mechanism, it cannot go deep idle and we remove the
-		 * CPU from the broadcast mask. We don't have to go
-		 * through the EXIT path as the local timer is not
-		 * shutdown.
-		 */
-		ret = broadcast_needs_cpu(bc, cpu);
-		if (ret)
-			cpumask_clear_cpu(cpu, tick_broadcast_oneshot_mask);
 	} else {
 		if (cpumask_test_and_clear_cpu(cpu, tick_broadcast_oneshot_mask)) {
 			clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT);
@@ -938,6 +948,13 @@ bool tick_broadcast_oneshot_available(vo
 	return bc ? bc->features & CLOCK_EVT_FEAT_ONESHOT : false;
 }
 
+#else
+int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+{
+	if (!tick_broadcast_device.evtdev)
+		return -EBUSY;
+	return 0;
+}
 #endif
 
 void __init tick_broadcast_init(void)
Index: tip/kernel/time/tick-common.c
===================================================================
--- tip.orig/kernel/time/tick-common.c
+++ tip/kernel/time/tick-common.c
@@ -342,6 +342,27 @@ out_bc:
 	tick_install_broadcast_device(newdev);
 }
 
+/**
+ * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode
+ * @state:	The target state (enter/exit)
+ *
+ * The system enters/leaves a state, where affected devices might stop
+ * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups.
+ *
+ * Called with interrupts disabled, so clockevents_lock is not
+ * required here because the local clock event device cannot go away
+ * under us.
+ */
+int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+{
+	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+
+	if (!(td->evtdev->features & CLOCK_EVT_FEAT_C3STOP))
+		return 0;
+
+	return __tick_broadcast_oneshot_control(state);
+}
+
 #ifdef CONFIG_HOTPLUG_CPU
 /*
  * Transfer the do_timer job away from a dying cpu.
Index: tip/kernel/time/tick-sched.h
===================================================================
--- tip.orig/kernel/time/tick-sched.h
+++ tip/kernel/time/tick-sched.h
@@ -71,4 +71,14 @@ extern void tick_cancel_sched_timer(int
 static inline void tick_cancel_sched_timer(int cpu) { }
 #endif
 
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+extern int __tick_broadcast_oneshot_control(enum tick_broadcast_state state);
+#else
+static inline int
+__tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+{
+	return -EBUSY;
+}
+#endif
+
 #endif

Re: [patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Sudeep Holla]

Hi Thomas,

On 05/07/15 21:53, Thomas Gleixner wrote:
> If no broadcast device is installed and the cpu local timers stop in
> deeper idle states, then there is currently nothing telling the idle
> code that it should not go into deep idle states, so the timers stop
> and nothing wakes up the cpus.
>
> Make the broadcast_enter/exit() functions independent of the
> configuration options and always check on enter:
>
> - whether the cpu local device is affected by idle states
> - whether a broadcast device is available
>
> This covers all possible config combinations including
> CONFIG_BROADCAST=n.
>
> Reported-by: Sudeep Holla <sudeep.holla@arm.com>

Sorry for the delay, took a while testing few configuration:

+--------------+--------+--------------+--------------------+
|   Configs    | PERIOD | HRTimers+NOHz|Cmdline(HR+NoHZ=off)|
+--------------+--------+--------------+--------------------+
| UP w/o H/W BC|   OK   |      OK      |        OK          |
+--------------+--------+--------------+--------------------+
| UP w/ H/W BC |   OK   |      OK      |        OK          |
+--------------+--------+--------------+--------------------+
|SMP w/o H/W BC|   OK*  |      OK      |        Not OK(**)  |
+--------------+--------+--------------+--------------------+
|SMP w/ H/W BC |   OK   |      OK      |        OK          |
+--------------+--------+--------------+--------------------+

H/W BC - Hardware Broadcast Timer source

(*) None of the CPUs enters deeper idle states losing local timers

(**)SMP build without Hardware Broadcast Timer source(i.e. one cpu is
the broadcast source) with HRTimers+NOHz configs but disabled in cmdline
fails to boot. On connecting debugger, I found all the cpus are in
shallow idle state(i.e. WFI in ARM) but with interrupts disabled.

I am not really keen on the failing configuration. We have never tested
that before, though I found it working with CPUIdle disabled.

So please feel free to add:
Tested-by: Sudeep Holla <sudeep.holla@arm.com>

Regards,
Sudeep

Re: [patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Thomas Gleixner]

On Mon, 6 Jul 2015, Sudeep Holla wrote:
> On 05/07/15 21:53, Thomas Gleixner wrote:
> > If no broadcast device is installed and the cpu local timers stop in
> > deeper idle states, then there is currently nothing telling the idle
> > code that it should not go into deep idle states, so the timers stop
> > and nothing wakes up the cpus.
> > 
> > Make the broadcast_enter/exit() functions independent of the
> > configuration options and always check on enter:
> > 
> > - whether the cpu local device is affected by idle states
> > - whether a broadcast device is available
> > 
> > This covers all possible config combinations including
> > CONFIG_BROADCAST=n.
> > 
> > Reported-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> Sorry for the delay, took a while testing few configuration:
> 
> +--------------+--------+--------------+--------------------+
> |   Configs    | PERIOD | HRTimers+NOHz|Cmdline(HR+NoHZ=off)|
> +--------------+--------+--------------+--------------------+
> | UP w/o H/W BC|   OK   |      OK      |        OK          |
> +--------------+--------+--------------+--------------------+
> | UP w/ H/W BC |   OK   |      OK      |        OK          |
> +--------------+--------+--------------+--------------------+
> |SMP w/o H/W BC|   OK*  |      OK      |        Not OK(**)  |
> +--------------+--------+--------------+--------------------+
> |SMP w/ H/W BC |   OK   |      OK      |        OK          |
> +--------------+--------+--------------+--------------------+
> 
> H/W BC - Hardware Broadcast Timer source
> 
> (*) None of the CPUs enters deeper idle states losing local timers
> 
> (**)SMP build without Hardware Broadcast Timer source(i.e. one cpu is
> the broadcast source) with HRTimers+NOHz configs but disabled in cmdline
> fails to boot. 

That's using the hrtimer broadcast mechanism, right?

> On connecting debugger, I found all the cpus are in
> shallow idle state(i.e. WFI in ARM) but with interrupts disabled.

And that means?
 
> I am not really keen on the failing configuration. We have never tested
> that before, though I found it working with CPUIdle disabled.

Well, we should figure out what happens while we are at it before
everything gets paged out again.

In the case of CONFIG_NOHZ=n and CONFIG_HIGHRES=n the broadcast
hrtimer is not compiled as it depends on CONFIG_TICK_ONESHOT, so it
works via the bc.evtdev == NULL check.

With either option enabled CONFIG_TICK_ONESHOT gets set, so the
broadcast timer gets installed but somehow does not work proper if
nohz and highres are disabled on the kernel command line.

Thanks,

	tglx

Re: [patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Sudeep Holla]

On 06/07/15 16:35, Thomas Gleixner wrote:
> On Mon, 6 Jul 2015, Sudeep Holla wrote:
>> On 05/07/15 21:53, Thomas Gleixner wrote:
>>> If no broadcast device is installed and the cpu local timers stop in
>>> deeper idle states, then there is currently nothing telling the idle
>>> code that it should not go into deep idle states, so the timers stop
>>> and nothing wakes up the cpus.
>>>
>>> Make the broadcast_enter/exit() functions independent of the
>>> configuration options and always check on enter:
>>>
>>> - whether the cpu local device is affected by idle states
>>> - whether a broadcast device is available
>>>
>>> This covers all possible config combinations including
>>> CONFIG_BROADCAST=n.
>>>
>>> Reported-by: Sudeep Holla <sudeep.holla@arm.com>
>>
>> Sorry for the delay, took a while testing few configuration:
>>
>> +--------------+--------+--------------+--------------------+
>> |   Configs    | PERIOD | HRTimers+NOHz|Cmdline(HR+NoHZ=off)|
>> +--------------+--------+--------------+--------------------+
>> | UP w/o H/W BC|   OK   |      OK      |        OK          |
>> +--------------+--------+--------------+--------------------+
>> | UP w/ H/W BC |   OK   |      OK      |        OK          |
>> +--------------+--------+--------------+--------------------+
>> |SMP w/o H/W BC|   OK*  |      OK      |        Not OK(**)  |
>> +--------------+--------+--------------+--------------------+
>> |SMP w/ H/W BC |   OK   |      OK      |        OK          |
>> +--------------+--------+--------------+--------------------+
>>
>> H/W BC - Hardware Broadcast Timer source
>>
>> (*) None of the CPUs enters deeper idle states losing local timers
>>
>> (**)SMP build without Hardware Broadcast Timer source(i.e. one cpu is
>> the broadcast source) with HRTimers+NOHz configs but disabled in cmdline
>> fails to boot.
>
> That's using the hrtimer broadcast mechanism, right?
>

Correct.

>> On connecting debugger, I found all the cpus are in
>> shallow idle state(i.e. WFI in ARM) but with interrupts disabled.
>
> And that means?
>

All CPUs have entered WFI with interrupts disabled and no way to wake 
them up.

>> I am not really keen on the failing configuration. We have never tested
>> that before, though I found it working with CPUIdle disabled.
>
> Well, we should figure out what happens while we are at it before
> everything gets paged out again.
>

True. I just wanted to mention that this patch works for all the
practical purposes.

> In the case of CONFIG_NOHZ=n and CONFIG_HIGHRES=n the broadcast
> hrtimer is not compiled as it depends on CONFIG_TICK_ONESHOT, so it
> works via the bc.evtdev == NULL check.
>
> With either option enabled CONFIG_TICK_ONESHOT gets set, so the
> broadcast timer gets installed but somehow does not work proper if
> nohz and highres are disabled on the kernel command line.
>

Let me know if you want to test something to help debug this configuration.

Regards,
Sudeep

Re: [patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Thomas Gleixner]

On Mon, 6 Jul 2015, Sudeep Holla wrote:
> On 06/07/15 16:35, Thomas Gleixner wrote:
> > Well, we should figure out what happens while we are at it before
> > everything gets paged out again.
> > 
> 
> True. I just wanted to mention that this patch works for all the
> practical purposes.
> 
> > In the case of CONFIG_NOHZ=n and CONFIG_HIGHRES=n the broadcast
> > hrtimer is not compiled as it depends on CONFIG_TICK_ONESHOT, so it
> > works via the bc.evtdev == NULL check.
> > 
> > With either option enabled CONFIG_TICK_ONESHOT gets set, so the
> > broadcast timer gets installed but somehow does not work proper if
> > nohz and highres are disabled on the kernel command line.
> > 
> 
> Let me know if you want to test something to help debug this configuration.

Can you try the following delta patch?

Thanks,

	tglx
---
Index: tip/kernel/time/tick-broadcast.c
===================================================================
--- tip.orig/kernel/time/tick-broadcast.c
+++ tip/kernel/time/tick-broadcast.c
@@ -360,14 +360,15 @@ void tick_broadcast_control(enum tick_br
 		cpumask_set_cpu(cpu, tick_broadcast_on);
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask)) {
 			/*
-			 * Only shutdown the cpu local device, if the
-			 * broadcast device exists and is in periodic
-			 * mode. The latter check prevents a hickup
-			 * during the switch from periodic to oneshot
-			 * mode.
+			 * Only shutdown the cpu local device, if:
+			 *
+			 * - the broadcast device exists
+			 * - the broadcast device is not a hrtimer based one
+			 * - the broadcast device is in periodic mode to
+			 *   avoid a hickup during switch to oneshot mode
 			 */
-			if (bc != NULL && tick_broadcast_device.mode ==
-			    TICKDEV_MODE_PERIODIC)
+			if (bc && !(bc->features & CLOCK_EVT_FEAT_HRTIMER) &&
+			    tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
 				clockevents_shutdown(dev);
 		}
 		break;
@@ -697,14 +698,18 @@ int __tick_broadcast_oneshot_control(enu
 		 * shutdown.
 		 */
 		ret = broadcast_needs_cpu(bc, cpu);
+		if (ret)
+			goto out;
 
 		/*
-		 * If the hrtimer broadcast check tells us that the
-		 * cpu cannot go deep idle, or if the broadcast device
-		 * is in periodic mode, we just return.
+		 * If the broadcast device is in periodic mode, we
+		 * return.
 		 */
-		if (ret || tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
-			goto out;
+		if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) {
+			/* If it is a hrtimer based broadcast, return busy */
+			if (bc->features & CLOCK_EVT_FEAT_HRTIMER)
+				ret = -EBUSY;
+		}
 
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_oneshot_mask)) {
 			WARN_ON_ONCE(cpumask_test_cpu(cpu, tick_broadcast_pending_mask));

Re: [patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Sudeep Holla]

On 06/07/15 17:06, Thomas Gleixner wrote:
> On Mon, 6 Jul 2015, Sudeep Holla wrote:
>> On 06/07/15 16:35, Thomas Gleixner wrote:
>>> Well, we should figure out what happens while we are at it before
>>> everything gets paged out again.
>>>
>>
>> True. I just wanted to mention that this patch works for all the
>> practical purposes.
>>
>>> In the case of CONFIG_NOHZ=n and CONFIG_HIGHRES=n the broadcast
>>> hrtimer is not compiled as it depends on CONFIG_TICK_ONESHOT, so it
>>> works via the bc.evtdev == NULL check.
>>>
>>> With either option enabled CONFIG_TICK_ONESHOT gets set, so the
>>> broadcast timer gets installed but somehow does not work proper if
>>> nohz and highres are disabled on the kernel command line.
>>>
>>
>> Let me know if you want to test something to help debug this configuration.
>
> Can you try the following delta patch?
>

I just tried the failing configuration. Yes I am able to boot now, 
however made below 2 observations:

1. As in the case of CONFIG_HZ_PERIODIC, none of the CPUs enters deeper
    idle states losing local timers. So the behaviour is same in both
    versions of periodic mode of timer operation.
2. After boot I am seeing the below warning:

------------[ cut here ]------------
WARNING: CPU: 1 PID: 0 at kernel/time/hrtimer.c:1247 
__hrtimer_run_queues+0x148/0x150()
Modules linked in:
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.2.0-rc1 #573
Hardware name: ARM Juno development board (r0) (DT)
Call trace:
[<ffffffc000089954>] dump_backtrace+0x0/0x124
[<ffffffc000089a88>] show_stack+0x10/0x1c
[<ffffffc0005d100c>] dump_stack+0x84/0xc8
[<ffffffc0000b3f34>] warn_slowpath_common+0x98/0xd0
[<ffffffc0000b402c>] warn_slowpath_null+0x14/0x20
[<ffffffc000101eec>] __hrtimer_run_queues+0x144/0x150
[<ffffffc0001028b4>] hrtimer_run_queues+0xb8/0xe8
[<ffffffc000101714>] update_process_times+0x28/0x6c
[<ffffffc00010d978>] tick_periodic+0x3c/0xb8
[<ffffffc00010da1c>] tick_handle_periodic+0x28/0x94
[<ffffffc0004d6154>] arch_timer_handler_phys+0x28/0x38
[<ffffffc0000f5964>] handle_percpu_devid_irq+0x74/0x9c
[<ffffffc0000f1668>] generic_handle_irq+0x30/0x4c
[<ffffffc0000f1984>] __handle_domain_irq+0x5c/0xac
[<ffffffc0000824a8>] gic_handle_irq+0x30/0x80
Exception stack(0xffffffc9768ffdb0 to 0xffffffc9768ffed0)
fda0:                                     8c1625f8 00000008 75da4800 
ffffffc9
fdc0: 768ffef0 ffffffc9 004b3448 ffffffc0 00000000 00000000 768fc000 
ffffffc9
fde0: 00000000 00000000 00000001 00000000 00000002 00000000 00000000 
00000000
fe00: da98d76d 001e70a3 000005dc 00000000 00000003 00000000 00000001 
00000000
fe20: 00000004 00000000 00000040 00000000 ffffffff ffffffff 00000000 
00000000
fe40: 008a3dd8 ffffffc0 ffffffff ffffffff 001a65a0 ffffffc0 004e2488 
00000000
fe60: f7ea3080 0000007f 8c1625f8 00000008 75da4800 ffffffc9 00000000 
00000000
fe80: 008da840 ffffffc0 8b7dc4e5 00000008 768fff70 ffffffc9 00884e20 
ffffffc0
fea0: 005e8770 ffffffc0 00896000 ffffffc0 768fc000 ffffffc9 768ffef0 
ffffffc9
fec0: 004b3408 ffffffc0 768ffef0 ffffffc9
[<ffffffc0000855a4>] el1_irq+0x64/0xd8
[<ffffffc0004b354c>] cpuidle_enter+0x14/0x20
[<ffffffc0000e80f8>] call_cpuidle+0x24/0x50
[<ffffffc0000e8268>] cpu_startup_entry+0x144/0x224
[<ffffffc00009010c>] secondary_start_kernel+0x124/0x14c
---[ end trace 87fd96b94f030e33 ]---

Re: [patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Thomas Gleixner]

On Mon, 6 Jul 2015, Sudeep Holla wrote:
> On 06/07/15 17:06, Thomas Gleixner wrote:
> 2. After boot I am seeing the below warning:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 0 at kernel/time/hrtimer.c:1247 
> __hrtimer_run_queues+0x148/0x150()
> Modules linked in:
> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.2.0-rc1 #573
> Hardware name: ARM Juno development board (r0) (DT)
> Call trace:
> [<ffffffc000089954>] dump_backtrace+0x0/0x124
> [<ffffffc000089a88>] show_stack+0x10/0x1c
> [<ffffffc0005d100c>] dump_stack+0x84/0xc8
> [<ffffffc0000b3f34>] warn_slowpath_common+0x98/0xd0
> [<ffffffc0000b402c>] warn_slowpath_null+0x14/0x20
> [<ffffffc000101eec>] __hrtimer_run_queues+0x144/0x150
> [<ffffffc0001028b4>] hrtimer_run_queues+0xb8/0xe8
> [<ffffffc000101714>] update_process_times+0x28/0x6c
> [<ffffffc00010d978>] tick_periodic+0x3c/0xb8
> [<ffffffc00010da1c>] tick_handle_periodic+0x28/0x94
> [<ffffffc0004d6154>] arch_timer_handler_phys+0x28/0x38
> [<ffffffc0000f5964>] handle_percpu_devid_irq+0x74/0x9c
> [<ffffffc0000f1668>] generic_handle_irq+0x30/0x4c
> [<ffffffc0000f1984>] __handle_domain_irq+0x5c/0xac
> [<ffffffc0000824a8>] gic_handle_irq+0x30/0x80

Bah. I fear Preeti's hrtimer-broadcast stuff recurses in the hrtimer
code.

tick_periodic()
  expire broadcast timer()
    handle_broadcast() {
      tick_periodic()
	 expire_other_timer()
    }

  warning triggers	 

Crap. Patch below should cure that.

Thanks,

	tglx
---
Index: tip/kernel/time/tick-broadcast.c
===================================================================
--- tip.orig/kernel/time/tick-broadcast.c
+++ tip/kernel/time/tick-broadcast.c
@@ -358,18 +358,21 @@ void tick_broadcast_control(enum tick_br
 		tick_broadcast_forced = 1;
 	case TICK_BROADCAST_ON:
 		cpumask_set_cpu(cpu, tick_broadcast_on);
-		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask)) {
-			/*
-			 * Only shutdown the cpu local device, if the
-			 * broadcast device exists and is in periodic
-			 * mode. The latter check prevents a hickup
-			 * during the switch from periodic to oneshot
-			 * mode.
-			 */
-			if (bc != NULL && tick_broadcast_device.mode ==
-			    TICKDEV_MODE_PERIODIC)
-				clockevents_shutdown(dev);
-		}
+
+		/*
+		 * Only shutdown the cpu local device, if:
+		 *
+		 * - the broadcast device exists
+		 * - the broadcast device is not a hrtimer based one
+		 * - the broadcast device is in periodic mode to
+		 *   avoid a hickup during switch to oneshot mode
+		 */
+		if (!bc || (bc->features & CLOCK_EVT_FEAT_HRTIMER) ||
+		    tick_broadcast_device.mode != TICKDEV_MODE_PERIODIC)
+			break;
+
+		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask))
+			clockevents_shutdown(dev);
 		break;
 
 	case TICK_BROADCAST_OFF:
@@ -697,14 +700,18 @@ int __tick_broadcast_oneshot_control(enu
 		 * shutdown.
 		 */
 		ret = broadcast_needs_cpu(bc, cpu);
+		if (ret)
+			goto out;
 
 		/*
-		 * If the hrtimer broadcast check tells us that the
-		 * cpu cannot go deep idle, or if the broadcast device
-		 * is in periodic mode, we just return.
+		 * If the broadcast device is in periodic mode, we
+		 * return.
 		 */
-		if (ret || tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
-			goto out;
+		if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) {
+			/* If it is a hrtimer based broadcast, return busy */
+			if (bc->features & CLOCK_EVT_FEAT_HRTIMER)
+				ret = -EBUSY;
+		}
 
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_oneshot_mask)) {
 			WARN_ON_ONCE(cpumask_test_cpu(cpu, tick_broadcast_pending_mask));

Re: [patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Sudeep Holla]

On 06/07/15 17:53, Thomas Gleixner wrote:
> On Mon, 6 Jul 2015, Sudeep Holla wrote:
>> On 06/07/15 17:06, Thomas Gleixner wrote:
>> 2. After boot I am seeing the below warning:
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 1 PID: 0 at kernel/time/hrtimer.c:1247
>> __hrtimer_run_queues+0x148/0x150()
>> Modules linked in:
>> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.2.0-rc1 #573
>> Hardware name: ARM Juno development board (r0) (DT)
>> Call trace:
>> [<ffffffc000089954>] dump_backtrace+0x0/0x124
>> [<ffffffc000089a88>] show_stack+0x10/0x1c
>> [<ffffffc0005d100c>] dump_stack+0x84/0xc8
>> [<ffffffc0000b3f34>] warn_slowpath_common+0x98/0xd0
>> [<ffffffc0000b402c>] warn_slowpath_null+0x14/0x20
>> [<ffffffc000101eec>] __hrtimer_run_queues+0x144/0x150
>> [<ffffffc0001028b4>] hrtimer_run_queues+0xb8/0xe8
>> [<ffffffc000101714>] update_process_times+0x28/0x6c
>> [<ffffffc00010d978>] tick_periodic+0x3c/0xb8
>> [<ffffffc00010da1c>] tick_handle_periodic+0x28/0x94
>> [<ffffffc0004d6154>] arch_timer_handler_phys+0x28/0x38
>> [<ffffffc0000f5964>] handle_percpu_devid_irq+0x74/0x9c
>> [<ffffffc0000f1668>] generic_handle_irq+0x30/0x4c
>> [<ffffffc0000f1984>] __handle_domain_irq+0x5c/0xac
>> [<ffffffc0000824a8>] gic_handle_irq+0x30/0x80
>
> Bah. I fear Preeti's hrtimer-broadcast stuff recurses in the hrtimer
> code.
>
> tick_periodic()
>    expire broadcast timer()
>      handle_broadcast() {
>        tick_periodic()
> 	 expire_other_timer()
>      }
>
>    warning triggers	
>
> Crap. Patch below should cure that.
>

This triggered the below crash on boot, looks like it's accessing
hrtimer->function which is null in periodic mode IIUC

Regards,
Sudeep

--->8

Bad mode in Synchronous Abort handler detected, code 0x86000005 -- IABT 
(current EL)
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.2.0-rc1 #577
Hardware name: ARM Juno development board (r0) (DT)
task: ffffffc976900000 ti: ffffffc9768fc000 task.ti: ffffffc9768fc000
PC is at 0x0
LR is at bc_handler+0x20/0x58
pc : [<0000000000000000>] lr : [<ffffffc00010f4a4>] pstate: a00001c5
sp : ffffffc9768ffbd0

Call trace:
[<          (null)>]           (null)
[<ffffffc000101e88>] __hrtimer_run_queues+0xe0/0x150
[<ffffffc0001028b4>] hrtimer_run_queues+0xb8/0xe8
[<ffffffc000101714>] update_process_times+0x28/0x6c
[<ffffffc00010d978>] tick_periodic+0x3c/0xb8
[<ffffffc00010da1c>] tick_handle_periodic+0x28/0x94
[<ffffffc0004d61d4>] arch_timer_handler_phys+0x28/0x38
[<ffffffc0000f5964>] handle_percpu_devid_irq+0x74/0x9c
[<ffffffc0000f1668>] generic_handle_irq+0x30/0x4c
[<ffffffc0000f1984>] __handle_domain_irq+0x5c/0xac
[<ffffffc0000824a8>] gic_handle_irq+0x30/0x80

Re: [patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Thomas Gleixner]

On Mon, 6 Jul 2015, Sudeep Holla wrote:
> This triggered the below crash on boot, looks like it's accessing
> hrtimer->function which is null in periodic mode IIUC
> 
> Regards,
> Sudeep

Gah. I have no idea how that gets queued. /me goes off to tweak x86 to
emulate that crap.
 
> --->8
> 
> Bad mode in Synchronous Abort handler detected, code 0x86000005 -- IABT
> (current EL)
> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.2.0-rc1 #577
> Hardware name: ARM Juno development board (r0) (DT)
> task: ffffffc976900000 ti: ffffffc9768fc000 task.ti: ffffffc9768fc000
> PC is at 0x0
> LR is at bc_handler+0x20/0x58
> pc : [<0000000000000000>] lr : [<ffffffc00010f4a4>] pstate: a00001c5
> sp : ffffffc9768ffbd0
> 
> Call trace:
> [<          (null)>]           (null)
> [<ffffffc000101e88>] __hrtimer_run_queues+0xe0/0x150
> [<ffffffc0001028b4>] hrtimer_run_queues+0xb8/0xe8
> [<ffffffc000101714>] update_process_times+0x28/0x6c
> [<ffffffc00010d978>] tick_periodic+0x3c/0xb8
> [<ffffffc00010da1c>] tick_handle_periodic+0x28/0x94
> [<ffffffc0004d61d4>] arch_timer_handler_phys+0x28/0x38
> [<ffffffc0000f5964>] handle_percpu_devid_irq+0x74/0x9c
> [<ffffffc0000f1668>] generic_handle_irq+0x30/0x4c
> [<ffffffc0000f1984>] __handle_domain_irq+0x5c/0xac
> [<ffffffc0000824a8>] gic_handle_irq+0x30/0x80
> 

Re: [patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Thomas Gleixner]

On Mon, 6 Jul 2015, Thomas Gleixner wrote:

> On Mon, 6 Jul 2015, Sudeep Holla wrote:
> > This triggered the below crash on boot, looks like it's accessing
> > hrtimer->function which is null in periodic mode IIUC
> > 
> > Regards,
> > Sudeep
> 
> Gah. I have no idea how that gets queued. /me goes off to tweak x86 to
> emulate that crap.

So with a less heat damaged brain, I think I was able to decode the
twisted logic behind all this.

Can you test the patch below please?

Thanks,

	tglx
---
Index: tip/include/linux/tick.h
===================================================================
--- tip.orig/include/linux/tick.h
+++ tip/include/linux/tick.h
@@ -67,11 +67,7 @@ extern void tick_broadcast_control(enum
 static inline void tick_broadcast_control(enum tick_broadcast_mode mode) { }
 #endif /* BROADCAST */
 
-#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
 extern int tick_broadcast_oneshot_control(enum tick_broadcast_state state);
-#else
-static inline int tick_broadcast_oneshot_control(enum tick_broadcast_state state) { return 0; }
-#endif
 
 static inline void tick_broadcast_enable(void)
 {
Index: tip/kernel/time/tick-broadcast.c
===================================================================
--- tip.orig/kernel/time/tick-broadcast.c
+++ tip/kernel/time/tick-broadcast.c
@@ -159,7 +159,7 @@ int tick_device_uses_broadcast(struct cl
 {
 	struct clock_event_device *bc = tick_broadcast_device.evtdev;
 	unsigned long flags;
-	int ret;
+	int ret = 0;
 
 	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
 
@@ -221,13 +221,14 @@ int tick_device_uses_broadcast(struct cl
 			 * If we kept the cpu in the broadcast mask,
 			 * tell the caller to leave the per cpu device
 			 * in shutdown state. The periodic interrupt
-			 * is delivered by the broadcast device.
+			 * is delivered by the broadcast device, if
+			 * the broadcast device exists and is not
+			 * hrtimer based.
 			 */
-			ret = cpumask_test_cpu(cpu, tick_broadcast_mask);
+			if (bc && !(bc->features & CLOCK_EVT_FEAT_HRTIMER))
+				ret = cpumask_test_cpu(cpu, tick_broadcast_mask);
 			break;
 		default:
-			/* Nothing to do */
-			ret = 0;
 			break;
 		}
 	}
@@ -265,8 +266,22 @@ static bool tick_do_broadcast(struct cpu
 	 * Check, if the current cpu is in the mask
 	 */
 	if (cpumask_test_cpu(cpu, mask)) {
+		struct clock_event_device *bc = tick_broadcast_device.evtdev;
+
 		cpumask_clear_cpu(cpu, mask);
-		local = true;
+		/*
+		 * We only run the local handler, if the broadcast
+		 * device is not hrtimer based. Otherwise we run into
+		 * a hrtimer recursion.
+		 *
+		 * local timer_interrupt()
+		 *   local_handler()
+		 *     expire_hrtimers()
+		 *       bc_handler()
+		 *         local_handler()
+		 *	     expire_hrtimers()
+		 */
+		local = !(bc->features & CLOCK_EVT_FEAT_HRTIMER);
 	}
 
 	if (!cpumask_empty(mask)) {
@@ -359,8 +374,16 @@ void tick_broadcast_control(enum tick_br
 	case TICK_BROADCAST_ON:
 		cpumask_set_cpu(cpu, tick_broadcast_on);
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask)) {
-			if (tick_broadcast_device.mode ==
-			    TICKDEV_MODE_PERIODIC)
+			/*
+			 * Only shutdown the cpu local device, if:
+			 *
+			 * - the broadcast device exists
+			 * - the broadcast device is not a hrtimer based one
+			 * - the broadcast device is in periodic mode to
+			 *   avoid a hickup during switch to oneshot mode
+			 */
+			if (bc && !(bc->features & CLOCK_EVT_FEAT_HRTIMER) &&
+			    tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
 				clockevents_shutdown(dev);
 		}
 		break;
@@ -662,71 +685,82 @@ static void broadcast_shutdown_local(str
 	clockevents_switch_state(dev, CLOCK_EVT_STATE_SHUTDOWN);
 }
 
-/**
- * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode
- * @state:	The target state (enter/exit)
- *
- * The system enters/leaves a state, where affected devices might stop
- * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups.
- *
- * Called with interrupts disabled, so clockevents_lock is not
- * required here because the local clock event device cannot go away
- * under us.
- */
-int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 {
 	struct clock_event_device *bc, *dev;
-	struct tick_device *td;
 	int cpu, ret = 0;
 	ktime_t now;
 
 	/*
-	 * Periodic mode does not care about the enter/exit of power
-	 * states
+	 * If there is no broadcast device, tell the caller not to go
+	 * into deep idle.
 	 */
-	if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
-		return 0;
+	if (!tick_broadcast_device.evtdev)
+		return -EBUSY;
 
-	/*
-	 * We are called with preemtion disabled from the depth of the
-	 * idle code, so we can't be moved away.
-	 */
-	td = this_cpu_ptr(&tick_cpu_device);
-	dev = td->evtdev;
-
-	if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
-		return 0;
+	dev = this_cpu_ptr(&tick_cpu_device)->evtdev;
 
 	raw_spin_lock(&tick_broadcast_lock);
 	bc = tick_broadcast_device.evtdev;
 	cpu = smp_processor_id();
 
 	if (state == TICK_BROADCAST_ENTER) {
+		/*
+		 * If the current CPU owns the hrtimer broadcast
+		 * mechanism, it cannot go deep idle and we do not add
+		 * the CPU to the broadcast mask. We don't have to go
+		 * through the EXIT path as the local timer is not
+		 * shutdown.
+		 */
+		ret = broadcast_needs_cpu(bc, cpu);
+		if (ret)
+			goto out;
+
+		/*
+		 * If the broadcast device is in periodic mode, we
+		 * return.
+		 */
+		if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) {
+			/* If it is a hrtimer based broadcast, return busy */
+			if (bc->features & CLOCK_EVT_FEAT_HRTIMER)
+				ret = -EBUSY;
+			goto out;
+		}
+
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_oneshot_mask)) {
 			WARN_ON_ONCE(cpumask_test_cpu(cpu, tick_broadcast_pending_mask));
+
+			/* Conditionally shut down the local timer. */
 			broadcast_shutdown_local(bc, dev);
+
 			/*
 			 * We only reprogram the broadcast timer if we
 			 * did not mark ourself in the force mask and
 			 * if the cpu local event is earlier than the
 			 * broadcast event. If the current CPU is in
 			 * the force mask, then we are going to be
-			 * woken by the IPI right away.
+			 * woken by the IPI right away; we return
+			 * busy, so the CPU does not try to go deep
+			 * idle.
 			 */
-			if (!cpumask_test_cpu(cpu, tick_broadcast_force_mask) &&
-			    dev->next_event.tv64 < bc->next_event.tv64)
+			if (cpumask_test_cpu(cpu, tick_broadcast_force_mask)) {
+				ret = -EBUSY;
+			} else if (dev->next_event.tv64 < bc->next_event.tv64) {
 				tick_broadcast_set_event(bc, cpu, dev->next_event);
+				/*
+				 * In case of hrtimer broadcasts the
+				 * programming might have moved the
+				 * timer to this cpu. If yes, remove
+				 * us from the broadcast mask and
+				 * return busy.
+				 */
+				ret = broadcast_needs_cpu(bc, cpu);
+				if (ret) {
+					cpumask_clear_cpu(cpu,
+						tick_broadcast_oneshot_mask);
+				}
+			}
 		}
-		/*
-		 * If the current CPU owns the hrtimer broadcast
-		 * mechanism, it cannot go deep idle and we remove the
-		 * CPU from the broadcast mask. We don't have to go
-		 * through the EXIT path as the local timer is not
-		 * shutdown.
-		 */
-		ret = broadcast_needs_cpu(bc, cpu);
-		if (ret)
-			cpumask_clear_cpu(cpu, tick_broadcast_oneshot_mask);
 	} else {
 		if (cpumask_test_and_clear_cpu(cpu, tick_broadcast_oneshot_mask)) {
 			clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT);
@@ -938,6 +972,16 @@ bool tick_broadcast_oneshot_available(vo
 	return bc ? bc->features & CLOCK_EVT_FEAT_ONESHOT : false;
 }
 
+#else
+int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+{
+	struct clock_event_device *bc = tick_broadcast_device.evtdev;
+
+	if (!bc || (bc->features & CLOCK_EVT_FEAT_HRTIMER)
+		return -EBUSY;
+
+	return 0;
+}
 #endif
 
 void __init tick_broadcast_init(void)
Index: tip/kernel/time/tick-common.c
===================================================================
--- tip.orig/kernel/time/tick-common.c
+++ tip/kernel/time/tick-common.c
@@ -343,6 +343,27 @@ out_bc:
 	tick_install_broadcast_device(newdev);
 }
 
+/**
+ * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode
+ * @state:	The target state (enter/exit)
+ *
+ * The system enters/leaves a state, where affected devices might stop
+ * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups.
+ *
+ * Called with interrupts disabled, so clockevents_lock is not
+ * required here because the local clock event device cannot go away
+ * under us.
+ */
+int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+{
+	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+
+	if (!(td->evtdev->features & CLOCK_EVT_FEAT_C3STOP))
+		return 0;
+
+	return __tick_broadcast_oneshot_control(state);
+}
+
 #ifdef CONFIG_HOTPLUG_CPU
 /*
  * Transfer the do_timer job away from a dying cpu.
Index: tip/kernel/time/tick-sched.h
===================================================================
--- tip.orig/kernel/time/tick-sched.h
+++ tip/kernel/time/tick-sched.h
@@ -71,4 +71,14 @@ extern void tick_cancel_sched_timer(int
 static inline void tick_cancel_sched_timer(int cpu) { }
 #endif
 
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+extern int __tick_broadcast_oneshot_control(enum tick_broadcast_state state);
+#else
+static inline int
+__tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+{
+	return -EBUSY;
+}
+#endif
+
 #endif

Re: [patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Sudeep Holla]

Hi Thomas,

On 07/07/15 08:31, Thomas Gleixner wrote:
> On Mon, 6 Jul 2015, Thomas Gleixner wrote:
>
>> On Mon, 6 Jul 2015, Sudeep Holla wrote:
>>> This triggered the below crash on boot, looks like it's accessing
>>> hrtimer->function which is null in periodic mode IIUC
>>>
>>> Regards,
>>> Sudeep
>>
>> Gah. I have no idea how that gets queued. /me goes off to tweak x86 to
>> emulate that crap.
>
> So with a less heat damaged brain, I think I was able to decode the
> twisted logic behind all this.
>
> Can you test the patch below please?
>


Yes I tested this patch for all the combinations I had mentioned in my
earlier email. Everything works as expected. Thanks a lot for the
patience. Please feel free to add:

Tested-by: Sudeep Holla <sudeep.holla@arm.com>

>   {
> Index: tip/kernel/time/tick-broadcast.c
> ===================================================================
> --- tip.orig/kernel/time/tick-broadcast.c
> +++ tip/kernel/time/tick-broadcast.c

> @@ -938,6 +972,16 @@ bool tick_broadcast_oneshot_available(vo
>   	return bc ? bc->features & CLOCK_EVT_FEAT_ONESHOT : false;
>   }
>
> +#else
> +int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
> +{
> +	struct clock_event_device *bc = tick_broadcast_device.evtdev;
> +
> +	if (!bc || (bc->features & CLOCK_EVT_FEAT_HRTIMER)

missing ')' at the end in the above statement

Regards,
Sudeep

Re: [patch 1/2] tick/broadcast: Prevent deep idle states if no broadcast device available

[Thomas Gleixner]

On Tue, 7 Jul 2015, Sudeep Holla wrote:
> Yes I tested this patch for all the combinations I had mentioned in my
> earlier email. Everything works as expected. Thanks a lot for the
> patience. Please feel free to add:
> 
> Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> >   {
> > Index: tip/kernel/time/tick-broadcast.c
> > ===================================================================
> > --- tip.orig/kernel/time/tick-broadcast.c
> > +++ tip/kernel/time/tick-broadcast.c
> 
> > @@ -938,6 +972,16 @@ bool tick_broadcast_oneshot_available(vo
> >   	return bc ? bc->features & CLOCK_EVT_FEAT_ONESHOT : false;
> >   }
> > 
> > +#else
> > +int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
> > +{
> > +	struct clock_event_device *bc = tick_broadcast_device.evtdev;
> > +
> > +	if (!bc || (bc->features & CLOCK_EVT_FEAT_HRTIMER)
> 
> missing ')' at the end in the above statement

Indeed. Did not compile that combo.
 
Thanks a lot for testing all the combos!!!

       tglx

[tip:timers/urgent] tick/broadcast: Prevent hrtimer recursion

[tip-bot for Thomas Gleixner]

Commit-ID:  8eb231261fdd20768db23863d00ef277de4b0543
Gitweb:     http://git.kernel.org/tip/8eb231261fdd20768db23863d00ef277de4b0543
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 7 Jul 2015 14:11:00 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 7 Jul 2015 18:46:47 +0200

tick/broadcast: Prevent hrtimer recursion

The hrtimer based broadcast vehicle can cause a hrtimer recursion
which went unnoticed until we changed the hrtimer expiry code to keep
track of the currently running timer.

local_timer_interrupt()
  local_handler()
    hrtimer_interrupt()
      expire_hrtimers()
        broadcast_hrtimer()
	  send_ipis()
	  local_handler()
	    hrtimer_interrupt()
	     ....

Solution is simple: Prevent the local handler call from the broadcast
code when the broadcast 'device' is hrtimer based.

[ Split out from a larger combo patch ]

Tested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Suzuki Poulose <Suzuki.Poulose@arm.com>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1507070929360.3916@nanos
---
 kernel/time/tick-broadcast.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index d39f32c..a762040 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -265,8 +265,22 @@ static bool tick_do_broadcast(struct cpumask *mask)
 	 * Check, if the current cpu is in the mask
 	 */
 	if (cpumask_test_cpu(cpu, mask)) {
+		struct clock_event_device *bc = tick_broadcast_device.evtdev;
+
 		cpumask_clear_cpu(cpu, mask);
-		local = true;
+		/*
+		 * We only run the local handler, if the broadcast
+		 * device is not hrtimer based. Otherwise we run into
+		 * a hrtimer recursion.
+		 *
+		 * local timer_interrupt()
+		 *   local_handler()
+		 *     expire_hrtimers()
+		 *       bc_handler()
+		 *         local_handler()
+		 *	     expire_hrtimers()
+		 */
+		local = !(bc->features & CLOCK_EVT_FEAT_HRTIMER);
 	}
 
 	if (!cpumask_empty(mask)) {

[tip:timers/urgent] tick/broadcast: Sanity check the shutdown of the local clock_event

[tip-bot for Thomas Gleixner]

Commit-ID:  e0454311903d3fd0f12a86c9e65d7b271c2bb05d
Gitweb:     http://git.kernel.org/tip/e0454311903d3fd0f12a86c9e65d7b271c2bb05d
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 7 Jul 2015 14:07:27 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 7 Jul 2015 18:46:47 +0200

tick/broadcast: Sanity check the shutdown of the local clock_event

The broadcast code shuts down the local clock event unconditionally
even if no broadcast device is installed or if the broadcast device is
hrtimer based.

Add proper sanity checks.

[ Split out from a larger combo patch ]

Reported-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Suzuki Poulose <Suzuki.Poulose@arm.com>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1507070929360.3916@nanos
---
 kernel/time/tick-broadcast.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index a762040..9877d0b 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -159,7 +159,7 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
 {
 	struct clock_event_device *bc = tick_broadcast_device.evtdev;
 	unsigned long flags;
-	int ret;
+	int ret = 0;
 
 	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
 
@@ -221,13 +221,14 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
 			 * If we kept the cpu in the broadcast mask,
 			 * tell the caller to leave the per cpu device
 			 * in shutdown state. The periodic interrupt
-			 * is delivered by the broadcast device.
+			 * is delivered by the broadcast device, if
+			 * the broadcast device exists and is not
+			 * hrtimer based.
 			 */
-			ret = cpumask_test_cpu(cpu, tick_broadcast_mask);
+			if (bc && !(bc->features & CLOCK_EVT_FEAT_HRTIMER))
+				ret = cpumask_test_cpu(cpu, tick_broadcast_mask);
 			break;
 		default:
-			/* Nothing to do */
-			ret = 0;
 			break;
 		}
 	}
@@ -373,8 +374,16 @@ void tick_broadcast_control(enum tick_broadcast_mode mode)
 	case TICK_BROADCAST_ON:
 		cpumask_set_cpu(cpu, tick_broadcast_on);
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask)) {
-			if (tick_broadcast_device.mode ==
-			    TICKDEV_MODE_PERIODIC)
+			/*
+			 * Only shutdown the cpu local device, if:
+			 *
+			 * - the broadcast device exists
+			 * - the broadcast device is not a hrtimer based one
+			 * - the broadcast device is in periodic mode to
+			 *   avoid a hickup during switch to oneshot mode
+			 */
+			if (bc && !(bc->features & CLOCK_EVT_FEAT_HRTIMER) &&
+			    tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
 				clockevents_shutdown(dev);
 		}
 		break;

[tip:timers/urgent] tick/broadcast: Make idle check independent from mode and config

[tip-bot for Thomas Gleixner]

Commit-ID:  f32dd117051185da6e923b35491a44d7debeeea5
Gitweb:     http://git.kernel.org/tip/f32dd117051185da6e923b35491a44d7debeeea5
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 7 Jul 2015 16:29:38 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 7 Jul 2015 18:46:47 +0200

tick/broadcast: Make idle check independent from mode and config

Currently the broadcast busy check, which prevents the idle code from
going into deep idle, works only in one shot mode.

If NOHZ and HIGHRES are off (config or command line) there is no
sanity check at all, so under certain conditions cpus are allowed to
go into deep idle, where the local timer stops, and are not woken up
again because there is no broadcast timer installed or a hrtimer based
broadcast device is not evaluated.

Move tick_broadcast_oneshot_control() into the common code and provide
proper subfunctions for the various config combinations.

The common check in tick_broadcast_oneshot_control() is for the C3STOP
misfeature flag of the local clock event device. If its not set, idle
can proceed. If set, further checks are necessary.

Provide checks for the trivial cases:

 - If broadcast is disabled in the config, then return busy

 - If oneshot mode (NOHZ/HIGHES) is disabled in the config, return
   busy if the broadcast device is hrtimer based.

 - If oneshot mode is enabled in the config call the original
   tick_broadcast_oneshot_control() function. That function needs
   extra checks which will be implemented in seperate patches.

[ Split out from a larger combo patch ]

Reported-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Suzuki Poulose <Suzuki.Poulose@arm.com>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1507070929360.3916@nanos
---
 include/linux/tick.h         |  4 ----
 kernel/time/tick-broadcast.c | 26 +++++++++++---------------
 kernel/time/tick-common.c    | 21 +++++++++++++++++++++
 kernel/time/tick-sched.h     | 10 ++++++++++
 4 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 3741ba1..6916dcb 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -67,11 +67,7 @@ extern void tick_broadcast_control(enum tick_broadcast_mode mode);
 static inline void tick_broadcast_control(enum tick_broadcast_mode mode) { }
 #endif /* BROADCAST */
 
-#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
 extern int tick_broadcast_oneshot_control(enum tick_broadcast_state state);
-#else
-static inline int tick_broadcast_oneshot_control(enum tick_broadcast_state state) { return 0; }
-#endif
 
 static inline void tick_broadcast_enable(void)
 {
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 9877d0b..ef77b16 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -685,18 +685,7 @@ static void broadcast_shutdown_local(struct clock_event_device *bc,
 	clockevents_switch_state(dev, CLOCK_EVT_STATE_SHUTDOWN);
 }
 
-/**
- * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode
- * @state:	The target state (enter/exit)
- *
- * The system enters/leaves a state, where affected devices might stop
- * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups.
- *
- * Called with interrupts disabled, so clockevents_lock is not
- * required here because the local clock event device cannot go away
- * under us.
- */
-int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 {
 	struct clock_event_device *bc, *dev;
 	struct tick_device *td;
@@ -717,9 +706,6 @@ int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 	td = this_cpu_ptr(&tick_cpu_device);
 	dev = td->evtdev;
 
-	if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
-		return 0;
-
 	raw_spin_lock(&tick_broadcast_lock);
 	bc = tick_broadcast_device.evtdev;
 	cpu = smp_processor_id();
@@ -961,6 +947,16 @@ bool tick_broadcast_oneshot_available(void)
 	return bc ? bc->features & CLOCK_EVT_FEAT_ONESHOT : false;
 }
 
+#else
+int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+{
+	struct clock_event_device *bc = tick_broadcast_device.evtdev;
+
+	if (!bc || (bc->features & CLOCK_EVT_FEAT_HRTIMER))
+		return -EBUSY;
+
+	return 0;
+}
 #endif
 
 void __init tick_broadcast_init(void)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 76446cb..55e13ef 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -343,6 +343,27 @@ out_bc:
 	tick_install_broadcast_device(newdev);
 }
 
+/**
+ * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode
+ * @state:	The target state (enter/exit)
+ *
+ * The system enters/leaves a state, where affected devices might stop
+ * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups.
+ *
+ * Called with interrupts disabled, so clockevents_lock is not
+ * required here because the local clock event device cannot go away
+ * under us.
+ */
+int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+{
+	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+
+	if (!(td->evtdev->features & CLOCK_EVT_FEAT_C3STOP))
+		return 0;
+
+	return __tick_broadcast_oneshot_control(state);
+}
+
 #ifdef CONFIG_HOTPLUG_CPU
 /*
  * Transfer the do_timer job away from a dying cpu.
diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
index 42fdf49..a4a8d4e 100644
--- a/kernel/time/tick-sched.h
+++ b/kernel/time/tick-sched.h
@@ -71,4 +71,14 @@ extern void tick_cancel_sched_timer(int cpu);
 static inline void tick_cancel_sched_timer(int cpu) { }
 #endif
 
+#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+extern int __tick_broadcast_oneshot_control(enum tick_broadcast_state state);
+#else
+static inline int
+__tick_broadcast_oneshot_control(enum tick_broadcast_state state)
+{
+	return -EBUSY;
+}
+#endif
+
 #endif

[tip:timers/urgent] tick/broadcast: Prevent deep idle if no broadcast device available

[tip-bot for Thomas Gleixner]

Commit-ID:  b78f3f3c898c824bf56ab55cfa59fc72be49c349
Gitweb:     http://git.kernel.org/tip/b78f3f3c898c824bf56ab55cfa59fc72be49c349
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 7 Jul 2015 16:34:32 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 7 Jul 2015 18:46:47 +0200

tick/broadcast: Prevent deep idle if no broadcast device available

Add a check for a installed broadcast device to the oneshot control
function and return busy if not.

[ Split out from a larger combo patch ]

Reported-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Suzuki Poulose <Suzuki.Poulose@arm.com>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1507070929360.3916@nanos
---
 kernel/time/tick-broadcast.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index ef77b16..fad3f78 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -693,6 +693,13 @@ int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 	ktime_t now;
 
 	/*
+	 * If there is no broadcast device, tell the caller not to go
+	 * into deep idle.
+	 */
+	if (!tick_broadcast_device.evtdev)
+		return -EBUSY;
+
+	/*
 	 * Periodic mode does not care about the enter/exit of power
 	 * states
 	 */

[tip:timers/urgent] tick/broadcast: Move the check for periodic mode inside state handling

[tip-bot for Thomas Gleixner]

Commit-ID:  e3ac79e087ffe8a1f953ed44a74acf7616cb0b25
Gitweb:     http://git.kernel.org/tip/e3ac79e087ffe8a1f953ed44a74acf7616cb0b25
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 7 Jul 2015 16:38:11 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 7 Jul 2015 18:46:47 +0200

tick/broadcast: Move the check for periodic mode inside state handling

We need to check more than the periodic mode for proper operation in
all runtime combinations. To avoid code duplication move the check
into the enter state handling.

No functional change.

[ Split out from a larger combo patch ]

Reported-and-tested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Suzuki Poulose <Suzuki.Poulose@arm.com>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1507070929360.3916@nanos
---
 kernel/time/tick-broadcast.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index fad3f78..83aa92e 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -688,7 +688,6 @@ static void broadcast_shutdown_local(struct clock_event_device *bc,
 int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 {
 	struct clock_event_device *bc, *dev;
-	struct tick_device *td;
 	int cpu, ret = 0;
 	ktime_t now;
 
@@ -699,25 +698,20 @@ int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 	if (!tick_broadcast_device.evtdev)
 		return -EBUSY;
 
-	/*
-	 * Periodic mode does not care about the enter/exit of power
-	 * states
-	 */
-	if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
-		return 0;
-
-	/*
-	 * We are called with preemtion disabled from the depth of the
-	 * idle code, so we can't be moved away.
-	 */
-	td = this_cpu_ptr(&tick_cpu_device);
-	dev = td->evtdev;
+	dev = this_cpu_ptr(&tick_cpu_device)->evtdev;
 
 	raw_spin_lock(&tick_broadcast_lock);
 	bc = tick_broadcast_device.evtdev;
 	cpu = smp_processor_id();
 
 	if (state == TICK_BROADCAST_ENTER) {
+		/*
+		 * If the broadcast device is in periodic mode, we
+		 * return.
+		 */
+		if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
+			goto out;
+
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_oneshot_mask)) {
 			WARN_ON_ONCE(cpumask_test_cpu(cpu, tick_broadcast_pending_mask));
 			broadcast_shutdown_local(bc, dev);

[tip:timers/urgent] tick/broadcast: Return busy if periodic mode and hrtimer broadcast

[tip-bot for Thomas Gleixner]

Commit-ID:  d33257264b0267a8fd20f6717abbb484c9e21130
Gitweb:     http://git.kernel.org/tip/d33257264b0267a8fd20f6717abbb484c9e21130
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 7 Jul 2015 17:45:22 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 7 Jul 2015 18:46:48 +0200

tick/broadcast: Return busy if periodic mode and hrtimer broadcast

If the system is in periodic mode and the broadcast device is hrtimer
based, return busy as we have no proper handling for this.

[ Split out from a larger combo patch ]

Tested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Suzuki Poulose <Suzuki.Poulose@arm.com>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1507070929360.3916@nanos
---
 kernel/time/tick-broadcast.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 83aa92e..da7b40f 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -709,8 +709,12 @@ int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 		 * If the broadcast device is in periodic mode, we
 		 * return.
 		 */
-		if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
+		if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) {
+			/* If it is a hrtimer based broadcast, return busy */
+			if (bc->features & CLOCK_EVT_FEAT_HRTIMER)
+				ret = -EBUSY;
 			goto out;
+		}
 
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_oneshot_mask)) {
 			WARN_ON_ONCE(cpumask_test_cpu(cpu, tick_broadcast_pending_mask));

[tip:timers/urgent] tick/broadcast: Return busy when IPI is pending

[tip-bot for Thomas Gleixner]

Commit-ID:  0cc5281aa592d0020868f6ccaed359b4ad7b2684
Gitweb:     http://git.kernel.org/tip/0cc5281aa592d0020868f6ccaed359b4ad7b2684
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 7 Jul 2015 16:45:15 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 7 Jul 2015 18:46:48 +0200

tick/broadcast: Return busy when IPI is pending

Tell the idle code not to go deep if the broadcast IPI is about to
arrive.

[ Split out from a larger combo patch ]

Tested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Suzuki Poulose <Suzuki.Poulose@arm.com>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1507070929360.3916@nanos
---
 kernel/time/tick-broadcast.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index da7b40f..70b47bc 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -725,11 +725,15 @@ int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 			 * if the cpu local event is earlier than the
 			 * broadcast event. If the current CPU is in
 			 * the force mask, then we are going to be
-			 * woken by the IPI right away.
+			 * woken by the IPI right away; we return
+			 * busy, so the CPU does not try to go deep
+			 * idle.
 			 */
-			if (!cpumask_test_cpu(cpu, tick_broadcast_force_mask) &&
-			    dev->next_event.tv64 < bc->next_event.tv64)
+			if (cpumask_test_cpu(cpu, tick_broadcast_force_mask)) {
+				ret = -EBUSY;
+			} else if (dev->next_event.tv64 < bc->next_event.tv64) {
 				tick_broadcast_set_event(bc, cpu, dev->next_event);
+			}
 		}
 		/*
 		 * If the current CPU owns the hrtimer broadcast

[tip:timers/urgent] tick/broadcast: Check for hrtimer broadcast active early

[tip-bot for Thomas Gleixner]

Commit-ID:  d5113e13a550bc9c2b53cc9944b8a06453c4a0a1
Gitweb:     http://git.kernel.org/tip/d5113e13a550bc9c2b53cc9944b8a06453c4a0a1
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 7 Jul 2015 16:43:04 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 7 Jul 2015 18:46:48 +0200

tick/broadcast: Check for hrtimer broadcast active early

If the current cpu is the one which has the hrtimer based broadcast
queued then we better return busy immediately instead of going through
loops and hoops to figure that out.

[ Split out from a larger combo patch ]

Tested-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Suzuki Poulose <Suzuki.Poulose@arm.com>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
Link: http://lkml.kernel.org/r/alpine.DEB.2.11.1507070929360.3916@nanos
---
 kernel/time/tick-broadcast.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 70b47bc..c8d731a 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -706,6 +706,17 @@ int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 
 	if (state == TICK_BROADCAST_ENTER) {
 		/*
+		 * If the current CPU owns the hrtimer broadcast
+		 * mechanism, it cannot go deep idle and we do not add
+		 * the CPU to the broadcast mask. We don't have to go
+		 * through the EXIT path as the local timer is not
+		 * shutdown.
+		 */
+		ret = broadcast_needs_cpu(bc, cpu);
+		if (ret)
+			goto out;
+
+		/*
 		 * If the broadcast device is in periodic mode, we
 		 * return.
 		 */
@@ -718,7 +729,10 @@ int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_oneshot_mask)) {
 			WARN_ON_ONCE(cpumask_test_cpu(cpu, tick_broadcast_pending_mask));
+
+			/* Conditionally shut down the local timer. */
 			broadcast_shutdown_local(bc, dev);
+
 			/*
 			 * We only reprogram the broadcast timer if we
 			 * did not mark ourself in the force mask and
@@ -733,18 +747,20 @@ int __tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 				ret = -EBUSY;
 			} else if (dev->next_event.tv64 < bc->next_event.tv64) {
 				tick_broadcast_set_event(bc, cpu, dev->next_event);
+				/*
+				 * In case of hrtimer broadcasts the
+				 * programming might have moved the
+				 * timer to this cpu. If yes, remove
+				 * us from the broadcast mask and
+				 * return busy.
+				 */
+				ret = broadcast_needs_cpu(bc, cpu);
+				if (ret) {
+					cpumask_clear_cpu(cpu,
+						tick_broadcast_oneshot_mask);
+				}
 			}
 		}
-		/*
-		 * If the current CPU owns the hrtimer broadcast
-		 * mechanism, it cannot go deep idle and we remove the
-		 * CPU from the broadcast mask. We don't have to go
-		 * through the EXIT path as the local timer is not
-		 * shutdown.
-		 */
-		ret = broadcast_needs_cpu(bc, cpu);
-		if (ret)
-			cpumask_clear_cpu(cpu, tick_broadcast_oneshot_mask);
 	} else {
 		if (cpumask_test_and_clear_cpu(cpu, tick_broadcast_oneshot_mask)) {
 			clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT);

[patch 2/2] tick/broadcast: Handle spurious interrupts gracefully

[Thomas Gleixner]

[-- Attachment #1: tick-broadcast-handle-spurious-interrupts.patch --]
[-- Type: text/plain, Size: 1778 bytes --]

Andriy reported that on a virtual machine the warning about negative
expiry time in the clock events programming code triggered:

hpet: hpet0 irq 40 for MSI
hpet: hpet1 irq 41 for MSI
Switching to clocksource hpet
WARNING: at kernel/time/clockevents.c:239

[<ffffffff810ce6eb>] clockevents_program_event+0xdb/0xf0
[<ffffffff810cf211>] tick_handle_periodic_broadcast+0x41/0x50
[<ffffffff81016525>] timer_interrupt+0x15/0x20

When the second hpet is installed as a per cpu timer the broadcast
event is not longer required and stopped, which sets the next_evt of
the broadcast device to KTIME_MAX.

If after that a spurious interrupt happens on the broadcast device,
then the current code blindly handles it and tries to reprogram the
broadcast device afterwards, which adds the period to
next_evt. KTIME_MAX + period results in a negative expiry value
causing the WARN_ON in the clockevents code to trigger.

Add a proper check for the state of the broadcast device into the
interrupt handler and return if the interrupt is spurious.

Reported-by: Andriy Gapon <avg@FreeBSD.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/tick-broadcast.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: tip/kernel/time/tick-broadcast.c
===================================================================
--- tip.orig/kernel/time/tick-broadcast.c
+++ tip/kernel/time/tick-broadcast.c
@@ -301,6 +301,13 @@ static void tick_handle_periodic_broadca
 	bool bc_local;
 
 	raw_spin_lock(&tick_broadcast_lock);
+
+	/* Handle spurious interrupts gracefully */
+	if (clockevent_state_shutdown(&tick_broadcast_device.evtdev)) {
+		raw_spin_unlock(&tick_broadcast_lock);
+		return;
+	}
+
 	bc_local = tick_do_periodic_broadcast();
 
 	if (clockevent_state_oneshot(dev)) {

Re: [patch 2/2] tick/broadcast: Handle spurious interrupts gracefully

[Sudeep Holla]

On Sun, Jul 5, 2015 at 9:53 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Andriy reported that on a virtual machine the warning about negative
> expiry time in the clock events programming code triggered:
>
> hpet: hpet0 irq 40 for MSI
> hpet: hpet1 irq 41 for MSI
> Switching to clocksource hpet
> WARNING: at kernel/time/clockevents.c:239
>
> [<ffffffff810ce6eb>] clockevents_program_event+0xdb/0xf0
> [<ffffffff810cf211>] tick_handle_periodic_broadcast+0x41/0x50
> [<ffffffff81016525>] timer_interrupt+0x15/0x20
>
> When the second hpet is installed as a per cpu timer the broadcast
> event is not longer required and stopped, which sets the next_evt of
> the broadcast device to KTIME_MAX.
>
> If after that a spurious interrupt happens on the broadcast device,
> then the current code blindly handles it and tries to reprogram the
> broadcast device afterwards, which adds the period to
> next_evt. KTIME_MAX + period results in a negative expiry value
> causing the WARN_ON in the clockevents code to trigger.
>
> Add a proper check for the state of the broadcast device into the
> interrupt handler and return if the interrupt is spurious.
>
> Reported-by: Andriy Gapon <avg@FreeBSD.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/time/tick-broadcast.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> Index: tip/kernel/time/tick-broadcast.c
> ===================================================================
> --- tip.orig/kernel/time/tick-broadcast.c
> +++ tip/kernel/time/tick-broadcast.c
> @@ -301,6 +301,13 @@ static void tick_handle_periodic_broadca
>         bool bc_local;
>
>         raw_spin_lock(&tick_broadcast_lock);
> +
> +       /* Handle spurious interrupts gracefully */
> +       if (clockevent_state_shutdown(&tick_broadcast_device.evtdev)) {

I tried this patch along with 1/2, found that this generating warning as
tick_broadcast_device.evtdev is of type ‘struct clock_event_device *’ already,
so it can be passed directly.

Regards,
Sudeep

Re: [patch 2/2] tick/broadcast: Handle spurious interrupts gracefully

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2177 bytes --]

On Mon, 6 Jul 2015, Sudeep Holla wrote:
> On Sun, Jul 5, 2015 at 9:53 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > Andriy reported that on a virtual machine the warning about negative
> > expiry time in the clock events programming code triggered:
> >
> > hpet: hpet0 irq 40 for MSI
> > hpet: hpet1 irq 41 for MSI
> > Switching to clocksource hpet
> > WARNING: at kernel/time/clockevents.c:239
> >
> > [<ffffffff810ce6eb>] clockevents_program_event+0xdb/0xf0
> > [<ffffffff810cf211>] tick_handle_periodic_broadcast+0x41/0x50
> > [<ffffffff81016525>] timer_interrupt+0x15/0x20
> >
> > When the second hpet is installed as a per cpu timer the broadcast
> > event is not longer required and stopped, which sets the next_evt of
> > the broadcast device to KTIME_MAX.
> >
> > If after that a spurious interrupt happens on the broadcast device,
> > then the current code blindly handles it and tries to reprogram the
> > broadcast device afterwards, which adds the period to
> > next_evt. KTIME_MAX + period results in a negative expiry value
> > causing the WARN_ON in the clockevents code to trigger.
> >
> > Add a proper check for the state of the broadcast device into the
> > interrupt handler and return if the interrupt is spurious.
> >
> > Reported-by: Andriy Gapon <avg@FreeBSD.org>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  kernel/time/tick-broadcast.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > Index: tip/kernel/time/tick-broadcast.c
> > ===================================================================
> > --- tip.orig/kernel/time/tick-broadcast.c
> > +++ tip/kernel/time/tick-broadcast.c
> > @@ -301,6 +301,13 @@ static void tick_handle_periodic_broadca
> >         bool bc_local;
> >
> >         raw_spin_lock(&tick_broadcast_lock);
> > +
> > +       /* Handle spurious interrupts gracefully */
> > +       if (clockevent_state_shutdown(&tick_broadcast_device.evtdev)) {
> 
> I tried this patch along with 1/2, found that this generating warning as
> tick_broadcast_device.evtdev is of type ‘struct clock_event_device *’ already,
> so it can be passed directly.

Right you are. The heat is melting my brain ...

[tip:timers/urgent] tick/broadcast: Handle spurious interrupts gracefully

[tip-bot for Thomas Gleixner]

Commit-ID:  c4288334818c81c946acb23d2319881f58c3d497
Gitweb:     http://git.kernel.org/tip/c4288334818c81c946acb23d2319881f58c3d497
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sun, 5 Jul 2015 20:53:17 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 7 Jul 2015 18:46:48 +0200

tick/broadcast: Handle spurious interrupts gracefully

Andriy reported that on a virtual machine the warning about negative
expiry time in the clock events programming code triggered:

hpet: hpet0 irq 40 for MSI
hpet: hpet1 irq 41 for MSI
Switching to clocksource hpet
WARNING: at kernel/time/clockevents.c:239

[<ffffffff810ce6eb>] clockevents_program_event+0xdb/0xf0
[<ffffffff810cf211>] tick_handle_periodic_broadcast+0x41/0x50
[<ffffffff81016525>] timer_interrupt+0x15/0x20

When the second hpet is installed as a per cpu timer the broadcast
event is not longer required and stopped, which sets the next_evt of
the broadcast device to KTIME_MAX.

If after that a spurious interrupt happens on the broadcast device,
then the current code blindly handles it and tries to reprogram the
broadcast device afterwards, which adds the period to
next_evt. KTIME_MAX + period results in a negative expiry value
causing the WARN_ON in the clockevents code to trigger.

Add a proper check for the state of the broadcast device into the
interrupt handler and return if the interrupt is spurious.

[ Folded in pointer fix from Sudeep ]

Reported-by: Andriy Gapon <avg@FreeBSD.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Link: http://lkml.kernel.org/r/20150705205221.802094647@linutronix.de
---
 kernel/time/tick-broadcast.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index c8d731a..ee3cf94 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -316,6 +316,13 @@ static void tick_handle_periodic_broadcast(struct clock_event_device *dev)
 	bool bc_local;
 
 	raw_spin_lock(&tick_broadcast_lock);
+
+	/* Handle spurious interrupts gracefully */
+	if (clockevent_state_shutdown(tick_broadcast_device.evtdev)) {
+		raw_spin_unlock(&tick_broadcast_lock);
+		return;
+	}
+
 	bc_local = tick_do_periodic_broadcast();
 
 	if (clockevent_state_oneshot(dev)) {