PM: Implement autosleep and "wake locks"

From: Rafael J. Wysocki <[email protected]>

Initialize wakeup source locks in wakeup_source_add() instead of wakeup_source_create(), because otherwise the locks of the wakeup sources that haven't been allocated with wakeup_source_create() aren't initialized and handled properly.

Signed-off-by Rafael J. Wysocki <[email protected]>

Quoted 7 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Initialize wakeup source locks in wakeup_source_add() instead of wakeup_source_create(), because otherwise the locks of the wakeup sources that haven't been allocated with wakeup_source_create() aren't initialized and handled properly.

Quoted 1 line

Signed-off-by Rafael J. Wysocki <[email protected]>

Ah, I've shot myself in the foot before, forgetting to init the wakeup source, so this should be good. Although, would a WARN_ON be better then just initializing the lock in add? That way bad behavior is more likely to be corrected, rather then just ignored.

From: Rafael J. Wysocki <[email protected]>

The current wakeup source deactivation code doesn't do anything when the counter of wakeup events in progress goes down to zero, which requires pm_get_wakeup_count() to poll that counter periodically. Although this reduces the average time it takes to deactivate a wakeup source, it also may lead to a substantial amount of unnecessary polling if there are extended periods of wakeup activity. Thus it seems reasonable to use a wait queue for signaling the "no wakeup events in progress" condition and remove the polling.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/wakeup.c |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Index: linux/drivers/base/power/wakeup.c
===================================================================

drivers/base/power/wakeup.c +4 -3

@@ -17,8 +17,6 @@
 
 #include "power.h"
 
-#define TIMEOUT		100
-
 /*
  * If set, the suspend/hibernate code will abort transitions to a sleep state
  * if wakeup events are registered during or immediately before the transition.

@@ -52,6 +50,8 @@ static void pm_wakeup_timer_fn(unsigned
 
 static LIST_HEAD(wakeup_sources);
 
+static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
+
 /**
  * wakeup_source_create - Create a struct wakeup_source object.
  * @name: Name of the new wakeup source.

@@ -84,7 +84,7 @@ void wakeup_source_destroy(struct wakeup
 	while (ws->active) {
 		spin_unlock_irq(&ws->lock);
 
-		schedule_timeout_interruptible(msecs_to_jiffies(TIMEOUT));
+		schedule_timeout_interruptible(msecs_to_jiffies(100));
 
 		spin_lock_irq(&ws->lock);
 	}

@@ -411,6 +411,7 @@ EXPORT_SYMBOL_GPL(pm_stay_awake);
  */
 static void wakeup_source_deactivate(struct wakeup_source *ws)
 {
+	unsigned int cnt, inpr;
 	ktime_t duration;
 	ktime_t now;
 

drivers/base/power/wakeup.c +6 -1

 /**

@@ -624,14 +629,19 @@ bool pm_wakeup_pending(void)
 bool pm_get_wakeup_count(unsigned int *count)
 {
 	unsigned int cnt, inpr;
+	DEFINE_WAIT(wait);
 
 	for (;;) {
+		prepare_to_wait(&wakeup_count_wait_queue, &wait,
+				TASK_INTERRUPTIBLE);
 		split_counters(&cnt, &inpr);
 		if (inpr == 0 || signal_pending(current))
 			break;
 		pm_wakeup_update_hit_counts();
-		schedule_timeout_interruptible(msecs_to_jiffies(TIMEOUT));
+
+		schedule();
 	}
+	finish_wait(&wakeup_count_wait_queue, &wait);
 
 	split_counters(&cnt, &inpr);
 	*count = cnt;

Quoted 56 lines · On Tue, 7 Feb 2012 02:04:19 +0100 "Rafael J. Wysocki" <[email protected]> wrote:

From: Rafael J. Wysocki <[email protected]>

The current wakeup source deactivation code doesn't do anything when the counter of wakeup events in progress goes down to zero, which requires pm_get_wakeup_count() to poll that counter periodically. Although this reduces the average time it takes to deactivate a wakeup source, it also may lead to a substantial amount of unnecessary polling if there are extended periods of wakeup activity. Thus it seems reasonable to use a wait queue for signaling the "no wakeup events in progress" condition and remove the polling.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/wakeup.c |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Index: linux/drivers/base/power/wakeup.c
===================================================================

drivers/base/power/wakeup.c +4 -3

@@ -17,8 +17,6 @@
 
 #include "power.h"
 
-#define TIMEOUT		100
-
 /*
  * If set, the suspend/hibernate code will abort transitions to a sleep state
  * if wakeup events are registered during or immediately before the transition.

@@ -52,6 +50,8 @@ static void pm_wakeup_timer_fn(unsigned
 
 static LIST_HEAD(wakeup_sources);
 
+static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
+
 /**
  * wakeup_source_create - Create a struct wakeup_source object.
  * @name: Name of the new wakeup source.

@@ -84,7 +84,7 @@ void wakeup_source_destroy(struct wakeup
 	while (ws->active) {
 		spin_unlock_irq(&ws->lock);
 
-		schedule_timeout_interruptible(msecs_to_jiffies(TIMEOUT));
+		schedule_timeout_interruptible(msecs_to_jiffies(100));
 
 		spin_lock_irq(&ws->lock);
 	}

@@ -411,6 +411,7 @@ EXPORT_SYMBOL_GPL(pm_stay_awake);
  */
 static void wakeup_source_deactivate(struct wakeup_source *ws)
 {
+	unsigned int cnt, inpr;
 	ktime_t duration;
 	ktime_t now;
 

drivers/base/power/wakeup.c static void wakeup_source_deactivate(str · line 452

Quoted 8 lines

drivers/base/power/wakeup.c +4 -0

@@ -444,6 +445,10 @@ static void wakeup_source_deactivate(str
 	 * couter of wakeup events in progress simultaneously.
 	 */
 	atomic_add(MAX_IN_PROGRESS, &combined_event_count);
+
+	split_counters(&cnt, &inpr);
+	if (!inpr)
+		wake_up_all(&wakeup_count_wait_queue);
 }

Would it be worth making this:

     if (!inpr && waitqueue_active(&wakeup_count_wait_queue))
		wake_up_all(&wakeup_count_wait_queue);

??
It would often save a spinlock.

Also was there a reason you used wake_up_all(). That is only really needed were EXCLUSIVE waits are happening, and there aren't any of those.

From: Rafael J. Wysocki <[email protected]>

The current wakeup source deactivation code doesn't do anything when the counter of wakeup events in progress goes down to zero, which requires pm_get_wakeup_count() to poll that counter periodically. Although this reduces the average time it takes to deactivate a wakeup source, it also may lead to a substantial amount of unnecessary polling if there are extended periods of wakeup activity. Thus it seems reasonable to use a wait queue for signaling the "no wakeup events in progress" condition and remove the polling.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/wakeup.c |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Index: linux/drivers/base/power/wakeup.c
===================================================================

drivers/base/power/wakeup.c +2 -2

@@ -17,8 +17,6 @@
 
 #include "power.h"
 
-#define TIMEOUT		100
-
 /*
  * If set, the suspend/hibernate code will abort transitions to a sleep state
  * if wakeup events are registered during or immediately before the transition.

@@ -52,6 +50,8 @@ static void pm_wakeup_timer_fn(unsigned
 
 static LIST_HEAD(wakeup_sources);
 
+static DECLARE_WAIT_QUEUE_HEAD(wakeup_count_wait_queue);
+
 /**
  * wakeup_source_create - Create a struct wakeup_source object.
  * @name: Name of the new wakeup source.

drivers/base/power/wakeup.c +11 -1

 		spin_lock_irq(&ws->lock);
 	}

@@ -411,6 +411,7 @@ EXPORT_SYMBOL_GPL(pm_stay_awake);
  */
 static void wakeup_source_deactivate(struct wakeup_source *ws)
 {
+	unsigned int cnt, inpr;
 	ktime_t duration;
 	ktime_t now;
 

@@ -444,6 +445,10 @@ static void wakeup_source_deactivate(str
 	 * couter of wakeup events in progress simultaneously.
 	 */
 	atomic_add(MAX_IN_PROGRESS, &combined_event_count);
+
+	split_counters(&cnt, &inpr);
+	if (!inpr)
+		wake_up_all(&wakeup_count_wait_queue);
 }
 
 /**

@@ -624,14 +629,19 @@ bool pm_wakeup_pending(void)
 bool pm_get_wakeup_count(unsigned int *count)
 {
 	unsigned int cnt, inpr;
+	DEFINE_WAIT(wait);
 
 	for (;;) {
+		prepare_to_wait(&wakeup_count_wait_queue, &wait,
+				TASK_INTERRUPTIBLE);
 		split_counters(&cnt, &inpr);
 		if (inpr == 0 || signal_pending(current))
 			break;
 		pm_wakeup_update_hit_counts();
-		schedule_timeout_interruptible(msecs_to_jiffies(TIMEOUT));
+
+		schedule();
 	}
+	finish_wait(&wakeup_count_wait_queue, &wait);
 
 	split_counters(&cnt, &inpr);
 	*count = cnt;

From: Rafael J. Wysocki <[email protected]>

Wakeup statistics used by Android are slightly different from what we have at the moment, so modify them to follow Android more closely.

Quoted 4 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Wakeup statistics used by Android are slightly different from what we have at the moment, so modify them to follow Android more closely.

...

drivers/base/power/wakeup.c static void wakeup_source_deactivate(str · line 448

Quoted 6 lines

drivers/base/power/wakeup.c +2 -0

@@ -438,6 +444,11 @@ static void wakeup_source_deactivate(str
       if (ktime_to_ns(duration) > ktime_to_ns(ws->max_time))
               ws->max_time = duration;
 
+       ws->last_time = now;
+       if (ws->has_timeout && time_after(jiffies, ws->timer_expires))

time_after_eq may work better (or increment the count from the timer).

I applied this patch and the expire counts I see for wakeup-sources that always time-out do not match the active count.

Quoted 10 lines · Rafael J. Wysocki

Quoted 4 lines

From: Rafael J. Wysocki <[email protected]>

Wakeup statistics used by Android are slightly different from what we have at the moment, so modify them to follow Android more closely.

...

Quoted 4 lines

drivers/base/power/wakeup.c +0 -0

@@ -438,6 +444,11 @@ static void wakeup_source_deactivate(str
        if (ktime_to_ns(duration) > ktime_to_ns(ws->max_time))
                ws->max_time = duration;
 

Quoted 4 lines

drivers/base/power/wakeup.c static void wakeup_source_deactivate(str · line 448

Quoted 2 lines

drivers/base/power/wakeup.c +2 -0

+       ws->last_time = now;
+       if (ws->has_timeout && time_after(jiffies, ws->timer_expires))

time_after_eq may work better (or increment the count from the timer).

I think incrementing the count from the timer is a better approach.

Quoted 2 lines

I applied this patch and the expire counts I see for wakeup-sources that always time-out do not match the active count.

I see.  The reason may also be that __pm_wakeup_event() increments ws->event_count even if the wakeup source is already active.

From: Rafael J. Wysocki <[email protected]>

Introduce a mechanism by which the kernel can trigger global transitions to a sleep state chosen by user space if there are no active wakeup sources.

It consists of a new sysfs attribute, /sys/power/autosleep, that can be written one of the strings returned by reads from /sys/power/state, a freezable ordered workqueue and a work item carrying out the "suspend" operations. If a string representing the system's sleep state is written to /sys/power/autosleep, the work item triggering transitions to that state is queued up and it requeues it self after every execution until user space writes "off" to /sys/power/autosleep. That work item enables the detection of wakeup events using the functions already defined in drivers/base/power/wakeup.c (with one small modification) and calls either pm_suspend(), or hibernate() to put the system into a sleep state. If a wakeup event is reported while the transition is in progress, it will abort the transition and the "system suspend" work item will be queued up again.

Hi all,

This series tests the theory that the easiest way to sell a once rejected feature is to advertise it under a different name.

Well, there actually are two different features, although they are closely related to each other. First, patch [6/8] introduces a feature that allows the kernel to trigger system suspend (or more generally a transition into a sleep state) whenever there are no active wakeup sources (no, they aren't called wakelocks). It is called "autosleep" here, but it was called a few different names in the past ("opportunistic suspend" was probably the most popular one). Second, patch [8/8] introduces "wake locks" that are, essentially, wakeup sources which may be created and manipulated by user space. Using them user space may control the autosleep feature introduced earlier.

This also is a kind of a proof of concept for the people who wanted me to show a kernel-based implementation of automatic suspend, so there you go. Please note, however, that it is done so that the user space "wake locks" interface is compatible with Android in support of its user space. I don't really like this interface, but since the Android's user space seems to rely on it, I'm fine with using it as is. YMMV.

Let me say a few words about every patch in the series individually.

[1/8] - This really is a bug fix, so it's v3.4 material. Nobody has stepped
on this bug so far, but it should be fixed anyway.

[2/8] - This is a freezer cleanup, worth doing anyway IMO, so v3.4 material too.

[3/8] - This is something we can do no problem, although completely optional
without the autosleep feature. Rather necessary with it, though.

[4/8] - This kind of reintroduces my original idea of using a wait queue for
waiting until there are no wakeup events in progress. Alan convinced me that it would be better to poll the counter to prevent wakeup_source_deactivate() from having to call wake_up_all() occasionally (that may be costly in fast paths), but then quite some people told me that the wait queue migh be better. I think that the polling will make much less sense with autosleep and user space "wake locks". Anyway, [4/8] is something we can do without those things too.

The patches above were given Sign-off-by tags, because I think they make some sense regardless of the features introcuded by the remaining patches that in turn are total RFC.

[5/8] - This changes wakeup source statistics so that they are more similar to
the statistics collected for wakelocks on Android. The file those statistics may be read from is still located in debugfs, though (I don't think it belongs to proc and its name is different from the analogous Android's file name anyway). It could be done without autosleep, but then it would be a bit pointless. BTW, this changes interfaces that _in_ _theory_ may be used by someone, but I'm not aware of anyone using them. If you are one, I'll be pleased to learn about that, so please tell me who you are. :-)

[6/8] - Autosleep implementation. I think the changelog explains the idea
quite well and the code is really nothing special. It doesn't really add anything new to the kernel in terms of infrastructure etc., it just uses the existing stuff to implement an alternative method of triggering system sleep transitions. Note, though, that the interface here is different from the Android's one, because Android actually modifies /sys/power/state to trigger something called "early suspend" (that is never going to be implemented in the "stock" kernel as long as I have any influence on it) and we simply can't do that in the mainline.

[7/8] - This adds a wakeup source statistics that only makes sense with
autosleep and (I believe) is analogous to the Android's prevent_suspend_time statistics. Nothing really special, but I didn't want wakeup_source_activate/deactivate() to take a common lock to avoid congestion.

[8/8] - This adds a user space interface to create, activate and deactivate
wakeup sources. Since the files it consists of are called wake_lock and wake_unlock, to follow Android, the objects the wakeup sources are wrapped into are called "wakelocks" (for added confusion). Since the interface doesn't provide any means to destroy those "wakelocks", I added a garbage collection mechanism to get rid of the unused ones, if any. I also tought it might be a good idea to put a limit on the number of those things that user space can operate simultaneously, so I did that too.

All in all, it's not as much code as I thought it would be and it seems to be relatively simple (which rises the question why the Android people didn't even _try_ to do something like this instead of slapping the "real" wakelocks onto the kernel FWIW). IMHO it doesn't add anything really new to the kernel, except for the user space interfaces that should be maintainable. At least I think I should be able to maintain them. :-)

All of the above has been tested very briefly on my test-bed Mackerel board and it quite obviously requires more thorough testing, but first I need to know if it makes sense to spend any more time on it.

IOW, I need to know your opinions!

All in all, it's not as much code as I thought it would be and it seems to be relatively simple (which rises the question why the Android people didn't even _try_ to do something like this instead of slapping the "real" wakelocks onto the kernel FWIW). IMHO it doesn't add anything really new to the kernel, except for the user space interfaces that should be maintainable. At least I think I should be able to maintain them. :-)

All of the above has been tested very briefly on my test-bed Mackerel board and it quite obviously requires more thorough testing, but first I need to know if it makes sense to spend any more time on it.

IOW, I need to know your opinions!

Quoted 7 lines · On Tue, 7 Feb 2012 02:00:55 +0100 "Rafael J. Wysocki" <[email protected]> wrote:

All in all, it's not as much code as I thought it would be and it seems to be relatively simple (which rises the question why the Android people didn't even _try_ to do something like this instead of slapping the "real" wakelocks onto the kernel FWIW). IMHO it doesn't add anything really new to the kernel, except for the user space interfaces that should be maintainable. At least I think I should be able to maintain them. :-)

Quoted 5 lines

All of the above has been tested very briefly on my test-bed Mackerel board and it quite obviously requires more thorough testing, but first I need to know if it makes sense to spend any more time on it.

IOW, I need to know your opinions!

I've got opinions!!!

I'll try to avoid the obvious bike-shedding about interface design...

The key point I want to make is that doing this in the kernel has one very import difference to doing it in userspace (which, as you know, I prefer) which may not be obvious to everyone at first sight. So I will try to make it apparent.

In the user-space solution that we have previously discussed, it is only necessary for the kernel to hold a wakeup_source active until the event is *visible* to user-space. So a low level driver can queue e.g. an input event and then deactivate their wakeup_source. The event can remain in the input queue without any wakeup_source being active and there is no risk of going to sleep inappropriately.
This is because - in the user-space approach - user-space must effectively poll every source of interesting wakeup events between the last wakeup_source being deactivate and the next attempt to suspend. This poll will notice the event sitting in a queue so that a well-written user-space will not go to sleep but will read the event.
(Note that this 'poll-of-every-device' need not be expensive. It can be a single 'poll' or 'select' or even 'read' on a pollfd).

In the kernel based approach that you have presented this is not the case. As the kernel will initiate suspend the moment the last wakeup_source is released (with no polling of other queues), there must be an unbroken chain of wakeup_sources from the initial interrupt all the way up to the user. In particular, any subsystem (such as 'input') must hold a wakeup_source active as long as any designated 'wakeup event' is in any of its queues. This means that the subsystem must be able to differentiate wakeup events from non-wakeup events.
This might be easy (maybe "all events are wakeup events" or "all events on this queue are wakeup events") but it is not obvious to me that that is the case.

To summarise: for this solution to be effective it also requires that
1/ every subsystem that carries wakeup events must know about wakeup_sources
and must activate/deactivate them as events are queued/dequeued.
2/ these subsystems must be able to differentiate between wakeup events and
non-wakeup events, and this must be a configurable decision.

Currently, understanding wakeup events is restricted to:
- drivers that are capable of configuring wakeup
- user-space which cares about wakeup
The proposed solution adds:
- intermediate subsystems which might queue wakeup events

I think that is a significant addition to make and not one to be made lightly. It might end up adding more code than you thought it would be :-)

Thanks for the opportunity to comment,

Hi,

Quoted 14 lines · NeilBrown

Quoted 11 lines · On Tue, 7 Feb 2012 02:00:55 +0100 "Rafael J. Wysocki" <[email protected]> wrote:

All in all, it's not as much code as I thought it would be and it seems to be relatively simple (which rises the question why the Android people didn't even _try_ to do something like this instead of slapping the "real" wakelocks onto the kernel FWIW). IMHO it doesn't add anything really new to the kernel, except for the user space interfaces that should be maintainable. At least I think I should be able to maintain them. :-)

All of the above has been tested very briefly on my test-bed Mackerel board and it quite obviously requires more thorough testing, but first I need to know if it makes sense to spend any more time on it.

Quoted 3 lines

Quoted 1 line

IOW, I need to know your opinions!

I've got opinions!!!

Good! :-)

It seems that no one else has.

Quoted 7 lines

I'll try to avoid the obvious bike-shedding about interface design...

The key point I want to make is that doing this in the kernel has one very import difference to doing it in userspace (which, as you know, I prefer) which may not be obvious to everyone at first sight. So I will try to make it apparent.

Quoted 13 lines

In the user-space solution that we have previously discussed, it is only necessary for the kernel to hold a wakeup_source active until the event is *visible* to user-space. So a low level driver can queue e.g. an input event and then deactivate their wakeup_source. The event can remain in the input queue without any wakeup_source being active and there is no risk of going to sleep inappropriately.
This is because - in the user-space approach - user-space must effectively poll every source of interesting wakeup events between the last wakeup_source being deactivate and the next attempt to suspend. This poll will notice the event sitting in a queue so that a well-written user-space will not go to sleep but will read the event.
(Note that this 'poll-of-every-device' need not be expensive. It can be a single 'poll' or 'select' or even 'read' on a pollfd).

So I see one little problem with that, which is that you'd need to teach user space developers what to do an how to do that correctly.

Also, when you say "user space", it isn't exactly clear whether you mean a power manager (that would carry out the attmepts to suspend) or applications (that would need to communicate with the power manager to let it know what they are doing). This is important, because in general, before deactivating a wakeup source the kernel subsystem should know that the associated event has become visible not only to the "polling" application, but also (perhaps indirectly) to the power manager, so that it doesn't trigger suspend too early.

Quoted 18 lines

In the kernel based approach that you have presented this is not the case. As the kernel will initiate suspend the moment the last wakeup_source is released (with no polling of other queues), there must be an unbroken chain of wakeup_sources from the initial interrupt all the way up to the user. In particular, any subsystem (such as 'input') must hold a wakeup_source active as long as any designated 'wakeup event' is in any of its queues. This means that the subsystem must be able to differentiate wakeup events from non-wakeup events.
This might be easy (maybe "all events are wakeup events" or "all events on this queue are wakeup events") but it is not obvious to me that that is the case.

To summarise: for this solution to be effective it also requires that
1/ every subsystem that carries wakeup events must know about wakeup_sources
and must activate/deactivate them as events are queued/dequeued.
2/ these subsystems must be able to differentiate between wakeup events and
non-wakeup events, and this must be a configurable decision.

Quoted 8 lines

Currently, understanding wakeup events is restricted to:
- drivers that are capable of configuring wakeup
- user-space which cares about wakeup
The proposed solution adds:
- intermediate subsystems which might queue wakeup events

I think that is a significant addition to make and not one to be made lightly. It might end up adding more code than you thought it would be :-)

I'm aware of that and I expect people to come up with patches adding the handling of wakeup events to a number of subsystems (this is kind of needed regardless of autosleep if we want to be sure that user space has actually consumed events we want it to take from us before suspending). However, I'm not expecting that to be a lot of code (I think we both can only speculate about that at this point) and those subsystems have maintainers and the decision whether or not to take that code is theirs.

That may be a long process, but at least we can see from Android what's needed and where.

Still, the point here is to give people something to start with so that they can take the Android user space, test it against the mainline and see what doesn't work and why and come up with fixes. Perhaps they will have better ideas than we think right now, but surely nothing more is going to happen without this starting point.

I'd like us and Android to use the same low-level data structures for power management and the same API eventually, at least for drivers. This is not the case at the moment and it's actively hurting us as a project quite a bit. If Android needs to add patches on top of whatever we have to get the desired functionality, I'm fine with that, as long as they don't require drivers to use APIs that are incompatible with the mainline. Insisting that Android should use a user-space-based autosleep implementation wouldn't help at all, because realistically this isn't going to happen.

Quoted 1 line

Thanks for the opportunity to comment,

No need to thank for that, it's Open Source after all ...

Thanks,
Rafael

On Fri, Feb 10, 2012 at 01:44:10AM +0100, Rafael J. Wysocki wrote:

Quoted 8 lines

I'd like us and Android to use the same low-level data structures for power management and the same API eventually, at least for drivers. This is not the case at the moment and it's actively hurting us as a project quite a bit. If Android needs to add patches on top of whatever we have to get the desired functionality, I'm fine with that, as long as they don't require drivers to use APIs that are incompatible with the mainline. Insisting that Android should use a user-space-based autosleep implementation wouldn't help at all, because realistically this isn't going to happen.

why not? I don't think having the PMS explicitly acknowledge a wake event is a big ask at all.

Quoted 1 line · mark gross

On Fri, Feb 10, 2012 at 01:44:10AM +0100, Rafael J. Wysocki wrote:

[...]

Quoted 9 lines

Quoted 8 lines

I'd like us and Android to use the same low-level data structures for power management and the same API eventually, at least for drivers.  This is not the case at the moment and it's actively hurting us as a project quite a bit. If Android needs to add patches on top of whatever we have to get the desired functionality, I'm fine with that, as long as they don't require drivers to use APIs that are incompatible with the mainline.  Insisting that Android should use a user-space-based autosleep implementation wouldn't help at all, because realistically this isn't going to happen.

Quoted 2 lines

why not?  I don't think having the PMS explicitly acknowledge a wake event is a big ask at all.

I'd like to hear what the Android people think about that, but somehow it seems to me they won't like it. :-)

Quoted 12 lines · Rafael J. Wysocki

Quoted 1 line · mark gross

On Fri, Feb 10, 2012 at 01:44:10AM +0100, Rafael J. Wysocki wrote:

[...]

Quoted 9 lines

Quoted 8 lines

I'd like us and Android to use the same low-level data structures for power management and the same API eventually, at least for drivers.  This is not the case at the moment and it's actively hurting us as a project quite a bit. If Android needs to add patches on top of whatever we have to get the desired functionality, I'm fine with that, as long as they don't require drivers to use APIs that are incompatible with the mainline.  Insisting that Android should use a user-space-based autosleep implementation wouldn't help at all, because realistically this isn't going to happen.

Quoted 6 lines

Quoted 2 lines

why not?  I don't think having the PMS explicitly acknowledge a wake event is a big ask at all.

I'd like to hear what the Android people think about that, but somehow it seems to me they won't like it. :-)

Correct.

The android power manager service does not handle wake events and therefore does not know when it is safe to acknowledge a wake event (assuming this acknowledgement re-triggers suspend). Other components handle the event and only notify the power manager if the event should change a state (e.g. turn the screen on). Some wake events, like the alarm used for battery monitoring, don't signal user space at all if the user visible state did not change. Other wake events are processed by lower level user-space services than the system-server where the power manager runs.

Signature · 1 line

Arve Hjønnevåg

Quoted 7 lines · On Tue, 7 Feb 2012 02:00:55 +0100 "Rafael J. Wysocki" <[email protected]> wrote:

All in all, it's not as much code as I thought it would be and it seems to be relatively simple (which rises the question why the Android people didn't even _try_ to do something like this instead of slapping the "real" wakelocks onto the kernel FWIW). IMHO it doesn't add anything really new to the kernel, except for the user space interfaces that should be maintainable. At least I think I should be able to maintain them. :-)

Quoted 5 lines

All of the above has been tested very briefly on my test-bed Mackerel board and it quite obviously requires more thorough testing, but first I need to know if it makes sense to spend any more time on it.

IOW, I need to know your opinions!

I've got opinions!!!

I'll try to avoid the obvious bike-shedding about interface design...

The key point I want to make is that doing this in the kernel has one very import difference to doing it in userspace (which, as you know, I prefer) which may not be obvious to everyone at first sight. So I will try to make it apparent.

In the user-space solution that we have previously discussed, it is only necessary for the kernel to hold a wakeup_source active until the event is *visible* to user-space. So a low level driver can queue e.g. an input event and then deactivate their wakeup_source. The event can remain in the input queue without any wakeup_source being active and there is no risk of going to sleep inappropriately.
This is because - in the user-space approach - user-space must effectively poll every source of interesting wakeup events between the last wakeup_source being deactivate and the next attempt to suspend. This poll will notice the event sitting in a queue so that a well-written user-space will not go to sleep but will read the event.

single 'poll' or 'select' or even 'read' on a pollfd).

In the kernel based approach that you have presented this is not the case. As the kernel will initiate suspend the moment the last wakeup_source is released (with no polling of other queues), there must be an unbroken chain of wakeup_sources from the initial interrupt all the way up to the user. In particular, any subsystem (such as 'input') must hold a wakeup_source active as long as any designated 'wakeup event' is in any of its queues. This means that the subsystem must be able to differentiate wakeup events from non-wakeup events.
This might be easy (maybe "all events are wakeup events" or "all events on this queue are wakeup events") but it is not obvious to me that that is the case.

To summarise: for this solution to be effective it also requires that
1/ every subsystem that carries wakeup events must know about wakeup_sources
and must activate/deactivate them as events are queued/dequeued.
2/ these subsystems must be able to differentiate between wakeup events and
non-wakeup events, and this must be a configurable decision.

Currently, understanding wakeup events is restricted to:
- drivers that are capable of configuring wakeup
- user-space which cares about wakeup
The proposed solution adds:
- intermediate subsystems which might queue wakeup events

I think that is a significant addition to make and not one to be made lightly. It might end up adding more code than you thought it would be :-)

Thanks for the opportunity to comment,
NeilBrown

Hi all,

This series tests the theory that the easiest way to sell a once rejected feature is to advertise it under a different name.

Well, there actually are two different features, although they are closely related to each other. First, patch [6/8] introduces a feature that allows the kernel to trigger system suspend (or more generally a transition into a sleep state) whenever there are no active wakeup sources (no, they aren't called wakelocks). It is called "autosleep" here, but it was called a few different names in the past ("opportunistic suspend" was probably the most popular one). Second, patch [8/8] introduces "wake locks" that are, essentially, wakeup sources which may be created and manipulated by user space. Using them user space may control the autosleep feature introduced earlier.

This also is a kind of a proof of concept for the people who wanted me to show a kernel-based implementation of automatic suspend, so there you go. Please note, however, that it is done so that the user space "wake locks" interface is compatible with Android in support of its user space. I don't really like this interface, but since the Android's user space seems to rely on it, I'm fine with using it as is. YMMV.

Let me say a few words about every patch in the series individually.

[1/8] - This really is a bug fix, so it's v3.4 material. Nobody has stepped
on this bug so far, but it should be fixed anyway.

[2/8] - This is a freezer cleanup, worth doing anyway IMO, so v3.4 material too.

[3/8] - This is something we can do no problem, although completely optional
without the autosleep feature. Rather necessary with it, though.

[4/8] - This kind of reintroduces my original idea of using a wait queue for
waiting until there are no wakeup events in progress. Alan convinced me that it would be better to poll the counter to prevent wakeup_source_deactivate() from having to call wake_up_all() occasionally (that may be costly in fast paths), but then quite some people told me that the wait queue migh be better. I think that the polling will make much less sense with autosleep and user space "wake locks". Anyway, [4/8] is something we can do without those things too.

The patches above were given Sign-off-by tags, because I think they make some sense regardless of the features introcuded by the remaining patches that in turn are total RFC.

[5/8] - This changes wakeup source statistics so that they are more similar to
the statistics collected for wakelocks on Android. The file those statistics may be read from is still located in debugfs, though (I don't think it belongs to proc and its name is different from the analogous Android's file name anyway). It could be done without autosleep, but then it would be a bit pointless. BTW, this changes interfaces that _in_ _theory_ may be used by someone, but I'm not aware of anyone using them. If you are one, I'll be pleased to learn about that, so please tell me who you are. :-)

[6/8] - Autosleep implementation. I think the changelog explains the idea
quite well and the code is really nothing special. It doesn't really add anything new to the kernel in terms of infrastructure etc., it just uses the existing stuff to implement an alternative method of triggering system sleep transitions. Note, though, that the interface here is different from the Android's one, because Android actually modifies /sys/power/state to trigger something called "early suspend" (that is never going to be implemented in the "stock" kernel as long as I have any influence on it) and we simply can't do that in the mainline.

[7/8] - This adds a wakeup source statistics that only makes sense with
autosleep and (I believe) is analogous to the Android's prevent_suspend_time statistics. Nothing really special, but I didn't want wakeup_source_activate/deactivate() to take a common lock to avoid congestion.

[8/8] - This adds a user space interface to create, activate and deactivate
wakeup sources. Since the files it consists of are called wake_lock and wake_unlock, to follow Android, the objects the wakeup sources are wrapped into are called "wakelocks" (for added confusion). Since the interface doesn't provide any means to destroy those "wakelocks", I added a garbage collection mechanism to get rid of the unused ones, if any. I also tought it might be a good idea to put a limit on the number of those things that user space can operate simultaneously, so I did that too.

All in all, it's not as much code as I thought it would be and it seems to be relatively simple (which rises the question why the Android people didn't even _try_ to do something like this instead of slapping the "real" wakelocks onto the kernel FWIW). IMHO it doesn't add anything really new to the kernel, except for the user space interfaces that should be maintainable. At least I think I should be able to maintain them. :-)

All of the above has been tested very briefly on my test-bed Mackerel board and it quite obviously requires more thorough testing, but first I need to know if it makes sense to spend any more time on it.

IOW, I need to know your opinions!

Thanks,
Rafael

All in all, it's not as much code as I thought it would be and it seems to be relatively simple (which rises the question why the Android people didn't even _try_ to do something like this instead of slapping the "real" wakelocks onto the kernel FWIW).  IMHO it doesn't add anything really new to the kernel, except for the user space interfaces that should be maintainable.  At least I think I should be able to maintain them. :-)

On Mon, Feb 6, 2012 at 5:00 PM, Rafael J. Wysocki <[email protected]> wrote:
...

Quoted 7 lines

All in all, it's not as much code as I thought it would be and it seems to be relatively simple (which rises the question why the Android people didn't even _try_ to do something like this instead of slapping the "real" wakelocks onto the kernel FWIW). IMHO it doesn't add anything really new to the kernel, except for the user space interfaces that should be maintainable. At least I think I should be able to maintain them. :-)

Replacing a working solution with an untested one takes time.

That said, I have recently tried replacing all our kernel wake-locks with a thin wrapper around wake-sources. This appears to mostly work,

but the wake-source timeout feature has some bugs or incompatible apis. An init api would also be useful for embedding wake-sources in other data structures without adding another memory allocation. Your patch to move the spinlock init to wakeup_source_add still require the struct to be zero initialized and the name set manually.

I needed to use two wake-sources per wake-lock since calling __pm_stay_awake after __pm_wakeup_event on a wake-source does not cancel the timeout. Unless there is a reason to keep this behavior I would like __pm_stay_awake to cancel any active timeout.

Destroying a wake-source also has some problems. If you call wakeup_source_destroy it will spin forever if the wake-source is active without a timeout. And, if you call __pm_relax then wakeup_source_destroy it could free the wake-source memory while the timer function is still running.

It also looks as if the wake_source can be immediately deactivated if you call __pm_wakeup_event at the same time as the previous timeout expired.

Quoted 7 lines · Rafael J. Wysocki

...
but the wake-source timeout feature has some bugs or incompatible apis. An init api would also be useful for embedding wake-sources in other data structures without adding another memory allocation. Your patch to move the spinlock init to wakeup_source_add still require the struct to be zero initialized and the name set manually.

That should be easy to fix.  What about the appended patch?

2012/2/14 Rafael J. Wysocki <[email protected]>:

Quoted 9 lines · Arve Hjønnevåg

Quoted 7 lines · Rafael J. Wysocki

...
but the wake-source timeout feature has some bugs or incompatible apis. An init api would also be useful for embedding wake-sources in other data structures without adding another memory allocation. Your patch to move the spinlock init to wakeup_source_add still require the struct to be zero initialized and the name set manually.

Quoted 2 lines

That should be easy to fix. What about the appended patch?

That works, but I still have to call more than one function before I can use the wakeup-source (wakeup_source_init and wakeup_source_add) and more than one function before I can free it (__pm_relax, wakeup_source_remove and wakeup_source_drop). Is there any reason to keep these separate?

Also, not copying the name when the caller provides the memory for the wakeup-source would be a closer match to the wakelock api. Most of our wakelocks pass a string constant as the name, and making a copy of that string is not useful. wake_lock_init is also safe to call from atomic context, but I don't know if anyone relies on this.

Quoted 10 lines · Arve Hjønnevåg

2012/2/14 Rafael J. Wysocki <[email protected]>:

Quoted 9 lines · Arve Hjønnevåg

Quoted 7 lines · Rafael J. Wysocki

...
but the wake-source timeout feature has some bugs or incompatible apis. An init api would also be useful for embedding wake-sources in other data structures without adding another memory allocation. Your patch to move the spinlock init to wakeup_source_add still require the struct to be zero initialized and the name set manually.

Quoted 8 lines

Quoted 2 lines

That should be easy to fix. What about the appended patch?

That works, but I still have to call more than one function before I can use the wakeup-source (wakeup_source_init and wakeup_source_add) and more than one function before I can free it (__pm_relax, wakeup_source_remove and wakeup_source_drop). Is there any reason to keep these separate?

Yes, there is. I think that wakeup_source_create/_destroy() should use the same initialization functions internally that will be used for externally allocated wakeup sources (to make sure that all wakeup source objects are initialized in exactly the same way).

Quoted 5 lines

Also, not copying the name when the caller provides the memory for the wakeup-source would be a closer match to the wakelock api. Most of our wakelocks pass a string constant as the name, and making a copy of that string is not useful. wake_lock_init is also safe to call from atomic context, but I don't know if anyone relies on this.

OK, below is another go. It doesn't copy the name if wakeup_source_init() is used (which also does the _add this time). I think, though, that copying the name is generally safer, because someone might use wakeup_source_init() with the name string allocated on the stack or otherwise temporary, which would be a bug with the new version.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/base/power/wakeup.c |   41 ++++++++++++++++++++++++++++++++++-------
 include/linux/pm_wakeup.h   |   20 ++++++++++++++++++++
 2 files changed, 54 insertions(+), 7 deletions(-)

Index: linux/drivers/base/power/wakeup.c
===================================================================

drivers/base/power/wakeup.c +34 -7

@@ -53,6 +53,23 @@ static void pm_wakeup_timer_fn(unsigned
 static LIST_HEAD(wakeup_sources);
 
 /**
+ * wakeup_source_prepare - Prepare a new wakeup source for initialization.
+ * @ws: Wakeup source to prepare.
+ * @name: Pointer to the name of the new wakeup source.
+ *
+ * Callers must ensure that the @name string won't be freed when @ws is still in
+ * use.
+ */
+void wakeup_source_prepare(struct wakeup_source *ws, const char *name)
+{
+	if (ws) {
+		memset(ws, 0, sizeof(*ws));
+		ws->name = name;
+	}
+}
+EXPORT_SYMBOL_GPL(wakeup_source_prepare);
+
+/**
  * wakeup_source_create - Create a struct wakeup_source object.
  * @name: Name of the new wakeup source.
  */

@@ -60,31 +77,41 @@ struct wakeup_source *wakeup_source_crea
 {
 	struct wakeup_source *ws;
 
-	ws = kzalloc(sizeof(*ws), GFP_KERNEL);
+	ws = kmalloc(sizeof(*ws), GFP_KERNEL);
 	if (!ws)
 		return NULL;
 
-	if (name)
-		ws->name = kstrdup(name, GFP_KERNEL);
-
+	wakeup_source_prepare(ws, name ? kstrdup(name, GFP_KERNEL) : NULL);
 	return ws;
 }
 EXPORT_SYMBOL_GPL(wakeup_source_create);
 
 /**
- * wakeup_source_destroy - Destroy a struct wakeup_source object.
- * @ws: Wakeup source to destroy.
+ * wakeup_source_drop - Prepare a struct wakeup_source object for destruction.
+ * @ws: Wakeup source to prepare for destruction.
  *
  * Callers must ensure that __pm_stay_awake() or __pm_wakeup_event() will never
  * be run in parallel with this function for the same wakeup source object.
  */
-void wakeup_source_destroy(struct wakeup_source *ws)
+void wakeup_source_drop(struct wakeup_source *ws)
 {
 	if (!ws)
 		return;
 
 	del_timer_sync(&ws->timer);
 	__pm_relax(ws);
+}
+EXPORT_SYMBOL_GPL(wakeup_source_drop);
+
+/**
+ * wakeup_source_destroy - Destroy a struct wakeup_source object.
+ * @ws: Wakeup source to destroy.
+ *
+ * Use only for wakeup source objects created with wakeup_source_create().
+ */
+void wakeup_source_destroy(struct wakeup_source *ws)
+{
+	wakeup_source_drop(ws);
 	kfree(ws->name);
 	kfree(ws);
 }

Index: linux/include/linux/pm_wakeup.h
===================================================================

include/linux/pm_wakeup.h +20 -1

@@ -73,7 +73,9 @@ static inline bool device_may_wakeup(str
 }
 
 /* drivers/base/power/wakeup.c */
+extern void wakeup_source_prepare(struct wakeup_source *ws, const char *name);
 extern struct wakeup_source *wakeup_source_create(const char *name);
+extern void wakeup_source_drop(struct wakeup_source *ws);
 extern void wakeup_source_destroy(struct wakeup_source *ws);
 extern void wakeup_source_add(struct wakeup_source *ws);
 extern void wakeup_source_remove(struct wakeup_source *ws);

@@ -103,11 +105,16 @@ static inline bool device_can_wakeup(str
 	return dev->power.can_wakeup;
 }
 
+static inline void wakeup_source_prepare(struct wakeup_source *ws,
+					 const char *name) {}
+
 static inline struct wakeup_source *wakeup_source_create(const char *name)
 {
 	return NULL;
 }
 
+static inline void wakeup_source_drop(struct wakeup_source *ws) {}
+
 static inline void wakeup_source_destroy(struct wakeup_source *ws) {}
 
 static inline void wakeup_source_add(struct wakeup_source *ws) {}

@@ -165,4 +172,17 @@ static inline void pm_wakeup_event(struc
 
 #endif /* !CONFIG_PM_SLEEP */
 
+static inline void wakeup_source_init(struct wakeup_source *ws,
+				      const char *name)
+{
+	wakeup_source_prepare(ws, name);
+	wakeup_source_add(ws);
+}
+
+static inline void wakeup_source_trash(struct wakeup_source *ws)
+{
+	wakeup_source_remove(ws);
+	wakeup_source_drop(ws);
+}
+
 #endif /* _LINUX_PM_WAKEUP_H */
--

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

So, is the new version more suitable than the previous one?

Quoted 10 lines

2012/2/14 Rafael J. Wysocki <[email protected]>:

Quoted 9 lines · Arve Hjønnevåg

Quoted 7 lines · Rafael J. Wysocki

...
but the wake-source timeout feature has some bugs or incompatible apis. An init api would also be useful for embedding wake-sources in other data structures without adding another memory allocation. Your patch to move the spinlock init to wakeup_source_add still require the struct to be zero initialized and the name set manually.

Quoted 8 lines

Quoted 2 lines

That should be easy to fix.  What about the appended patch?

That works, but I still have to call more than one function before I can use the wakeup-source (wakeup_source_init and wakeup_source_add) and more than one function before I can free it (__pm_relax, wakeup_source_remove and wakeup_source_drop). Is there any reason to keep these separate?

Yes, there is.  I think that wakeup_source_create/_destroy() should use the same initialization functions internally that will be used for externally allocated wakeup sources (to make sure that all wakeup source objects are initialized in exactly the same way).

Quoted 5 lines

Also, not copying the name when the caller provides the memory for the wakeup-source would be a closer match to the wakelock api. Most of our wakelocks pass a string constant as the name, and making a copy of that string is not useful. wake_lock_init is also safe to call from atomic context, but I don't know if anyone relies on this.

OK, below is another go.  It doesn't copy the name if wakeup_source_init() is used (which also does the _add this time).  I think, though, that copying the name is generally safer, because someone might use wakeup_source_init() with the name string allocated on the stack or otherwise temporary, which would be a bug with the new version.

2012/2/15 Rafael J. Wysocki <[email protected]>:

Quoted 20 lines · Arve Hjønnevåg

Quoted 18 lines

2012/2/14 Rafael J. Wysocki <[email protected]>:

Quoted 11 lines · Arve Hjønnevåg

Quoted 7 lines · Rafael J. Wysocki

...
but the wake-source timeout feature has some bugs or incompatible apis. An init api would also be useful for embedding wake-sources in other data structures without adding another memory allocation. Your patch to move the spinlock init to wakeup_source_add still require the struct to be zero initialized and the name set manually.

That should be easy to fix. What about the appended patch?

That works, but I still have to call more than one function before I can use the wakeup-source (wakeup_source_init and wakeup_source_add) and more than one function before I can free it (__pm_relax, wakeup_source_remove and wakeup_source_drop). Is there any reason to keep these separate?

Quoted 5 lines

Yes, there is. I think that wakeup_source_create/_destroy() should use the same initialization functions internally that will be used for externally allocated wakeup sources (to make sure that all wakeup source objects are initialized in exactly the same way).

I agree with that, but is it useful to export these helper functions?

Quoted 6 lines

Quoted 5 lines

Also, not copying the name when the caller provides the memory for the wakeup-source would be a closer match to the wakelock api. Most of our wakelocks pass a string constant as the name, and making a copy of that string is not useful. wake_lock_init is also safe to call from atomic context, but I don't know if anyone relies on this.

Quoted 6 lines

OK, below is another go. It doesn't copy the name if wakeup_source_init() is used (which also does the _add this time). I think, though, that copying the name is generally safer, because someone might use wakeup_source_init() with the name string allocated on the stack or otherwise temporary, which would be a bug with the new version.

I prefer this version. I have not seen a bug where someone passed a temporary as the wakelock name, I assume since this will show up immediately in the stats file.

Hi all,

This series tests the theory that the easiest way to sell a once rejected feature is to advertise it under a different name.

Well, there actually are two different features, although they are closely related to each other. First, patch [6/8] introduces a feature that allows the kernel to trigger system suspend (or more generally a transition into a sleep state) whenever there are no active wakeup sources (no, they aren't called wakelocks). It is called "autosleep" here, but it was called a few different names in the past ("opportunistic suspend" was probably the most popular one). Second, patch [8/8] introduces "wake locks" that are, essentially, wakeup sources which may be created and manipulated by user space. Using them user space may control the autosleep feature introduced earlier.

This also is a kind of a proof of concept for the people who wanted me to show a kernel-based implementation of automatic suspend, so there you go. Please note, however, that it is done so that the user space "wake locks" interface is compatible with Android in support of its user space. I don't really like this interface, but since the Android's user space seems to rely on it, I'm fine with using it as is. YMMV.

Let me say a few words about every patch in the series individually.

[1/8] - This really is a bug fix, so it's v3.4 material. Nobody has stepped
on this bug so far, but it should be fixed anyway.

[2/8] - This is a freezer cleanup, worth doing anyway IMO, so v3.4 material too.

[3/8] - This is something we can do no problem, although completely optional
without the autosleep feature. Rather necessary with it, though.

[4/8] - This kind of reintroduces my original idea of using a wait queue for
waiting until there are no wakeup events in progress. Alan convinced me that it would be better to poll the counter to prevent wakeup_source_deactivate() from having to call wake_up_all() occasionally (that may be costly in fast paths), but then quite some people told me that the wait queue migh be better. I think that the polling will make much less sense with autosleep and user space "wake locks". Anyway, [4/8] is something we can do without those things too.

The patches above were given Sign-off-by tags, because I think they make some sense regardless of the features introcuded by the remaining patches that in turn are total RFC.

[5/8] - This changes wakeup source statistics so that they are more similar to
the statistics collected for wakelocks on Android. The file those statistics may be read from is still located in debugfs, though (I don't think it belongs to proc and its name is different from the analogous Android's file name anyway). It could be done without autosleep, but then it would be a bit pointless. BTW, this changes interfaces that _in_ _theory_ may be used by someone, but I'm not aware of anyone using them. If you are one, I'll be pleased to learn about that, so please tell me who you are. :-)

[6/8] - Autosleep implementation. I think the changelog explains the idea
quite well and the code is really nothing special. It doesn't really add anything new to the kernel in terms of infrastructure etc., it just uses the existing stuff to implement an alternative method of triggering system sleep transitions. Note, though, that the interface here is different from the Android's one, because Android actually modifies /sys/power/state to trigger something called "early suspend" (that is never going to be implemented in the "stock" kernel as long as I have any influence on it) and we simply can't do that in the mainline.

[7/8] - This adds a wakeup source statistics that only makes sense with
autosleep and (I believe) is analogous to the Android's prevent_suspend_time statistics. Nothing really special, but I didn't want wakeup_source_activate/deactivate() to take a common lock to avoid congestion.

[8/8] - This adds a user space interface to create, activate and deactivate
wakeup sources. Since the files it consists of are called wake_lock and wake_unlock, to follow Android, the objects the wakeup sources are wrapped into are called "wakelocks" (for added confusion). Since the interface doesn't provide any means to destroy those "wakelocks", I added a garbage collection mechanism to get rid of the unused ones, if any. I also tought it might be a good idea to put a limit on the number of those things that user space can operate simultaneously, so I did that too.

All of the above has been tested very briefly on my test-bed Mackerel board and it quite obviously requires more thorough testing, but first I need to know if it makes sense to spend any more time on it.

From: Arve Hjønnevåg <[email protected]>

Add a new ioctl, EVIOCSWAKEUPSRC, to attach a wakeup source object to an evdev client event queue, such that it will be active whenever the queue is not empty. Then, all events in the queue will be regarded as wakeup events in progress and pm_get_wakeup_count() will block (or return false if woken up by a signal) until they are removed from the queue. In consequence, if the checking of wakeup events is enabled (e.g. throught the /sys/power/wakeup_count interface), the system won't be able to go into a sleep state until the queue is empty.

This allows user space processes to handle situations in which they want to do a select() on an evdev descriptor, so they go to sleep until there are some events to read from the device's queue, and then they don't want the system to go into a sleep state until all the events are read (presumably for further processing). Of course, if they don't want the system to go into a sleep state _after_ all the events have been read from the queue, they have to use a separate mechanism that will prevent the system from doing that and it has to be activated before reading the first event (that also may be the last one).

[rjw: Removed unnecessary checks, changed the names of the new ioctls
and the names of the functions that add/remove wakeup source objects to/from evdev clients, modified the changelog.

Signed-off-by Arve Hjønnevåg <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/input/evdev.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/input.h |    3 ++
 2 files changed, 58 insertions(+)

Index: linux/drivers/input/evdev.c
===================================================================

drivers/input/evdev.c +8 -0

@@ -43,6 +43,7 @@ struct evdev_client {
 	unsigned int tail;
 	unsigned int packet_head; /* [future] position of the first element of next packet */
 	spinlock_t buffer_lock; /* protects access to buffer, head and tail */
+	struct wakeup_source *wakeup_source;
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;

@@ -75,10 +76,12 @@ static void evdev_pass_event(struct evde
 		client->buffer[client->tail].value = 0;
 
 		client->packet_head = client->tail;
+		__pm_relax(client->wakeup_source);
 	}
 
 	if (event->type == EV_SYN && event->code == SYN_REPORT) {
 		client->packet_head = client->head;
+		__pm_stay_awake(client->wakeup_source);
 		kill_fasync(&client->fasync, SIGIO, POLL_IN);
 	}
 

@@ -255,6 +258,8 @@ static int evdev_release(struct inode *i
 	mutex_unlock(&evdev->mutex);
 
 	evdev_detach_client(evdev, client);
+	wakeup_source_unregister(client->wakeup_source);
+
 	kfree(client);
 
 	evdev_close_device(evdev);

@@ -373,6 +378,8 @@ static int evdev_fetch_next_event(struct
 	if (have_event) {
 		*event = client->buffer[client->tail++];
 		client->tail &= client->bufsize - 1;
+		if (client->packet_head == client->tail)
+			__pm_relax(client->wakeup_source);
 	}
 
 	spin_unlock_irq(&client->buffer_lock);

@@ -623,6 +630,45 @@ static int evdev_handle_set_keycode_v2(s
 	return input_set_keycode(dev, &ke);
 }
 
+static int evdev_attach_wakeup_source(struct evdev *evdev,

Quoted 11 lines · Rafael J. Wysocki

From: Arve Hjønnevåg <[email protected]>

Add a new ioctl, EVIOCSWAKEUPSRC, to attach a wakeup source object to an evdev client event queue, such that it will be active whenever the queue is not empty.  Then, all events in the queue will be regarded as wakeup events in progress and pm_get_wakeup_count() will block (or return false if woken up by a signal) until they are removed from the queue.  In consequence, if the checking of wakeup events is enabled (e.g. throught the /sys/power/wakeup_count interface), the system won't be able to go into a sleep state until the queue is empty.

Quoted 10 lines

This allows user space processes to handle situations in which they want to do a select() on an evdev descriptor, so they go to sleep until there are some events to read from the device's queue, and then they don't want the system to go into a sleep state until all the events are read (presumably for further processing).  Of course, if they don't want the system to go into a sleep state _after_ all the events have been read from the queue, they have to use a separate mechanism that will prevent the system from doing that and it has to be activated before reading the first event (that also may be the last one).

I haven't seen this idea mentioned before but I must admit I haven't been following this thread too closely so apologies (and don't bother rehashing) if it has:

Could you just add this to epoll so that any fd userspace chooses would be capable of doing this without introducing potentially ecclectic ioctl interfaces?

Quoted 2 lines

+     snprintf(name, sizeof(name), "%s-%d",
+              dev_name(&evdev->dev), task_tgid_vnr(current));

This does not look like it will work well with tasks in different pid namespaces. What should happen, I think, is the wakeup_source should hold a reference to either the struct pid of current or current itself. Then when someone reads the file you should get the pid vnr in the reader's pid namespace. That way instead of a bogus pid vnr 0 would show up if "current" here is not in the reader's pid namepsace.

Quoted 23 lines · Matt Helsley

Quoted 11 lines · Rafael J. Wysocki

From: Arve Hjønnevåg <[email protected]>

Add a new ioctl, EVIOCSWAKEUPSRC, to attach a wakeup source object to an evdev client event queue, such that it will be active whenever the queue is not empty. Then, all events in the queue will be regarded as wakeup events in progress and pm_get_wakeup_count() will block (or return false if woken up by a signal) until they are removed from the queue. In consequence, if the checking of wakeup events is enabled (e.g. throught the /sys/power/wakeup_count interface), the system won't be able to go into a sleep state until the queue is empty.

Quoted 10 lines

This allows user space processes to handle situations in which they want to do a select() on an evdev descriptor, so they go to sleep until there are some events to read from the device's queue, and then they don't want the system to go into a sleep state until all the events are read (presumably for further processing). Of course, if they don't want the system to go into a sleep state _after_ all the events have been read from the queue, they have to use a separate mechanism that will prevent the system from doing that and it has to be activated before reading the first event (that also may be the last one).

Quoted 8 lines

I haven't seen this idea mentioned before but I must admit I haven't been following this thread too closely so apologies (and don't bother rehashing) if it has:

Could you just add this to epoll so that any fd userspace chooses would be capable of doing this without introducing potentially ecclectic ioctl interfaces?

This is an interesting idea, but I'm not sure how well it would work.

I looked at the epoll code and it looks like it is possible to activate the wakeup-source from the wait queue function it uses.

The epoll callback will happen without holding evdev client buffer_lock, so the wakeup-source and buffer state will not always be in sync (this may be OK, but require more thought). This callback is also called if no data was added to the queue we are polling on because another client has grabbed the input device (is this a bug or intended?).

There is no call into the epoll code when input queue is emptied, so we can't deactivate the wakeup-source until epoll_wait is called again. This also should be workable, but result in different stats.

It does not look like the normal poll and select interfaces can be extended the same way (since they remove themselves from the wait-queue before returning to user-space), so user-space has to be changed to use epoll even if select or poll would be a better fit.

I don't know how many other drivers this would work for. The input driver will wake up user-space from the same thread or interrupt handler that queued the event, but other drivers may defer this to another thread which makes an epoll wakeup-source insufficient.

...

Quoted 10 lines

drivers/input/evdev.c static int evdev_handle_set_keycode_v2(s · line 643

Quoted 2 lines

drivers/input/evdev.c +2 -0

+     snprintf(name, sizeof(name), "%s-%d",
+              dev_name(&evdev->dev), task_tgid_vnr(current));

This does not look like it will work well with tasks in different pid namespaces. What should happen, I think, is the wakeup_source should hold a reference to either the struct pid of current or current itself. Then when someone reads the file you should get the pid vnr in the reader's pid namespace. That way instead of a bogus pid vnr 0 would show up if "current" here is not in the reader's pid namepsace.

The pid here is only used for debugging purposes, and used less than the dev_name. I don't think tracking pid namespaces is worth the trouble here, so if this is a real problem we can just drop the pid from the name for now.

Quoted 23 lines · Matt Helsley

Quoted 11 lines · Rafael J. Wysocki

From: Arve Hjønnevåg <[email protected]>

Add a new ioctl, EVIOCSWAKEUPSRC, to attach a wakeup source object to an evdev client event queue, such that it will be active whenever the queue is not empty.  Then, all events in the queue will be regarded as wakeup events in progress and pm_get_wakeup_count() will block (or return false if woken up by a signal) until they are removed from the queue.  In consequence, if the checking of wakeup events is enabled (e.g. throught the /sys/power/wakeup_count interface), the system won't be able to go into a sleep state until the queue is empty.

Quoted 10 lines

This allows user space processes to handle situations in which they want to do a select() on an evdev descriptor, so they go to sleep until there are some events to read from the device's queue, and then they don't want the system to go into a sleep state until all the events are read (presumably for further processing).  Of course, if they don't want the system to go into a sleep state _after_ all the events have been read from the queue, they have to use a separate mechanism that will prevent the system from doing that and it has to be activated before reading the first event (that also may be the last one).

Quoted 8 lines

I haven't seen this idea mentioned before but I must admit I haven't been following this thread too closely so apologies (and don't bother rehashing) if it has:

Could you just add this to epoll so that any fd userspace chooses would be capable of doing this without introducing potentially ecclectic ioctl interfaces?

This is an interesting idea, but I'm not sure how well it would work.

I looked at the epoll code and it looks like it is possible to activate the wakeup-source from the wait queue function it uses. The epoll callback will happen without holding evdev client buffer_lock, so the wakeup-source and buffer state will not always be in sync (this may be OK, but require more thought). This callback is also called if no data was added to the queue we are polling on because another client has grabbed the input device (is this a bug or intended?).

There is no call into the epoll code when input queue is emptied, so we can't deactivate the wakeup-source until epoll_wait is called again. This also should be workable, but result in different stats.

It does not look like the normal poll and select interfaces can be extended the same way (since they remove themselves from the wait-queue before returning to user-space), so user-space has to be

changed to use epoll even if select or poll would be a better fit.

I don't know how many other drivers this would work for. The input driver will wake up user-space from the same thread or interrupt handler that queued the event, but other drivers may defer this to another thread which makes an epoll wakeup-source insufficient.

Quoted 7 lines

Quoted 7 lines

Of course, if
they don't want the system to go into a sleep state _after_ all the events have been read from the queue, they have to use a separate mechanism that will prevent the system from doing that and it has to be activated before reading the first event (that also may be the
last one).

...

Quoted 10 lines

Quoted 2 lines

+     snprintf(name, sizeof(name), "%s-%d",
+              dev_name(&evdev->dev), task_tgid_vnr(current));

This does not look like it will work well with tasks in different pid namespaces. What should happen, I think, is the wakeup_source should hold a reference to either the struct pid of current or current itself. Then when someone reads the file you should get the pid vnr in the reader's pid namespace. That way instead of a bogus pid vnr 0 would show up if "current" here is not in the reader's pid namepsace.

The pid here is only used for debugging purposes, and used less than the dev_name. I don't think tracking pid namespaces is worth the trouble here, so if this is a real problem we can just drop the pid from the name for now.

Quoted 11 lines · Rafael J. Wysocki

From: Arve Hjønnevåg <[email protected]>

Add a new ioctl, EVIOCSWAKEUPSRC, to attach a wakeup source object to an evdev client event queue, such that it will be active whenever the queue is not empty. Then, all events in the queue will be regarded as wakeup events in progress and pm_get_wakeup_count() will block (or return false if woken up by a signal) until they are removed from the queue. In consequence, if the checking of wakeup events is enabled (e.g. throught the /sys/power/wakeup_count interface), the system won't be able to go into a sleep state until the queue is empty.

Quoted 10 lines

This allows user space processes to handle situations in which they want to do a select() on an evdev descriptor, so they go to sleep until there are some events to read from the device's queue, and then they don't want the system to go into a sleep state until all the events are read (presumably for further processing). Of course, if they don't want the system to go into a sleep state _after_ all the events have been read from the queue, they have to use a separate mechanism that will prevent the system from doing that and it has to be activated before reading the first event (that also may be the last one).

I haven't seen this idea mentioned before but I must admit I haven't been following this thread too closely so apologies (and don't bother rehashing) if it has:

Could you just add this to epoll so that any fd userspace chooses would be capable of doing this without introducing potentially ecclectic ioctl interfaces?

struct epoll_event ev;

epfd = epoll_create1(EPOLL_STAY_AWAKE_SET);
ev.data.ptr = foo;
epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &ev);

Which could be useful because you can put one epollfd in another's epoll set. Or maybe as an EPOLLKEEPAWAKE flag in the event struct sort of like EPOLLET:

epfd = epoll_create1(0);
ev.events = EPOLLIN|EPOLLKEEPAWAKE;
epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &ev);

Quoted 41 lines · Matt Helsley

Quoted 11 lines · Rafael J. Wysocki

From: Arve Hjønnevåg <[email protected]>

Add a new ioctl, EVIOCSWAKEUPSRC, to attach a wakeup source object to an evdev client event queue, such that it will be active whenever the queue is not empty. Then, all events in the queue will be regarded as wakeup events in progress and pm_get_wakeup_count() will block (or return false if woken up by a signal) until they are removed from the queue. In consequence, if the checking of wakeup events is enabled (e.g. throught the /sys/power/wakeup_count interface), the system won't be able to go into a sleep state until the queue is empty.

Quoted 10 lines

This allows user space processes to handle situations in which they want to do a select() on an evdev descriptor, so they go to sleep until there are some events to read from the device's queue, and then they don't want the system to go into a sleep state until all the events are read (presumably for further processing). Of course, if they don't want the system to go into a sleep state _after_ all the events have been read from the queue, they have to use a separate mechanism that will prevent the system from doing that and it has to be activated before reading the first event (that also may be the last one).

I haven't seen this idea mentioned before but I must admit I haven't been following this thread too closely so apologies (and don't bother rehashing) if it has:

Could you just add this to epoll so that any fd userspace chooses would be capable of doing this without introducing potentially ecclectic ioctl interfaces?

struct epoll_event ev;

epfd = epoll_create1(EPOLL_STAY_AWAKE_SET);
ev.data.ptr = foo;
epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &ev);

Which could be useful because you can put one epollfd in another's epoll set. Or maybe as an EPOLLKEEPAWAKE flag in the event struct sort of like EPOLLET:

Quoted 3 lines

epfd = epoll_create1(0);
ev.events = EPOLLIN|EPOLLKEEPAWAKE;
epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &ev);

Do you mean something like the patch below, or something different?

Rafael

---
 drivers/input/evdev.c     |   55 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/eventpoll.c            |   15 +++++++++++-
 include/linux/eventpoll.h |    6 +++++
 include/linux/fs.h        |    1 
 4 files changed, 76 insertions(+), 1 deletion(-)

Index: linux/include/linux/fs.h
===================================================================

include/linux/fs.h +1 -0

@@ -1604,6 +1604,7 @@ struct file_operations {
 	ssize_t (*aio_write) (struct kiocb *, const struct iovec *, unsigned long, loff_t);
 	int (*readdir) (struct file *, void *, filldir_t);
 	unsigned int (*poll) (struct file *, struct poll_table_struct *);
+	void (*epoll_ctl) (struct file *, int, unsigned int);
 	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);

Index: linux/fs/eventpoll.c
===================================================================

fs/eventpoll.c +14 -1

@@ -609,6 +609,10 @@ static int ep_remove(struct eventpoll *e
 	unsigned long flags;
 	struct file *file = epi->ffd.file;
 
+	/* Notify the underlying driver that the polling has completed */
+	if (file->f_op->epoll_ctl)
+		file->f_op->epoll_ctl(file, EPOLL_CTL_DEL, epi->event.events);
+
 	/*
 	 * Removes poll wait queue hooks. We _have_ to do this without holding
 	 * the "ep->lock" otherwise a deadlock might occur. This because of the

@@ -1094,6 +1098,10 @@ static int ep_insert(struct eventpoll *e
 	epq.epi = epi;
 	init_poll_funcptr(&epq.pt, ep_ptable_queue_proc);
 
+	/* Notify the underlying driver that we want to poll it */
+	if (tfile->f_op->epoll_ctl)
+		tfile->f_op->epoll_ctl(tfile, EPOLL_CTL_ADD, event->events);
+
 	/*
 	 * Attach the item to the poll hooks and get current event bits.
 	 * We can safely use the file* here because its usage count has

@@ -1185,6 +1193,7 @@ error_unregister:
  */
 static int ep_modify(struct eventpoll *ep, struct epitem *epi, struct epoll_event *event)
 {
+	struct file *file = epi->ffd.file;
 	int pwake = 0;
 	unsigned int revents;
 

@@ -1196,11 +1205,15 @@ static int ep_modify(struct eventpoll *e
 	epi->event.events = event->events;
 	epi->event.data = event->data; /* protected by mtx */
 
+	/* Notify the underlying driver of the change */
+	if (file->f_op->epoll_ctl)
+		file->f_op->epoll_ctl(file, EPOLL_CTL_MOD, event->events);
+
 	/*
 	 * Get current event bits. We can safely use the file* here because
 	 * its usage count has been increased by the caller of this function.
 	 */
-	revents = epi->ffd.file->f_op->poll(epi->ffd.file, NULL);
+	revents = file->f_op->poll(file, NULL);
 
 	/*
 	 * If the item is "hot" and it is not registered inside the ready

Index: linux/drivers/input/evdev.c
===================================================================

drivers/input/evdev.c +55 -0

@@ -16,6 +16,7 @@
 #define EVDEV_BUF_PACKETS	8
 
 #include <linux/poll.h>
+#include <linux/eventpoll.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/module.h>

@@ -43,6 +44,7 @@ struct evdev_client {
 	unsigned int tail;
 	unsigned int packet_head; /* [future] position of the first element of next packet */
 	spinlock_t buffer_lock; /* protects access to buffer, head and tail */
+	struct wakeup_source *wakeup_source;
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;

@@ -75,10 +77,12 @@ static void evdev_pass_event(struct evde
 		client->buffer[client->tail].value = 0;
 
 		client->packet_head = client->tail;
+		__pm_relax(client->wakeup_source);
 	}
 
 	if (event->type == EV_SYN && event->code == SYN_REPORT) {
 		client->packet_head = client->head;
+		__pm_stay_awake(client->wakeup_source);
 		kill_fasync(&client->fasync, SIGIO, POLL_IN);
 	}
 

@@ -255,6 +259,8 @@ static int evdev_release(struct inode *i
 	mutex_unlock(&evdev->mutex);
 
 	evdev_detach_client(evdev, client);
+	wakeup_source_unregister(client->wakeup_source);
+
 	kfree(client);
 
 	evdev_close_device(evdev);

@@ -373,6 +379,8 @@ static int evdev_fetch_next_event(struct
 	if (have_event) {
 		*event = client->buffer[client->tail++];
 		client->tail &= client->bufsize - 1;
+		if (client->packet_head == client->tail)
+			__pm_relax(client->wakeup_source);
 	}
 
 	spin_unlock_irq(&client->buffer_lock);

@@ -433,6 +441,52 @@ static unsigned int evdev_poll(struct fi
 	return mask;
 }
 
+static void evdev_client_attach_wakeup_source(struct evdev_client *client)
+{
+	struct wakeup_source *ws;
+
+	ws = wakeup_source_register(dev_name(&client->evdev->dev));
+	spin_lock_irq(&client->buffer_lock);
+	client->wakeup_source = ws;
+	if (client->packet_head != client->tail)
+		__pm_stay_awake(client->wakeup_source);
+	spin_unlock_irq(&client->buffer_lock);
+}
+
+static void evdev_client_detach_wakeup_source(struct evdev_client *client)
+{
+	struct wakeup_source *ws;
+
+	spin_lock_irq(&client->buffer_lock);
+	ws = client->wakeup_source;
+	client->wakeup_source = NULL;
+	spin_unlock_irq(&client->buffer_lock);
+	wakeup_source_unregister(ws);
+}
+
+static void evdev_epoll_ctl(struct file *file, int op,
+				    unsigned int events)
+{
+	struct evdev_client *client = file->private_data;
+
+	switch (op) {
+	case EPOLL_CTL_ADD:
+		if ((events & EPOLLWAKEUP) && !client->wakeup_source)
+			evdev_client_attach_wakeup_source(client);
+		break;
+	case EPOLL_CTL_DEL:
+		if (events & EPOLLWAKEUP)
+			evdev_client_detach_wakeup_source(client);
+		break;
+	case EPOLL_CTL_MOD:
+		/* 'events' is the new events mask (after the change) */
+		if ((events & EPOLLWAKEUP) && !client->wakeup_source)
+			evdev_client_attach_wakeup_source(client);
+		else if (!(events & EPOLLWAKEUP))
+			evdev_client_detach_wakeup_source(client);
+	}
+}
+
 #ifdef CONFIG_COMPAT
 
 #define BITS_PER_LONG_COMPAT (sizeof(compat_long_t) * 8)

@@ -845,6 +899,7 @@ static const struct file_operations evde
 	.read		= evdev_read,
 	.write		= evdev_write,
 	.poll		= evdev_poll,
+	.epoll_ctl	= evdev_epoll_ctl,
 	.open		= evdev_open,
 	.release	= evdev_release,
 	.unlocked_ioctl	= evdev_ioctl,

Index: linux/include/linux/eventpoll.h
===================================================================

include/linux/eventpoll.h +6 -0

@@ -26,6 +26,12 @@
 #define EPOLL_CTL_DEL 2
 #define EPOLL_CTL_MOD 3
 
+/*
+ * Request the handling of system wakeup events so as to prevent automatic
+ * system suspends from happening while those events are being processed.
+ */
+#define EPOLLWAKEUP (1 << 29)
+
 /* Set the One Shot behaviour for the target file descriptor */
 #define EPOLLONESHOT (1 << 30)
 

Quoted 46 lines · Rafael J. Wysocki

Quoted 44 lines · Matt Helsley

Quoted 11 lines · Rafael J. Wysocki

From: Arve Hjønnevåg <[email protected]>

Add a new ioctl, EVIOCSWAKEUPSRC, to attach a wakeup source object to an evdev client event queue, such that it will be active whenever the queue is not empty. Then, all events in the queue will be regarded as wakeup events in progress and pm_get_wakeup_count() will block (or return false if woken up by a signal) until they are removed from the queue. In consequence, if the checking of wakeup events is enabled (e.g. throught the /sys/power/wakeup_count interface), the system won't be able to go into a sleep state until the queue is empty.

Quoted 10 lines

This allows user space processes to handle situations in which they want to do a select() on an evdev descriptor, so they go to sleep until there are some events to read from the device's queue, and then they don't want the system to go into a sleep state until all the events are read (presumably for further processing). Of course, if they don't want the system to go into a sleep state _after_ all the events have been read from the queue, they have to use a separate mechanism that will prevent the system from doing that and it has to be activated before reading the first event (that also may be the last one).

I haven't seen this idea mentioned before but I must admit I haven't been following this thread too closely so apologies (and don't bother rehashing) if it has:

Could you just add this to epoll so that any fd userspace chooses would be capable of doing this without introducing potentially ecclectic ioctl interfaces?

struct epoll_event ev;

epfd = epoll_create1(EPOLL_STAY_AWAKE_SET);
ev.data.ptr = foo;
epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &ev);

Which could be useful because you can put one epollfd in another's epoll set. Or maybe as an EPOLLKEEPAWAKE flag in the event struct sort of like EPOLLET:

epfd = epoll_create1(0);
ev.events = EPOLLIN|EPOLLKEEPAWAKE;
epoll_ctl(epfd, EPOLL_CTL_ADD, fd, &ev);