Proposal for optimistic suspend idea.

So this is a *VERY VERY* rough draft of patches in order to try to illustrate an idea I've been thinking about since the wakelocks and suspend-blockers debates happened.

While I doubt the idea is all that novel, I haven't really seen anything similar discussed yet. Additionally this has very much been influenced by discussions with Arve Hj�nnev�g, Mark Gross, Rafael Wysocki, Alan Stern, Kevin Hilman, Magnus Damm and others, so credits to all of them for clarifying the numerous points of views and requirements that exist, and helping me better formulate the idea (even if they don't like it :).

there were three main requirements needed for Android.

1) Method for the kernel to inhibit suspend during some critical section
2) Method for userland to inhibit suspend during some critical section
3) Method using the first two to inhibit suspend from the point that a wakeup event occurs, until userland has consumed that event.

The third item is the most subtle, and seems to be often overlooked.

The wakelock/suspend_blockers proposal solved #3 by using the following pattern in userland:

	wake_lock();
	do_stuff();
	wake_unlock();

	select();	//kernel will grab a wakelock when event occurs

	wake_lock();
	read();		//kernel will drop wakelock when data is read

	do_stuff()
	wake_unlock();	

This pattern allows the kernel to inhibit suspend from the wakeup event until the new data from the event (such as a keyboard press) was read. The select allows userland to wait until data has arrived, before grabbing its own wakelock, and then reading the data (where the kernels' wakelock is then dropped).

This provides proper overlapping critical sections from the wakeup event all the way to userland consuming the event.

Recently, the pm_stay_awake/pm_relax and wakeup_source infrastructure has been included into mainline. Some have claimed that this infrastructure is sufficient to implement wakelock-like functionality, but because the wakeup_count only provides a count of the number of wakeup events, it does not provide a way to know if userland has consumed the event. So this third requirement is not yet met.

In my mind, one disadvantage of the wakelock/suspend-blocker API is that it requires userland to know what devices are wakeup devices, and must follow the pattern above interlocking wake_lock calls between select and read. This makes it a somewhat special case API, with very suspend-specific rules.

Instead, it seems to me that we could look at this more from the schedulers point of view. We could have some way for tasks to mark themselves as "important", and that would allow us to decide if suspend could occur or not.

In this (again very very rough) draft patch queue, I've used a new SCHED_STAYAWAKE sched_setsched() flag to be used by applications to mark themselves as important. This notifies the system that it should not suspend until the task un-marks itself or dies.

Now, the problem with just some scheduler call is that it really isn't any different from the userland wakelock api.

However, here is where the idea is different. Instead of userland having to know what devices are wakeup sources and dropping a wakelock before blocking on them, and then having to do a dance to reacquire the wakelock between the select and the read(), why not leave it all up to the kernel.

The application would mark itself as important, and do whatever it needs to do. Should it end up blocking on a device that is backed by a wakeup source, the kernel could understand that, and de-boost the task's importance when it blocks. Then when the wakeup event occurs, when the task is woken up, the kernel would re-boost the task to its original importance.

This can be imagined as a sort of upside down priority inheritance.

Here is an example userland testcase: suspend-block-test.c:

/* Build: gcc -lrt suspend-block-test.c -o suspend-block-test */ #include <stdio.h>
#include <sched.h>

#define SCHED_STAYAWAKE 0x0f000000
#define CLOCK_BOOTTIME 7
#define CLOCK_BOOTTIME_ALARM 9

void main(void)

{
	struct sched_param param;
	int err;
	param.sched_priority = 0;
	sched_setscheduler(0, SCHED_STAYAWAKE, &param);
	while (1) {
		struct timespec ts1, ts2;
		char input[256];
		printf("Waiting for input:");
		scanf("%s", input);

		ts1.tv_sec = 10;
		ts1.tv_nsec = 0;
		printf("Sleeping for 10 seconds (BOOTTIME)\n");
		clock_nanosleep(CLOCK_BOOTTIME, 0, &ts1, &ts2);
		printf("wakeup!\n");

		printf("Sleeping for 10 seconds (BOOTTIME_ALARM)\n");
		clock_nanosleep(CLOCK_BOOTTIME_ALARM, 0, &ts1, &ts2);
		printf("wakeup!\n");
	}
	return;
}

As you can see, after marking itself as important, the application doesn't need to do anything else special. The system will only allow suspends to occur while the task is blocked in clock_nanosleep() on the RTC backed BOOTTIME_ALARM clockid.

Now, this simplified API comes at the cost that it relies hevily on the kernel to do the right thing. And the kernel must be able to know what devices are backed by wakeup events or not, and also must protect the wakeup event to task-wakeup path such that a suspend will not trigger before the task is re-boosted. These paths can be long and varied, through threaded-irqs, soft-irqs, workqueues and even timers. So protecting those paths in the kernel may add quite a bit of complexity. Possibly so much that this really becomes infeasible.

There are also other complicated cases, like selecting on multiple fds, where some are backed by wakeup events and some aren't, where its not clear if de-boosting or not de-boosting would be the right decision.

Further, its likely additional work will be needed refining the wakeup_source code so we can better distinquish between wake up events and in-kernel critical section where we want to block suspend. Currently they are one and the same. But as those in-kernel critical sections are added, its likely the process of suspending the system may run across code paths that have such critical sections. And while those critical sections need to complete before suspend finishes, we don't want them to cause suspend to abort, as it would if a wakeup event occured.

None the less, because the userland API is a bit simpler, and entrusts the kernel to do the right thing, it may also be a more flexible userland API to live with for the comming decades. I could imagine it being useful for non-suspend power related scheduling decisions, such as if the scheduler should enforce idleness (via something like idle cycle injection) to save power or not. Or maybe such a flag could be used to decide if a task's timers are important enough to schedule timer irqs in the near future or not.

But that's all blue-sky talk.

For now, I'd just be interested in what folks think about the concept with regards to the wakelock discussions. Where it might not be sufficient? Or what other disadvantages might it have? Are there any varients to this idea that would be better?

Again, at this point I'm not trying to push this specific idea forward for inclusion, but I'm hoping to just put it forward for review to see if it can't help spur some constructive discussion about it or alternative approaches.

The following patch queue implements:
* Changes to the suspend code to enforce that suspend will not occur while a wakeup is in progress. This is not strictly required, but seems useful to me.
* Changes to the scheduler to manage both the per-task importance and global active count, as well as re-boosting on task wakeup.
* Changes to the RTC layers to inhibit suspend on the path from IRQ to the task wakeup call.
* Change to the alarmtimer nanosleep call to deboost before blocking

I'll admit there are a lot of problems with the current patches, but hopefully they communicate the idea well enough to allow for discussion.

Again, big thanks to Mark Gross, Rafael Wysocki, and Alan Stern for listening and helping me better formulate the idea, and letting me know where they saw problems with it.

thanks
-john

John Stultz (6):
[RFC] suspend: Block suspend when wakeups are in-progress [RFC] sched: Add support for SCHED_STAYAWAKE flag [RFC] rtc: rtc-cmos: Add pm_stay_awake/pm_relax calls around IRQ [RFC] rtc: interface: Add pm_stay_awake/pm_relax chaining rtc
workqueue processing
[RFC] alarmtimer: Add pm_stay_awake /pm_relax calls [RFC] alarmtimer: Deboost on nanosleep

 drivers/base/power/wakeup.c |   13 ++++++
 drivers/rtc/interface.c     |   16 ++++++-
 drivers/rtc/rtc-cmos.c      |   13 +++++-
 include/linux/sched.h       |   12 +++++
 include/linux/suspend.h     |    3 +
 kernel/exit.c               |    2 +
 kernel/fork.c               |    2 +
 kernel/power/hibernate.c    |    5 ++
 kernel/power/suspend.c      |    5 ++
 kernel/sched.c              |  101 +++++++++++++++++++++++++++++++++++++++++++
 kernel/time/alarmtimer.c    |   48 +++++++++++++++++++--
 11 files changed, 212 insertions(+), 8 deletions(-)

1.7.3.2.146.gca209

With the current pm_stay_awake/pm_relax api, reads to /sys/power/wakeup_count will block when pm_stay_awake() has been called. Then once pm_relax() returns, the read will unblock and return the wakeup_count value. This value can be echo'ed back into wakeup_count, and if no other wakeup events have occured, the system can be suspended by calling "echo mem > /sys/power/state".

This method is somewhat advisory, as if a wakeup event has occured between the reading of /sys/power/wakeup_count and the attempt to suspend, that attempt to suspend will fail. However, if a second attmept to suspend is tried, without checking /sys/power/wakeup_count, the suspend will succeed.

Similarly, if pm_stay_awake() has been called, and then a suspend is attepted wihtout checking /sys/power/wakeup_count, the suspend will succeed, despite the pm_relax() call not having been made.

This patch tries to make the pm_stay_awake() call a bit more enforcing, such that any attempt to suspend that occurs while a wakeup is in progress will fail. Once the matching pm_relax() has been called, suspend will succeed.

This does not change the blocking behavior of /sys/power/wakeup_count, or the suspend failure if a stale wakeup count has been echo'ed into the /sys/power/wakeup_count.

Also modified the hibernate path in the same way.

---
 drivers/base/power/wakeup.c |   13 +++++++++++++
 include/linux/suspend.h     |    3 +++
 kernel/power/hibernate.c    |    5 +++++
 kernel/power/suspend.c      |    5 +++++
 4 files changed, 26 insertions(+), 0 deletions(-)

index 84f7c7d..0722873 100644
@@ -580,6 +580,19 @@ static void pm_wakeup_update_hit_counts(void)
 	rcu_read_unlock();
 }
 
+
+bool pm_wakeup_in_progress(void)
+{
+	unsigned int cnt, inpr;
+	unsigned long flags;
+
+	spin_lock_irqsave(&events_lock, flags);
+	split_counters(&cnt, &inpr);
+	spin_unlock_irqrestore(&events_lock, flags);
+
+	return (inpr != 0);
+}
+
 /**
  * pm_wakeup_pending - Check if power transition in progress should be aborted.
  *

index 6bbcef2..c9403a1 100644
@@ -294,6 +294,7 @@ extern int unregister_pm_notifier(struct notifier_block *nb);
 extern bool events_check_enabled;
 
 extern bool pm_wakeup_pending(void);
+extern bool pm_wakeup_in_progress(void);
 extern bool pm_get_wakeup_count(unsigned int *count);
 extern bool pm_save_wakeup_count(unsigned int count);
 #else /* !CONFIG_PM_SLEEP */

@@ -311,6 +312,8 @@ static inline int unregister_pm_notifier(struct notifier_block *nb)
 #define pm_notifier(fn, pri)	do { (void)(fn); } while (0)
 
 static inline bool pm_wakeup_pending(void) { return false; }
+static inline bool pm_wakeup_in_progress(void) { return false; }
+
 #endif /* !CONFIG_PM_SLEEP */
 
 extern struct mutex pm_mutex;

index 8f7b1db..22dc22c 100644
@@ -611,6 +611,11 @@ int hibernate(void)
 		goto Unlock;
 	}
 
+	if (pm_wakeup_in_progress()) {
+		error = -EBUSY;
+		goto Unlock;
+	}
+
 	pm_prepare_console();
 	error = pm_notifier_call_chain(PM_HIBERNATION_PREPARE);
 	if (error)

index b6b71ad..b2dba52 100644
@@ -281,6 +281,11 @@ int enter_state(suspend_state_t state)
 	if (!mutex_trylock(&pm_mutex))
 		return -EBUSY;
 
+	if (pm_wakeup_in_progress()) {
+		error = -EBUSY;
+		goto Unlock;
+	}
+
 	printk(KERN_INFO "PM: Syncing filesystems ... ");
 	sys_sync();
 	printk("done.\n");

1.7.3.2.146.gca209

This is a draft proof of concept on how a stayawake scheduler flag could be used to inhibit suspend from userland.

I'm in no way married to this specific api, but this acts a a concrete example of the following idea I'd like to propose:

First there is some method for a task to mark and unmark itself as "important".

While there are any "important" tasks, no matter if they are runnable or not, suspend could not occur (this is not unlike Android's userland wakelocks).

Now, If an "important" task were to block on a device that the kernel knows to be a wake-up source, the kerenl can choose to de-boost the "important" task, so that while blocked, it would not be considered "important".

Upon task wakeup, the kernel would re-boost the task back to its prior level of importance.

One can sort of imagine this as an upside-down priority inheritance.

This patch provides the API for a task to mark and umark itself as "important" and block suspend, as well as the hook on wakeup to reboost any de-boosted tasks.

Now, for corrrectness, in order to avoid races with suspend attempts that might occur after a wakeup event but before the "important" task is reboosted on wakeup, there would need to be over-lapping pm_stay_awake and pm_relax chaining, so the entire IRQ->task wakeup path prohibited suspend.

---
 include/linux/sched.h |   12 ++++++
 kernel/exit.c         |    2 +
 kernel/fork.c         |    2 +
 kernel/sched.c        |  101 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 117 insertions(+), 0 deletions(-)

index 4ac2c05..3557838 100644
@@ -41,6 +41,8 @@
 #define SCHED_IDLE		5
 /* Can be ORed in to make sure the process is reverted back to SCHED_NORMAL on fork */
 #define SCHED_RESET_ON_FORK     0x40000000
+#define SCHED_STAYAWAKE		0x0f000000
+
 
 #ifdef __KERNEL__
 

@@ -1566,6 +1568,10 @@ struct task_struct {
 		unsigned long memsw_nr_pages; /* uncharged mem+swap usage */
 	} memcg_batch;
 #endif
+
+	int task_active_count;
+	int task_active_boosted;
+
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 	atomic_t ptrace_bp_refcnt;
 #endif

@@ -1753,6 +1759,12 @@ static inline void put_task_struct(struct task_struct *t)
 extern void task_times(struct task_struct *p, cputime_t *ut, cputime_t *st);
 extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *st);
 
+
+extern void sched_inc_active_count(void);
+extern void sched_dec_active_count(void);
+extern void sched_deboost_task_active_count(struct task_struct *p);
+extern void sched_boost_task_active_count(struct task_struct *p);
+
 /*
  * Per process flags
  */

index 2913b35..71f7bd4 100644
@@ -994,6 +994,8 @@ NORET_TYPE void do_exit(long code)
 	 */
 	perf_event_exit_task(tsk);
 
+	sched_deboost_task_active_count(tsk);
+
 	cgroup_exit(tsk, 1);
 
 	if (group_dead)

index 8e6b6f4..c79c4fb 100644
@@ -1213,6 +1213,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	p->memcg_batch.do_batch = 0;
 	p->memcg_batch.memcg = NULL;
 #endif
+	p->task_active_count = 0;
+	p->task_active_boosted = 0;
 
 	/* Perform scheduler related setup. Assign this task to a CPU. */
 	sched_fork(p);

1.7.3.2.146.gca209

Flag the rtc-cmos IRQ event as a wakeup event using pm_stay_awake and pm_relax()

---
 drivers/rtc/rtc-cmos.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

index 05beb6c..1cc4688 100644
@@ -538,11 +538,15 @@ static struct bin_attribute nvram = {
 
 static struct cmos_rtc	cmos_rtc;
 
+static struct wakeup_source *rtc_cmos_wakelock;
+
 static irqreturn_t cmos_interrupt(int irq, void *p)
 {
 	u8		irqstat;
 	u8		rtc_control;
 
+	__pm_stay_awake(rtc_cmos_wakelock);
+
 	spin_lock(&rtc_lock);
 
 	/* When the HPET interrupt handler calls us, the interrupt

@@ -573,9 +577,12 @@ static irqreturn_t cmos_interrupt(int irq, void *p)
 
 	if (is_intr(irqstat)) {
 		rtc_update_irq(p, 1, irqstat);
+		__pm_relax(rtc_cmos_wakelock);
 		return IRQ_HANDLED;
-	} else
-		return IRQ_NONE;
+	}
+
+	__pm_relax(rtc_cmos_wakelock);
+	return IRQ_NONE;
 }
 
 #ifdef	CONFIG_PNP

@@ -1153,6 +1160,8 @@ static int __init cmos_init(void)
 {
 	int retval = 0;
 
+	rtc_cmos_wakelock = wakeup_source_register("rtc_cmos");
+
 #ifdef	CONFIG_PNP
 	retval = pnp_register_driver(&cmos_pnp_driver);
 	if (retval == 0)

1.7.3.2.146.gca209

It seems that only the 0/6 patch in this sequence was given any attention. However it appears to me that 3, 4, and 5 stand on their own and are clearly bug-fixes - the bug being that the wake-up event generated by the CMOS rtc is not being counted as a wakeup event by the power core.

I'm less clear on 5, but that is probably simply that I don't understand this code very well yet.

So maybe resend 4,5,6 by themselves and get them accepted? Or maybe Rafael could just pick them up and review/apply them as they are ???

Thanks,
NeilBrown

To ensure suspend events don't trigger between the irq and the rtc workqueue function running, use pm_stay_awake/pm_relax.

---
 drivers/rtc/interface.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

index 44e91e5..ba1c437 100644
@@ -19,6 +19,15 @@
 static int rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer);
 static void rtc_timer_remove(struct rtc_device *rtc, struct rtc_timer *timer);
 
+static struct wakeup_source *rtc_interface_wakelock;
+static int __init rtc_interface_wakelock_init(void)
+{
+       rtc_interface_wakelock = wakeup_source_register("rtc_interface");
+       return 0;
+}
+core_initcall(rtc_interface_wakelock_init);
+
+
 static int __rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm)
 {
 	int err;

@@ -563,6 +572,7 @@ enum hrtimer_restart rtc_pie_update_irq(struct hrtimer *timer)
 void rtc_update_irq(struct rtc_device *rtc,
 		unsigned long num, unsigned long events)
 {
+	__pm_stay_awake(rtc_interface_wakelock);
 	schedule_work(&rtc->irqwork);
 }
 EXPORT_SYMBOL_GPL(rtc_update_irq);

@@ -751,9 +761,10 @@ static int rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer)
 		alarm.time = rtc_ktime_to_tm(timer->node.expires);
 		alarm.enabled = 1;
 		err = __rtc_set_alarm(rtc, &alarm);
-		if (err == -ETIME)
+		if (err == -ETIME) {
+			__pm_stay_awake(rtc_interface_wakelock);
 			schedule_work(&rtc->irqwork);
-		else if (err) {
+		} else if (err) {
 			timerqueue_del(&rtc->timerqueue, &timer->node);
 			timer->enabled = 0;
 			return err;

@@ -849,6 +860,7 @@ again:
 	}
 
 	mutex_unlock(&rtc->ops_lock);
+	__pm_relax(rtc_interface_wakelock);
 }
 
 

1.7.3.2.146.gca209

This provides wakelock like chaining to protects the RTC wakeup path to the hrtimer firing of the alarmtimer.

---
 kernel/time/alarmtimer.c |   44 ++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 42 insertions(+), 2 deletions(-)

index ea5e1a9..00ee80f 100644
@@ -46,12 +46,17 @@ static struct alarm_base {
 static ktime_t freezer_delta;
 static DEFINE_SPINLOCK(freezer_delta_lock);
 
+
 #ifdef CONFIG_RTC_CLASS
 /* rtc timer and device for setting alarm wakeups at suspend */
 static struct rtc_timer		rtctimer;
 static struct rtc_device	*rtcdev;
 static DEFINE_SPINLOCK(rtcdev_lock);
 
+static struct wakeup_source *alarmtimer_wakelock;
+static int stay_awake;
+static DEFINE_SPINLOCK(stay_awake_lock);
+
 /**
  * has_wakealarm - check rtc device has wakealarm ability
  * @dev: current device

@@ -73,6 +78,19 @@ static int has_wakealarm(struct device *dev, void *name_ptr)
 	return 1;
 }
 
+/* rtctimer function called by first rtc interrupt after suspend */
+void alarmtimer_resume_call(void* p)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&stay_awake_lock, flags);
+	if (stay_awake == 1) {
+		stay_awake = 2;
+		__pm_stay_awake(alarmtimer_wakelock);
+	}
+	spin_unlock_irqrestore(&stay_awake_lock, flags);
+}
+
 /**
  * alarmtimer_get_rtcdev - Return selected rtcdevice
  *

@@ -99,7 +117,7 @@ static struct rtc_device *alarmtimer_get_rtcdev(void)
 			 * rtc_open takes its own.
 			 */
 			put_device(dev);
-			rtc_timer_init(&rtctimer, NULL, NULL);
+			rtc_timer_init(&rtctimer, alarmtimer_resume_call, NULL);
 		}
 	}
 	ret = rtcdev;

@@ -158,6 +176,8 @@ static void alarmtimer_remove(struct alarm_base *base, struct alarm *alarm)
 }
 
 
+
+
 /**
  * alarmtimer_fired - Handles alarm hrtimer being fired.
  * @timer: pointer to hrtimer being run

@@ -175,6 +195,8 @@ static enum hrtimer_restart alarmtimer_fired(struct hrtimer *timer)
 	ktime_t now;
 	int ret = HRTIMER_NORESTART;
 
+
+	__pm_stay_awake(alarmtimer_wakelock);
 	spin_lock_irqsave(&base->lock, flags);
 	now = base->gettime();
 	while ((next = timerqueue_getnext(&base->timerqueue))) {

@@ -206,6 +228,13 @@ static enum hrtimer_restart alarmtimer_fired(struct hrtimer *timer)
 	}
 	spin_unlock_irqrestore(&base->lock, flags);
 
+	spin_lock_irqsave(&stay_awake_lock, flags);
+	if (stay_awake == 2)
+		__pm_relax(alarmtimer_wakelock);
+	stay_awake = 0;
+	spin_unlock_irqrestore(&stay_awake_lock, flags);
+	__pm_relax(alarmtimer_wakelock);
+
 	return ret;
 
 }

@@ -225,12 +254,14 @@ static int alarmtimer_suspend(struct device *dev)
 {
 	struct rtc_time tm;
 	ktime_t min, now;
+	int min_base; 
 	unsigned long flags;
 	struct rtc_device *rtc;
 	int i;
 
 	spin_lock_irqsave(&freezer_delta_lock, flags);
 	min = freezer_delta;
+	min_base = 0;
 	freezer_delta = ktime_set(0, 0);
 	spin_unlock_irqrestore(&freezer_delta_lock, flags);
 

@@ -251,8 +282,10 @@ static int alarmtimer_suspend(struct device *dev)
 		if (!next)
 			continue;
 		delta = ktime_sub(next->expires, base->gettime());
-		if (!min.tv64 || (delta.tv64 < min.tv64))
+		if (!min.tv64 || (delta.tv64 < min.tv64)) {
 			min = delta;
+			min_base = i;
+		}
 	}
 	if (min.tv64 == 0)
 		return 0;

@@ -266,6 +299,10 @@ static int alarmtimer_suspend(struct device *dev)
 	now = rtc_tm_to_ktime(tm);
 	now = ktime_add(now, min);
 
+	spin_lock_irqsave(&stay_awake_lock, flags);
+	stay_awake=1;
+	spin_unlock_irqrestore(&stay_awake_lock, flags);
+
 	rtc_timer_start(rtc, &rtctimer, now, ktime_set(0, 0));
 
 	return 0;

@@ -703,6 +740,9 @@ static int __init alarmtimer_init(void)
 		.nsleep		= alarm_timer_nsleep,
 	};
 
+	alarmtimer_wakelock = wakeup_source_register("alarmtimer");
+	stay_awake=0;
+
 	posix_timers_register_clock(CLOCK_REALTIME_ALARM, &alarm_clock);
 	posix_timers_register_clock(CLOCK_BOOTTIME_ALARM, &alarm_clock);
 

1.7.3.2.146.gca209

Example of deboosting tasks before blocking on a wakeup source like alarmtimer based nanosleep.

With this final patch, the kernel will be able to suspend when SCHED_STAYAWAKE flagged tasks are blocked on the alarmtimer.

---
 kernel/time/alarmtimer.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

index 00ee80f..c855d40 100644
@@ -568,9 +568,10 @@ static int alarmtimer_do_nsleep(struct alarm *alarm, ktime_t absexp)
 	do {
 		set_current_state(TASK_INTERRUPTIBLE);
 		alarm_start(alarm, absexp, ktime_set(0, 0));
-		if (likely(alarm->data))
+		if (likely(alarm->data)) {
+			sched_deboost_task_active_count(current);
 			schedule();
-
+		}
 		alarm_cancel(alarm);
 	} while (alarm->data && !signal_pending(current));
 

1.7.3.2.146.gca209

I would like to know why people still think wakelocks are remotely sane?

anything? What's wrong with regular idle?

So no, you've got a massive major NAK for anything touching the scheduler for this utter braindamage.

Well. Regular idle still takes up more power with my desktop then I could save with suspend.

My personal use case: I do nightly backups with rdiff-backup. I'd like to schedule those backup using an alarm-timer, so I could suspend my system when I'm not using it. So far, so good, that all works.

However, if my system tries to suspend itself after 15 minutes of X input idle, and my backup at 2am takes more then 15 minutes, then the backup gets interrupted. Because rdiff-backup is more of a transactional style backup, it then has to roll back any incomplete changes and try again the next night, which will surely take more then 15 minutes, etc.

I could try to inhibit suspend by making requests to my desktop environment, so the desktop-specific power management daemon won't trigger suspend. But given recent behavior, I don't trust that not to break when I upgrade my system, or if I get frustrated with one desktop environment, that I won't have to use a different api for whatever other environment I pick next.

Another use case I've heard about are systems that have firmware updates that are remotely triggered. Should the system go into suspend while the firmware update is going on, you end up with a brick.

Having to have multiple distro/release specific quirks to get the power-management-daemon to inhibit suspend is annoying enough, but then you also have to deal with custom changes by administrator, or remote power management systems like power nap, which might also echo "mem" into /sys/power/state when you're not expecting it. A kernel method to really block suspend would be nice. While this doesn't necessarily need to be conflated with wakelock style suspend, there is some need to allow userland to block suspend at the kernel level, and once you have that, I can't imagine folks not trying to stretch that into something like wakelocks. So you might as well at least try to design it reasonably well to start.

And again, this doesn't have to be suspend specific. As I mentioned, one way of reducing power drain by increasing timer slack, or just not scheduling processes for some chunk of time. However, there really isn't any good scheduler primitives that allow userspace to communicate when that is ok or not.

I personally think there is a growing need for a more power-aware scheduling class. In talking with others, I've said I sometimes think of my proposal as a form of "opportunistic scheduling", where the system is only going to spend power to allow specific tasks to run. Since those important tasks will do things that block for short amounts of time (disk io, etc), less-important tasks can opportunistically use the idle cycles of the active task. But when the active tasks are finished, we stop scheduling anyone else. There are some folks looking at trying to use cgroups for this sort of prioritizing, but that has issues with priority inversion style issues when sharing resources across cgroups.

Fair enough, I didn't really expect a warm welcome from you on this. :) So I'll take my lumps.

But while I understand you see this as crap, I'd be interested if you think the approach is maybe an improvement or not over prior attempts? While I'm not picky about the specific API being sched_setscheduler, I see a conceptual benefit to this approach, as it provides information to the scheduler that would allow the scheduler to make other informed decisions.

Where as other attempts which really didn't involve the scheduler at all, and just suspended based only on if there were any active critical sections. Causing some to charge that it created a second-scheduler.

For my proposal, there could also be other cases that might parallel the priority inheritance code, where a "important" task A is blocked waiting on some resource held by a non-important task B which is blocked on a device that is backed by a wakeup source. In that case, you could maybe pass the "importance" from task A to task B, then allowing B to be deboosted while blocked on the wakeup source, and allow suspend to safely occur. Granted, this gets pretty complex, and isn't really necessary, but I can imagine interested folks could hole up in academia for awhile on similar approaches.

So with these sorts of parallels, it seems this sort of thing should be connected in the scheduler in some way, no?

And again, I'm not sending these patches to push for inclusion, but only just to communicate the idea and to get folks discussing the merits and faults of this implementation compared with wakelocks or other alternatives they might have.

thanks
-john

Blame Intel ;-) Personally I loathe suspend because it kills all my network links.

So your fail is to tie suspend to the input inactivity instead of the completion of your backup thingy.

Kick the friggin Desktop folks already for messing up. I mean, because userspace is incompetent this needs to go in the kernel? Ere long we'll have a kernel based GUI if we go that route.

Bricks are good for throwing at those Desktop folks ;-)

How about you create a daemon tasked with managing /sys/power/state and change /sys/power/state such that it can be opened only once, then that daemon can keep the fd open and everything else trying to poke at it will get a fail.

Then fix up the fallout.

I'm probably stupid, but what?! Why would the scheduler want to care about this nonsense?

What you should do (and what Android should have done) is change the runtime so you mandate power aware apps, and anything violating the runtime gets killed.

For Desktop apps this probably involves D-Bus or whatnot, where the system tells the apps what state it is in. Apps should then respect this state.

For instance anybody trying to draw to an X surface after they've been told the screen is off should get kicked. (And before people go whinge about d-bus having to wake all tasks to get the msgs across, which wastes power; if you fix the runtime up far enough the attempt of drawing could return this information.)

I'm not quite sure how timer-slack comes into this, because every app receiving random wakeups (no matter what slack) after its been told it should quiesce is a fail, with the exception of the wakeup for telling it its good to go again (but that comes _after_ the system policy change, so its fine).

That's just insane.. why bother running anything but the 'important' tasks. Idle is more power aware than running random crap tasks that have no business running in the first place.

IOW you should stop tasks from being runnable in the first place, once you're in a situation where you've got random runnable processes you've failed.

Nothing the scheduler can do about that.

Also, this is a fucked up definition of power-aware scheduling. Normally power-aware scheduling is about optimizing throughput/watt, and that's a hard enough problem. No reason to conflate the issue with shitty userspace that doesn't know what the fuck its doing.

No its still wakelocks, its still trying to force a shitty bunch of userspace that doesn't know shit into half-way behaving.

And from experience (having an Android phone) it simply doesn't work worth shit.. there's plenty apps out there that suck battery like nobodies business, so clearly all the wakelock crap in the world doesn't help one whit.

So stop fucking about and start fixing the runtime.

Where I'm sitting, the moment you need to scheduler to interfere you've already failed. Tasks that you don't want to run shouldn't be runnable, full stop.

That only because they're shit, see above.

No, clearly B was runnable to begin with, someone forced its ass to sleep, they fail. Never allow a task to go into indefinite sleep while holding a resource.

Same for the kernel, we don't allow a return to userspace with a kernel lock held, or a call into the freezer while holding a lock. Why would you want to allow this for userspace.

I think I've made my position clear.

Well, its both. If the backup runs very long, and I'm using the machine in the morning, I don't want the end of my backup to suspend the system.

Well, to be fair to the desktop guys, they have been working to try to provide a DBUS api to handle this.

But even with a proper DBUS api, there's still the race when I walk away from my computer 15 minutes before the backup starts.

In that case, my backup application's alarm timer fires and schedules the backup, but then before the backup application runs and sends its DBUS message to block suspend, the suspend occurs. And yea, that's probably not a problem for my use, but it limits any similar power savings from an environment where reliability might actually matter.

Read that last bit again, as it has seemingly been missed over and over in these discussions. This ability to make sure wake up events are consumed by userland before suspending again is key.

[snip]

That's actually pretty interesting. It doesn't handle the race issues between wakeup event and event consumption by userland, but not a bad tool to have in the toolbox as we look at other approaches.

For the same reason it cares about SCHED_FIFO.

Sure, and real-time tasks should coordinate with all of userland to just make sure no one gets in the way! And those applications should respect that! Why is all that *useless* code in the kernel!? :)

So you're suggesting we rewrite everything in the debian package archives to use DBUS and abide by some sort of userland power policy?

I'll admit its a terrible straw-man, but this starts to sound like: "We don't need protected memory! Just rewrite all the apps so they don't accidentally overwrite kernel structures!"

Don't get me wrong, I do think that there are power-optimizations to be had using more "runtime" regulated behavior. You're right, when the screen is powered off, the clock applet shouldn't be trying to make X calls to update itself every minute.

But I think once you get away from some theoretical system, containing only properly behaving apps, having some tools in the kernel to enforce a certain type of behavior (and having the kernel's ability to handle more complex cases like importance inheritance) might be helpful.

Its really not that different conceptually from aligning timers. Making sure that when we fire, we expire as many timers as we can in one go, and run all the tasks that need to run, so we can go back to idle for as long as possible.

But instead of idling "until the next timer group", we split stuff we don't care that much about (but needs to be there), and stuff we do care about, and only schedule the hardware to fire for the events we do care about.

Consider your desktop. Consider servers. Are really ontop of every task and are sure its not inefficient, or doesn't have some edge case bug where it just flips out and chews cpu (I'm looking at you flashplayer!). The real world is filled with crap.

I disagree. Why are the inmates running the asylum? The scheduler decides what runs when and where. We're not at the mercy of bad applications, they're at the mercy of the scheduler.

And I do get that its a hard enough problem, and have nothing but respect for your work there. But I suspect its likely to get harder as it gets more important. Once the throughput side is maxed out, being able to further reduce the watt side of the equation is going to have value. And being able to do that without rewriting all of userland is compelling to folks. That is why suspend is being exploited in a number of these cases (in more or less hackish ways, depending).

Its like using sleeping spinlocks instead of having to rewrite every driver so that they didn't have any long held critical sections. One works in practice and the other is better in theory.

One can configure a Linux system to run like crap, therefore everyone's
focus on performance didn't help one whit? Come on, nothing is a
cure-all. But that's not really an argument against improving something or providing necessary tools to allow for certain types of optimizations.

And don't take me for an Android apologist. I think the poor battery anecdotes that pervade are a big problem. One issue I suspect with Android's wakelocks is that many are timeout based, which keeps things active for longer then necessary. Additionally, since all wakelock-izing of drivers happens out of the tree, there's no sanity reviewing, so extending those timeouts to avoid issues are quick fixes that get phones out the door, but at the price of battery life.

I believe that one of the benefits of my proposal is that it avoids the need for such timeouts. But I realize at this point in the conversation comparing apples vs oranges probably isn't productive if you're argument is "round things suck". :)

SIGSTOP the world? (and melt^H^H^H^Hfreeze with you!)

You want them to run, its just a matter of when.

Maybe the word "resource" was too general. Consider circular buffer semaphores used to sync producer/consumer with my earlier example above. Tasks waiting on tasks waiting on input are not an uncommon pattern.

Thanks again for the feedback and critique. I recognize you are very passionate in your distaste for wakelocks and similar ideas, and I don't mean to wind you up too much arguing with you.

I'll happily admit that my proposal and the vision I see of how the scheduler could function here may have real issues and might add unnecessary complexity.

But I also want to separate my specific solution from the problem at large. I do think that there are issues that my proposal and wakelocks address that the hand-wavy "just do it in userspace" rebuttals don't deal with (again specifically: wakeup event consumption in userland before the next suspend).

I do think my idea is neat, so we can spend/burn time debating it, but if you have viable suggestions (and honestly, I think rewriting all of userland isn't viable) on solving the larger problem, I'm all ears.

thanks again!
-john

In fact they should, its impossible to develop two sets of real-time applications and shove them on the same box without co-ordination.

SCHED_FIFO is an utter trainwreck, you're talking to the guy who gave a conference talk on why SCHED_FIFO sucks.

The only reason we have SCHED_FIFO is because of POSIX, and its one of those things where they fucked up worse than usual (mostly because people often don't realize how broken it is).

That sounds interesting. Do you have a link to the paper and/or presentation?

Thanks,
Richard

Not really, I'm also the guy who doesn't write papers and has minimal slides (and has lost those he had).

But the gist is that SCHED_FIFO doesn't provide proper resource control:
its possible to overload the system, nor does it provide proper resource
isolation: (like already stated) its impossible to fold two properly working RT systems onto one machine and still have them work correctly (even when the combined utilization < 1).

So you want to add something that sends a SIGBUS or similar to an application if it gets a wakeup when its not supposed to? That's what happens with memory protection, you get killed.

And yes, when Arjan did his powertop thing people did go fix up everything that made the top.

It really isn't that hard to fix most of these apps. But the problem with Android is that the battery usage thing it has is utterly useless. It mostly just tells you wireless or whatnot is sucking power, not what app is causing that. (And of course the fact that its all fucking Java crap, which I'll refuse to touch anyway).

This results in people (me) uninstalling everything they installed since last it worked well and then cursing this crappy shit for wasting their time.

I really think Android is a pile of privacy violating shit, I can't say I actually enjoy using the phone. I'm at a point where I can call with it (without it telling google whoem all I know and where I'm at) and browse the interwebs and have decided to screw apps, they don't work anyway.

The only reason I'm not back to the N900 is because the Android thing is a lot faster at rendering web (better cpu etc..).

Although that is more reactive then what I'm thinking.

I'm thinking more enforcement on the front end, where instead of bunching timers around time intervals, we bunch timers around the events of "important" processes.

So its not applications get wakeups when they aren't supposed to, its that we don't schedule timer interrupts for their timers, and we don't schedule them unless the system is "active" running an "important" task (allowing them to exploit the small idle cycles of the "important" task, which are likely too short to allow for any deep-idle state to trigger).

Basically we want the system to be in as deep an idle as possible for as long as possible. Bunching both timers and task execution allows this. But instead of pushing things out maybe a second, can we stall tasks for minutes or hours? If we start doing that, we're going to need some way for tasks to communicate that: "now is a really bad time for you to freeze me".

So I'm trying to come up with an API that we can use for that issue as well (since it starts to approximates the suspend case as those forced idle times grow).

I'll see if I can't start playing with some patches to try to demonstrate the idea in a non-suspend context.

Yea, the really terrible offenders did improve and I think that was great! But now how do we deal with the hundreds of normal applications, that when combined still cause quite a bit of wakeup noise? How do we really get to minutes of deep idle on a "normal" system?

Sure Android has issues, no argument there.

But realize that while part of my motivation is to try to mend the Android fork, another part is that the need for extreme power efficiency is not just about phones. I'm trying to find a solution that works for both on servers with classic linux distros as well as the Android environment (or better, is demonstratively superior to the wakelocks solution - fixing all your complaints about your phones battery life!).

So arguments about how Android userland sucks aren't really relevant.

I'm sorry my patch doesn't address the privacy concerns of using an Android (or really any) cell phone. :)

thanks
-john

But but but, my badly coded bouncing cows thing simply doesn't need to run when we wake up to refill the sound buffers for the mp3 player while the screen is still off!

Yet the wakelock thing will wake the system and lets us schedule bouncing cows just fine..

I really don't get your argument. It just doesn't make any sense. What I'm saying is, what about those apps we really don't care about, and really don't need to be there.

You're right. While the sound buffers are being refilled, there might be idle cycles that the wasteful bouncing cow app gets to run during.

And I'm also not disagreeing that blocking draw events from the framework if the screen is off would be smart.

But I think there's a difference between wasteful bouncing cow app, and the normal everyday background tasks that run on a standard linux system.

I just think that we need a way to block the background noise of normal systems, so there should be some way for the kernel to distinguish between background noise and not.

thanks
-john

Yes, this is why I loathe to update to a new distro, there's bound to new and improved *kit-daemon crap about which does random wakeups even though you know you're not using any of it, and when you're trying to uninstall that junk it thinks it needs to uninstall the world :-(

On servers you mostly can, and I mostly have, wiped all that stuff.

As for the Desktop, yes it is an unforgivable pile of shit. Even without flash, I've got 4G of memory and I can't read email and browse the web at the same time. Something is broken.

About Flash, just say no, its one of the things Apple did get right ;-)

How the fuck does the scheduler know your bouncing cows crap really shouldn't be running because the screen is off? There is absolutely _NOTHING_ the scheduler can do about that.

The user said he wanted this app to run, the app is runnable, we run it. That simple.

You know, once you drop the whole suspend notion that race goes away.

Esp. on the mobile hardware there really isn't anything different between a deep idle state and suspend.

So just make the thing idle and your suspend race goes away.

There's still things like the cgroup-freezer if you really want to force stuff down, but really your core system should be sane and not actually do anything unless asked.

Also, I'm all for aggressive enforcement, if it doesn't obey they rules, kill it. Don't mess about with freezing it, that never does the users any favour.

If they find their app dies a lot, they'll complain to the dev, if the dev doesn't fix his stuff, the app gets a shitty rating and nobody will bother installing it again, problem sorted.

We don't try to be friendly in the face of memory protection violations either.

Well, except timer noise and device irq noise.

Maybe hardware vendors will surprise me, but I don't think the power difference between suspend and idle will get close enough in a reasonable amount of time on server hardware.

Even then, I doubt you'll see standard distros that get minutes of un-interrupted deep-idle.

How long until you realistically expect to leave your laptop overnight off of AC without suspending it (and not have it be on fumes in the morning)?

I think the cgroup-freezer is closer to the lines I'm thinking of, but with the potential to do "importance" inheritance so interactions between tasks in different groups (be they cgroups or sched classes) can work normally.

thanks
-john

And yet, rewriting all of userspace is exactly what Android did, they had the perfect opportunity of doing it right, but they came up with the worst solution ever.

As for my laptop, I really would like the firefox guys to add a 'feature' that stops JS on non-visible tabs and preferably also drop all data after a little while.

firefox is the #1 wakeup thing on my machine and its mostly invisible (minimized) and when its not, I can only stare at a single tab anyway.

Oh yeah, same here.

I even went so far as to do

kill -STOP $(pidof firefox-bin)

at times simply because the thing was getting out of control. And to be honest, the moment a thinner browser comes out with authors who actually do care about not bloating the crap out of it, I'm jumping on it.

Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Hi John,
Hi Peter,

That's not Intel, but all of the PC vendors who don't care about anything but Windows compatibility.

Well, I only use suspend when I would disconnect from the network anyway.

I think this is a variant of a more general issue (see below).

The problem is not outright incompetence, but the fact that this would require creating a layer of user space between the apps and the kernel that would have to be shared by all the existing variants of user space, or else applications working with one variant of the user space "middleware" won't be compatible with another one.

I guess you'd use the same argument against cgroups, right?

This alone doesn't require any new kernel interfaces IMO, although it requires some additional conditions to be met, which is not the case in practice (see below).

_If_ there is a power management daemon, it doesn't have to do things like this. It may work just as described in Section 5 of this paper:

http://lwn.net/images/pdf/suspend_blockers.pdf

The problem is that no one has ever tried to implement the PM daemon.

I think there are two questions to ask here:

(1) If "opportunistic" suspend is not used (i.e. the only way to make the system suspend is to write "mem" to /sys/power/state), can we handle the race issues between wakeup events and event consumption by userland?

(2) If the answer to (1) is "yes" and "opportunistic" suspend _is_ going to be used, is the mechanism addressing (1) sufficient for it to work correctly?

While (2) is only relevant if one wants to use "opportunistic" suspend (like Android), (1) is relevant in general.

Now, I believe that the answer to (1) is "yes" under the following conditions:
* There is only one user space process attempting to use /sys/power/state (ie. the power manager daemon).
* This process cooperates with all user space processes consuming wakeup events (as described in the article mentioned above).

Unfortunately, as I said, I'm not aware of any power management daemon implementations like this used in real life. [Well, I could create a proof-of-concept one, but quite evidently people want me to spend time on other things.] This means that _in_ _practice_ the answer to (1) is "no", which I think John is trying to address (perhaps among other things).

The practice seems to be that (a) there are multiple processes that can try to write to /sys/power/state (and we have hibernation that can use a different interface for that matter) and (b) those processes don't cooperate with each other in any way close to sanity.

Moreover, user space consumers of wakeup events (or more precisely, consumers of input data associated with wakeup events) have no way to talk to the processes operating /sys/power/state, so they can't indicate whether or not it really is a good idea to suspend at the moment.

Arguably, those issues can be addressed by modifying user space, but there are a few things to take into consideration here:
* Is it realistic to expect that they're ever going to be addressed this way?
* If so, is the cost of addressing them at the user space level lower or higher than the cost of a solution involving a kernel interface?
* If it is higher, then perhaps we can do that at the kernel level after all?

Rafael

Yes, I have heard about people wanting O_PONIES ...

If someone came up with a firmware update mechanism which is not coping with unexpected interruption of any kind, then wakelocks are not making any difference.

Please collect the resulting bricks and shove them back to those who thought that remote firmware updates do not have to be engineered and the resulting fallout can be blamed on the kernel.

We have proper mechanisms in place to handle such stuff, but they need proper overall design and definitely a bit more brain usage than just yelling "wakelock".

Thanks,

tglx

O_PONIES_WITH_HEADMOUNTED_WOODCUTTING_LASERS?

And it would be great if some of that brain usage was spent to review and critique what I'm actually proposing, rather then just yelling "wakelock". :P

I apologize for being probably too verbose in my mails, but I did originally admit that the firmware update issue is a simpler problem and doesn't necessarily need the same solution as the races around my nightly backups. But I do think that some thought should be put into the different use cases that seem to desire similar things, so that an appropriate design can be created, instead of a collection of short-term hacks.

More brain usage, and proper design. At least with that, I think we agree. :)

thanks
-john

Working on it :)

Yes, we want use cases, which can actually justify something like the proposed.

Firmware update is _not_ one of them because it needs a proper design to be completely failsafe and just preventing the box to suspend is not helping that goal at all. You have to deal with broken network connections, resets, power outage and more to make it failsafe. And dealing with all of that covers the unintended suspend already. It simply does not matter whether it happens or not.

And that's why I'm ranting about such arguments, as they will just guide people into the delusion of solving hard problems like safe firmware updates with the wrong mechanisms.

The whole wakelock discussion has been full of delusions from the very beginning and we really need to eliminate the lunatic arguments so we can look at the real remaining ones (if any), which might justify them.

Right, and proper design does not exclude user space. It very much starts there.

Thanks,

tglx