Re: [PATCH] clockevents: return error from tick_broadcast_oneshot_control if !GENERIC_CLOCKEVENTS_BROADCAST

[Thomas Gleixner]

On Thu, 25 Jun 2015, Sudeep Holla wrote:

> tick_broadcast_enter returns 0 when CPU can switch to broadcast
> timer and non-zero otherwise. However when GENERIC_CLOCKEVENTS_BROADCAST
> and TICK_ONESHOT are disabled, tick_broadcast_oneshot_control returns 0
> which indicates to the CPUIdle framework that the CPU can enter deeper
> idle states even when the CPU local timer will be shutdown. If the
> target state needs broadcast but not broadcast timer is available, then
> the CPU can not resume back from that idle state.
> 
> This patch returns error when there's no broadcast timer support
> available so that CPUIdle framework prevents the CPU from entering any
> idle states losing the local timer.

That's wrong and breaks stuff which does not require the broadcast
nonsense.

If TICK_ONESHOT is disabled, then everything is in periodic mode and
tick_broadcast_enter() rightfully returns 0. Ditto for 'highres=off'
on the command line.

But there is a case which is not correctly handled right now. That's
what you are trying to solve in the wrong way.

If
   GENERIC_CLOCKEVENTS_BROADCAST=n

or

   GENERIC_CLOCKEVENTS_BROADCAST=y and no broadcast device is available,

AND cpu local tick device has the C3STOP flag set,

then we have no way to tell the idle code that going deep is not
allowed.

So we need to be smarter than blindly changing a return
value. Completely untested patch below.

Thanks,

	tglx
---
diff --git a/include/linux/tick.h b/include/linux/tick.h
index 4191b5623a28..8731a58dd747 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -63,11 +63,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 d39f32cdd1b5..52c8d01b956b 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -662,24 +662,15 @@ 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;
 	int cpu, ret = 0;
 	ktime_t now;
 
+	if (!tick_broadcast_device.evtdev)
+		return -EBUSY;
+
 	/*
 	 * Periodic mode does not care about the enter/exit of power
 	 * states
@@ -687,15 +678,7 @@ int tick_broadcast_oneshot_control(enum tick_broadcast_state state)
 	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 (!(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;
@@ -938,6 +921,13 @@ 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)
+{
+	if (!tick_broadcast_device.evtdev)
+		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 17f144450050..f66c11a30348 100644
--- a/kernel/time/tick-common.c
+++ b/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.
diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
index 42fdf4958bcc..a4a8d4e9baa1 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/