PM/Sleep: Timer quiesce in freeze state

The patch is based on v3.18.

Freeze is a general power saving state that processes are frozen, devices are suspended and CPUs are in idle state. However, when the system enters freeze state, there are a few timers keep ticking and hence consumes more power unnecessarily. The observed timer events in freeze state are:
- tick_sched_timer
- watchdog lockup detector
- realtime scheduler period timer

The system power consumption in freeze state will be reduced significantly if we quiesce these timers.

The patch is tested on:
- Sandybrdige-EP system, both RTC alarm and power button are able to wake the system up from freeze state.
- HP laptop EliteBook 8460p, both RTC alarm and power button are able to wake the system up from freeze state.
- Baytrail-T(ASUS_T100) platform, power button is able to wake the system up from freeze state.

Suggested-by Thomas Gleixner <[email protected]>

Signed-off-by Aubrey Li <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Len Brown <[email protected]>

Cc Alan Cox <[email protected]>

---
 drivers/cpuidle/cpuidle.c   | 13 ++++++++
 include/linux/clockchips.h  |  4 +++
 include/linux/suspend.h     |  4 +++
 include/linux/timekeeping.h |  2 ++
 kernel/power/suspend.c      | 50 ++++++++++++++++++++++++++---
 kernel/sched/idle.c         | 45 ++++++++++++++++++++++++++
 kernel/softirq.c            |  5 +--
 kernel/time/clockevents.c   | 13 ++++++++
 kernel/time/tick-common.c   | 53 +++++++++++++++++++++++++++++++
 kernel/time/tick-internal.h |  3 ++
 kernel/time/timekeeping.c   | 77 +++++++++++++++++++++++++++++++++------------
 11 files changed, 243 insertions(+), 26 deletions(-)

drivers/cpuidle/cpuidle.c +13 -0

index 125150d..b9a3ada 100644
@@ -20,6 +20,7 @@
 #include <linux/hrtimer.h>
 #include <linux/module.h>
 #include <trace/events/power.h>
+#include <linux/suspend.h>
 
 #include "cpuidle.h"
 

@@ -119,6 +120,18 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 	ktime_t time_start, time_end;
 	s64 diff;
 
+	/*
+	 * under the freeze scenario, the timekeeping is suspended
+	 * as well as the clock source device, so we bypass the idle
+	 * counter update in freeze idle
+	 */
+	if (in_freeze()) {
+		entered_state = target_state->enter(dev, drv, index);
+		if (!cpuidle_state_is_coupled(dev, drv, entered_state))
+			local_irq_enable();
+		return entered_state;
+	}
+
 	trace_cpu_idle_rcuidle(index, dev->cpu);
 	time_start = ktime_get();
 

include/linux/clockchips.h +4 -0

index 2e4cb67..d118e0b 100644
@@ -18,6 +18,9 @@ enum clock_event_nofitiers {
 	CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
 	CLOCK_EVT_NOTIFY_SUSPEND,
 	CLOCK_EVT_NOTIFY_RESUME,
+	CLOCK_EVT_NOTIFY_FREEZE_PREPARE,
+	CLOCK_EVT_NOTIFY_FREEZE,
+	CLOCK_EVT_NOTIFY_UNFREEZE,
 	CLOCK_EVT_NOTIFY_CPU_DYING,
 	CLOCK_EVT_NOTIFY_CPU_DEAD,
 };

@@ -95,6 +98,7 @@ enum clock_event_mode {
  */
 struct clock_event_device {
 	void			(*event_handler)(struct clock_event_device *);
+	void			(*real_handler)(struct clock_event_device *);
 	int			(*set_next_event)(unsigned long evt,
 						  struct clock_event_device *);
 	int			(*set_next_ktime)(ktime_t expires,

include/linux/suspend.h +4 -0

index 3388c1b..86a651c 100644
@@ -203,6 +203,8 @@ extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
 extern void freeze_set_ops(const struct platform_freeze_ops *ops);
 extern void freeze_wake(void);
+extern bool in_freeze(void);
+extern bool idle_should_freeze(void);
 
 /**
  * arch_suspend_disable_irqs - disable IRQs for suspend

@@ -230,6 +232,8 @@ static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
 static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {}
 static inline void freeze_wake(void) {}
+static inline bool in_freeze(void) { return false; }
+static inline bool idle_should_freeze(void) { return false; }
 #endif /* !CONFIG_SUSPEND */
 
 /* struct pbe is used for creating lists of pages that should be restored

include/linux/timekeeping.h +2 -0

index 1caa6b0..07957a9 100644
@@ -5,6 +5,8 @@
 
 void timekeeping_init(void);
 extern int timekeeping_suspended;
+extern void timekeeping_freeze(void);
+extern void timekeeping_unfreeze(void);
 
 /*
  * Get and set timeofday

kernel/power/suspend.c +46 -4

index c347e3c..6467fb8 100644
@@ -28,6 +28,7 @@
 #include <linux/ftrace.h>
 #include <trace/events/power.h>
 #include <linux/compiler.h>
+#include <linux/clockchips.h>
 
 #include "power.h"
 

@@ -37,7 +38,15 @@ const char *pm_states[PM_SUSPEND_MAX];
 static const struct platform_suspend_ops *suspend_ops;
 static const struct platform_freeze_ops *freeze_ops;
 static DECLARE_WAIT_QUEUE_HEAD(suspend_freeze_wait_head);
-static bool suspend_freeze_wake;
+
+/* freeze state machine */
+enum freeze_state {
+	FREEZE_STATE_NONE,      /* not in freeze */
+	FREEZE_STATE_ENTER,     /* enter freeze */
+	FREEZE_STATE_WAKE,      /* in freeze wakeup context */
+};
+
+static enum freeze_state suspend_freeze_state;
 
 void freeze_set_ops(const struct platform_freeze_ops *ops)
 {

@@ -46,23 +55,56 @@ void freeze_set_ops(const struct platform_freeze_ops *ops)
 	unlock_system_sleep();
 }
 
+bool in_freeze(void)
+{
+	return (suspend_freeze_state > FREEZE_STATE_NONE);
+}
+EXPORT_SYMBOL_GPL(in_freeze);
+
+bool idle_should_freeze(void)
+{
+	return (suspend_freeze_state == FREEZE_STATE_ENTER);
+}
+EXPORT_SYMBOL_GPL(idle_should_freeze);
+
 static void freeze_begin(void)
 {
-	suspend_freeze_wake = false;
+	suspend_freeze_state = FREEZE_STATE_NONE;
 }
 
 static void freeze_enter(void)
 {
+	suspend_freeze_state = FREEZE_STATE_ENTER;
+	get_online_cpus();
 	cpuidle_use_deepest_state(true);
 	cpuidle_resume();
-	wait_event(suspend_freeze_wait_head, suspend_freeze_wake);
+	clockevents_notify(CLOCK_EVT_NOTIFY_FREEZE_PREPARE, NULL);
+	/*
+	 * push all the CPUs into freeze idle loop
+	 */
+	wake_up_all_idle_cpus();
+	printk(KERN_INFO "PM: suspend to idle\n");
+	/*
+	 * put the current CPU into wait queue so that this CPU
+	 * is able to enter freeze idle loop as well
+	 */
+	wait_event(suspend_freeze_wait_head,
+		(suspend_freeze_state == FREEZE_STATE_WAKE));
+	printk(KERN_INFO "PM: resume from freeze\n");
 	cpuidle_pause();
 	cpuidle_use_deepest_state(false);
+	put_online_cpus();
+	suspend_freeze_state = FREEZE_STATE_NONE;
 }
 
 void freeze_wake(void)
 {
-	suspend_freeze_wake = true;
+	if (!in_freeze())
+		return;
+	/*
+	 * wake freeze task up
+	 */
+	suspend_freeze_state = FREEZE_STATE_WAKE;
 	wake_up(&suspend_freeze_wait_head);
 }
 EXPORT_SYMBOL_GPL(freeze_wake);

kernel/sched/idle.c +45 -0

index c47fce7..f28f8cb 100644
@@ -7,6 +7,7 @@
 #include <linux/tick.h>
 #include <linux/mm.h>
 #include <linux/stackprotector.h>
+#include <linux/suspend.h>
 
 #include <asm/tlb.h>
 

@@ -182,6 +183,45 @@ exit_idle:
 }
 
 /*
+ * cpu idle freeze function
+ */
+static void cpu_idle_freeze(void)
+{
+	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+
+	/*
+	 * suspend tick, the last CPU suspend timekeeping
+	 */
+	clockevents_notify(CLOCK_EVT_NOTIFY_FREEZE, NULL);
+	/*
+	 * idle loop here if idle_should_freeze()
+	 */
+	while (idle_should_freeze()) {
+		int next_state;
+		/*
+		 * interrupt must be disabled before cpu enters idle
+		 */
+		local_irq_disable();
+
+		next_state = cpuidle_select(drv, dev);
+		if (next_state < 0) {
+			arch_cpu_idle();
+			continue;
+		}
+		/*
+		 * cpuidle_enter will return with interrupt enabled
+		 */
+		cpuidle_enter(drv, dev, next_state);
+	}
+
+	/*
+	 * resume tick, the first wakeup CPU resume timekeeping
+	 */
+	clockevents_notify(CLOCK_EVT_NOTIFY_UNFREEZE, NULL);
+}
+
+/*
  * Generic idle loop implementation
  *
  * Called with polling cleared.

@@ -208,6 +248,11 @@ static void cpu_idle_loop(void)
 			if (cpu_is_offline(smp_processor_id()))
 				arch_cpu_idle_dead();
 
+			if (idle_should_freeze()) {
+				cpu_idle_freeze();
+				continue;
+			}
+
 			local_irq_disable();
 			arch_cpu_idle_enter();
 

kernel/softirq.c +3 -2

index 0699add..a231bf6 100644
@@ -26,6 +26,7 @@
 #include <linux/smpboot.h>
 #include <linux/tick.h>
 #include <linux/irq.h>
+#include <linux/suspend.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/irq.h>

@@ -321,7 +322,7 @@ asmlinkage __visible void do_softirq(void)
 void irq_enter(void)
 {
 	rcu_irq_enter();
-	if (is_idle_task(current) && !in_interrupt()) {
+	if (is_idle_task(current) && !in_interrupt() && !in_freeze()) {
 		/*
 		 * Prevent raise_softirq from needlessly waking up ksoftirqd
 		 * here, as softirq will be serviced on return from interrupt.

@@ -364,7 +365,7 @@ static inline void tick_irq_exit(void)
 
 	/* Make sure that timer wheel updates are propagated */
 	if ((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu)) {
-		if (!in_interrupt())
+		if (!in_interrupt() && !in_freeze())
 			tick_nohz_irq_exit();
 	}
 #endif

kernel/time/clockevents.c +13 -0

index 5544990..6d9a4a3 100644
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/smp.h>
 #include <linux/device.h>
+#include <linux/suspend.h>
 
 #include "tick-internal.h"
 

@@ -579,6 +580,18 @@ int clockevents_notify(unsigned long reason, void *arg)
 		tick_resume();
 		break;
 
+	case CLOCK_EVT_NOTIFY_FREEZE_PREPARE:
+		tick_freeze_prepare();
+		break;
+
+	case CLOCK_EVT_NOTIFY_FREEZE:
+		tick_freeze();
+		break;
+
+	case CLOCK_EVT_NOTIFY_UNFREEZE:
+		tick_unfreeze();
+		break;
+
 	case CLOCK_EVT_NOTIFY_CPU_DEAD:
 		tick_shutdown_broadcast_oneshot(arg);
 		tick_shutdown_broadcast(arg);

kernel/time/tick-common.c +53 -0

index 7efeedf..0bbc886 100644
@@ -19,6 +19,7 @@
 #include <linux/profile.h>
 #include <linux/sched.h>
 #include <linux/module.h>
+#include <linux/suspend.h>
 
 #include <asm/irq_regs.h>
 

@@ -51,6 +52,16 @@ ktime_t tick_period;
 int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT;
 
 /*
+ * Tick device is per CPU device, when we freeze the timekeeping stuff, we
+ * want to freeze the tick device on all of the online CPUs.
+ *
+ * tick_freeze_target_depth is a counter used for freezing tick device, the
+ * initial value of it is online CPU number. When it is counted down to ZERO,
+ * all of the tick devices are freezed.
+ */
+static unsigned int tick_freeze_target_depth;
+
+/*
  * Debugging: see timer_list.c
  */
 struct tick_device *tick_get_device(int cpu)

@@ -375,15 +386,21 @@ void tick_shutdown(unsigned int *cpup)
 void tick_suspend(void)
 {
 	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+	struct clock_event_device *dev = td->evtdev;
 
+	dev->real_handler = dev->event_handler;
+	dev->event_handler = clockevents_handle_noop;
 	clockevents_shutdown(td->evtdev);
 }
 
 void tick_resume(void)
 {
 	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+	struct clock_event_device *dev = td->evtdev;
 	int broadcast = tick_resume_broadcast();
 
+	dev->event_handler = dev->real_handler;
+	dev->real_handler = NULL;
 	clockevents_set_mode(td->evtdev, CLOCK_EVT_MODE_RESUME);
 
 	if (!broadcast) {

@@ -394,6 +411,42 @@ void tick_resume(void)
 	}
 }
 
+void tick_freeze_prepare(void)
+{
+	tick_freeze_target_depth = num_online_cpus();
+}
+
+void tick_freeze(void)
+{
+	/*
+	 * This is serialized against a concurrent wakeup
+	 * via clockevents_lock
+	 */
+	tick_freeze_target_depth--;
+	tick_suspend();
+
+	/*
+	 * the last tick_suspend CPU suspends timekeeping
+	 */
+	if (!tick_freeze_target_depth)
+		timekeeping_freeze();
+}
+
+void tick_unfreeze(void)
+{
+	/*
+	 * the first wakeup CPU resumes timekeeping
+	 */
+	if (timekeeping_suspended) {
+		timekeeping_unfreeze();
+		touch_softlockup_watchdog();
+		tick_resume();
+		hrtimers_resume();
+	} else {
+		tick_resume();
+	}
+}
+
 /**
  * tick_init - initialize the tick control
  */

kernel/time/tick-internal.h +3 -0

index 366aeb4..8b5bab6 100644
@@ -27,6 +27,9 @@ extern void tick_handover_do_timer(int *cpup);
 extern void tick_shutdown(unsigned int *cpup);
 extern void tick_suspend(void);
 extern void tick_resume(void);
+extern void tick_freeze_prepare(void);
+extern void tick_freeze(void);
+extern void tick_unfreeze(void);
 extern bool tick_check_replacement(struct clock_event_device *curdev,
 				   struct clock_event_device *newdev);
 extern void tick_install_replacement(struct clock_event_device *dev);

kernel/time/timekeeping.c +57 -20

index ec1791f..a11065f 100644
@@ -1107,14 +1107,7 @@ void timekeeping_inject_sleeptime(struct timespec *delta)
 	clock_was_set();
 }
 
-/**
- * timekeeping_resume - Resumes the generic timekeeping subsystem.
- *
- * This is for the generic clocksource timekeeping.
- * xtime/wall_to_monotonic/jiffies/etc are
- * still managed by arch specific suspend/resume code.
- */
-static void timekeeping_resume(void)
+static void timekeeping_resume_compensate_time(void)
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
 	struct clocksource *clock = tk->tkr.clock;

@@ -1127,9 +1120,6 @@ static void timekeeping_resume(void)
 	read_persistent_clock(&tmp);
 	ts_new = timespec_to_timespec64(tmp);
 
-	clockevents_resume();
-	clocksource_resume();
-
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 	write_seqcount_begin(&tk_core.seq);
 

@@ -1186,16 +1176,9 @@ static void timekeeping_resume(void)
 	timekeeping_update(tk, TK_MIRROR | TK_CLOCK_WAS_SET);
 	write_seqcount_end(&tk_core.seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
-
-	touch_softlockup_watchdog();
-
-	clockevents_notify(CLOCK_EVT_NOTIFY_RESUME, NULL);
-
-	/* Resume hrtimers */
-	hrtimers_resume();
 }
 
-static int timekeeping_suspend(void)
+static void timekeeping_suspend_get_time(void)
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
 	unsigned long flags;

@@ -1242,11 +1225,65 @@ static int timekeeping_suspend(void)
 	timekeeping_update(tk, TK_MIRROR);
 	write_seqcount_end(&tk_core.seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+}
 
-	clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL);
+/*
+ * The following operations:
+ *	clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL);
+ *	clockevents_notify(CLOCK_EVT_NOTIFY_RESUME, NULL);
+ * are moved out of
+ *	timekeeping_freeze() and timekeeping_unfreeze()
+ * and, replaced by
+ *	tick_suspend() and tick_resume()
+ * and, put into:
+ *	tick_freeze() and tick_unfreeze()
+ * so we avoid clockevents_lock multiple access
+ */
+void timekeeping_freeze(void)
+{
+	/*
+	 * clockevents_lock being held
+	 */
+	timekeeping_suspend_get_time();
 	clocksource_suspend();
 	clockevents_suspend();
+}
 
+void timekeeping_unfreeze(void)
+{
+	/*
+	 * clockevents_lock being held
+	 */
+	clockevents_resume();
+	clocksource_resume();
+	timekeeping_resume_compensate_time();
+}
+
+/**
+ * timekeeping_resume - Resumes the generic timekeeping subsystem.
+ *
+ * This is for the generic clocksource timekeeping.
+ * xtime/wall_to_monotonic/jiffies/etc are
+ * still managed by arch specific suspend/resume code.
+ */
+static void timekeeping_resume(void)
+{
+	clockevents_resume();
+	clocksource_resume();
+	timekeeping_resume_compensate_time();
+
+	touch_softlockup_watchdog();
+	clockevents_notify(CLOCK_EVT_NOTIFY_RESUME, NULL);
+	/* Resume hrtimers */
+	hrtimers_resume();
+}
+
+static int timekeeping_suspend(void)
+{
+	timekeeping_suspend_get_time();
+	clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL);
+	clocksource_suspend();
+	clockevents_suspend();
 	return 0;
 }
 

Happy New Year, can you please take a look at this patch now?

Quoted 563 lines

The patch is based on v3.18.

Freeze is a general power saving state that processes are frozen, devices are suspended and CPUs are in idle state. However, when the system enters freeze state, there are a few timers keep ticking and hence consumes more power unnecessarily. The observed timer events in freeze state are:
- tick_sched_timer
- watchdog lockup detector
- realtime scheduler period timer

The system power consumption in freeze state will be reduced significantly if we quiesce these timers.

The patch is tested on:
- Sandybrdige-EP system, both RTC alarm and power button are able to wake the system up from freeze state.
- HP laptop EliteBook 8460p, both RTC alarm and power button are able to wake the system up from freeze state.
- Baytrail-T(ASUS_T100) platform, power button is able to wake the system up from freeze state.

Suggested-by Thomas Gleixner <[email protected]>

Signed-off-by Aubrey Li <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Len Brown <[email protected]>

Cc Alan Cox <[email protected]>

---
 drivers/cpuidle/cpuidle.c   | 13 ++++++++
 include/linux/clockchips.h  |  4 +++
 include/linux/suspend.h     |  4 +++
 include/linux/timekeeping.h |  2 ++
 kernel/power/suspend.c      | 50 ++++++++++++++++++++++++++---
 kernel/sched/idle.c         | 45 ++++++++++++++++++++++++++
 kernel/softirq.c            |  5 +--
 kernel/time/clockevents.c   | 13 ++++++++
 kernel/time/tick-common.c   | 53 +++++++++++++++++++++++++++++++
 kernel/time/tick-internal.h |  3 ++
 kernel/time/timekeeping.c   | 77 +++++++++++++++++++++++++++++++++------------
 11 files changed, 243 insertions(+), 26 deletions(-)

drivers/cpuidle/cpuidle.c +13 -0

index 125150d..b9a3ada 100644
@@ -20,6 +20,7 @@
 #include <linux/hrtimer.h>
 #include <linux/module.h>
 #include <trace/events/power.h>
+#include <linux/suspend.h>
 
 #include "cpuidle.h"
 

@@ -119,6 +120,18 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 	ktime_t time_start, time_end;
 	s64 diff;
 
+	/*
+	 * under the freeze scenario, the timekeeping is suspended
+	 * as well as the clock source device, so we bypass the idle
+	 * counter update in freeze idle
+	 */
+	if (in_freeze()) {
+		entered_state = target_state->enter(dev, drv, index);
+		if (!cpuidle_state_is_coupled(dev, drv, entered_state))
+			local_irq_enable();
+		return entered_state;
+	}
+
 	trace_cpu_idle_rcuidle(index, dev->cpu);
 	time_start = ktime_get();
 

include/linux/clockchips.h +4 -0

index 2e4cb67..d118e0b 100644
@@ -18,6 +18,9 @@ enum clock_event_nofitiers {
 	CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
 	CLOCK_EVT_NOTIFY_SUSPEND,
 	CLOCK_EVT_NOTIFY_RESUME,
+	CLOCK_EVT_NOTIFY_FREEZE_PREPARE,
+	CLOCK_EVT_NOTIFY_FREEZE,
+	CLOCK_EVT_NOTIFY_UNFREEZE,
 	CLOCK_EVT_NOTIFY_CPU_DYING,
 	CLOCK_EVT_NOTIFY_CPU_DEAD,
 };

@@ -95,6 +98,7 @@ enum clock_event_mode {
  */
 struct clock_event_device {
 	void			(*event_handler)(struct clock_event_device *);
+	void			(*real_handler)(struct clock_event_device *);
 	int			(*set_next_event)(unsigned long evt,
 						  struct clock_event_device *);
 	int			(*set_next_ktime)(ktime_t expires,

include/linux/suspend.h +4 -0

index 3388c1b..86a651c 100644
@@ -203,6 +203,8 @@ extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
 extern void freeze_set_ops(const struct platform_freeze_ops *ops);
 extern void freeze_wake(void);
+extern bool in_freeze(void);
+extern bool idle_should_freeze(void);
 
 /**
  * arch_suspend_disable_irqs - disable IRQs for suspend

@@ -230,6 +232,8 @@ static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
 static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {}
 static inline void freeze_wake(void) {}
+static inline bool in_freeze(void) { return false; }
+static inline bool idle_should_freeze(void) { return false; }
 #endif /* !CONFIG_SUSPEND */
 
 /* struct pbe is used for creating lists of pages that should be restored

include/linux/timekeeping.h +2 -0

index 1caa6b0..07957a9 100644
@@ -5,6 +5,8 @@
 
 void timekeeping_init(void);
 extern int timekeeping_suspended;
+extern void timekeeping_freeze(void);
+extern void timekeeping_unfreeze(void);
 
 /*
  * Get and set timeofday

kernel/power/suspend.c +46 -4

index c347e3c..6467fb8 100644
@@ -28,6 +28,7 @@
 #include <linux/ftrace.h>
 #include <trace/events/power.h>
 #include <linux/compiler.h>
+#include <linux/clockchips.h>
 
 #include "power.h"
 

@@ -37,7 +38,15 @@ const char *pm_states[PM_SUSPEND_MAX];
 static const struct platform_suspend_ops *suspend_ops;
 static const struct platform_freeze_ops *freeze_ops;
 static DECLARE_WAIT_QUEUE_HEAD(suspend_freeze_wait_head);
-static bool suspend_freeze_wake;
+
+/* freeze state machine */
+enum freeze_state {
+	FREEZE_STATE_NONE,      /* not in freeze */
+	FREEZE_STATE_ENTER,     /* enter freeze */
+	FREEZE_STATE_WAKE,      /* in freeze wakeup context */
+};
+
+static enum freeze_state suspend_freeze_state;
 
 void freeze_set_ops(const struct platform_freeze_ops *ops)
 {

@@ -46,23 +55,56 @@ void freeze_set_ops(const struct platform_freeze_ops *ops)
 	unlock_system_sleep();
 }
 
+bool in_freeze(void)
+{
+	return (suspend_freeze_state > FREEZE_STATE_NONE);
+}
+EXPORT_SYMBOL_GPL(in_freeze);
+
+bool idle_should_freeze(void)
+{
+	return (suspend_freeze_state == FREEZE_STATE_ENTER);
+}
+EXPORT_SYMBOL_GPL(idle_should_freeze);
+
 static void freeze_begin(void)
 {
-	suspend_freeze_wake = false;
+	suspend_freeze_state = FREEZE_STATE_NONE;
 }
 
 static void freeze_enter(void)
 {
+	suspend_freeze_state = FREEZE_STATE_ENTER;
+	get_online_cpus();
 	cpuidle_use_deepest_state(true);
 	cpuidle_resume();
-	wait_event(suspend_freeze_wait_head, suspend_freeze_wake);
+	clockevents_notify(CLOCK_EVT_NOTIFY_FREEZE_PREPARE, NULL);
+	/*
+	 * push all the CPUs into freeze idle loop
+	 */
+	wake_up_all_idle_cpus();
+	printk(KERN_INFO "PM: suspend to idle\n");
+	/*
+	 * put the current CPU into wait queue so that this CPU
+	 * is able to enter freeze idle loop as well
+	 */
+	wait_event(suspend_freeze_wait_head,
+		(suspend_freeze_state == FREEZE_STATE_WAKE));
+	printk(KERN_INFO "PM: resume from freeze\n");
 	cpuidle_pause();
 	cpuidle_use_deepest_state(false);
+	put_online_cpus();
+	suspend_freeze_state = FREEZE_STATE_NONE;
 }
 
 void freeze_wake(void)
 {
-	suspend_freeze_wake = true;
+	if (!in_freeze())
+		return;
+	/*
+	 * wake freeze task up
+	 */
+	suspend_freeze_state = FREEZE_STATE_WAKE;
 	wake_up(&suspend_freeze_wait_head);
 }
 EXPORT_SYMBOL_GPL(freeze_wake);

kernel/sched/idle.c +45 -0

index c47fce7..f28f8cb 100644
@@ -7,6 +7,7 @@
 #include <linux/tick.h>
 #include <linux/mm.h>
 #include <linux/stackprotector.h>
+#include <linux/suspend.h>
 
 #include <asm/tlb.h>
 

@@ -182,6 +183,45 @@ exit_idle:
 }
 
 /*
+ * cpu idle freeze function
+ */
+static void cpu_idle_freeze(void)
+{
+	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+
+	/*
+	 * suspend tick, the last CPU suspend timekeeping
+	 */
+	clockevents_notify(CLOCK_EVT_NOTIFY_FREEZE, NULL);
+	/*
+	 * idle loop here if idle_should_freeze()
+	 */
+	while (idle_should_freeze()) {
+		int next_state;
+		/*
+		 * interrupt must be disabled before cpu enters idle
+		 */
+		local_irq_disable();
+
+		next_state = cpuidle_select(drv, dev);
+		if (next_state < 0) {
+			arch_cpu_idle();
+			continue;
+		}
+		/*
+		 * cpuidle_enter will return with interrupt enabled
+		 */
+		cpuidle_enter(drv, dev, next_state);
+	}
+
+	/*
+	 * resume tick, the first wakeup CPU resume timekeeping
+	 */
+	clockevents_notify(CLOCK_EVT_NOTIFY_UNFREEZE, NULL);
+}
+
+/*
  * Generic idle loop implementation
  *
  * Called with polling cleared.

@@ -208,6 +248,11 @@ static void cpu_idle_loop(void)
 			if (cpu_is_offline(smp_processor_id()))
 				arch_cpu_idle_dead();
 
+			if (idle_should_freeze()) {
+				cpu_idle_freeze();
+				continue;
+			}
+
 			local_irq_disable();
 			arch_cpu_idle_enter();
 

kernel/softirq.c +3 -2

index 0699add..a231bf6 100644
@@ -26,6 +26,7 @@
 #include <linux/smpboot.h>
 #include <linux/tick.h>
 #include <linux/irq.h>
+#include <linux/suspend.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/irq.h>

@@ -321,7 +322,7 @@ asmlinkage __visible void do_softirq(void)
 void irq_enter(void)
 {
 	rcu_irq_enter();
-	if (is_idle_task(current) && !in_interrupt()) {
+	if (is_idle_task(current) && !in_interrupt() && !in_freeze()) {
 		/*
 		 * Prevent raise_softirq from needlessly waking up ksoftirqd
 		 * here, as softirq will be serviced on return from interrupt.

@@ -364,7 +365,7 @@ static inline void tick_irq_exit(void)
 
 	/* Make sure that timer wheel updates are propagated */
 	if ((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu)) {
-		if (!in_interrupt())
+		if (!in_interrupt() && !in_freeze())
 			tick_nohz_irq_exit();
 	}
 #endif

kernel/time/clockevents.c +13 -0

index 5544990..6d9a4a3 100644
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/smp.h>
 #include <linux/device.h>
+#include <linux/suspend.h>
 
 #include "tick-internal.h"
 

@@ -579,6 +580,18 @@ int clockevents_notify(unsigned long reason, void *arg)
 		tick_resume();
 		break;
 
+	case CLOCK_EVT_NOTIFY_FREEZE_PREPARE:
+		tick_freeze_prepare();
+		break;
+
+	case CLOCK_EVT_NOTIFY_FREEZE:
+		tick_freeze();
+		break;
+
+	case CLOCK_EVT_NOTIFY_UNFREEZE:
+		tick_unfreeze();
+		break;
+
 	case CLOCK_EVT_NOTIFY_CPU_DEAD:
 		tick_shutdown_broadcast_oneshot(arg);
 		tick_shutdown_broadcast(arg);

kernel/time/tick-common.c +53 -0

index 7efeedf..0bbc886 100644
@@ -19,6 +19,7 @@
 #include <linux/profile.h>
 #include <linux/sched.h>
 #include <linux/module.h>
+#include <linux/suspend.h>
 
 #include <asm/irq_regs.h>
 

@@ -51,6 +52,16 @@ ktime_t tick_period;
 int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT;
 
 /*
+ * Tick device is per CPU device, when we freeze the timekeeping stuff, we
+ * want to freeze the tick device on all of the online CPUs.
+ *
+ * tick_freeze_target_depth is a counter used for freezing tick device, the
+ * initial value of it is online CPU number. When it is counted down to ZERO,
+ * all of the tick devices are freezed.
+ */
+static unsigned int tick_freeze_target_depth;
+
+/*
  * Debugging: see timer_list.c
  */
 struct tick_device *tick_get_device(int cpu)

@@ -375,15 +386,21 @@ void tick_shutdown(unsigned int *cpup)
 void tick_suspend(void)
 {
 	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+	struct clock_event_device *dev = td->evtdev;
 
+	dev->real_handler = dev->event_handler;
+	dev->event_handler = clockevents_handle_noop;
 	clockevents_shutdown(td->evtdev);
 }
 
 void tick_resume(void)
 {
 	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+	struct clock_event_device *dev = td->evtdev;
 	int broadcast = tick_resume_broadcast();
 
+	dev->event_handler = dev->real_handler;
+	dev->real_handler = NULL;
 	clockevents_set_mode(td->evtdev, CLOCK_EVT_MODE_RESUME);
 
 	if (!broadcast) {

@@ -394,6 +411,42 @@ void tick_resume(void)
 	}
 }
 
+void tick_freeze_prepare(void)
+{
+	tick_freeze_target_depth = num_online_cpus();
+}
+
+void tick_freeze(void)
+{
+	/*
+	 * This is serialized against a concurrent wakeup
+	 * via clockevents_lock
+	 */
+	tick_freeze_target_depth--;
+	tick_suspend();
+
+	/*
+	 * the last tick_suspend CPU suspends timekeeping
+	 */
+	if (!tick_freeze_target_depth)
+		timekeeping_freeze();
+}
+
+void tick_unfreeze(void)
+{
+	/*
+	 * the first wakeup CPU resumes timekeeping
+	 */
+	if (timekeeping_suspended) {
+		timekeeping_unfreeze();
+		touch_softlockup_watchdog();
+		tick_resume();
+		hrtimers_resume();
+	} else {
+		tick_resume();
+	}
+}
+
 /**
  * tick_init - initialize the tick control
  */

kernel/time/tick-internal.h +3 -0

index 366aeb4..8b5bab6 100644
@@ -27,6 +27,9 @@ extern void tick_handover_do_timer(int *cpup);
 extern void tick_shutdown(unsigned int *cpup);
 extern void tick_suspend(void);
 extern void tick_resume(void);
+extern void tick_freeze_prepare(void);
+extern void tick_freeze(void);
+extern void tick_unfreeze(void);
 extern bool tick_check_replacement(struct clock_event_device *curdev,
 				   struct clock_event_device *newdev);
 extern void tick_install_replacement(struct clock_event_device *dev);

kernel/time/timekeeping.c +50 -20

index ec1791f..a11065f 100644
@@ -1107,14 +1107,7 @@ void timekeeping_inject_sleeptime(struct timespec *delta)
 	clock_was_set();
 }
 
-/**
- * timekeeping_resume - Resumes the generic timekeeping subsystem.
- *
- * This is for the generic clocksource timekeeping.
- * xtime/wall_to_monotonic/jiffies/etc are
- * still managed by arch specific suspend/resume code.
- */
-static void timekeeping_resume(void)
+static void timekeeping_resume_compensate_time(void)
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
 	struct clocksource *clock = tk->tkr.clock;

@@ -1127,9 +1120,6 @@ static void timekeeping_resume(void)
 	read_persistent_clock(&tmp);
 	ts_new = timespec_to_timespec64(tmp);
 
-	clockevents_resume();
-	clocksource_resume();
-
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 	write_seqcount_begin(&tk_core.seq);
 

@@ -1186,16 +1176,9 @@ static void timekeeping_resume(void)
 	timekeeping_update(tk, TK_MIRROR | TK_CLOCK_WAS_SET);
 	write_seqcount_end(&tk_core.seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
-
-	touch_softlockup_watchdog();
-
-	clockevents_notify(CLOCK_EVT_NOTIFY_RESUME, NULL);
-
-	/* Resume hrtimers */
-	hrtimers_resume();
 }
 
-static int timekeeping_suspend(void)
+static void timekeeping_suspend_get_time(void)
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
 	unsigned long flags;

@@ -1242,11 +1225,65 @@ static int timekeeping_suspend(void)
 	timekeeping_update(tk, TK_MIRROR);
 	write_seqcount_end(&tk_core.seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
+}
 
-	clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL);
+/*
+ * The following operations:
+ *	clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL);
+ *	clockevents_notify(CLOCK_EVT_NOTIFY_RESUME, NULL);
+ * are moved out of
+ *	timekeeping_freeze() and timekeeping_unfreeze()
+ * and, replaced by
+ *	tick_suspend() and tick_resume()
+ * and, put into:
+ *	tick_freeze() and tick_unfreeze()
+ * so we avoid clockevents_lock multiple access
+ */
+void timekeeping_freeze(void)
+{
+	/*
+	 * clockevents_lock being held
+	 */
+	timekeeping_suspend_get_time();
 	clocksource_suspend();
 	clockevents_suspend();
+}
 
+void timekeeping_unfreeze(void)
+{
+	/*
+	 * clockevents_lock being held
+	 */
+	clockevents_resume();
+	clocksource_resume();
+	timekeeping_resume_compensate_time();
+}
+
+/**
+ * timekeeping_resume - Resumes the generic timekeeping subsystem.
+ *
+ * This is for the generic clocksource timekeeping.
+ * xtime/wall_to_monotonic/jiffies/etc are
+ * still managed by arch specific suspend/resume code.
+ */
+static void timekeeping_resume(void)
+{
+	clockevents_resume();
+	clocksource_resume();
+	timekeeping_resume_compensate_time();
+
+	touch_softlockup_watchdog();
+	clockevents_notify(CLOCK_EVT_NOTIFY_RESUME, NULL);
+	/* Resume hrtimers */
+	hrtimers_resume();
+}

kernel/time/timekeeping.c static int timekeeping_suspend(void) · line 1289

Quoted 10 lines

kernel/time/timekeeping.c +7 -0

+
+static int timekeeping_suspend(void)
+{
+	timekeeping_suspend_get_time();
+	clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL);
+	clocksource_suspend();
+	clockevents_suspend();
 	return 0;
 }
 

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

include/linux/clockchips.h +18 -0

+static void cpu_idle_freeze(void)
+{
+	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+
+	/*
+	 * suspend tick, the last CPU suspend timekeeping
+	 */
+	clockevents_notify(CLOCK_EVT_NOTIFY_FREEZE, NULL);
+	/*
+	 * idle loop here if idle_should_freeze()
+	 */
+	while (idle_should_freeze()) {
+		int next_state;
+		/*
+		 * interrupt must be disabled before cpu enters idle
+		 */
+		local_irq_disable();

void irq_enter(void)
{

include/linux/clockchips.h +1 -1

 	rcu_irq_enter();
-	if (is_idle_task(current) && !in_interrupt()) {
+	if (is_idle_task(current) && !in_interrupt() && !in_freeze()) {
 		/*
 		 * Prevent raise_softirq from needlessly waking up ksoftirqd
 		 * here, as softirq will be serviced on return from interrupt.

include/linux/clockchips.h +20 -0

+void tick_freeze(void)
+{
+	/*
+	 * This is serialized against a concurrent wakeup
+	 * via clockevents_lock
+	 */
+	tick_freeze_target_depth--;
+	tick_suspend();
+
+	/*
+	 * the last tick_suspend CPU suspends timekeeping
+	 */
+	if (!tick_freeze_target_depth)
+		timekeeping_freeze();
+}
+
+void tick_unfreeze(void)
+{
+	/*
+	 * the first wakeup CPU resumes timekeeping

include/linux/clockchips.h enum clock_event_nofitiers { · line 23

Quoted 11 lines · Aubrey

include/linux/clockchips.h +3 -0

index 2e4cb67..d118e0b 100644
@@ -18,6 +18,9 @@ enum clock_event_nofitiers {
 	CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
 	CLOCK_EVT_NOTIFY_SUSPEND,
 	CLOCK_EVT_NOTIFY_RESUME,
+	CLOCK_EVT_NOTIFY_FREEZE_PREPARE,
+	CLOCK_EVT_NOTIFY_FREEZE,
+	CLOCK_EVT_NOTIFY_UNFREEZE,

Can we please stop adding more crap to that notifier thing? I rather see that go away than being expanded.

include/linux/clockchips.h enum clock_event_mode { · line 101

Quoted 6 lines

include/linux/clockchips.h +1 -0

@@ -95,6 +98,7 @@ enum clock_event_mode {
  */
 struct clock_event_device {
 	void			(*event_handler)(struct clock_event_device *);
+	void			(*real_handler)(struct clock_event_device *);

This is really not the place to put this. We have the hotpath functions together at the beginning of the struct and not some random stuff used once in a while. Think about cache lines.

A proper explanation why you need this at all is missing.

include/linux/clockchips.h enum clock_event_mode { · line 104

Quoted 3 lines

include/linux/clockchips.h +2 -0

 /*
+ * cpu idle freeze function
+ */

How is that comment helpful?

Not at all. But its simpler to add pointless comments than to document the important and non obvious stuff, right?

Quoted 19 lines

include/linux/clockchips.h +18 -0

+static void cpu_idle_freeze(void)
+{
+	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+
+	/*
+	 * suspend tick, the last CPU suspend timekeeping
+	 */
+	clockevents_notify(CLOCK_EVT_NOTIFY_FREEZE, NULL);
+	/*
+	 * idle loop here if idle_should_freeze()
+	 */
+	while (idle_should_freeze()) {
+		int next_state;
+		/*
+		 * interrupt must be disabled before cpu enters idle
+		 */
+		local_irq_disable();

include/linux/clockchips.h enum clock_event_mode {

Quoted 10 lines

include/linux/clockchips.h +10 -0

+
+		next_state = cpuidle_select(drv, dev);
+		if (next_state < 0) {
+			arch_cpu_idle();
+			continue;
+		}
+		/*
+		 * cpuidle_enter will return with interrupt enabled
+		 */
+		cpuidle_enter(drv, dev, next_state);

How is that supposed to work?

If timekeeping is not yet unfrozen, then any interrupt handling code which calls anything time related is going to hit lala land.

You must guarantee that timekeeping is unfrozen before any interrupt is handled. If you cannot guarantee that, you cannot freeze timekeeping ever.

The cpu local tick device is less critical, but it happens to work by chance, not by design.

Quoted 11 lines

void irq_enter(void)
{

include/linux/clockchips.h +1 -1

 	rcu_irq_enter();
-	if (is_idle_task(current) && !in_interrupt()) {
+	if (is_idle_task(current) && !in_interrupt() && !in_freeze()) {
 		/*
 		 * Prevent raise_softirq from needlessly waking up ksoftirqd
 		 * here, as softirq will be serviced on return from interrupt.

@@ -364,7 +365,7 @@ static inline void tick_irq_exit(void)
 

include/linux/clockchips.h static inline void tick_irq_exit(void) · line 369

Quoted 5 lines

include/linux/clockchips.h +1 -1

 	/* Make sure that timer wheel updates are propagated */
 	if ((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu)) {
-		if (!in_interrupt())
+		if (!in_interrupt() && !in_freeze())
 			tick_nohz_irq_exit();

We keep adding conditional over conditionals to the irq_enter/exit hotpath. Can we please find some more intelligent solution for this?

include/linux/clockchips.h void tick_shutdown(unsigned int *cpup) · line 392

Quoted 9 lines

include/linux/clockchips.h +3 -0

@@ -375,15 +386,21 @@ void tick_shutdown(unsigned int *cpup)
 void tick_suspend(void)
 {
 	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+	struct clock_event_device *dev = td->evtdev;
 
+	dev->real_handler = dev->event_handler;
+	dev->event_handler = clockevents_handle_noop;

Lacks a proper explanation. What's the point of this exercise?

include/linux/clockchips.h void tick_shutdown(unsigned int *cpup) · line 395

Quoted 4 lines

include/linux/clockchips.h +3 -0

+void tick_freeze_prepare(void)
+{
+	tick_freeze_target_depth = num_online_cpus();

So we have a 'notifier' callback just to store the number of online cpus? That's beyond silly. What's wrong with having a frozen counter and comparing that to num_online_cpus()?

Quoted 26 lines

include/linux/clockchips.h +26 -0

+void tick_freeze(void)
+{
+	/*
+	 * This is serialized against a concurrent wakeup
+	 * via clockevents_lock
+	 */
+	tick_freeze_target_depth--;
+	tick_suspend();
+
+	/*
+	 * the last tick_suspend CPU suspends timekeeping
+	 */
+	if (!tick_freeze_target_depth)
+		timekeeping_freeze();
+}
+
+void tick_unfreeze(void)
+{
+	/*
+	 * the first wakeup CPU resumes timekeeping
+	 */
+	if (timekeeping_suspended) {
+		timekeeping_unfreeze();
+		touch_softlockup_watchdog();
+		tick_resume();
+		hrtimers_resume();

include/linux/clockchips.h

Quoted 4 lines

include/linux/clockchips.h +4 -0

+	} else {
+		tick_resume();
+	}
+}

This is really horrible. We now have basically the same code for freeze/unfreeze and suspend/resume just in different files and with slightly different functionality.

And we have that just because you want to (ab)use clockevents_lock for serialization.

Thanks,

tglx

Quoted 2 lines · Thomas Gleixner

Can we please stop adding more crap to that notifier thing? I rather see that go away than being expanded.

Are you referring to FREEZE_PREPARE or remove all of FREEZE staff at all?

What's the disadvantage of adding more notifier?

Quoted 10 lines

kernel/sched/idle.c exit_idle: · line 215

Quoted 4 lines

kernel/sched/idle.c +4 -0

+		/*
+		 * cpuidle_enter will return with interrupt enabled
+		 */
+		cpuidle_enter(drv, dev, next_state);

How is that supposed to work?

If timekeeping is not yet unfrozen, then any interrupt handling code which calls anything time related is going to hit lala land.

Quoted 6 lines

You must guarantee that timekeeping is unfrozen before any interrupt is handled. If you cannot guarantee that, you cannot freeze timekeeping ever.

The cpu local tick device is less critical, but it happens to work by chance, not by design.

There are two way to guarantee this: the first way is, disable interrupt before timekeeping frozen and enable interrupt after timekeeping is unfrozen. However, we need to handle wakeup handler before unfreeze timekeeping to wake freeze task up from wait queue.

So we have to go the other way, the other way is, we ignore time related calls during freeze, like what I added in irq_enter below.

Or, we need to re-implement freeze wait and wake up mechanism?

It looks like a notifier is unpopular, I was trying to avoid a global variable across different kernel modules. But yes, I can change. May I know the disadvantage of notifier callback?

The V2 patch was using stop machine mechanism according to Peter's suggestion. This V3 patch, IIRC, clockevents_lock was your suggestion.

Quoted 7 lines · Aubrey

Quoted 2 lines · Thomas Gleixner

Can we please stop adding more crap to that notifier thing? I rather see that go away than being expanded.

Are you referring to FREEZE_PREPARE or remove all of FREEZE staff at all?

What's the disadvantage of adding more notifier?

clockevents_notify() is not a notifier. Its a multiplex call and I want to get rid of it and replace it with explicit functions.

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Aubrey

Quoted 2 lines · Thomas Gleixner

Can we please stop adding more crap to that notifier thing? I rather see that go away than being expanded.

Are you referring to FREEZE_PREPARE or remove all of FREEZE staff at all?

What's the disadvantage of adding more notifier?

clockevents_notify() is not a notifier. Its a multiplex call and I want to get rid of it and replace it with explicit functions.

OK, so perhaps we need to move _SUSPEND/_RESUME out of there to start with?

As far as I can say, clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL) and clockevents_notify(CLOCK_EVT_NOTIFY_RESUME, NULL) are each only called from one place and moreover, since they are in syscore_ops, we don't need any locking around them.

So what about the patch below?

Quoted 1 line · Aubrey

On 2015/1/22 18:15, Thomas Gleixner wrote:

Quoted 17 lines

Quoted 10 lines

kernel/sched/idle.c exit_idle: · line 215

Quoted 4 lines

kernel/sched/idle.c +4 -0

+		/*
+		 * cpuidle_enter will return with interrupt enabled
+		 */
+		cpuidle_enter(drv, dev, next_state);

How is that supposed to work?

If timekeeping is not yet unfrozen, then any interrupt handling code which calls anything time related is going to hit lala land.

Quoted 6 lines

You must guarantee that timekeeping is unfrozen before any interrupt is handled. If you cannot guarantee that, you cannot freeze timekeeping ever.

The cpu local tick device is less critical, but it happens to work by chance, not by design.

Quoted 7 lines

There are two way to guarantee this: the first way is, disable interrupt before timekeeping frozen and enable interrupt after timekeeping is unfrozen. However, we need to handle wakeup handler before unfreeze timekeeping to wake freeze task up from wait queue.

So we have to go the other way, the other way is, we ignore time related calls during freeze, like what I added in irq_enter below.

Groan. You just do not call in irq_enter/exit(), but what prevents any interrupt handler or whatever to call into the time/timer code after interrupts got reenabled?

Nothing.

Quoted 1 line

Or, we need to re-implement freeze wait and wake up mechanism?

You need to make sure in the low level idle implementation that this cannot happen.

tick_freeze()

{
	raw_spin_lock(&tick_freeze_lock);
	tick_frozen++;
	if (tick_frozen == num_online_cpus())
		timekeeping_suspend();
	else
		tick_suspend_local();
	raw_spin_unlock(&tick_freeze_lock);
}

tick_unfreeze()

{
	raw_spin_lock(&tick_freeze_lock);
	if (tick_frozen == num_online_cpus())
		timekeeping_resume();
	else
		tick_resume_local();
	tick_frozen--;
	raw_spin_unlock(&tick_freeze_lock);
}

idle_freeze()

{
	local_irq_disable();

	tick_freeze();

	/* Must keep interrupts disabled! */
       	go_deep_idle()

	tick_unfreeze();

	local_irq_enable();
}

That's the only way you can do it proper, everything else will just be a horrible mess of bandaids and duct tape.

So that does not need any of the irq_enter/exit conditionals, it does not need the real_handler hack. It just works.

The only remaining issue might be a NMI calling into ktime_get_mono_fast_ns() before timekeeping is resumed. Its probably a non issue on x86/tsc, but it might be a problem on other platforms which turn off devices, clocks, It's not rocket science to prevent that.

Quoted 13 lines · Rafael J. Wysocki

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Aubrey

Quoted 2 lines · Thomas Gleixner

Can we please stop adding more crap to that notifier thing? I rather see that go away than being expanded.

Are you referring to FREEZE_PREPARE or remove all of FREEZE staff at all?

What's the disadvantage of adding more notifier?

clockevents_notify() is not a notifier. Its a multiplex call and I want to get rid of it and replace it with explicit functions.

Quoted 8 lines

OK, so perhaps we need to move _SUSPEND/_RESUME out of there to start with?

As far as I can say, clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL) and clockevents_notify(CLOCK_EVT_NOTIFY_RESUME, NULL) are each only called from one place and moreover, since they are in syscore_ops, we don't need any locking around them.

So what about the patch below?

I'm cleaning up the whole replacement of notify. The stuff below is part of it.

Quoted 1 line

kernel/time/timekeeping.c static int timekeeping_suspend(void) · line 1303

Quoted 3 lines

kernel/time/timekeeping.c +2 -1

-	clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL);
+	tick_suspend();
+	tick_suspend_broadcast();

That's exactly the stuff I don't want to see. Blind code move.

tick_suspend_broadcast() wants to be called from tick_suspend().

Still compiling and testing a gazillion of combinations.

Quoted 5 lines · Thomas Gleixner

The only remaining issue might be a NMI calling into ktime_get_mono_fast_ns() before timekeeping is resumed. Its probably a non issue on x86/tsc, but it might be a problem on other platforms which turn off devices, clocks, It's not rocket science to prevent that.

I don't see any users of ktime_get_mono_fast_ns() at all, unless some non-trivial macros are involved. At least grepping for it only returns the definition, declarations and the line in trace.c.

Quoted 7 lines · Rafael J. Wysocki

Quoted 5 lines · Thomas Gleixner

The only remaining issue might be a NMI calling into ktime_get_mono_fast_ns() before timekeeping is resumed. Its probably a non issue on x86/tsc, but it might be a problem on other platforms which turn off devices, clocks, It's not rocket science to prevent that.

Quoted 3 lines

I don't see any users of ktime_get_mono_fast_ns() at all, unless some non-trivial macros are involved. At least grepping for it only returns the definition, declarations and the line in trace.c.

You can trace in NMI and perf is going to use ktime_get_mono_fast_ns() eventually as well.

Quoted 8 lines · Thomas Gleixner

Quoted 7 lines · Rafael J. Wysocki

Quoted 5 lines · Thomas Gleixner

The only remaining issue might be a NMI calling into ktime_get_mono_fast_ns() before timekeeping is resumed. Its probably a non issue on x86/tsc, but it might be a problem on other platforms which turn off devices, clocks, It's not rocket science to prevent that.

Quoted 6 lines

Quoted 3 lines

I don't see any users of ktime_get_mono_fast_ns() at all, unless some non-trivial macros are involved. At least grepping for it only returns the definition, declarations and the line in trace.c.

You can trace in NMI and perf is going to use ktime_get_mono_fast_ns() eventually as well.

So I'm not sure how to intercept that to be honest. Any hints?

Quoted 16 lines · Rafael J. Wysocki

Quoted 14 lines · Thomas Gleixner

Quoted 10 lines · Rafael J. Wysocki

Quoted 5 lines · Thomas Gleixner

The only remaining issue might be a NMI calling into ktime_get_mono_fast_ns() before timekeeping is resumed. Its probably a non issue on x86/tsc, but it might be a problem on other platforms which turn off devices, clocks, It's not rocket science to prevent that.

I don't see any users of ktime_get_mono_fast_ns() at all, unless some non-trivial macros are involved. At least grepping for it only returns the definition, declarations and the line in trace.c.

You can trace in NMI and perf is going to use ktime_get_mono_fast_ns() eventually as well.

Quoted 1 line

So I'm not sure how to intercept that to be honest. Any hints?

If we suspend timekeeping we can make the fast timekeeper point at a dummy readout function which returns the time at suspend, if the clock source does not resume automagically.

As I said TSC should be a non issue, but other stuff might be. We could make it conditional on CLOCK_SOURCE_SUSPEND_NONSTOP perhaps.

Quoted 28 lines · Thomas Gleixner

Quoted 13 lines · Rafael J. Wysocki

Quoted 11 lines · Thomas Gleixner

Quoted 7 lines · Aubrey

Quoted 2 lines · Thomas Gleixner

Can we please stop adding more crap to that notifier thing? I rather see that go away than being expanded.

Are you referring to FREEZE_PREPARE or remove all of FREEZE staff at all?

What's the disadvantage of adding more notifier?

clockevents_notify() is not a notifier. Its a multiplex call and I want to get rid of it and replace it with explicit functions.

Quoted 8 lines

OK, so perhaps we need to move _SUSPEND/_RESUME out of there to start with?

As far as I can say, clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL) and clockevents_notify(CLOCK_EVT_NOTIFY_RESUME, NULL) are each only called from one place and moreover, since they are in syscore_ops, we don't need any locking around them.

So what about the patch below?

I'm cleaning up the whole replacement of notify. The stuff below is part of it.

Quoted 1 line

Quoted 6 lines

kernel/time/timekeeping.c static int timekeeping_suspend(void) · line 1303

Quoted 3 lines

kernel/time/timekeeping.c +2 -1

-	clockevents_notify(CLOCK_EVT_NOTIFY_SUSPEND, NULL);
+	tick_suspend();
+	tick_suspend_broadcast();

That's exactly the stuff I don't want to see. Blind code move.

At least it's clear what the patch does. :-)

Quoted 1 line

tick_suspend_broadcast() wants to be called from tick_suspend().

OK

Quoted 1 line

Still compiling and testing a gazillion of combinations.

OK, so it looks like we need to wait with the suspend to idle changes until this lands.

Quoted 2 lines · Thomas Gleixner

Quoted 1 line · Aubrey

On 2015/1/22 18:15, Thomas Gleixner wrote:

[...]

Quoted 75 lines

Quoted 17 lines

Quoted 10 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+		/*
+		 * cpuidle_enter will return with interrupt enabled
+		 */
+		cpuidle_enter(drv, dev, next_state);

How is that supposed to work?

If timekeeping is not yet unfrozen, then any interrupt handling code which calls anything time related is going to hit lala land.

Quoted 6 lines

You must guarantee that timekeeping is unfrozen before any interrupt is handled. If you cannot guarantee that, you cannot freeze timekeeping ever.

The cpu local tick device is less critical, but it happens to work by chance, not by design.

Quoted 7 lines

There are two way to guarantee this: the first way is, disable interrupt before timekeeping frozen and enable interrupt after timekeeping is unfrozen. However, we need to handle wakeup handler before unfreeze timekeeping to wake freeze task up from wait queue.

So we have to go the other way, the other way is, we ignore time related calls during freeze, like what I added in irq_enter below.

Groan. You just do not call in irq_enter/exit(), but what prevents any interrupt handler or whatever to call into the time/timer code after interrupts got reenabled?

Nothing.

Quoted 1 line

Or, we need to re-implement freeze wait and wake up mechanism?

You need to make sure in the low level idle implementation that this cannot happen.

tick_freeze()

{
	raw_spin_lock(&tick_freeze_lock);
	tick_frozen++;
	if (tick_frozen == num_online_cpus())
		timekeeping_suspend();
	else
		tick_suspend_local();
	raw_spin_unlock(&tick_freeze_lock);
}

tick_unfreeze()

{
	raw_spin_lock(&tick_freeze_lock);
	if (tick_frozen == num_online_cpus())
		timekeeping_resume();
	else
		tick_resume_local();
	tick_frozen--;
	raw_spin_unlock(&tick_freeze_lock);
}

idle_freeze()

{
	local_irq_disable();

	tick_freeze();

	/* Must keep interrupts disabled! */
       	go_deep_idle()

	tick_unfreeze();

	local_irq_enable();
}

That's the only way you can do it proper, everything else will just be a horrible mess of bandaids and duct tape.

Quoted 2 lines

So that does not need any of the irq_enter/exit conditionals, it does not need the real_handler hack. It just works.

As long as go_deep_idle() above does not enable interrupts. This means we won't be able to use some C-states for suspend-to-idle (hald-induced C1 on some x86 for one example), but that's not a very big deal.

Quoted 6 lines

The only remaining issue might be a NMI calling into ktime_get_mono_fast_ns() before timekeeping is resumed. Its probably a non issue on x86/tsc, but it might be a problem on other platforms which turn off devices, clocks, It's not rocket science to prevent that.

I don't see any users of ktime_get_mono_fast_ns() at all, unless some non-trivial macros are involved. At least grepping for it only returns the definition, declarations and the line in trace.c.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 81 lines · Rafael J. Wysocki

Quoted 2 lines · Thomas Gleixner

Quoted 1 line · Aubrey

On 2015/1/22 18:15, Thomas Gleixner wrote:

[...]

Quoted 75 lines

Quoted 17 lines

Quoted 10 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+		/*
+		 * cpuidle_enter will return with interrupt enabled
+		 */
+		cpuidle_enter(drv, dev, next_state);

How is that supposed to work?

If timekeeping is not yet unfrozen, then any interrupt handling code which calls anything time related is going to hit lala land.

Quoted 6 lines

You must guarantee that timekeeping is unfrozen before any interrupt is handled. If you cannot guarantee that, you cannot freeze timekeeping ever.

The cpu local tick device is less critical, but it happens to work by chance, not by design.

Quoted 7 lines

There are two way to guarantee this: the first way is, disable interrupt before timekeeping frozen and enable interrupt after timekeeping is unfrozen. However, we need to handle wakeup handler before unfreeze timekeeping to wake freeze task up from wait queue.

So we have to go the other way, the other way is, we ignore time related calls during freeze, like what I added in irq_enter below.

Groan. You just do not call in irq_enter/exit(), but what prevents any interrupt handler or whatever to call into the time/timer code after interrupts got reenabled?

Nothing.

Quoted 1 line

Or, we need to re-implement freeze wait and wake up mechanism?

You need to make sure in the low level idle implementation that this cannot happen.

tick_freeze()

{
	raw_spin_lock(&tick_freeze_lock);
	tick_frozen++;
	if (tick_frozen == num_online_cpus())
		timekeeping_suspend();
	else
		tick_suspend_local();
	raw_spin_unlock(&tick_freeze_lock);
}

tick_unfreeze()

{
	raw_spin_lock(&tick_freeze_lock);
	if (tick_frozen == num_online_cpus())
		timekeeping_resume();
	else
		tick_resume_local();
	tick_frozen--;
	raw_spin_unlock(&tick_freeze_lock);
}

idle_freeze()

{
	local_irq_disable();

	tick_freeze();

	/* Must keep interrupts disabled! */
       	go_deep_idle()

	tick_unfreeze();

	local_irq_enable();
}

That's the only way you can do it proper, everything else will just be a horrible mess of bandaids and duct tape.

Quoted 6 lines

Quoted 2 lines

So that does not need any of the irq_enter/exit conditionals, it does not need the real_handler hack. It just works.

As long as go_deep_idle() above does not enable interrupts. This means we won't be able to use some C-states for suspend-to-idle (hald-induced C1 on some x86 for one example), but that's not a very big deal.

Does the legacy ACPI system IO method to enter C2/C3 need interrupt enabled as well?

Do we need some platform ops to cover those legacy platforms? Different platform go different branch here.

Quoted 89 lines · Aubrey

Quoted 81 lines · Rafael J. Wysocki

Quoted 2 lines · Thomas Gleixner

Quoted 1 line · Aubrey

On 2015/1/22 18:15, Thomas Gleixner wrote:

[...]

Quoted 75 lines

Quoted 17 lines

Quoted 10 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+		/*
+		 * cpuidle_enter will return with interrupt enabled
+		 */
+		cpuidle_enter(drv, dev, next_state);

How is that supposed to work?

If timekeeping is not yet unfrozen, then any interrupt handling code which calls anything time related is going to hit lala land.

Quoted 6 lines

You must guarantee that timekeeping is unfrozen before any interrupt is handled. If you cannot guarantee that, you cannot freeze timekeeping ever.

The cpu local tick device is less critical, but it happens to work by chance, not by design.

Quoted 7 lines

There are two way to guarantee this: the first way is, disable interrupt before timekeeping frozen and enable interrupt after timekeeping is unfrozen. However, we need to handle wakeup handler before unfreeze timekeeping to wake freeze task up from wait queue.

So we have to go the other way, the other way is, we ignore time related calls during freeze, like what I added in irq_enter below.

Groan. You just do not call in irq_enter/exit(), but what prevents any interrupt handler or whatever to call into the time/timer code after interrupts got reenabled?

Nothing.

Quoted 1 line

Or, we need to re-implement freeze wait and wake up mechanism?

You need to make sure in the low level idle implementation that this cannot happen.

tick_freeze()

{
	raw_spin_lock(&tick_freeze_lock);
	tick_frozen++;
	if (tick_frozen == num_online_cpus())
		timekeeping_suspend();
	else
		tick_suspend_local();
	raw_spin_unlock(&tick_freeze_lock);
}

tick_unfreeze()

{
	raw_spin_lock(&tick_freeze_lock);
	if (tick_frozen == num_online_cpus())
		timekeeping_resume();
	else
		tick_resume_local();
	tick_frozen--;
	raw_spin_unlock(&tick_freeze_lock);
}

idle_freeze()

{
	local_irq_disable();

	tick_freeze();

	/* Must keep interrupts disabled! */
       	go_deep_idle()

	tick_unfreeze();

	local_irq_enable();
}

That's the only way you can do it proper, everything else will just be a horrible mess of bandaids and duct tape.

Quoted 6 lines

Quoted 2 lines

So that does not need any of the irq_enter/exit conditionals, it does not need the real_handler hack. It just works.

As long as go_deep_idle() above does not enable interrupts. This means we won't be able to use some C-states for suspend-to-idle (hald-induced C1 on some x86 for one example), but that's not a very big deal.

Quoted 5 lines

Does the legacy ACPI system IO method to enter C2/C3 need interrupt enabled as well?

Do we need some platform ops to cover those legacy platforms? Different platform go different branch here.

No, we don't.

I think this needs to be addressed in a different way overall. If you don't mind, I'd like to prepare my own version of the patch at this point. That likely will be simpler than trying to explain what I'd like to do and I guess I'll need a few iterations to get something acceptable anyway.

Quoted 7 lines · Aubrey

Quoted 2 lines · Thomas Gleixner

Can we please stop adding more crap to that notifier thing? I rather see that go away than being expanded.

Are you referring to FREEZE_PREPARE or remove all of FREEZE staff at all?

What's the disadvantage of adding more notifier?

clockevents_notify() is not a notifier. Its a multiplex call and I want to get rid of it and replace it with explicit functions.

Quoted 17 lines

Quoted 10 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+		/*
+		 * cpuidle_enter will return with interrupt enabled
+		 */
+		cpuidle_enter(drv, dev, next_state);

How is that supposed to work?

If timekeeping is not yet unfrozen, then any interrupt handling code which calls anything time related is going to hit lala land.

Quoted 6 lines

You must guarantee that timekeeping is unfrozen before any interrupt is handled. If you cannot guarantee that, you cannot freeze timekeeping ever.

The cpu local tick device is less critical, but it happens to work by chance, not by design.

Quoted 7 lines

There are two way to guarantee this: the first way is, disable interrupt before timekeeping frozen and enable interrupt after timekeeping is unfrozen. However, we need to handle wakeup handler before unfreeze timekeeping to wake freeze task up from wait queue.

So we have to go the other way, the other way is, we ignore time related calls during freeze, like what I added in irq_enter below.

Groan. You just do not call in irq_enter/exit(), but what prevents any interrupt handler or whatever to call into the time/timer code after interrupts got reenabled?

Nothing.

Quoted 1 line

Or, we need to re-implement freeze wait and wake up mechanism?

You need to make sure in the low level idle implementation that this cannot happen.

tick_freeze()

{
	raw_spin_lock(&tick_freeze_lock);
	tick_frozen++;
	if (tick_frozen == num_online_cpus())
		timekeeping_suspend();
	else
		tick_suspend_local();
	raw_spin_unlock(&tick_freeze_lock);
}

tick_unfreeze()

{
	raw_spin_lock(&tick_freeze_lock);
	if (tick_frozen == num_online_cpus())
		timekeeping_resume();
	else
		tick_resume_local();
	tick_frozen--;
	raw_spin_unlock(&tick_freeze_lock);
}

idle_freeze()

{
	local_irq_disable();

	tick_freeze();

	/* Must keep interrupts disabled! */
       	go_deep_idle()

	tick_unfreeze();

	local_irq_enable();
}

That's the only way you can do it proper, everything else will just be a horrible mess of bandaids and duct tape.

So that does not need any of the irq_enter/exit conditionals, it does not need the real_handler hack. It just works.

The only remaining issue might be a NMI calling into ktime_get_mono_fast_ns() before timekeeping is resumed. Its probably a non issue on x86/tsc, but it might be a problem on other platforms which turn off devices, clocks, It's not rocket science to prevent that.

Quoted 2 lines · Thomas Gleixner

Quoted 1 line · Aubrey

On 2015/1/22 18:15, Thomas Gleixner wrote:

Quoted 43 lines

You need to make sure in the low level idle implementation that this cannot happen.

tick_freeze()

{
	raw_spin_lock(&tick_freeze_lock);
	tick_frozen++;
	if (tick_frozen == num_online_cpus())
		timekeeping_suspend();
	else
		tick_suspend_local();
	raw_spin_unlock(&tick_freeze_lock);
}

tick_unfreeze()

{
	raw_spin_lock(&tick_freeze_lock);
	if (tick_frozen == num_online_cpus())
		timekeeping_resume();
	else
		tick_resume_local();
	tick_frozen--;
	raw_spin_unlock(&tick_freeze_lock);
}

idle_freeze()

{
	local_irq_disable();

	tick_freeze();

	/* Must keep interrupts disabled! */
       	go_deep_idle()

	tick_unfreeze();

	local_irq_enable();
}

That's the only way you can do it proper, everything else will just be a horrible mess of bandaids and duct tape.

Quoted 8 lines

So that does not need any of the irq_enter/exit conditionals, it does not need the real_handler hack. It just works.

The only remaining issue might be a NMI calling into ktime_get_mono_fast_ns() before timekeeping is resumed. Its probably a non issue on x86/tsc, but it might be a problem on other platforms which turn off devices, clocks, It's not rocket science to prevent that.

So the patch below is my version of the $subject one trying to take the above suggestion into account. It doesn not address the ktime_get_mono_fast_ns() in NMI case at this stage, because quite frankly I'd prefer to get the core changes right in the first place.

The new ->enter_freeze callback is there in struct cpuidle_state, because we generally cannot trust the existing ->enter callbacks to keep interrupts disabled all the time. Some of them enable interrupts temporarily and some idle entry methods enable interrupts automatically on exit. So ->enter_freeze is required to keep interrupts disabled and it is safe to suspend/resume timekeeping around it.

The rest is reasonably straightforward. If suspend-to-idle is requested, couidle_idle_call() calls cpuidle_enter_freeze() that bypasses the governor and selects the deepest suitable state. If ->enter_freeze is available, it will be used along with tick_freeze()/tick_unfreeze() and the normal idle entry code path will be used otherwise.

void freeze_wake(void)
{

(quoted diff) +10 -0

+	unsigned long flags;
+
+	spin_lock_irqsave(&suspend_freeze_lock, flags);
+	if (suspend_freeze_state > FREEZE_STATE_NONE) {
+		suspend_freeze_state = FREEZE_STATE_WAKE;
+		wake_up(&suspend_freeze_wait_head);
+	}
+	spin_unlock_irqrestore(&suspend_freeze_lock, flags);
 }
 
 
+static void enter_freeze_proper(struct cpuidle_driver *drv,
+				struct cpuidle_device *dev, int index)

Quoted 11 lines · Rafael J. Wysocki

void freeze_wake(void)
{

kernel/power/suspend.c +8 -0

+	unsigned long flags;
+
+	spin_lock_irqsave(&suspend_freeze_lock, flags);
+	if (suspend_freeze_state > FREEZE_STATE_NONE) {
+		suspend_freeze_state = FREEZE_STATE_WAKE;
+		wake_up(&suspend_freeze_wait_head);
+	}
+	spin_unlock_irqrestore(&suspend_freeze_lock, flags);
 }

drivers/cpuidle/cpuidle.c static int cpuidle_find_deepest_state(st · line 106

Quoted 12 lines

drivers/cpuidle/cpuidle.c +12 -0

+static void enter_freeze_proper(struct cpuidle_driver *drv,
+				struct cpuidle_device *dev, int index)
+{
+	tick_freeze();
+	drv->states[index].enter_freeze(dev, drv, index);
+	/*
+	 * timekeeping_resume() that will be called by tick_unfreeze() for the
+	 * last CPU executing it calls functions containing RCU read-side
+	 * critical sections, so tell RCU about that.
+	 */
+	RCU_NONIDLE(tick_unfreeze());
+}

So I'm a wee bit confused; if we use an enter_freeze() state that keeps interrupts disabled; who is going to call the freeze_wake() thing?

Quoted 14 lines · Peter Zijlstra

Quoted 11 lines · Rafael J. Wysocki

void freeze_wake(void)
{

kernel/power/suspend.c +8 -0

+	unsigned long flags;
+
+	spin_lock_irqsave(&suspend_freeze_lock, flags);
+	if (suspend_freeze_state > FREEZE_STATE_NONE) {
+		suspend_freeze_state = FREEZE_STATE_WAKE;
+		wake_up(&suspend_freeze_wait_head);
+	}
+	spin_unlock_irqrestore(&suspend_freeze_lock, flags);
 }

Quoted 15 lines

drivers/cpuidle/cpuidle.c static int cpuidle_find_deepest_state(st · line 106

Quoted 12 lines

drivers/cpuidle/cpuidle.c +12 -0

+static void enter_freeze_proper(struct cpuidle_driver *drv,
+				struct cpuidle_device *dev, int index)
+{
+	tick_freeze();
+	drv->states[index].enter_freeze(dev, drv, index);
+	/*
+	 * timekeeping_resume() that will be called by tick_unfreeze() for the
+	 * last CPU executing it calls functions containing RCU read-side
+	 * critical sections, so tell RCU about that.
+	 */
+	RCU_NONIDLE(tick_unfreeze());
+}

So I'm a wee bit confused; if we use an enter_freeze() state that keeps interrupts disabled; who is going to call the freeze_wake() thing?

Ah, I think I see, so we wake up, keep the interrupt pending, re-enable the tick and time and everybody, then re-enable interrupts, take the interrupt and go around the idle loop to find we need a reschedule etc..

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines · Thomas Gleixner

Quoted 1 line · Aubrey

On 2015/1/22 18:15, Thomas Gleixner wrote:

[cut]

Quoted 64 lines

Quoted 43 lines

You need to make sure in the low level idle implementation that this cannot happen.

tick_freeze()

{
	raw_spin_lock(&tick_freeze_lock);
	tick_frozen++;
	if (tick_frozen == num_online_cpus())
		timekeeping_suspend();
	else
		tick_suspend_local();
	raw_spin_unlock(&tick_freeze_lock);
}

tick_unfreeze()

{
	raw_spin_lock(&tick_freeze_lock);
	if (tick_frozen == num_online_cpus())
		timekeeping_resume();
	else
		tick_resume_local();
	tick_frozen--;
	raw_spin_unlock(&tick_freeze_lock);
}

idle_freeze()

{
	local_irq_disable();

	tick_freeze();

	/* Must keep interrupts disabled! */
       	go_deep_idle()

	tick_unfreeze();

	local_irq_enable();
}

That's the only way you can do it proper, everything else will just be a horrible mess of bandaids and duct tape.

Quoted 8 lines

So that does not need any of the irq_enter/exit conditionals, it does not need the real_handler hack. It just works.

The only remaining issue might be a NMI calling into ktime_get_mono_fast_ns() before timekeeping is resumed. Its probably a non issue on x86/tsc, but it might be a problem on other platforms which turn off devices, clocks, It's not rocket science to prevent that.

So the patch below is my version of the $subject one trying to take the above suggestion into account. It doesn not address the ktime_get_mono_fast_ns() in NMI case at this stage, because quite frankly I'd prefer to get the core changes right in the first place.

The new ->enter_freeze callback is there in struct cpuidle_state, because we generally cannot trust the existing ->enter callbacks to keep interrupts disabled all the time. Some of them enable interrupts temporarily and some idle entry methods enable interrupts automatically on exit. So ->enter_freeze is required to keep interrupts disabled and it is safe to suspend/resume timekeeping around it.

Quoted 5 lines

The rest is reasonably straightforward. If suspend-to-idle is requested, couidle_idle_call() calls cpuidle_enter_freeze() that bypasses the governor and selects the deepest suitable state. If ->enter_freeze is available, it will be used along with tick_freeze()/tick_unfreeze() and the normal idle entry code path will be used otherwise.

In the meantime I learned how to tell RCU that it actually should track stuff inside of the idle loop, so cpuidle_enter_freeze() can go under the rcu_idle_enter/exit() now which makes it look more like "real" idle.

Also I noticed that freeze_enter() could lose a wakeup event if it happened between dpm_suspend_noirq() and the point where suspend_freeze_state is set, so that race should be closed now.

In my testing (with a hacked ACPI cpuidle driver) I get a number of wakeups (usually no more than 30) in enter_freeze_proper() per suspend-to-idle cycle, but that number doesn't seem to depend on the length of sleep, so my theory is that this happens early until things relax sufficiently.

ktime_get_mono_fast_ns() is not covered yet, but I'm going to get to that next.

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines

So I'm a wee bit confused; if we use an enter_freeze() state that keeps interrupts disabled; who is going to call the freeze_wake() thing?

Ah, I think I see, so we wake up, keep the interrupt pending, re-enable the tick and time and everybody, then re-enable interrupts, take the interrupt and go around the idle loop to find we need a reschedule etc..

Exactly.

Quoted 5 lines · Rafael J. Wysocki

Quoted 3 lines · Peter Zijlstra

Quoted 2 lines

So I'm a wee bit confused; if we use an enter_freeze() state that keeps interrupts disabled; who is going to call the freeze_wake() thing?

Quoted 5 lines

Quoted 3 lines

Ah, I think I see, so we wake up, keep the interrupt pending, re-enable the tick and time and everybody, then re-enable interrupts, take the interrupt and go around the idle loop to find we need a reschedule etc..

Exactly.

So x86 mwait can do this;

what other archs can 'sleep' and keep interrupts disabled?

It looks like the ARM WFI thing wakes on pending interrupts and doesn't actually require interrupts to be enabled, so that too would work.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

The only remaining issue might be a NMI calling into ktime_get_mono_fast_ns() before timekeeping is resumed. Its probably a non issue on x86/tsc, but it might be a problem on other platforms which turn off devices, clocks, It's not rocket science to prevent that.

I'm not sure if this is what you meant, but here it goes.

The idea is to set up a dummy readout base for the fast timekeeper during timekeeping_suspend() such that it will always return the same number of cycles. After the last timekeeping_update() in timekeeping_suspend() we read the clocksource and store the result as cycles_at_suspend. The readout base from the current timekeeper is copied onto the dummy and the ->read pointer of the dummy is set to a routine unconditionally returning cycles_at_suspend. Next, the dummy is passed to update_fast_timekeeper() (which has been modified slightly to accept the readout base instead of a whole timekeeper).

Then, if I'm not mistaken, ktime_get_mono_fast_ns() should work until the subsequent timekeeping_resume() and then the proper readout base for the fast timekeeper will be restored by the timekeeping_update() called right after we've cleared timekeeping_suspended.

Index: linux-pm/kernel/time/timekeeping.c
===================================================================

kernel/time/timekeeping.c +16 -9

@@ -230,9 +230,7 @@ static inline s64 timekeeping_get_ns_raw
 
 /**
  * update_fast_timekeeper - Update the fast and NMI safe monotonic timekeeper.
- * @tk:		The timekeeper from which we take the update
- * @tkf:	The fast timekeeper to update
- * @tbase:	The time base for the fast timekeeper (mono/raw)
+ * @tkr: Timekeeping readout base from which we take the update
  *
  * We want to use this from any context including NMI and tracing /
  * instrumenting the timekeeping code itself.

@@ -244,11 +242,11 @@ static inline s64 timekeeping_get_ns_raw
  * smp_wmb();	<- Ensure that the last base[1] update is visible
  * tkf->seq++;
  * smp_wmb();	<- Ensure that the seqcount update is visible
- * update(tkf->base[0], tk);
+ * update(tkf->base[0], tkr);
  * smp_wmb();	<- Ensure that the base[0] update is visible
  * tkf->seq++;
  * smp_wmb();	<- Ensure that the seqcount update is visible
- * update(tkf->base[1], tk);
+ * update(tkf->base[1], tkr);
  *
  * The reader side does:
  *

@@ -269,7 +267,7 @@ static inline s64 timekeeping_get_ns_raw
  * slightly wrong timestamp (a few nanoseconds). See
  * @ktime_get_mono_fast_ns.
  */
-static void update_fast_timekeeper(struct timekeeper *tk)
+static void update_fast_timekeeper(struct tk_read_base *tkr)
 {
 	struct tk_read_base *base = tk_fast_mono.base;
 

@@ -277,7 +275,7 @@ static void update_fast_timekeeper(struc
 	raw_write_seqcount_latch(&tk_fast_mono.seq);
 
 	/* Update base[0] */
-	memcpy(base, &tk->tkr, sizeof(*base));
+	memcpy(base, tkr, sizeof(*base));
 
 	/* Force readers back to base[0] */
 	raw_write_seqcount_latch(&tk_fast_mono.seq);

@@ -462,7 +460,7 @@ static void timekeeping_update(struct ti
 		memcpy(&shadow_timekeeper, &tk_core.timekeeper,
 		       sizeof(tk_core.timekeeper));
 
-	update_fast_timekeeper(tk);
+	update_fast_timekeeper(&tk->tkr);
 }
 
 /**

@@ -1251,9 +1249,18 @@ static void timekeeping_resume(void)
 	hrtimers_resume();
 }
 
-static int timekeeping_suspend(void)
+static struct tk_read_base tkr_dummy;
+static cycle_t cycles_at_suspend;
+
+static cycle_t dummy_clock_read(struct clocksource *cs)
+{
+	return cycles_at_suspend;
+}
+
+int timekeeping_suspend(void)
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
+	struct clocksource *clock = tk->tkr.clock;
 	unsigned long flags;
 	struct timespec64		delta, delta_delta;
 	static struct timespec64	old_delta;

@@ -1296,6 +1303,14 @@ static int timekeeping_suspend(void)
 	}
 
 	timekeeping_update(tk, TK_MIRROR);

Complete patch with that modification is appended. In the next few days I'm going to split it into smaller parts and send along with cpuidle driver patches implementing ->enter_freeze.

Please let me know what you think.

kernel/sched/idle.c +8 -0

@@ -104,6 +105,21 @@ static void cpuidle_idle_call(void)
 	rcu_idle_enter();
 
 	/*
+	 * Suspend-to-idle ("freeze") is a system state in which all user space
+	 * has been frozen, all I/O devices have been suspended and the only
+	 * activity happens here and in iterrupts (if any).  In that case bypass
+	 * the cpuidle governor and go stratight for the deepest idle state
+	 * available.  Possibly also suspend the local tick and the entire
+	 * timekeeping to prevent timer interrupts from kicking us out of idle
+	 * until a proper wakeup interrupt happens.
+	 */

Quoted 25 lines · Rafael J. Wysocki

Complete patch with that modification is appended. In the next few days I'm going to split it into smaller parts and send along with cpuidle driver patches implementing ->enter_freeze.

Please let me know what you think.

kernel/sched/idle.c +15 -0

@@ -104,6 +105,21 @@ static void cpuidle_idle_call(void)
 	rcu_idle_enter();
 
 	/*
+	 * Suspend-to-idle ("freeze") is a system state in which all user space
+	 * has been frozen, all I/O devices have been suspended and the only
+	 * activity happens here and in iterrupts (if any).  In that case bypass
+	 * the cpuidle governor and go stratight for the deepest idle state
+	 * available.  Possibly also suspend the local tick and the entire
+	 * timekeeping to prevent timer interrupts from kicking us out of idle
+	 * until a proper wakeup interrupt happens.
+	 */
+	if (idle_should_freeze()) {
+		cpuidle_enter_freeze();
+		local_irq_enable();
+		goto exit_idle;
+	}
+
+	/*

kernel/sched/idle.c static void cpuidle_idle_call(void) · line 125

Quoted 3 lines

kernel/sched/idle.c +0 -0

 	 * Ask the cpuidle framework to choose a convenient idle state.
 	 * Fall back to the default arch idle method on errors.
 	 */

I was hoping to not have to put that into the regular idle path; say maybe share a single special branch with the play-dead call. People seem to start complaining about the total amount of time it takes to just 'run' the idle path.

Now I don't think we can do that, because we need the arch_cpu_idle_enter() nonsense for the one but not the other; also all this really only makes sense in the cpuidle context, so nothing to be done about that.

In any case, you could make that:

static inline bool idle_should_freeze(void)

{
	return unlikely(suspend_freeze_state == FREEZE_STATE_ENTER);
}

which should help a bit I suppose.

drivers/cpuidle/cpuidle.c static int cpuidle_find_deepest_state(st · line 99

Quoted 5 lines

drivers/cpuidle/cpuidle.c +5 -0

+static void enter_freeze_proper(struct cpuidle_driver *drv,
+                               struct cpuidle_device *dev, int index)
+{
+       tick_freeze();
+       drv->states[index].enter_freeze(dev, drv, index);

This is slightly different from cpuidle_enter() in that it does not consider the coupled states nonsense, is that on purpose? And if so, does that want a comment?

drivers/cpuidle/cpuidle.c static int cpuidle_find_deepest_state(st · line 106

Quoted 7 lines

drivers/cpuidle/cpuidle.c +7 -0

+       /*
+        * timekeeping_resume() that will be called by tick_unfreeze() for the
+        * last CPU executing it calls functions containing RCU read-side
+        * critical sections, so tell RCU about that.
+        */
+       RCU_NONIDLE(tick_unfreeze());
+}

But over all it looks fine to me.