[markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[mark gross]

resending to wider list for discussion
----- Forwarded message from mark gross <markgross@thengar.org> -----

Subject: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)
Date: Tue, 20 Sep 2011 13:33:05 -0700
From: mark gross <markgross@thengar.org>
To: linux-pm@lists.linux-foundation.org
Reply-To: markgross@thegnar.org
Cc: arve@android.com, markgross@thegnar.org, Alan Stern <stern@rowland.harvard.edu>, amit.kucheria@linaro.org, farrowg@sg.ibm.com, "Rafael J. Wysocki" <rjw@sisk.pl>

The following patch set implement an (untested) solution to the
following problems.

1) a method for making a system unable to suspend for critical sections
of time.

2) providing a race free method for the acknowledgment of wake event
processing before re-entry into suspend can happen.

It is my opinion that hardship of the infamous wake lock patches and
subsequent time sink is the conflation of these two needs that any
system that wakes from suspend if automatic suspend and wake ups is
going to work.

The android wake lock way of solving these issues is bit of a one off
for selected subsystems (alarm and input WRT notification of wake
events) and I don't think they generalize well and makes it hard to hand
off wake lock critical sections between kernel and user mode without
having a timeout interface (which is by definition racy).

I think by explicitly separating these two requirements and building on
the new-ish wakeup.c code thats in today's kernels we can come close to
solving these needs.

The following patches explicitly address these 2 issues without
conflating them with each other.  Because notification is very different
from defining a critical section and confusing them is a bad idea.  I
like the directness of each and hope these will help get people focused
on the problem definition more than the implementation.  (mine or anyone
else's)

Also note the attached python programs below that attempt to show how
the ABI's could be used:

pms.py -- stub program of a power manager service that hits /sys/power/*
interface  (code is incomplete but gets the idea across)

dont_sleep.py -- show how a process could block suspending using the pm_qos
interface in patch 2.

button-wake.py -- shows how the wake events should be handled WRT patch
1.

--mark


Signed-off-by: markgross <markgross@thegnar.org>



#!/user/bin/python

#
# python program to talk to /dev/pwrButton misc device to make sure 
# the wakeup.c complaint user mode power manager doesn't re-enter suspend
# before this python program say's "OK"
# assumptions:
# pwrbutton driver registers a wakeup source with name "pwrButton"
# user mode power manager follows wakeup.c defined protocol of reading
# /sys/power/wake_count and writing to back to it before writing to
# /sys/power/state.  (the read is blocking if wakeup event is "active")



import struct

def Notification():
    iofile = open("/dev/pwrButton", "rw")
    # blocking select
    iofile.select()
    count = iofile.read()
    print "detected wake event now doing important stuff"
    # now its time to ACK the wake event(s) so another suspend can happen.
    # this will unblock the read of /sys/power/wake_count so the next suspend
    # can be attempted.
    print "acknowledge wake event(s)"
    iofile.write(count * '.')
    iofile.flush()
    iofile.close()


def main():
    while True:
        Notification()


if "__main__" == __name__:
    main()



#!/usr/bin/python

# power manager service program for implementing aggressive power management 
# policies of sleeping when no other program has a request to not sleep.
# Basically a python program that implement Android power management
# Applications can hold user mode "partial" and "full" wake locks.
# Partial wake locks prevent suspending but allow the display to turn off.
# full wake locks keep the display on.
# applications grab and release wakelocks through a d-bus (?) interface defined 
# by this python program.  TBD

import thread
import time

partial_wake_locks = []
full_wake_locks = []

def screen_off(off):
    if off:
        #turn off display and back light
        return
    else:
        #turn on display and back light
        return


def suspend_thread():
    global partial_wake_locks
    global full_wake_locks
    stateIO = open("/sys/power/state", "rw")
    countIO = open("/sys/power/wakeup_count", "rw")
    while True:
        #grab thread lock here
        if len(partial_wake_locks) == 0 and len(full_wake_locks) == 0:
        # release thread lock
            wakeup_count = countIO.read() #blocks until previous wakeup has been acknowledged
            countIO.write(wakeup_count)
            countIO.flush()
            #double check that no locks are held after maybe waking up.
            #grab thread lock here
            if len(partial_wake_locks) == 0 and len(full_wake_locks) == 0:
            # release thread lock
                stateIO.write("mem")
                stateIO.flush()
            else:
                # release thread lock
                continue
        else:
            # release thread lock
            continue
        
        time.sleep(1.0)


def dbus_thread():
    # update partial and full wake lock lists based on dbus activities
    # hold thread lock when updating lists...
    if len(partial_wake_locks) == 0:
    # release thread lock
        screen_off(0)
    else:
        # release thread lock
        screen_off(1)

    return

def main():
    return
    #init stuff and kick off do_Dbus and suspend threads
    # just sit and wait for something to happen.
    # do a join wait on for either of the two child threads to complete

if "__main__" == __name__:
    main()



#!/user/bin/python

#
# python program to talk to /dev/suspend_block to make sure system doesn't
# suspend in a critical section of code.
#

import struct, time


def main():
    iofile = open("/dev/suspend_block", "w")
    s = struct.pack('=i', 1)
    iofile.write(s)
    iofile.flush()
    while 1:
        time.sleep(10.0)

    iofile.close()

if "__main__" == __name__:
    main()




----- End forwarded message -----

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On Sun, 2 Oct 2011 09:44:56 -0700 mark gross <markgross@thegnar.org> wrote:

> resending to wider list for discussion
> ----- Forwarded message from mark gross <markgross@thengar.org> -----
> 
> Subject: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)
> Date: Tue, 20 Sep 2011 13:33:05 -0700
> From: mark gross <markgross@thengar.org>
> To: linux-pm@lists.linux-foundation.org
> Reply-To: markgross@thegnar.org
> Cc: arve@android.com, markgross@thegnar.org, Alan Stern <stern@rowland.harvard.edu>, amit.kucheria@linaro.org, farrowg@sg.ibm.com, "Rafael J. Wysocki" <rjw@sisk.pl>
> 
> The following patch set implement an (untested) solution to the
> following problems.
> 
> 1) a method for making a system unable to suspend for critical sections
> of time.

We already have this.  A properly requested suspend (following wakeup_count
protocol) is unable to complete between wakeup_source_activate() and
wake_source_deactivate() - these delimit the critical sections.

What more than this do you need?

If user-space wants to prevent suspend, it just needs some sort of protocol
for talking to the user-space process which follows the correct protocol to
initiate suspend.  That isn't a kernel problem.

> 
> 2) providing a race free method for the acknowledgment of wake event
> processing before re-entry into suspend can happen.

Again, this is a user-space problem.  It is user-space which requests
suspend.  It shouldn't request it until it has checked that there are no wake
events that need processing - and should use the wakeup_count protocol to
avoid races with wakeup events happening after it has checked.

i.e. there is no kernel-space problem to solve here (except for possible
bugs).

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[mark gross]

On Sat, Oct 08, 2011 at 10:14:39PM +1100, NeilBrown wrote:
> On Sun, 2 Oct 2011 09:44:56 -0700 mark gross <markgross@thegnar.org> wrote:
> 
> > resending to wider list for discussion
> > ----- Forwarded message from mark gross <markgross@thengar.org> -----
> > 
> > Subject: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)
> > Date: Tue, 20 Sep 2011 13:33:05 -0700
> > From: mark gross <markgross@thengar.org>
> > To: linux-pm@lists.linux-foundation.org
> > Reply-To: markgross@thegnar.org
> > Cc: arve@android.com, markgross@thegnar.org, Alan Stern <stern@rowland.harvard.edu>, amit.kucheria@linaro.org, farrowg@sg.ibm.com, "Rafael J. Wysocki" <rjw@sisk.pl>
> > 
> > The following patch set implement an (untested) solution to the
> > following problems.
> > 
> > 1) a method for making a system unable to suspend for critical sections
> > of time.
> 
> We already have this.  A properly requested suspend (following wakeup_count
> protocol) is unable to complete between wakeup_source_activate() and
> wake_source_deactivate() - these delimit the critical sections.
> 
> What more than this do you need?

sometimes devices that are not wake up sources need critical sections
where suspend is a problem.

> If user-space wants to prevent suspend, it just needs some sort of protocol
> for talking to the user-space process which follows the correct protocol to
> initiate suspend.  That isn't a kernel problem.

The devices that I've seen that need to block suspend don't have a
communication interface to user mode. 

But, you are right the devices that need this sort of thing could
register as wakeup sources and block suspend as well.

FWIW This part of the patch set was to wrap up a proposal I got last
year from some folks to try to provide somewhat compatible semantics to
wakelock's for the android and linux kernel community.

I include it for completeness. 
> 
> > 
> > 2) providing a race free method for the acknowledgment of wake event
> > processing before re-entry into suspend can happen.
> 
> Again, this is a user-space problem.  It is user-space which requests
> suspend.  It shouldn't request it until it has checked that there are no wake
> events that need processing - and should use the wakeup_count protocol to
> avoid races with wakeup events happening after it has checked.

Here you are wrong, or missing the point.  The kernel needs to be
notified from user mode that an update event has been consumed by
whoever cares about it before the next suspend can happen.  The fact
that there are time outs in the existing wake event code points to this
shortcoming in the current implementation.

I suppose one could rig up the user mode suspend daemon with
notification callbacks between event consumers across the user mode
stack but its really complex to get it right and forces a solution to a
problem better solved in kernel mode be done with hacky user mode
gyrations that may ripple wildly across user mode.

Also it is the kernel that is currently deciding when to unblock the
suspend daemon for the next suspend attempt.  Why not build on that and
make is so we don't need the time outs?

> i.e. there is no kernel-space problem to solve here (except for possible
> bugs).

Just a race between the kernel allowing a suspend and the user mode code
having time to consume the last wake event.

--mark

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[Rafael J. Wysocki]

On Saturday, October 08, 2011, mark gross wrote:
> On Sat, Oct 08, 2011 at 10:14:39PM +1100, NeilBrown wrote:
> > On Sun, 2 Oct 2011 09:44:56 -0700 mark gross <markgross@thegnar.org> wrote:
> > 
> > > resending to wider list for discussion
> > > ----- Forwarded message from mark gross <markgross@thengar.org> -----
> > > 
> > > Subject: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)
> > > Date: Tue, 20 Sep 2011 13:33:05 -0700
> > > From: mark gross <markgross@thengar.org>
> > > To: linux-pm@lists.linux-foundation.org
> > > Reply-To: markgross@thegnar.org
> > > Cc: arve@android.com, markgross@thegnar.org, Alan Stern <stern@rowland.harvard.edu>, amit.kucheria@linaro.org, farrowg@sg.ibm.com, "Rafael J. Wysocki" <rjw@sisk.pl>
> > > 
> > > The following patch set implement an (untested) solution to the
> > > following problems.
> > > 
> > > 1) a method for making a system unable to suspend for critical sections
> > > of time.
> > 
> > We already have this.  A properly requested suspend (following wakeup_count
> > protocol) is unable to complete between wakeup_source_activate() and
> > wake_source_deactivate() - these delimit the critical sections.
> > 
> > What more than this do you need?
> 
> sometimes devices that are not wake up sources need critical sections
> where suspend is a problem.
> 
> > If user-space wants to prevent suspend, it just needs some sort of protocol
> > for talking to the user-space process which follows the correct protocol to
> > initiate suspend.  That isn't a kernel problem.
> 
> The devices that I've seen that need to block suspend don't have a
> communication interface to user mode. 
> 
> But, you are right the devices that need this sort of thing could
> register as wakeup sources and block suspend as well.
> 
> FWIW This part of the patch set was to wrap up a proposal I got last
> year from some folks to try to provide somewhat compatible semantics to
> wakelock's for the android and linux kernel community.
> 
> I include it for completeness. 
> > 
> > > 
> > > 2) providing a race free method for the acknowledgment of wake event
> > > processing before re-entry into suspend can happen.
> > 
> > Again, this is a user-space problem.  It is user-space which requests
> > suspend.  It shouldn't request it until it has checked that there are no wake
> > events that need processing - and should use the wakeup_count protocol to
> > avoid races with wakeup events happening after it has checked.
> 
> Here you are wrong, or missing the point.  The kernel needs to be
> notified from user mode that an update event has been consumed by
> whoever cares about it before the next suspend can happen.

This, in fact, isn't correct.  I have tried to turn your (and John's)
attention to this for quite a few times already.

The point is that the entity about to trigger suspend (that need not be the
kernel!) has to communicate with the processes that consume wakeup events
beforehand.  In theory this communication can happen entirely in user
space, but that would involve quite complicated interactions between
processes, so nobody does that in practice.

The only "problem" that can't be solved entirely in user space, which is
what John turned my attention to during the LPC, is that it may be
possible to suspend when processes that should be asked about whether or
not to suspend are sleeping and that may be done _without_ actually asking
those processes for permission.  The difficult part is, if we suspend in
such a situation, we need to wait until all of those processes have a chance
to run before attempting to suspend again.

> The fact that there are time outs in the existing wake event code points to
> this shortcoming in the current implementation.

Actually, the timeouts serve a different purpose.  Namely, there are wakeup
events that aren't actually consumed by anyone above the layer signaling the
event (think about Wake-on-LAN via a magic packet) and if such an event
happens, we can't suspend at once, because we need to assume that it happened
for a reason, so whoever triggered the event has to be given a chance to do
whatever he needed to wake up the system for.  This cannot be achieved without
timeouts.
 
> I suppose one could rig up the user mode suspend daemon with
> notification callbacks between event consumers across the user mode
> stack but its really complex to get it right and forces a solution to a
> problem better solved in kernel mode be done with hacky user mode
> gyrations that may ripple wildly across user mode.

Agreed.

> Also it is the kernel that is currently deciding when to unblock the
> suspend daemon for the next suspend attempt.  Why not build on that and
> make is so we don't need the time outs?
> 
> > i.e. there is no kernel-space problem to solve here (except for possible
> > bugs).
> 
> Just a race between the kernel allowing a suspend and the user mode code
> having time to consume the last wake event.

That's correct.

Thanks,
Rafael

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[Alan Stern]

On Sat, 8 Oct 2011, Rafael J. Wysocki wrote:

> On Saturday, October 08, 2011, mark gross wrote:
> > On Sat, Oct 08, 2011 at 10:14:39PM +1100, NeilBrown wrote:
> > > On Sun, 2 Oct 2011 09:44:56 -0700 mark gross <markgross@thegnar.org> wrote:
> > > 
> > > > resending to wider list for discussion
> > > > ----- Forwarded message from mark gross <markgross@thengar.org> -----
> > > > 
> > > > Subject: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)
> > > > Date: Tue, 20 Sep 2011 13:33:05 -0700
> > > > From: mark gross <markgross@thengar.org>
> > > > To: linux-pm@lists.linux-foundation.org
> > > > Reply-To: markgross@thegnar.org
> > > > Cc: arve@android.com, markgross@thegnar.org, Alan Stern <stern@rowland.harvard.edu>, amit.kucheria@linaro.org, farrowg@sg.ibm.com, "Rafael J. Wysocki" <rjw@sisk.pl>
> > > > 
> > > > The following patch set implement an (untested) solution to the
> > > > following problems.
> > > > 
> > > > 1) a method for making a system unable to suspend for critical sections
> > > > of time.
> > > 
> > > We already have this.  A properly requested suspend (following wakeup_count
> > > protocol) is unable to complete between wakeup_source_activate() and
> > > wake_source_deactivate() - these delimit the critical sections.
> > > 
> > > What more than this do you need?
> > 
> > sometimes devices that are not wake up sources need critical sections
> > where suspend is a problem.

Mark, can you give some examples?  This isn't the same as the firmware 
update thing, is it?  That deserves to be handled by a completely 
separate mechanism.

> > > If user-space wants to prevent suspend, it just needs some sort of protocol
> > > for talking to the user-space process which follows the correct protocol to
> > > initiate suspend.  That isn't a kernel problem.
> > 
> > The devices that I've seen that need to block suspend don't have a
> > communication interface to user mode. 
> > 
> > But, you are right the devices that need this sort of thing could
> > register as wakeup sources and block suspend as well.
> > 
> > FWIW This part of the patch set was to wrap up a proposal I got last
> > year from some folks to try to provide somewhat compatible semantics to
> > wakelock's for the android and linux kernel community.
> > 
> > I include it for completeness. 

Although many of these problems could be solved by adopting a suitable
protocol in userspace, currently there is no such protocol.  Even if
one did exist, the process of getting all the relevant programs to
adopt it would take quite a while.  This is a case where a problem can
be solved either in the kernel or in userspace, and the in-kernel
solution may be simpler.

> > > > 2) providing a race free method for the acknowledgment of wake event
> > > > processing before re-entry into suspend can happen.
> > > 
> > > Again, this is a user-space problem.  It is user-space which requests
> > > suspend.  It shouldn't request it until it has checked that there are no wake
> > > events that need processing - and should use the wakeup_count protocol to
> > > avoid races with wakeup events happening after it has checked.
> > 
> > Here you are wrong, or missing the point.  The kernel needs to be
> > notified from user mode that an update event has been consumed by
> > whoever cares about it before the next suspend can happen.
> 
> This, in fact, isn't correct.  I have tried to turn your (and John's)
> attention to this for quite a few times already.
> 
> The point is that the entity about to trigger suspend (that need not be the
> kernel!) has to communicate with the processes that consume wakeup events
> beforehand.  In theory this communication can happen entirely in user
> space, but that would involve quite complicated interactions between
> processes, so nobody does that in practice.
> 
> The only "problem" that can't be solved entirely in user space, which is
> what John turned my attention to during the LPC, is that it may be
> possible to suspend when processes that should be asked about whether or
> not to suspend are sleeping and that may be done _without_ actually asking
> those processes for permission.

Essentially, there has to be a way for these processes to say "It's 
okay to suspend until I tell you otherwise."  That could also be part 
of the userspace protocol (if it existed).

>  The difficult part is, if we suspend in
> such a situation, we need to wait until all of those processes have a chance
> to run before attempting to suspend again.

Of course, a power-management daemon could handle that part easily.  
Doing it in the kernel is more difficult.

> > The fact that there are time outs in the existing wake event code points to
> > this shortcoming in the current implementation.
> 
> Actually, the timeouts serve a different purpose.  Namely, there are wakeup
> events that aren't actually consumed by anyone above the layer signaling the
> event (think about Wake-on-LAN via a magic packet) and if such an event
> happens, we can't suspend at once, because we need to assume that it happened
> for a reason, so whoever triggered the event has to be given a chance to do
> whatever he needed to wake up the system for.  This cannot be achieved without
> timeouts.

Also there are events that may or may not get consumed, but it is
impossible to tell at the time whether or not a consumer exists.  If
one does exist then good, the system can suspend again when the
consumer is finished.  But if not, the system can only wait until a
timeout.

> > I suppose one could rig up the user mode suspend daemon with
> > notification callbacks between event consumers across the user mode
> > stack but its really complex to get it right and forces a solution to a
> > problem better solved in kernel mode be done with hacky user mode
> > gyrations that may ripple wildly across user mode.
> 
> Agreed.
> 
> > Also it is the kernel that is currently deciding when to unblock the
> > suspend daemon for the next suspend attempt.  Why not build on that and
> > make is so we don't need the time outs?
> > 
> > > i.e. there is no kernel-space problem to solve here (except for possible
> > > bugs).
> > 
> > Just a race between the kernel allowing a suspend and the user mode code
> > having time to consume the last wake event.
> 
> That's correct.

Alan Stern

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5041 bytes --]

On Sat, 8 Oct 2011 11:16:38 -0700 mark gross <markgross@thegnar.org> wrote:

> On Sat, Oct 08, 2011 at 10:14:39PM +1100, NeilBrown wrote:
> > On Sun, 2 Oct 2011 09:44:56 -0700 mark gross <markgross@thegnar.org> wrote:
> > 
> > > resending to wider list for discussion
> > > ----- Forwarded message from mark gross <markgross@thengar.org> -----
> > > 
> > > Subject: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)
> > > Date: Tue, 20 Sep 2011 13:33:05 -0700
> > > From: mark gross <markgross@thengar.org>
> > > To: linux-pm@lists.linux-foundation.org
> > > Reply-To: markgross@thegnar.org
> > > Cc: arve@android.com, markgross@thegnar.org, Alan Stern <stern@rowland.harvard.edu>, amit.kucheria@linaro.org, farrowg@sg.ibm.com, "Rafael J. Wysocki" <rjw@sisk.pl>
> > > 
> > > The following patch set implement an (untested) solution to the
> > > following problems.
> > > 
> > > 1) a method for making a system unable to suspend for critical sections
> > > of time.
> > 
> > We already have this.  A properly requested suspend (following wakeup_count
> > protocol) is unable to complete between wakeup_source_activate() and
> > wake_source_deactivate() - these delimit the critical sections.
> > 
> > What more than this do you need?
> 
> sometimes devices that are not wake up sources need critical sections
> where suspend is a problem.

I agree with Alan that an example would help here.
My naive perspective is that any device is either acting on behalf of a
user-space program, so it should disable suspend, or on behalf of some
external event, so that event is ipso-facto a wakeup event.

> > 
> > > 
> > > 2) providing a race free method for the acknowledgment of wake event
> > > processing before re-entry into suspend can happen.
> > 
> > Again, this is a user-space problem.  It is user-space which requests
> > suspend.  It shouldn't request it until it has checked that there are no wake
> > events that need processing - and should use the wakeup_count protocol to
> > avoid races with wakeup events happening after it has checked.
> 
> Here you are wrong, or missing the point.  The kernel needs to be
> notified from user mode that an update event has been consumed by
> whoever cares about it before the next suspend can happen.  The fact
> that there are time outs in the existing wake event code points to this
> shortcoming in the current implementation.

 ... or I have a different perspective.
A write to wakeup_count is a notification to the kernel that all wakeup
events that had commenced prior to that same number being read from
wakeup_count have been consumed.

So we already have a mechanism for the notification that you want.

> 
> I suppose one could rig up the user mode suspend daemon with
> notification callbacks between event consumers across the user mode
> stack but its really complex to get it right and forces a solution to a
> problem better solved in kernel mode be done with hacky user mode
> gyrations that may ripple wildly across user mode.

I suspect it is in here that the key to our different perspectives lies.
I think than any solution must "ripple wildly across user mode" if by that
you mean that more applications and daemons will need to be power-aware and
make definitive decisions about when they cannot tolerate suspend.
Whether those apps and daemons tell the kernel "don't suspend now" or tell
some user-space daemon "don't suspend now" is fairly irrelevant when
assessing the total impact on user-space.

I think a fairly simple protocol involving file locking can be perfectly
adequate to communicate needs relating to suspend-or-don't-suspend among
user-space processes.


> 
> Also it is the kernel that is currently deciding when to unblock the
> suspend daemon for the next suspend attempt.  Why not build on that and
> make is so we don't need the time outs?

Suspend is a joint decision by user-space and kernel-space.  Each part should
participate according to its expertise.
The kernel can make use of information generated by drivers in the kernel.
User-space can consolidate information generated by user-space processes.


> 
> > i.e. there is no kernel-space problem to solve here (except for possible
> > bugs).
> 
> Just a race between the kernel allowing a suspend and the user mode code
> having time to consume the last wake event.
>

Providing that the source of the wake event does not deactivate the
wakeup_source before the event is visible to userspace, this race is easily
avoided in userspace:

   - read wakeup_count
   - check all possible wakeup events.
   - if there were none, write back to wakeup_count and request a suspend.

This is race-free.

If some wakeup_source is deactivated before the event is visible to
user-space, then that is a bug and should be fixed.
If there is some particular case where it is non-trivial to fix that bug,
then that would certainly be worth exploring in detail.

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[mark gross]

On Sat, Oct 08, 2011 at 08:57:42PM +0200, Rafael J. Wysocki wrote:
> On Saturday, October 08, 2011, mark gross wrote:
> > On Sat, Oct 08, 2011 at 10:14:39PM +1100, NeilBrown wrote:
> > > On Sun, 2 Oct 2011 09:44:56 -0700 mark gross <markgross@thegnar.org> wrote:
> > > 
> > > > resending to wider list for discussion
> > > > ----- Forwarded message from mark gross <markgross@thengar.org> -----
> > > > 
> > > > Subject: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)
> > > > Date: Tue, 20 Sep 2011 13:33:05 -0700
> > > > From: mark gross <markgross@thengar.org>
> > > > To: linux-pm@lists.linux-foundation.org
> > > > Reply-To: markgross@thegnar.org
> > > > Cc: arve@android.com, markgross@thegnar.org, Alan Stern <stern@rowland.harvard.edu>, amit.kucheria@linaro.org, farrowg@sg.ibm.com, "Rafael J. Wysocki" <rjw@sisk.pl>
> > > > 
> > > > The following patch set implement an (untested) solution to the
> > > > following problems.
> > > > 
> > > > 1) a method for making a system unable to suspend for critical sections
> > > > of time.
> > > 
> > > We already have this.  A properly requested suspend (following wakeup_count
> > > protocol) is unable to complete between wakeup_source_activate() and
> > > wake_source_deactivate() - these delimit the critical sections.
> > > 
> > > What more than this do you need?
> > 
> > sometimes devices that are not wake up sources need critical sections
> > where suspend is a problem.
> > 
> > > If user-space wants to prevent suspend, it just needs some sort of protocol
> > > for talking to the user-space process which follows the correct protocol to
> > > initiate suspend.  That isn't a kernel problem.
> > 
> > The devices that I've seen that need to block suspend don't have a
> > communication interface to user mode. 
> > 
> > But, you are right the devices that need this sort of thing could
> > register as wakeup sources and block suspend as well.
> > 
> > FWIW This part of the patch set was to wrap up a proposal I got last
> > year from some folks to try to provide somewhat compatible semantics to
> > wakelock's for the android and linux kernel community.
> > 
> > I include it for completeness. 
> > > 
> > > > 
> > > > 2) providing a race free method for the acknowledgment of wake event
> > > > processing before re-entry into suspend can happen.
> > > 
> > > Again, this is a user-space problem.  It is user-space which requests
> > > suspend.  It shouldn't request it until it has checked that there are no wake
> > > events that need processing - and should use the wakeup_count protocol to
> > > avoid races with wakeup events happening after it has checked.
> > 
> > Here you are wrong, or missing the point.  The kernel needs to be
> > notified from user mode that an update event has been consumed by
> > whoever cares about it before the next suspend can happen.
> 
> This, in fact, isn't correct.  I have tried to turn your (and John's)
> attention to this for quite a few times already.
yup.

> The point is that the entity about to trigger suspend (that need not be the
> kernel!) has to communicate with the processes that consume wakeup events
> beforehand.  In theory this communication can happen entirely in user
> space, but that would involve quite complicated interactions between
> processes, so nobody does that in practice.
yes.

> The only "problem" that can't be solved entirely in user space, which is
> what John turned my attention to during the LPC, is that it may be
> possible to suspend when processes that should be asked about whether or
> not to suspend are sleeping and that may be done _without_ actually asking
> those processes for permission.  The difficult part is, if we suspend in
> such a situation, we need to wait until all of those processes have a chance
> to run before attempting to suspend again.
I'm not trying to address this.  I see this as an interesting
optimization that distracts from the issue this patch set is trying to
make better.  


> > The fact that there are time outs in the existing wake event code points to
> > this shortcoming in the current implementation.
> 
> Actually, the timeouts serve a different purpose.  Namely, there are wakeup
> events that aren't actually consumed by anyone above the layer signaling the
> event (think about Wake-on-LAN via a magic packet) and if such an event
> happens, we can't suspend at once, because we need to assume that it happened
> for a reason, so whoever triggered the event has to be given a chance to do
> whatever he needed to wake up the system for.  This cannot be achieved without
> timeouts.
Thats why I created the notification interface.  Couldn't the process
that needs a chance to do its work register for notification instead of
having a timeout with the hope that it was long enough?

> > I suppose one could rig up the user mode suspend daemon with
> > notification callbacks between event consumers across the user mode
> > stack but its really complex to get it right and forces a solution to a
> > problem better solved in kernel mode be done with hacky user mode
> > gyrations that may ripple wildly across user mode.
> 
> Agreed.
> 
> > Also it is the kernel that is currently deciding when to unblock the
> > suspend daemon for the next suspend attempt.  Why not build on that and
> > make is so we don't need the time outs?
> > 
> > > i.e. there is no kernel-space problem to solve here (except for possible
> > > bugs).
> > 
> > Just a race between the kernel allowing a suspend and the user mode code
> > having time to consume the last wake event.
> 
> That's correct.
> 
> Thanks,
> Rafael

sorry for the lat reply.

--mark

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[mark gross]

On Sat, Oct 08, 2011 at 04:07:01PM -0400, Alan Stern wrote:
> On Sat, 8 Oct 2011, Rafael J. Wysocki wrote:
> 
> > On Saturday, October 08, 2011, mark gross wrote:
> > > On Sat, Oct 08, 2011 at 10:14:39PM +1100, NeilBrown wrote:
> > > > On Sun, 2 Oct 2011 09:44:56 -0700 mark gross <markgross@thegnar.org> wrote:
> > > > 
> > > > > resending to wider list for discussion
> > > > > ----- Forwarded message from mark gross <markgross@thengar.org> -----
> > > > > 
> > > > > Subject: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)
> > > > > Date: Tue, 20 Sep 2011 13:33:05 -0700
> > > > > From: mark gross <markgross@thengar.org>
> > > > > To: linux-pm@lists.linux-foundation.org
> > > > > Reply-To: markgross@thegnar.org
> > > > > Cc: arve@android.com, markgross@thegnar.org, Alan Stern <stern@rowland.harvard.edu>, amit.kucheria@linaro.org, farrowg@sg.ibm.com, "Rafael J. Wysocki" <rjw@sisk.pl>
> > > > > 
> > > > > The following patch set implement an (untested) solution to the
> > > > > following problems.
> > > > > 
> > > > > 1) a method for making a system unable to suspend for critical sections
> > > > > of time.
> > > > 
> > > > We already have this.  A properly requested suspend (following wakeup_count
> > > > protocol) is unable to complete between wakeup_source_activate() and
> > > > wake_source_deactivate() - these delimit the critical sections.
> > > > 
> > > > What more than this do you need?
> > > 
> > > sometimes devices that are not wake up sources need critical sections
> > > where suspend is a problem.
> 
> Mark, can you give some examples?  This isn't the same as the firmware 
> update thing, is it?  That deserves to be handled by a completely 
> separate mechanism.

The biggest example I know of is any usb gadget implementation while
connected to a USB host.  If the device is connected to the USB bus then
it needs to not suspend because it cannot respond to the USB commands
for bus power.  i.e. if its connected to a laptop and the suspend the
laptop the gadget needs to handle the USB protocol packets.

Also, when charging battery under OS control we don't want to suspend
because it will be hard to respond to thermal events  or changing
battery conditions if the system is in suspend.

> > > > If user-space wants to prevent suspend, it just needs some sort of protocol
> > > > for talking to the user-space process which follows the correct protocol to
> > > > initiate suspend.  That isn't a kernel problem.
> > > 
> > > The devices that I've seen that need to block suspend don't have a
> > > communication interface to user mode. 
> > > 
> > > But, you are right the devices that need this sort of thing could
> > > register as wakeup sources and block suspend as well.
> > > 
> > > FWIW This part of the patch set was to wrap up a proposal I got last
> > > year from some folks to try to provide somewhat compatible semantics to
> > > wakelock's for the android and linux kernel community.
> > > 
> > > I include it for completeness. 
> 
> Although many of these problems could be solved by adopting a suitable
> protocol in userspace, currently there is no such protocol.  Even if
> one did exist, the process of getting all the relevant programs to
> adopt it would take quite a while.  This is a case where a problem can
> be solved either in the kernel or in userspace, and the in-kernel
> solution may be simpler.

I do think a better solution would involve some kernel coordination.

I don't want to split hairs on if its 100% possible to implement a
correct solution only in user mode.  but, I think there are corner cases
that may be hard or impossible to get right with only user mode.  See
gadget issue above.  Not all gadgets have a user mode daemon backing up
the kernel part do they?

--mark

> 
> > > > > 2) providing a race free method for the acknowledgment of wake event
> > > > > processing before re-entry into suspend can happen.
> > > > 
> > > > Again, this is a user-space problem.  It is user-space which requests
> > > > suspend.  It shouldn't request it until it has checked that there are no wake
> > > > events that need processing - and should use the wakeup_count protocol to
> > > > avoid races with wakeup events happening after it has checked.
> > > 
> > > Here you are wrong, or missing the point.  The kernel needs to be
> > > notified from user mode that an update event has been consumed by
> > > whoever cares about it before the next suspend can happen.
> > 
> > This, in fact, isn't correct.  I have tried to turn your (and John's)
> > attention to this for quite a few times already.
> > 
> > The point is that the entity about to trigger suspend (that need not be the
> > kernel!) has to communicate with the processes that consume wakeup events
> > beforehand.  In theory this communication can happen entirely in user
> > space, but that would involve quite complicated interactions between
> > processes, so nobody does that in practice.
> > 
> > The only "problem" that can't be solved entirely in user space, which is
> > what John turned my attention to during the LPC, is that it may be
> > possible to suspend when processes that should be asked about whether or
> > not to suspend are sleeping and that may be done _without_ actually asking
> > those processes for permission.
> 
> Essentially, there has to be a way for these processes to say "It's 
> okay to suspend until I tell you otherwise."  That could also be part 
> of the userspace protocol (if it existed).
> 
> >  The difficult part is, if we suspend in
> > such a situation, we need to wait until all of those processes have a chance
> > to run before attempting to suspend again.
> 
> Of course, a power-management daemon could handle that part easily.  
> Doing it in the kernel is more difficult.
> 
> > > The fact that there are time outs in the existing wake event code points to
> > > this shortcoming in the current implementation.
> > 
> > Actually, the timeouts serve a different purpose.  Namely, there are wakeup
> > events that aren't actually consumed by anyone above the layer signaling the
> > event (think about Wake-on-LAN via a magic packet) and if such an event
> > happens, we can't suspend at once, because we need to assume that it happened
> > for a reason, so whoever triggered the event has to be given a chance to do
> > whatever he needed to wake up the system for.  This cannot be achieved without
> > timeouts.
> 
> Also there are events that may or may not get consumed, but it is
> impossible to tell at the time whether or not a consumer exists.  If
> one does exist then good, the system can suspend again when the
> consumer is finished.  But if not, the system can only wait until a
> timeout.
> 
> > > I suppose one could rig up the user mode suspend daemon with
> > > notification callbacks between event consumers across the user mode
> > > stack but its really complex to get it right and forces a solution to a
> > > problem better solved in kernel mode be done with hacky user mode
> > > gyrations that may ripple wildly across user mode.
> > 
> > Agreed.
> > 
> > > Also it is the kernel that is currently deciding when to unblock the
> > > suspend daemon for the next suspend attempt.  Why not build on that and
> > > make is so we don't need the time outs?
> > > 
> > > > i.e. there is no kernel-space problem to solve here (except for possible
> > > > bugs).
> > > 
> > > Just a race between the kernel allowing a suspend and the user mode code
> > > having time to consume the last wake event.
> > 
> > That's correct.
> 
> Alan Stern
> 

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[mark gross]

On Sun, Oct 09, 2011 at 09:31:00AM +1100, NeilBrown wrote:
> On Sat, 8 Oct 2011 11:16:38 -0700 mark gross <markgross@thegnar.org> wrote:
> 
> > On Sat, Oct 08, 2011 at 10:14:39PM +1100, NeilBrown wrote:
> > > On Sun, 2 Oct 2011 09:44:56 -0700 mark gross <markgross@thegnar.org> wrote:
> > > 
> > > > resending to wider list for discussion
> > > > ----- Forwarded message from mark gross <markgross@thengar.org> -----
> > > > 
> > > > Subject: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)
> > > > Date: Tue, 20 Sep 2011 13:33:05 -0700
> > > > From: mark gross <markgross@thengar.org>
> > > > To: linux-pm@lists.linux-foundation.org
> > > > Reply-To: markgross@thegnar.org
> > > > Cc: arve@android.com, markgross@thegnar.org, Alan Stern <stern@rowland.harvard.edu>, amit.kucheria@linaro.org, farrowg@sg.ibm.com, "Rafael J. Wysocki" <rjw@sisk.pl>
> > > > 
> > > > The following patch set implement an (untested) solution to the
> > > > following problems.
> > > > 
> > > > 1) a method for making a system unable to suspend for critical sections
> > > > of time.
> > > 
> > > We already have this.  A properly requested suspend (following wakeup_count
> > > protocol) is unable to complete between wakeup_source_activate() and
> > > wake_source_deactivate() - these delimit the critical sections.
> > > 
> > > What more than this do you need?
> > 
> > sometimes devices that are not wake up sources need critical sections
> > where suspend is a problem.
> 
> I agree with Alan that an example would help here.
> My naive perspective is that any device is either acting on behalf of a
> user-space program, so it should disable suspend, or on behalf of some
> external event, so that event is ipso-facto a wakeup event.

battery changing and usb gadget compilance are the ones I can think of
at the moment.  I guess the FW / BIOS update may be another (but is a
bit weak as an example)

I'm having a hard time thinking of anything else :( 

> 
> > > 
> > > > 
> > > > 2) providing a race free method for the acknowledgment of wake event
> > > > processing before re-entry into suspend can happen.
> > > 
> > > Again, this is a user-space problem.  It is user-space which requests
> > > suspend.  It shouldn't request it until it has checked that there are no wake
> > > events that need processing - and should use the wakeup_count protocol to
> > > avoid races with wakeup events happening after it has checked.
> > 
> > Here you are wrong, or missing the point.  The kernel needs to be
> > notified from user mode that an update event has been consumed by
> > whoever cares about it before the next suspend can happen.  The fact
> > that there are time outs in the existing wake event code points to this
> > shortcoming in the current implementation.
> 
>  ... or I have a different perspective.
> A write to wakeup_count is a notification to the kernel that all wakeup
> events that had commenced prior to that same number being read from
> wakeup_count have been consumed.
> 
> So we already have a mechanism for the notification that you want.

If I understand you correctly; those processes still need an efficient
way of notification that the wake event came in.

> 
> > 
> > I suppose one could rig up the user mode suspend daemon with
> > notification callbacks between event consumers across the user mode
> > stack but its really complex to get it right and forces a solution to a
> > problem better solved in kernel mode be done with hacky user mode
> > gyrations that may ripple wildly across user mode.
> 
> I suspect it is in here that the key to our different perspectives lies.
> I think than any solution must "ripple wildly across user mode" if by that
> you mean that more applications and daemons will need to be power-aware and
> make definitive decisions about when they cannot tolerate suspend.
> Whether those apps and daemons tell the kernel "don't suspend now" or tell
> some user-space daemon "don't suspend now" is fairly irrelevant when
> assessing the total impact on user-space.
> 
> I think a fairly simple protocol involving file locking can be perfectly
> adequate to communicate needs relating to suspend-or-don't-suspend among
> user-space processes.

I'm not sure how the wake event notification would get to the right
process(es) are that need to get the notification in your concept.  But
after the processes that need to consume the event are notified (by
magic???) then some file locking could be done to coordinate between
those and some suspend daemon.


> 
> > 
> > Also it is the kernel that is currently deciding when to unblock the
> > suspend daemon for the next suspend attempt.  Why not build on that and
> > make is so we don't need the time outs?
> 
> Suspend is a joint decision by user-space and kernel-space.  Each part should
> participate according to its expertise.
> The kernel can make use of information generated by drivers in the kernel.
> User-space can consolidate information generated by user-space processes.
> 
> 
> > 
> > > i.e. there is no kernel-space problem to solve here (except for possible
> > > bugs).
> > 
> > Just a race between the kernel allowing a suspend and the user mode code
> > having time to consume the last wake event.
> >
> 
> Providing that the source of the wake event does not deactivate the
> wakeup_source before the event is visible to userspace, this race is easily
> avoided in userspace:

Well this is the crux of the issue.  How are you going to ack the wake
event in a sane way?  Its not quite the same as in interrupt.  knowing
when to ack it takes both user and kernel mode coordination.  I think.

> 
>    - read wakeup_count
suspend daemon blocked here when not ok to suspend.

>    - check all possible wakeup events.
but there is a user mode process that needs to chew on this wake up
event.  It needs to be notified (somehow) then it needs to coordinate
with the suspend daemon before the next suspend happens.

>    - if there were none, write back to wakeup_count and request a suspend.
> 
> This is race-free.
I don't agree.

> 
> If some wakeup_source is deactivated before the event is visible to
> user-space, then that is a bug and should be fixed.
one way is to set up overlapping critical sections from the wake event
up to user mode (we can call the patch "wakelock.patch" ...  I kid...

The wake source is at the very bottom of the kernel mode stack.
Basically an interrupt is the wake event.  How can the ISR possible know
when its ok to deactivate the wake event?  There needs to be a
formalized hand shake.

> If there is some particular case where it is non-trivial to fix that bug,
> then that would certainly be worth exploring in detail.
>

I don't think you accept the idea that there is a notification problem.
Fine, Do you at least agree that my problem statement is approximately
what we thing the original wake lock design attempted to address?

If we can converge on something then maybe we can move forward.  (a bit)


Clearly the use case I'm talking about with out talking about it is the
Android wake lock problem.  I think my problem statement comes pretty
close to what it is trying to address.

I think my implementation concept is semantically close enough that I
think I could hack an android implementation to work with it without
having to change the framework API's and only minor tweaks to a few
services (that consume wake events).  And I think my idea provides a
more correct solution to the basic problem than the original wake lock
design.  i.e.  no over lapping critical sections up and down the stack
or the need for wake_locks with (racy) timeouts.

--mark

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 16095 bytes --]

On Wed, 12 Oct 2011 20:48:05 -0700 mark gross <markgross@thegnar.org> wrote:

> On Sun, Oct 09, 2011 at 09:31:00AM +1100, NeilBrown wrote:
> > On Sat, 8 Oct 2011 11:16:38 -0700 mark gross <markgross@thegnar.org> wrote:
> > 
> > > On Sat, Oct 08, 2011 at 10:14:39PM +1100, NeilBrown wrote:
> > > > On Sun, 2 Oct 2011 09:44:56 -0700 mark gross <markgross@thegnar.org> wrote:
> > > > 
> > > > > resending to wider list for discussion
> > > > > ----- Forwarded message from mark gross <markgross@thengar.org> -----
> > > > > 
> > > > > Subject: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)
> > > > > Date: Tue, 20 Sep 2011 13:33:05 -0700
> > > > > From: mark gross <markgross@thengar.org>
> > > > > To: linux-pm@lists.linux-foundation.org
> > > > > Reply-To: markgross@thegnar.org
> > > > > Cc: arve@android.com, markgross@thegnar.org, Alan Stern <stern@rowland.harvard.edu>, amit.kucheria@linaro.org, farrowg@sg.ibm.com, "Rafael J. Wysocki" <rjw@sisk.pl>
> > > > > 
> > > > > The following patch set implement an (untested) solution to the
> > > > > following problems.
> > > > > 
> > > > > 1) a method for making a system unable to suspend for critical sections
> > > > > of time.
> > > > 
> > > > We already have this.  A properly requested suspend (following wakeup_count
> > > > protocol) is unable to complete between wakeup_source_activate() and
> > > > wake_source_deactivate() - these delimit the critical sections.
> > > > 
> > > > What more than this do you need?
> > > 
> > > sometimes devices that are not wake up sources need critical sections
> > > where suspend is a problem.
> > 
> > I agree with Alan that an example would help here.
> > My naive perspective is that any device is either acting on behalf of a
> > user-space program, so it should disable suspend, or on behalf of some
> > external event, so that event is ipso-facto a wakeup event.
> 
> battery changing and usb gadget compilance are the ones I can think of
> at the moment.  I guess the FW / BIOS update may be another (but is a
> bit weak as an example)
> 
> I'm having a hard time thinking of anything else :( 

These examples are certainly credible.

In general, I think of the kernel as a service provider.  It doesn't do
things on it's own, it just performs tasks on the request of some user-space
process.  In such cases, the user-space process would act to disable suspend
(by telling the suspend daemon not to).
However I accept that there are some activities that do not have a user-space
component.  One that is close to my heart is the NFS server.  The kernel
contains both the support services and the core logic.
In general I think such designs are problematic and best avoided, but I
accept that they are inevitable.
If the monitoring of battery charging and the management of a usb
gadget are being handled entirely in the kernel, then so be it.  The control
logic still has a right to disable suspend, so that has to be in the kernel.

It can do this currently by activating a wakeup_source.  This could be seen as
a slightly ugly interface, because it isn't really a wakeup.  However this is
simply a naming issue.  If we rename it to say_awake_request (or
caffeine_source). which can still be activated or deactivated, then the
ugliness goes away and the functionality remains.

In short: if control logic is in the kernel (which should be avoided but
sometimes is required) then it is reasonable for that control logic to be able
to disable suspend, and can do this today by activating a wakeup_source.


> 
> > 
> > > > 
> > > > > 
> > > > > 2) providing a race free method for the acknowledgment of wake event
> > > > > processing before re-entry into suspend can happen.
> > > > 
> > > > Again, this is a user-space problem.  It is user-space which requests
> > > > suspend.  It shouldn't request it until it has checked that there are no wake
> > > > events that need processing - and should use the wakeup_count protocol to
> > > > avoid races with wakeup events happening after it has checked.
> > > 
> > > Here you are wrong, or missing the point.  The kernel needs to be
> > > notified from user mode that an update event has been consumed by
> > > whoever cares about it before the next suspend can happen.  The fact
> > > that there are time outs in the existing wake event code points to this
> > > shortcoming in the current implementation.
> > 
> >  ... or I have a different perspective.
> > A write to wakeup_count is a notification to the kernel that all wakeup
> > events that had commenced prior to that same number being read from
> > wakeup_count have been consumed.
> > 
> > So we already have a mechanism for the notification that you want.
> 
> If I understand you correctly; those processes still need an efficient
> way of notification that the wake event came in.

[[while re-reading my reply, I realise that I might have misunderstood the
above. Are you talking about an efficient notification to the process that
the event has happened, or from the process that the event has been consumed?
I assumed the former and it still seems the best way to read that sentence.
But if you mean the later... I think I've covered that too, but it makes the
following paragraph irrelevant.  In that case, ignore it please]]

Is that a big ask?
If the wake event is an RTC alarm, then the process will be using
select/poll/epoll/whatever on an open fd on /dev/rtc and can easily check if
there are any pending events.
If the wake event is a button press, or lid opening, it will probably come in
through the input subsystem and again 'poll' will do the trick.
If it is a usb device sending a wakeup, it will probably come through input
too.
A character arriving on a UART is equally detectable with poll.

Are there important wakeup events which will not cause a 'poll' to wake up?
or is poll not "efficient" enough?



> 
> > 
> > > 
> > > I suppose one could rig up the user mode suspend daemon with
> > > notification callbacks between event consumers across the user mode
> > > stack but its really complex to get it right and forces a solution to a
> > > problem better solved in kernel mode be done with hacky user mode
> > > gyrations that may ripple wildly across user mode.
> > 
> > I suspect it is in here that the key to our different perspectives lies.
> > I think than any solution must "ripple wildly across user mode" if by that
> > you mean that more applications and daemons will need to be power-aware and
> > make definitive decisions about when they cannot tolerate suspend.
> > Whether those apps and daemons tell the kernel "don't suspend now" or tell
> > some user-space daemon "don't suspend now" is fairly irrelevant when
> > assessing the total impact on user-space.
> > 
> > I think a fairly simple protocol involving file locking can be perfectly
> > adequate to communicate needs relating to suspend-or-don't-suspend among
> > user-space processes.
> 
> I'm not sure how the wake event notification would get to the right
> process(es) are that need to get the notification in your concept.  But
> after the processes that need to consume the event are notified (by
> magic???) then some file locking could be done to coordinate between
> those and some suspend daemon.

Not magic.  epoll.
Any process that wants to handle wakeup events just needs to keep an fd open
on the source of the event (and negotiate with the suspend daemon at times).


> 
> 
> > 
> > > 
> > > Also it is the kernel that is currently deciding when to unblock the
> > > suspend daemon for the next suspend attempt.  Why not build on that and
> > > make is so we don't need the time outs?
> > 
> > Suspend is a joint decision by user-space and kernel-space.  Each part should
> > participate according to its expertise.
> > The kernel can make use of information generated by drivers in the kernel.
> > User-space can consolidate information generated by user-space processes.
> > 
> > 
> > > 
> > > > i.e. there is no kernel-space problem to solve here (except for possible
> > > > bugs).
> > > 
> > > Just a race between the kernel allowing a suspend and the user mode code
> > > having time to consume the last wake event.
> > >
> > 
> > Providing that the source of the wake event does not deactivate the
> > wakeup_source before the event is visible to userspace, this race is easily
> > avoided in userspace:
> 
> Well this is the crux of the issue.  How are you going to ack the wake
> event in a sane way?  Its not quite the same as in interrupt.  knowing
> when to ack it takes both user and kernel mode coordination.  I think.

This is the beauty that of the wakeup_count approach - you don't need to ack
the event.  You don't even need to consume the event (though it would be dumb
not to).
All a user-space process needs to do is tell the suspend daemon "I've done all
that I want to do".  It first checks if there is anything that it wants to do
of course.

> 
> > 
> >    - read wakeup_count
> suspend daemon blocked here when not ok to suspend.
> 
> >    - check all possible wakeup events.
> but there is a user mode process that needs to chew on this wake up
> event.  It needs to be notified (somehow) then it needs to coordinate
> with the suspend daemon before the next suspend happens.
> 

This is "simply" a bit of IPC.
Any program that wants to handle wake events needs to register with the
suspend daemon.  The daemon says "Hey everyone, I'm about to suspend".
Each registered program then needs to say either "OK", or "Wait, don't
suspend now".
If everyone says "OK", it tries to suspend.  If anyone says "No, wait", it
waits until they all say "OK, I'm done" at which point it starts at the top
of the loop again.

This is just a software engineering issue though.

An alternate approach would be for each process to give (via unix domain
sockets) an fd to the suspend daemon.  The suspend daemon just needs to check
(using e.g. select) that none of the fds are ready.  If they are, it can enter
a more heavy-weight conversation with the process in question.  If not, it
can safely suspend.



> >    - if there were none, write back to wakeup_count and request a suspend.
> > 
> > This is race-free.
> I don't agree.

Can you describe a race?
Here is the sequence as I see it.

0: some user-space process is blocking suspend by telling the suspend daemon
   not to suspend.  There are no pending kernel wakeup events.
   All processes that handle wakeup events are registered with the daemon.
1: last blocking user-space  process releases its "don't suspend" lock.
2: suspend-daemon decides to initiate suspend.
3: suspend_daemon reads wakeup_count and remembers number.
4: suspend daemon tells all registered clients that suspend is imminent.
5: each client executes 'poll' or 'select' or whatever and discovers that
   there are no events.
6: each client tells daemon that it is OK to suspend
7: when all votes are in, suspend daemon checks that no process is requesting
   that suspend be blocked.
8: if that succeeds, suspend daemon writes the number back to wakeup_count
9: if that succeeds, suspend daemon daemon writes 'mem' to 'state'.
10: goto 1

If a wake_event happens after 3 and before 8, the write at 8 will fail.
If a wake_event happens after 8, and before 9, the suspend will abort.
If a wake_event happens after 9, the suspend will resume
If a wake_event happens before 3, one of the processes will get an event
notification from select or poll or whatever, and will ask the suspend
daemon not to suspend just now and this will be noticed at 7, so 8 and 9
will be skipped and we go straight to 10.

No race.
 
> > 
> > If some wakeup_source is deactivated before the event is visible to
> > user-space, then that is a bug and should be fixed.
> one way is to set up overlapping critical sections from the wake event
> up to user mode (we can call the patch "wakelock.patch" ...  I kid...
> 
> The wake source is at the very bottom of the kernel mode stack.
> Basically an interrupt is the wake event.  How can the ISR possible know
> when its ok to deactivate the wake event?  There needs to be a
> formalized hand shake.

Yes.  But the handshake only needs to get as far as being visible to
user-space, the event doesn't need to be consumed by user-space.

See John Stultz's patches to kernel.org
[PATCH 3/6] [RFC] rtc: rtc-cmos: Add pm_stay_awake/pm_relax calls around IRQ
[PATCH 4/6] [RFC] rtc: interface: Add pm_stay_awake/pm_relax chaining rtc workqueue processing

They add the required handshake to rtc.
The ISR typically activates the wakeup event and queues something to a
kernel thread.  That thread then does whatever is needed, makes sure the
event is visible to user-space, and then deactivates the wakeup event.

(I'm not certain John's patches get it all right in every case, I don't
know the code well enough, but it looks like the right sort of thing).

Once the event is visible to user-space, the handshake with the suspend
daemon ensure that the event gets consumed if anyone is interested in it.


> 
> > If there is some particular case where it is non-trivial to fix that bug,
> > then that would certainly be worth exploring in detail.
> >
> 
> I don't think you accept the idea that there is a notification problem.

Nope, but I'm keen for you to convince me.  Identify a wakeup event that
cannot be made visible to poll (or to user-space by some other
mechanism) before the wakeup_source needs to be deactivated.  Or if I've
misunderstood what sort of notification is problematic, help me understand.


> Fine, Do you at least agree that my problem statement is approximately
> what we thing the original wake lock design attempted to address?

If by your "problem statement" you mean this:

> 1) a method for making a system unable to suspend for critical sections
> of time.
> 
> 2) providing a race free method for the acknowledgment of wake event
> processing before re-entry into suspend can happen.

Then yes I agree, though I would state it with a slightly different emphasis:

1) a method for ensuring a system does not suspend during critical sections
   of time 
2) providing a race free method for the handling of wake events before
   re-entry into suspend happens.

Your text sounds the like the kernel is forcing user-space to do the required
thing.  My text is meant to only say that the kernel allows user-space to do
the required thing.
Maybe not an important difference.


> 
> If we can converge on something then maybe we can move forward.  (a bit)
> 
> 
> Clearly the use case I'm talking about with out talking about it is the
> Android wake lock problem.  I think my problem statement comes pretty
> close to what it is trying to address.
> 
> I think my implementation concept is semantically close enough that I
> think I could hack an android implementation to work with it without
> having to change the framework API's and only minor tweaks to a few
> services (that consume wake events).  And I think my idea provides a
> more correct solution to the basic problem than the original wake lock
> design.  i.e.  no over lapping critical sections up and down the stack
> or the need for wake_locks with (racy) timeouts.

Your code essentially creates functionality in the kernel to match the IPC
that I suggest doing in user-space  ... and we really don't need the kernel to
provide yet another IPC mechanism.

I'm quite sure you or I could write code in user-space to work with today's
kernel design to implement Android's wake-lock API.
(there are bugs that need to be fixed of course, like the fact that the RTC
wakeup doesn't activate a wake_source ... unless John's patch has gone in).

( I would doing using lock files and change notifications: 
  https://lwn.net/Articles/460902/  )


NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[Alan Stern]

On Wed, 12 Oct 2011, mark gross wrote:

> > > > sometimes devices that are not wake up sources need critical sections
> > > > where suspend is a problem.
> > 
> > Mark, can you give some examples?  This isn't the same as the firmware 
> > update thing, is it?  That deserves to be handled by a completely 
> > separate mechanism.
> 
> The biggest example I know of is any usb gadget implementation while
> connected to a USB host.  If the device is connected to the USB bus then
> it needs to not suspend because it cannot respond to the USB commands
> for bus power.  i.e. if its connected to a laptop and the suspend the
> laptop the gadget needs to handle the USB protocol packets.
> 
> Also, when charging battery under OS control we don't want to suspend
> because it will be hard to respond to thermal events  or changing
> battery conditions if the system is in suspend.

This is like the firmware update problem -- it has nothing to do with 
wakeups.  It could be handled very easily either by abusing a wakeup 
source as Neil suggested or by adding a different mechanism for 
preventing system sleeps.

> > Although many of these problems could be solved by adopting a suitable
> > protocol in userspace, currently there is no such protocol.  Even if
> > one did exist, the process of getting all the relevant programs to
> > adopt it would take quite a while.  This is a case where a problem can
> > be solved either in the kernel or in userspace, and the in-kernel
> > solution may be simpler.
> 
> I do think a better solution would involve some kernel coordination.
> 
> I don't want to split hairs on if its 100% possible to implement a
> correct solution only in user mode.  but, I think there are corner cases
> that may be hard or impossible to get right with only user mode.  See
> gadget issue above.  Not all gadgets have a user mode daemon backing up
> the kernel part do they?

I don't know.  But if they don't have some sort of userspace component
for power management then they never go into suspend, because the
kernel doesn't initiate suspends by itself.

Alan Stern

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[Alan Stern]

On Thu, 13 Oct 2011, NeilBrown wrote:

> Nope, but I'm keen for you to convince me.  Identify a wakeup event that
> cannot be made visible to poll (or to user-space by some other
> mechanism) before the wakeup_source needs to be deactivated.  Or if I've
> misunderstood what sort of notification is problematic, help me understand.

Here's an example (just for kicks, not completely relevant to your
discussion): A USB keyboard key release.  Unlike key presses, key
releases need not generate input events.  If no processes are
monitoring the raw keyboard event queue then the release is not visible
to userspace at all, hence not visible before the wakeup_source needs
to be deactivated.

Alan Stern

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[mark gross]

On Thu, Oct 13, 2011 at 11:06:35AM -0400, Alan Stern wrote:
> On Wed, 12 Oct 2011, mark gross wrote:
> 
> > > > > sometimes devices that are not wake up sources need critical sections
> > > > > where suspend is a problem.
> > > 
> > > Mark, can you give some examples?  This isn't the same as the firmware 
> > > update thing, is it?  That deserves to be handled by a completely 
> > > separate mechanism.
> > 
> > The biggest example I know of is any usb gadget implementation while
> > connected to a USB host.  If the device is connected to the USB bus then
> > it needs to not suspend because it cannot respond to the USB commands
> > for bus power.  i.e. if its connected to a laptop and the suspend the
> > laptop the gadget needs to handle the USB protocol packets.
> > 
> > Also, when charging battery under OS control we don't want to suspend
> > because it will be hard to respond to thermal events  or changing
> > battery conditions if the system is in suspend.
> 
> This is like the firmware update problem -- it has nothing to do with 
> wakeups.  It could be handled very easily either by abusing a wakeup 
> source as Neil suggested or by adding a different mechanism for 
> preventing system sleeps.

yes.  This is just another mechanism to prevent sleeps.

> > > Although many of these problems could be solved by adopting a suitable
> > > protocol in userspace, currently there is no such protocol.  Even if
> > > one did exist, the process of getting all the relevant programs to
> > > adopt it would take quite a while.  This is a case where a problem can
> > > be solved either in the kernel or in userspace, and the in-kernel
> > > solution may be simpler.
> > 
> > I do think a better solution would involve some kernel coordination.
> > 
> > I don't want to split hairs on if its 100% possible to implement a
> > correct solution only in user mode.  but, I think there are corner cases
> > that may be hard or impossible to get right with only user mode.  See
> > gadget issue above.  Not all gadgets have a user mode daemon backing up
> > the kernel part do they?
> 
> I don't know.  But if they don't have some sort of userspace component
> for power management then they never go into suspend, because the
> kernel doesn't initiate suspends by itself.

yes, but the kernel can / does block user initiated suspends from
various user mode initiators.  All this is for is to block suspend entry
until the driver is ok with it.  this patch is nothing more than that
really.

--mark

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[mark gross]

On Thu, Oct 13, 2011 at 04:35:26PM +1100, NeilBrown wrote:
> On Wed, 12 Oct 2011 20:48:05 -0700 mark gross <markgross@thegnar.org> wrote:
> 
> > On Sun, Oct 09, 2011 at 09:31:00AM +1100, NeilBrown wrote:
> > > On Sat, 8 Oct 2011 11:16:38 -0700 mark gross <markgross@thegnar.org> wrote:
> > > 
> > > > On Sat, Oct 08, 2011 at 10:14:39PM +1100, NeilBrown wrote:
> > > > > On Sun, 2 Oct 2011 09:44:56 -0700 mark gross <markgross@thegnar.org> wrote:
> > > > > 
> > > > > > resending to wider list for discussion
> > > > > > ----- Forwarded message from mark gross <markgross@thengar.org> -----
> > > > > > 
> > > > > > Subject: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)
> > > > > > Date: Tue, 20 Sep 2011 13:33:05 -0700
> > > > > > From: mark gross <markgross@thengar.org>
> > > > > > To: linux-pm@lists.linux-foundation.org
> > > > > > Reply-To: markgross@thegnar.org
> > > > > > Cc: arve@android.com, markgross@thegnar.org, Alan Stern <stern@rowland.harvard.edu>, amit.kucheria@linaro.org, farrowg@sg.ibm.com, "Rafael J. Wysocki" <rjw@sisk.pl>
> > > > > > 
> > > > > > The following patch set implement an (untested) solution to the
> > > > > > following problems.
> > > > > > 
> > > > > > 1) a method for making a system unable to suspend for critical sections
> > > > > > of time.
> > > > > 
> > > > > We already have this.  A properly requested suspend (following wakeup_count
> > > > > protocol) is unable to complete between wakeup_source_activate() and
> > > > > wake_source_deactivate() - these delimit the critical sections.
> > > > > 
> > > > > What more than this do you need?
> > > > 
> > > > sometimes devices that are not wake up sources need critical sections
> > > > where suspend is a problem.
> > > 
> > > I agree with Alan that an example would help here.
> > > My naive perspective is that any device is either acting on behalf of a
> > > user-space program, so it should disable suspend, or on behalf of some
> > > external event, so that event is ipso-facto a wakeup event.
> > 
> > battery changing and usb gadget compilance are the ones I can think of
> > at the moment.  I guess the FW / BIOS update may be another (but is a
> > bit weak as an example)
> > 
> > I'm having a hard time thinking of anything else :( 
> 
> These examples are certainly credible.
> 
> In general, I think of the kernel as a service provider.  It doesn't do
> things on it's own, it just performs tasks on the request of some user-space
> process.  In such cases, the user-space process would act to disable suspend
> (by telling the suspend daemon not to).
> However I accept that there are some activities that do not have a user-space
> component.  One that is close to my heart is the NFS server.  The kernel
> contains both the support services and the core logic.
> In general I think such designs are problematic and best avoided, but I
> accept that they are inevitable.
> If the monitoring of battery charging and the management of a usb
> gadget are being handled entirely in the kernel, then so be it.  The control
> logic still has a right to disable suspend, so that has to be in the kernel.
> 
> It can do this currently by activating a wakeup_source.  This could be seen as
> a slightly ugly interface, because it isn't really a wakeup.  However this is
> simply a naming issue.  If we rename it to say_awake_request (or
> caffeine_source). which can still be activated or deactivated, then the
> ugliness goes away and the functionality remains.
> 
> In short: if control logic is in the kernel (which should be avoided but
> sometimes is required) then it is reasonable for that control logic to be able
> to disable suspend, and can do this today by activating a wakeup_source.

oops you are correct (I think.)  This mechanism could be used to block
suspend just as well as my pm_qos hack.  

I think I'll drop my pm_qos patch for suspend blocking.

We still have to handle the notification issue.


> 
> > 
> > > 
> > > > > 
> > > > > > 
> > > > > > 2) providing a race free method for the acknowledgment of wake event
> > > > > > processing before re-entry into suspend can happen.
> > > > > 
> > > > > Again, this is a user-space problem.  It is user-space which requests
> > > > > suspend.  It shouldn't request it until it has checked that there are no wake
> > > > > events that need processing - and should use the wakeup_count protocol to
> > > > > avoid races with wakeup events happening after it has checked.
> > > > 
> > > > Here you are wrong, or missing the point.  The kernel needs to be
> > > > notified from user mode that an update event has been consumed by
> > > > whoever cares about it before the next suspend can happen.  The fact
> > > > that there are time outs in the existing wake event code points to this
> > > > shortcoming in the current implementation.
> > > 
> > >  ... or I have a different perspective.
> > > A write to wakeup_count is a notification to the kernel that all wakeup
> > > events that had commenced prior to that same number being read from
> > > wakeup_count have been consumed.
> > > 
> > > So we already have a mechanism for the notification that you want.
> > 
> > If I understand you correctly; those processes still need an efficient
> > way of notification that the wake event came in.
> 
> [[while re-reading my reply, I realise that I might have misunderstood the
> above. Are you talking about an efficient notification to the process that
> the event has happened, or from the process that the event has been consumed?

Both actually.  There can be processes that need to be notified of wake
events and potentially other processes that consume them.  (think network
packet wake ups and multiple processes that consume different packets)

Those processes need some sort of notification mechanism to ACK the wake
so the next suspend could be attempted by the user mode "suspend daemon"

> I assumed the former and it still seems the best way to read that sentence.
> But if you mean the later... I think I've covered that too, but it makes the
> following paragraph irrelevant.  In that case, ignore it please]]
> 
> Is that a big ask?
> If the wake event is an RTC alarm, then the process will be using
> select/poll/epoll/whatever on an open fd on /dev/rtc and can easily check if
> there are any pending events.
> If the wake event is a button press, or lid opening, it will probably come in
> through the input subsystem and again 'poll' will do the trick.
> If it is a usb device sending a wakeup, it will probably come through input
> too.
> A character arriving on a UART is equally detectable with poll.
> 
> Are there important wakeup events which will not cause a 'poll' to wake up?
> or is poll not "efficient" enough?

I think there are usability and portability issues with using poll across
the random device node, but yes.  Poll is efficient enough to wake the
different processes that would need notification of a wake event.

The problem is that the suspend daemon also gets woken up and starts a
race with the other wake event consumers for the next suspend. 

I acknowledge that we could have a dmesg or socket handshake between the
suspend daemon and the event consumers such that the race could be
handled in user mode.  But, I think it would be nicer all around to have
a consolidated interface that handles the usability problem and
simplifies the complexity of the user mode implementation.



> 
> 
> 
> > 
> > > 
> > > > 
> > > > I suppose one could rig up the user mode suspend daemon with
> > > > notification callbacks between event consumers across the user mode
> > > > stack but its really complex to get it right and forces a solution to a
> > > > problem better solved in kernel mode be done with hacky user mode
> > > > gyrations that may ripple wildly across user mode.
> > > 
> > > I suspect it is in here that the key to our different perspectives lies.
> > > I think than any solution must "ripple wildly across user mode" if by that
> > > you mean that more applications and daemons will need to be power-aware and
> > > make definitive decisions about when they cannot tolerate suspend.
> > > Whether those apps and daemons tell the kernel "don't suspend now" or tell
> > > some user-space daemon "don't suspend now" is fairly irrelevant when
> > > assessing the total impact on user-space.
> > > 
> > > I think a fairly simple protocol involving file locking can be perfectly
> > > adequate to communicate needs relating to suspend-or-don't-suspend among
> > > user-space processes.
> > 
> > I'm not sure how the wake event notification would get to the right
> > process(es) are that need to get the notification in your concept.  But
> > after the processes that need to consume the event are notified (by
> > magic???) then some file locking could be done to coordinate between
> > those and some suspend daemon.
> 
> Not magic.  epoll.
> Any process that wants to handle wakeup events just needs to keep an fd open
> on the source of the event (and negotiate with the suspend daemon at times).
> 
> 
> > 
> > 
> > > 
> > > > 
> > > > Also it is the kernel that is currently deciding when to unblock the
> > > > suspend daemon for the next suspend attempt.  Why not build on that and
> > > > make is so we don't need the time outs?
> > > 
> > > Suspend is a joint decision by user-space and kernel-space.  Each part should
> > > participate according to its expertise.
> > > The kernel can make use of information generated by drivers in the kernel.
> > > User-space can consolidate information generated by user-space processes.
> > > 
> > > 
> > > > 
> > > > > i.e. there is no kernel-space problem to solve here (except for possible
> > > > > bugs).
> > > > 
> > > > Just a race between the kernel allowing a suspend and the user mode code
> > > > having time to consume the last wake event.
> > > >
> > > 
> > > Providing that the source of the wake event does not deactivate the
> > > wakeup_source before the event is visible to userspace, this race is easily
> > > avoided in userspace:
> > 
> > Well this is the crux of the issue.  How are you going to ack the wake
> > event in a sane way?  Its not quite the same as in interrupt.  knowing
> > when to ack it takes both user and kernel mode coordination.  I think.
> 
> This is the beauty that of the wakeup_count approach - you don't need to ack
> the event.  You don't even need to consume the event (though it would be dumb
> not to).
> All a user-space process needs to do is tell the suspend daemon "I've done all
> that I want to do".  It first checks if there is anything that it wants to do
> of course.
> 
> > 
> > > 
> > >    - read wakeup_count
> > suspend daemon blocked here when not ok to suspend.
> > 
> > >    - check all possible wakeup events.
> > but there is a user mode process that needs to chew on this wake up
> > event.  It needs to be notified (somehow) then it needs to coordinate
> > with the suspend daemon before the next suspend happens.
> > 
> 
> This is "simply" a bit of IPC.
> Any program that wants to handle wake events needs to register with the
> suspend daemon.  The daemon says "Hey everyone, I'm about to suspend".
> Each registered program then needs to say either "OK", or "Wait, don't
> suspend now".
> If everyone says "OK", it tries to suspend.  If anyone says "No, wait", it
> waits until they all say "OK, I'm done" at which point it starts at the top
> of the loop again.
> 
> This is just a software engineering issue though.
> 
> An alternate approach would be for each process to give (via unix domain
> sockets) an fd to the suspend daemon.  The suspend daemon just needs to check
> (using e.g. select) that none of the fds are ready.  If they are, it can enter
> a more heavy-weight conversation with the process in question.  If not, it
> can safely suspend.

yes.  I think you are correct.  I need to go back and re-evaluate if the
user mode notification implementation can be more simple than what I was
thinking.

At this point in the discussion I think we agree the notification
handling can be done in user mode.  my patch only attempts to
consolidate the notification and coordination through the wake event
sysfs node.

I cannot say if its worth putting this in the kernel until I attempt a
prototype of doing it in user mode.

> 
> 
> > >    - if there were none, write back to wakeup_count and request a suspend.
> > > 
> > > This is race-free.
> > I don't agree.
> 
> Can you describe a race?
> Here is the sequence as I see it.
> 
> 0: some user-space process is blocking suspend by telling the suspend daemon
>    not to suspend.  There are no pending kernel wakeup events.
>    All processes that handle wakeup events are registered with the daemon.
> 1: last blocking user-space  process releases its "don't suspend" lock.
> 2: suspend-daemon decides to initiate suspend.
> 3: suspend_daemon reads wakeup_count and remembers number.
> 4: suspend daemon tells all registered clients that suspend is imminent.
> 5: each client executes 'poll' or 'select' or whatever and discovers that
>    there are no events.
> 6: each client tells daemon that it is OK to suspend
> 7: when all votes are in, suspend daemon checks that no process is requesting
>    that suspend be blocked.
> 8: if that succeeds, suspend daemon writes the number back to wakeup_count
> 9: if that succeeds, suspend daemon daemon writes 'mem' to 'state'.
> 10: goto 1
> 
> If a wake_event happens after 3 and before 8, the write at 8 will fail.
> If a wake_event happens after 8, and before 9, the suspend will abort.
> If a wake_event happens after 9, the suspend will resume
> If a wake_event happens before 3, one of the processes will get an event
> notification from select or poll or whatever, and will ask the suspend
> daemon not to suspend just now and this will be noticed at 7, so 8 and 9
> will be skipped and we go straight to 10.
> 
> No race.

With the suspend daemon designed as above I see no race either.  I was
thinking of a more trivial suspend daemon design and trying to fix it up
in the kernel.


>  
> > > 
> > > If some wakeup_source is deactivated before the event is visible to
> > > user-space, then that is a bug and should be fixed.
> > one way is to set up overlapping critical sections from the wake event
> > up to user mode (we can call the patch "wakelock.patch" ...  I kid...
> > 
> > The wake source is at the very bottom of the kernel mode stack.
> > Basically an interrupt is the wake event.  How can the ISR possible know
> > when its ok to deactivate the wake event?  There needs to be a
> > formalized hand shake.
> 
> Yes.  But the handshake only needs to get as far as being visible to
> user-space, the event doesn't need to be consumed by user-space.
> 
> See John Stultz's patches to kernel.org
> [PATCH 3/6] [RFC] rtc: rtc-cmos: Add pm_stay_awake/pm_relax calls around IRQ
> [PATCH 4/6] [RFC] rtc: interface: Add pm_stay_awake/pm_relax chaining rtc workqueue processing
> 
> They add the required handshake to rtc.
> The ISR typically activates the wakeup event and queues something to a
> kernel thread.  That thread then does whatever is needed, makes sure the
> event is visible to user-space, and then deactivates the wakeup event.
> 
> (I'm not certain John's patches get it all right in every case, I don't
> know the code well enough, but it looks like the right sort of thing).
> 
> Once the event is visible to user-space, the handshake with the suspend
> daemon ensure that the event gets consumed if anyone is interested in it.

Thanks! I'll look at those.

> 
> > 
> > > If there is some particular case where it is non-trivial to fix that bug,
> > > then that would certainly be worth exploring in detail.
> > >
> > 
> > I don't think you accept the idea that there is a notification problem.
> 
> Nope, but I'm keen for you to convince me.  Identify a wakeup event that
> cannot be made visible to poll (or to user-space by some other
> mechanism) before the wakeup_source needs to be deactivated.  Or if I've
> misunderstood what sort of notification is problematic, help me understand.

I think you have convinced me to give moving the notification burden
into the suspend daemon a try before pushing my patch any further.

i.e. I think my patch is unnecessary now.

> 
> > Fine, Do you at least agree that my problem statement is approximately
> > what we thing the original wake lock design attempted to address?
> 
> If by your "problem statement" you mean this:
> 
> > 1) a method for making a system unable to suspend for critical sections
> > of time.
> > 
> > 2) providing a race free method for the acknowledgment of wake event
> > processing before re-entry into suspend can happen.
> 
> Then yes I agree, though I would state it with a slightly different emphasis:
> 
> 1) a method for ensuring a system does not suspend during critical sections
>    of time 
> 2) providing a race free method for the handling of wake events before
>    re-entry into suspend happens.
> 
> Your text sounds the like the kernel is forcing user-space to do the required
> thing.  My text is meant to only say that the kernel allows user-space to do
> the required thing.
> Maybe not an important difference.
> 
> 
> > 
> > If we can converge on something then maybe we can move forward.  (a bit)
> > 
> > 
> > Clearly the use case I'm talking about with out talking about it is the
> > Android wake lock problem.  I think my problem statement comes pretty
> > close to what it is trying to address.
> > 
> > I think my implementation concept is semantically close enough that I
> > think I could hack an android implementation to work with it without
> > having to change the framework API's and only minor tweaks to a few
> > services (that consume wake events).  And I think my idea provides a
> > more correct solution to the basic problem than the original wake lock
> > design.  i.e.  no over lapping critical sections up and down the stack
> > or the need for wake_locks with (racy) timeouts.
> 
> Your code essentially creates functionality in the kernel to match the IPC
> that I suggest doing in user-space  ... and we really don't need the kernel to
> provide yet another IPC mechanism.
> 
> I'm quite sure you or I could write code in user-space to work with today's
> kernel design to implement Android's wake-lock API.
> (there are bugs that need to be fixed of course, like the fact that the RTC
> wakeup doesn't activate a wake_source ... unless John's patch has gone in).
> 
> ( I would doing using lock files and change notifications: 
>   https://lwn.net/Articles/460902/  )

Thanks for helping me with this.  The next step is to write some user
mode code.

I drop my patches at this time.

--mark

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

On Thu, 13 Oct 2011 11:16:23 -0400 (EDT) Alan Stern
<stern@rowland.harvard.edu> wrote:

> On Thu, 13 Oct 2011, NeilBrown wrote:
> 
> > Nope, but I'm keen for you to convince me.  Identify a wakeup event that
> > cannot be made visible to poll (or to user-space by some other
> > mechanism) before the wakeup_source needs to be deactivated.  Or if I've
> > misunderstood what sort of notification is problematic, help me understand.
> 
> Here's an example (just for kicks, not completely relevant to your
> discussion): A USB keyboard key release.  Unlike key presses, key
> releases need not generate input events.  If no processes are
> monitoring the raw keyboard event queue then the release is not visible
> to userspace at all, hence not visible before the wakeup_source needs
> to be deactivated.
> 
> Alan Stern

As you say, not completely relevant.

If a tree falls in a forest with no one to here, does it make a sound?

similarly if an event happens that no-one is looking for, is it visible?
It doesn't really matter.

So at most this is a case of "is not made visible" rather than "cannot be
made visible".

The key-release just needs to clear the "key is pressed" state so that
auto-repeat stops and if it was a modifier, the modification is discarded.
That is all trivially done in some kernel driver while the wakeup_source is
active.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[mark gross]

On Fri, Oct 14, 2011 at 07:01:16AM -0700, mark gross wrote:
> On Thu, Oct 13, 2011 at 04:35:26PM +1100, NeilBrown wrote:
> > On Wed, 12 Oct 2011 20:48:05 -0700 mark gross <markgross@thegnar.org> wrote:
> >

snip

> > Can you describe a race?
> > Here is the sequence as I see it.
> > 
> > 0: some user-space process is blocking suspend by telling the suspend daemon
> >    not to suspend.  There are no pending kernel wakeup events.
> >    All processes that handle wakeup events are registered with the daemon.
> > 1: last blocking user-space  process releases its "don't suspend" lock.
> > 2: suspend-daemon decides to initiate suspend.
> > 3: suspend_daemon reads wakeup_count and remembers number.
> > 4: suspend daemon tells all registered clients that suspend is imminent.
> > 5: each client executes 'poll' or 'select' or whatever and discovers that
> >    there are no events.
> > 6: each client tells daemon that it is OK to suspend
> > 7: when all votes are in, suspend daemon checks that no process is requesting
> >    that suspend be blocked.
> > 8: if that succeeds, suspend daemon writes the number back to wakeup_count
> > 9: if that succeeds, suspend daemon daemon writes 'mem' to 'state'.
> > 10: goto 1
> > 
> > If a wake_event happens after 3 and before 8, the write at 8 will fail.
> > If a wake_event happens after 8, and before 9, the suspend will abort.
> > If a wake_event happens after 9, the suspend will resume
> > If a wake_event happens before 3, one of the processes will get an event
> > notification from select or poll or whatever, and will ask the suspend
> > daemon not to suspend just now and this will be noticed at 7, so 8 and 9
> > will be skipped and we go straight to 10.
> > 
> > No race.
> 
> With the suspend daemon designed as above I see no race either.  I was
> thinking of a more trivial suspend daemon design and trying to fix it up
> in the kernel.
>

After responding to this yesterday I still feel I'm missing something
with respect to wake event acknowledgment.  After a wake source becomes
active how does it know its ok to become deactivate at the driver level?

The race I'm now worried about is say a button press going up through
the event layers.  The event manager process, or X, (registered with the
suspend daemon) is in a race to get the event and the socket based
notification handshake described above.

i.e. how is the key event delivery vrs unblocking of the suspend-daemon
read of wakeup_count in step 3 race dealt with?

Sorry for getting confused and flip flopping on you.  This is pretty
subtle stuff for me.

--mark

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[Alan Stern]

On Sat, 15 Oct 2011, NeilBrown wrote:

> On Thu, 13 Oct 2011 11:16:23 -0400 (EDT) Alan Stern
> <stern@rowland.harvard.edu> wrote:
> 
> > On Thu, 13 Oct 2011, NeilBrown wrote:
> > 
> > > Nope, but I'm keen for you to convince me.  Identify a wakeup event that
> > > cannot be made visible to poll (or to user-space by some other
> > > mechanism) before the wakeup_source needs to be deactivated.  Or if I've
> > > misunderstood what sort of notification is problematic, help me understand.
> > 
> > Here's an example (just for kicks, not completely relevant to your
> > discussion): A USB keyboard key release.  Unlike key presses, key
> > releases need not generate input events.  If no processes are
> > monitoring the raw keyboard event queue then the release is not visible
> > to userspace at all, hence not visible before the wakeup_source needs
> > to be deactivated.
> > 
> > Alan Stern
> 
> As you say, not completely relevant.
> 
> If a tree falls in a forest with no one to here, does it make a sound?
> 
> similarly if an event happens that no-one is looking for, is it visible?
> It doesn't really matter.

That's a different question, but I'll answer it anyway: Yes, it does
matter.  If the kernel is unable to _know_ that nobody is looking for
an event, it has to _assume_ that somebody is.  Then what should happen 
if it turns out that nobody really is looking for it?

> So at most this is a case of "is not made visible" rather than "cannot be
> made visible".

In this case it's the same thing.  How can a key release be made 
visible?

> The key-release just needs to clear the "key is pressed" state so that
> auto-repeat stops and if it was a modifier, the modification is discarded.
> That is all trivially done in some kernel driver while the wakeup_source is
> active.

In other words, if the event is discarded from within the kernel then 
the wakeup_source can be deactivated at that time.  That's fine -- but 
it indicates that your original request above was phrased wrongly.  You 
should have asked for an example of a wakeup_source which the kernel 
must not deactivate without a userspace handshake, but which cannot be 
made visible by poll or some other similar mechanism.

Alan Stern

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4153 bytes --]

On Sat, 15 Oct 2011 07:05:37 -0700 mark gross <markgross@thegnar.org> wrote:

> On Fri, Oct 14, 2011 at 07:01:16AM -0700, mark gross wrote:
> > On Thu, Oct 13, 2011 at 04:35:26PM +1100, NeilBrown wrote:
> > > On Wed, 12 Oct 2011 20:48:05 -0700 mark gross <markgross@thegnar.org> wrote:
> > >
> 
> snip
> 
> > > Can you describe a race?
> > > Here is the sequence as I see it.
> > > 
> > > 0: some user-space process is blocking suspend by telling the suspend daemon
> > >    not to suspend.  There are no pending kernel wakeup events.
> > >    All processes that handle wakeup events are registered with the daemon.
> > > 1: last blocking user-space  process releases its "don't suspend" lock.
> > > 2: suspend-daemon decides to initiate suspend.
> > > 3: suspend_daemon reads wakeup_count and remembers number.
> > > 4: suspend daemon tells all registered clients that suspend is imminent.
> > > 5: each client executes 'poll' or 'select' or whatever and discovers that
> > >    there are no events.
> > > 6: each client tells daemon that it is OK to suspend
> > > 7: when all votes are in, suspend daemon checks that no process is requesting
> > >    that suspend be blocked.
> > > 8: if that succeeds, suspend daemon writes the number back to wakeup_count
> > > 9: if that succeeds, suspend daemon daemon writes 'mem' to 'state'.
> > > 10: goto 1
> > > 
> > > If a wake_event happens after 3 and before 8, the write at 8 will fail.
> > > If a wake_event happens after 8, and before 9, the suspend will abort.
> > > If a wake_event happens after 9, the suspend will resume
> > > If a wake_event happens before 3, one of the processes will get an event
> > > notification from select or poll or whatever, and will ask the suspend
> > > daemon not to suspend just now and this will be noticed at 7, so 8 and 9
> > > will be skipped and we go straight to 10.
> > > 
> > > No race.
> > 
> > With the suspend daemon designed as above I see no race either.  I was
> > thinking of a more trivial suspend daemon design and trying to fix it up
> > in the kernel.
> >
> 
> After responding to this yesterday I still feel I'm missing something
> with respect to wake event acknowledgment.  After a wake source becomes
> active how does it know its ok to become deactivate at the driver level?
> 
> The race I'm now worried about is say a button press going up through
> the event layers.  The event manager process, or X, (registered with the
> suspend daemon) is in a race to get the event and the socket based
> notification handshake described above.
> 
> i.e. how is the key event delivery vrs unblocking of the suspend-daemon
> read of wakeup_count in step 3 race dealt with?
> 
> Sorry for getting confused and flip flopping on you.  This is pretty
> subtle stuff for me.
>

Not a problem.   I'll see if I can help.

For button presses, the wakeup_source needs to remain active only until
input_event() has been called on the event and the EV_SYN/SYN_REPORT event.
Then it can relax.

input_event will have called input_handle_event -> input_pass_event.
Depending on exactly which fds are open, this might have called
  evdev_event -> evdev_pass_event
which actually places the event in client->buffer.
Then evdev_event will wake up evdev->wait which will alert 'poll' etc.

So if the button press starts before the read request, then by the time the
read completes, the input event will have been queued and any process with an
appropriate file descriptor open will be able to see the event (e.g select
will return, read on a non-blocking fd will return an event rather than
EAGAIN).   The process will actually do a select or a read or
whatever because at step 4 the daemon will tell it to.   When it sees the
event it will probably request that suspend be blocked for the moment, so
when the daemon gets to step 7 it will decide to skip the rest of the suspend
sequence and go back to 0.

If the button press starts after the read of wakeup_count, the process might
not see the event, but the wakeup_count protocol will ensure we go around the
loop again.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3445 bytes --]

On Sat, 15 Oct 2011 14:45:37 -0400 (EDT) Alan Stern
<stern@rowland.harvard.edu> wrote:

> On Sat, 15 Oct 2011, NeilBrown wrote:
> 
> > On Thu, 13 Oct 2011 11:16:23 -0400 (EDT) Alan Stern
> > <stern@rowland.harvard.edu> wrote:
> > 
> > > On Thu, 13 Oct 2011, NeilBrown wrote:
> > > 
> > > > Nope, but I'm keen for you to convince me.  Identify a wakeup event that
> > > > cannot be made visible to poll (or to user-space by some other
> > > > mechanism) before the wakeup_source needs to be deactivated.  Or if I've
> > > > misunderstood what sort of notification is problematic, help me understand.
> > > 
> > > Here's an example (just for kicks, not completely relevant to your
> > > discussion): A USB keyboard key release.  Unlike key presses, key
> > > releases need not generate input events.  If no processes are
> > > monitoring the raw keyboard event queue then the release is not visible
> > > to userspace at all, hence not visible before the wakeup_source needs
> > > to be deactivated.
> > > 
> > > Alan Stern
> > 
> > As you say, not completely relevant.
> > 
> > If a tree falls in a forest with no one to here, does it make a sound?
> > 
> > similarly if an event happens that no-one is looking for, is it visible?
> > It doesn't really matter.
> 
> That's a different question, but I'll answer it anyway: Yes, it does
> matter.  If the kernel is unable to _know_ that nobody is looking for
> an event, it has to _assume_ that somebody is.  Then what should happen 
> if it turns out that nobody really is looking for it?

Same answer - it doesn't really matter.
In every case, the kernel's responsibility is to make the sure the event is
visible to any watching user-space process before it releases the
wakeup_source.
What user-space does then is up to user-space.
If no-one is watching the kernel is free to drop it at any stage - as soon as
it discovers no-one is watching.
When the input layer gets an event, it iterated through a list of fds which
are open on the event source and queues it to each one.  This list might be
empty - no big deal.  It was still a wakeup_event.  If we were suspended, the
write to /sys/power/state will now complete, the suspend daemon will go back
around its loop, nothing will seem to be happening, so the system will go
back to sleep.

> 
> > So at most this is a case of "is not made visible" rather than "cannot be
> > made visible".
> 
> In this case it's the same thing.  How can a key release be made 
> visible?

/dev/input/eventX??  the evdev driver presents key-down and key-up events
separately.

> 
> > The key-release just needs to clear the "key is pressed" state so that
> > auto-repeat stops and if it was a modifier, the modification is discarded.
> > That is all trivially done in some kernel driver while the wakeup_source is
> > active.
> 
> In other words, if the event is discarded from within the kernel then 
> the wakeup_source can be deactivated at that time.  That's fine -- but 
> it indicates that your original request above was phrased wrongly.  You 
> should have asked for an example of a wakeup_source which the kernel 
> must not deactivate without a userspace handshake, but which cannot be 
> made visible by poll or some other similar mechanism.

Yes, I agree that is more precise statement of what I was trying to say - and
precision is important here.

Thanks!

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[Alan Stern]

On Sun, 16 Oct 2011, NeilBrown wrote:

> > > > Here's an example (just for kicks, not completely relevant to your
> > > > discussion): A USB keyboard key release.  Unlike key presses, key
> > > > releases need not generate input events.  If no processes are
> > > > monitoring the raw keyboard event queue then the release is not visible
> > > > to userspace at all, hence not visible before the wakeup_source needs
> > > > to be deactivated.
> > > > 
> > > > Alan Stern
> > > 
> > > As you say, not completely relevant.
> > > 
> > > If a tree falls in a forest with no one to here, does it make a sound?
> > > 
> > > similarly if an event happens that no-one is looking for, is it visible?
> > > It doesn't really matter.
> > 
> > That's a different question, but I'll answer it anyway: Yes, it does
> > matter.  If the kernel is unable to _know_ that nobody is looking for
> > an event, it has to _assume_ that somebody is.  Then what should happen 
> > if it turns out that nobody really is looking for it?
> 
> Same answer - it doesn't really matter.
> In every case, the kernel's responsibility is to make the sure the event is
> visible to any watching user-space process before it releases the
> wakeup_source.
> What user-space does then is up to user-space.
> If no-one is watching the kernel is free to drop it at any stage - as soon as
> it discovers no-one is watching.
> When the input layer gets an event, it iterated through a list of fds which
> are open on the event source and queues it to each one.  This list might be
> empty - no big deal.  It was still a wakeup_event.  If we were suspended, the
> write to /sys/power/state will now complete, the suspend daemon will go back
> around its loop, nothing will seem to be happening, so the system will go
> back to sleep.

All right, let's make things a little more complicated.

Here's what actually happens when a USB keyboard generates a wakeup 
request.  The system wakes up, of course, but there's no particular 
indication of the cause.  In particular, the usbhid driver has no way 
to know directly that the keyboard was the reason for the wakeup.

In addition, usbhid can't poll keyboards to see if they have an event
to report.  (In theory it could -- the HID protocol allows for this --
but many keyboards don't support that part of the protocol properly.)  
It has to wait until the keyboard gets around to reporting the event, 
which can take 10 ms or more.

Taken together, this means usbhid must activate a wakeup_source every
time it wakes up.  If a keyboard event report is received reasonably
quickly then good, it can deactivate the wakeup_source at the right
time.  But if not, all the driver can do is time out the wakeup_source
after some delay.  I don't see any way to avoid it.

Alan Stern

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2433 bytes --]

On Sat, 15 Oct 2011 21:49:44 -0400 (EDT) Alan Stern
<stern@rowland.harvard.edu> wrote:


> All right, let's make things a little more complicated.

Let's call it "realistic".   It is good to have some realism to make sure our
abstract discussions actually mean something.

> 
> Here's what actually happens when a USB keyboard generates a wakeup 
> request.  The system wakes up, of course, but there's no particular 
> indication of the cause.  In particular, the usbhid driver has no way 
> to know directly that the keyboard was the reason for the wakeup.
> 
> In addition, usbhid can't poll keyboards to see if they have an event
> to report.  (In theory it could -- the HID protocol allows for this --
> but many keyboards don't support that part of the protocol properly.)  
> It has to wait until the keyboard gets around to reporting the event, 
> which can take 10 ms or more.
> 
> Taken together, this means usbhid must activate a wakeup_source every
> time it wakes up.  If a keyboard event report is received reasonably
> quickly then good, it can deactivate the wakeup_source at the right
> time.  But if not, all the driver can do is time out the wakeup_source
> after some delay.  I don't see any way to avoid it.

I have to agree with you there.
This is similar to Rafael's example of a Wake-on-LAN packet arriving.  At
that point there is nothing you can do except wait a little while expecting
more information.

You could see this as a case where the wake-up event isn't even visible to
the kernel, so there is obviously no way to make it visible to user-space.

Or you could see it as a wake-up event that is expected to be delivered over
a long period of time (many msecs).  The kernel gathers the wake-up event,
makes it visible to user-space (once it actually arrives), and then releases
the wakeup_source.

So it is a good example and highlights the difficulty of defining exactly
what a wake-up event it, and of what it means to be "visible".

I think it still fits in your rephrasing of my question which - if I rephrase
it as a requirement - is roughly,

  A wakeup-event that needs to be handled by user-space must be visible to
  user-space before the driver deactivates the wakeup_source.

A requirement which, in this case, means the driver needs to hold  the
wakeup_source for an extended time using a timeout, just as you say.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[mark gross]

Sorry for going AWOL on this thread.  I had some biz travel and fires to
fight.

On Mon, Oct 17, 2011 at 08:37:17AM +1100, NeilBrown wrote:
> On Sat, 15 Oct 2011 21:49:44 -0400 (EDT) Alan Stern
> <stern@rowland.harvard.edu> wrote:
> 
> 
> > All right, let's make things a little more complicated.
> 
> Let's call it "realistic".   It is good to have some realism to make sure our
> abstract discussions actually mean something.
> 
> > 
> > Here's what actually happens when a USB keyboard generates a wakeup 
> > request.  The system wakes up, of course, but there's no particular 
> > indication of the cause.  In particular, the usbhid driver has no way 
> > to know directly that the keyboard was the reason for the wakeup.
> > 
> > In addition, usbhid can't poll keyboards to see if they have an event
> > to report.  (In theory it could -- the HID protocol allows for this --
> > but many keyboards don't support that part of the protocol properly.)  
> > It has to wait until the keyboard gets around to reporting the event, 
> > which can take 10 ms or more.
> > 
> > Taken together, this means usbhid must activate a wakeup_source every
> > time it wakes up.  If a keyboard event report is received reasonably
> > quickly then good, it can deactivate the wakeup_source at the right
> > time.  But if not, all the driver can do is time out the wakeup_source
> > after some delay.  I don't see any way to avoid it.
> 
> I have to agree with you there.
> This is similar to Rafael's example of a Wake-on-LAN packet arriving.  At
> that point there is nothing you can do except wait a little while expecting
> more information.
> 
> You could see this as a case where the wake-up event isn't even visible to
> the kernel, so there is obviously no way to make it visible to user-space.
> 
> Or you could see it as a wake-up event that is expected to be delivered over
> a long period of time (many msecs).  The kernel gathers the wake-up event,
> makes it visible to user-space (once it actually arrives), and then releases
> the wakeup_source.
> 
> So it is a good example and highlights the difficulty of defining exactly
> what a wake-up event it, and of what it means to be "visible".
> 
> I think it still fits in your rephrasing of my question which - if I rephrase
> it as a requirement - is roughly,
> 
>   A wakeup-event that needs to be handled by user-space must be visible to
>   user-space before the driver deactivates the wakeup_source.
> 
> A requirement which, in this case, means the driver needs to hold  the
> wakeup_source for an extended time using a timeout, just as you say.

Timeout?  why can't we define a proper notification handshake for such
things?  Timeouts are never right IMO.

--mark



> Thanks,
> NeilBrown

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2985 bytes --]

On Sun, 23 Oct 2011 18:18:04 -0700 mark gross <markgross@thegnar.org> wrote:

> Sorry for going AWOL on this thread.  I had some biz travel and fires to
> fight.

:-)  Life happens.  Welcome back.


> > So it is a good example and highlights the difficulty of defining exactly
> > what a wake-up event it, and of what it means to be "visible".
> > 
> > I think it still fits in your rephrasing of my question which - if I rephrase
> > it as a requirement - is roughly,
> > 
> >   A wakeup-event that needs to be handled by user-space must be visible to
> >   user-space before the driver deactivates the wakeup_source.
> > 
> > A requirement which, in this case, means the driver needs to hold  the
> > wakeup_source for an extended time using a timeout, just as you say.
> 
> Timeout?  why can't we define a proper notification handshake for such
> things?  Timeouts are never right IMO.
> 

I thought that before, but I have come around to the opposite way of thinking
thanks to some instructive examples from Alan and Rafael.

Some things are simply not visible to the OS.  We can expect them to be
happening but we cannot be sure and there is no clear signal that they aren't
actually happening.  So we need a timeout.

- OS cannot detect state of user's brain, so uses a time-out to blank the
  screen after a period of inactivity.
- USB cannot (I think) know which USB device triggered a wake-from-suspend,
  and in any case cannot directly ask the device why it woke from suspend.
  It has to wait for the device to tell it (in response to a regular
  'interrupt' poll I assume - but it isn't guaranteed to be reported on the
  first poll) - or timeout and give up waiting.
- A wake-on-Lan packet may wake a suspend up, but not be further visible to
  the OS.  And even if it was, the 'SYN' packet or maybe 'ARP' packet that
  might be expected to follow it is invisible until it actually arrives, and
  it may not ever come.  So we need a timeout to give up waiting after a
  while.

When-ever we are dealing with external events we need timeouts - and
wake-from-suspend is primarily about external events.

So *somewhere* in the handling of wakeup events there must be timeouts.
Whether that should be explicit in the wakeup-source is possibly a separate
question.  Maybe you could argue that the low level device driver must have
timeouts anyway, so it should use them to explicitly deactivate the source
rather than the source having it's own timeout.
I'm not sure that argument would work though it might.
But saying "Timeouts are never right" cannot work - unless you mean it in a
much more restricted sense than I think you mean it.

(I can agree that it is *best* if timeouts never fire - if direct action
causes the wakeup-source to deactivate long before the timeout.  I agree that
is the best case and probably should be the common case, but I cannot see how
it can be the only case).

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[mark gross]

On Mon, Oct 24, 2011 at 12:50:34PM +1100, NeilBrown wrote:
> On Sun, 23 Oct 2011 18:18:04 -0700 mark gross <markgross@thegnar.org> wrote:
> 
> > Sorry for going AWOL on this thread.  I had some biz travel and fires to
> > fight.
> 
> :-)  Life happens.  Welcome back.
> 
> 
> > > So it is a good example and highlights the difficulty of defining exactly
> > > what a wake-up event it, and of what it means to be "visible".
> > > 
> > > I think it still fits in your rephrasing of my question which - if I rephrase
> > > it as a requirement - is roughly,
> > > 
> > >   A wakeup-event that needs to be handled by user-space must be visible to
> > >   user-space before the driver deactivates the wakeup_source.
> > > 
> > > A requirement which, in this case, means the driver needs to hold  the
> > > wakeup_source for an extended time using a timeout, just as you say.
> > 
> > Timeout?  why can't we define a proper notification handshake for such
> > things?  Timeouts are never right IMO.
> > 
> 
> I thought that before, but I have come around to the opposite way of thinking
> thanks to some instructive examples from Alan and Rafael.
> 
> Some things are simply not visible to the OS.  We can expect them to be
> happening but we cannot be sure and there is no clear signal that they aren't
> actually happening.  So we need a timeout.

um

> 
> - OS cannot detect state of user's brain, so uses a time-out to blank the
>   screen after a period of inactivity.
true, but user interaction has not been part of this discussion.  The
ability of usermode code to offer user interaction has.

> - USB cannot (I think) know which USB device triggered a wake-from-suspend,
>   and in any case cannot directly ask the device why it woke from suspend.
>   It has to wait for the device to tell it (in response to a regular
>   'interrupt' poll I assume - but it isn't guaranteed to be reported on the
>   first poll) - or timeout and give up waiting.
maybe if you are unwilling to change the user mode stack that is reading
these events.  But if you where they you shouldn't need time outs.

> - A wake-on-Lan packet may wake a suspend up, but not be further visible to
>   the OS.  And even if it was, the 'SYN' packet or maybe 'ARP' packet that
>   might be expected to follow it is invisible until it actually arrives, and
>   it may not ever come.  So we need a timeout to give up waiting after a
>   while.
A time out here is just an optimization to avoid re-waking too often.

> 
> When-ever we are dealing with external events we need timeouts - and
> wake-from-suspend is primarily about external events.

This is going too far.   I see wakes up as very analogous to interrupt
devices.  We have interrupts no one seem to care about, and we tend to
have devices that don't time out waiting for an ACK from the OS, where
the OS needs to have a timeout for the latency of IRQ handling. 


> So *somewhere* in the handling of wakeup events there must be timeouts.
> Whether that should be explicit in the wakeup-source is possibly a separate
> question.  Maybe you could argue that the low level device driver must have
> timeouts anyway, so it should use them to explicitly deactivate the source
> rather than the source having it's own timeout.
> I'm not sure that argument would work though it might.
> But saying "Timeouts are never right" cannot work - unless you mean it in a
> much more restricted sense than I think you mean it.

I think they are never right.  But, accept that they are useful for some
case.  i.e. I think having them and using them is a hack to avoid
changing legacy programs dependent on the input subsystem where USB wake
events is a boundary case like the USB or optimizations as in the wake
on lan example. 

It should be possible to implement a system (if you are willing to do
the work) that is correct without timeouts.

> (I can agree that it is *best* if timeouts never fire - if direct action
> causes the wakeup-source to deactivate long before the timeout.  I agree that
> is the best case and probably should be the common case, but I cannot see how
> it can be the only case).

well I still maintain that as a practical matter whatever time out value
chosen is wrong for someone, in some way.  I can't stress this point
enough.  I really worry about *possibility* of correctness whenever a
time out is considered as part of an API.

Time outs may be needed but, I don't think they should be part of the
core API's other than seldom used special needs API for the occasional
corner case.

--mark

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[Brian Swetland]

On Sun, Oct 23, 2011 at 6:50 PM, NeilBrown <neilb@suse.de> wrote:
> But saying "Timeouts are never right" cannot work - unless you mean it in a
> much more restricted sense than I think you mean it.
>
> (I can agree that it is *best* if timeouts never fire - if direct action
> causes the wakeup-source to deactivate long before the timeout.  I agree that
> is the best case and probably should be the common case, but I cannot see how
> it can be the only case).

This was our conclusion when originally building the Android wakelock
APIs.  Timeouts are undesirable, and whenever we can ensure there's a
clear handoff of "ownership" of a wakeup event, so it's fully
accounted for, that's the preferred way to go.  But reality gives us
situations where we can't know with a certainty what woke us up (or we
won't know until some time has passed -- USB resume, etc) and in those
situations, timeouts are the best solution we have.  Over time we've
always hoped to have fewer and fewer timeouts, but I'm not certain
that we'll ever reach zero, for non-trivial systems.

Brian

Re: [markgross@thengar.org: [RFC] wake up notifications and suspend blocking (aka more wakelock stuff)]

[Alan Stern]

On Mon, 24 Oct 2011, mark gross wrote:

> > > Timeout?  why can't we define a proper notification handshake for such
> > > things?  Timeouts are never right IMO.
> > > 
> > 
> > I thought that before, but I have come around to the opposite way of thinking
> > thanks to some instructive examples from Alan and Rafael.
> > 
> > Some things are simply not visible to the OS.  We can expect them to be
> > happening but we cannot be sure and there is no clear signal that they aren't
> > actually happening.  So we need a timeout.
> 
> um

> > - USB cannot (I think) know which USB device triggered a wake-from-suspend,
> >   and in any case cannot directly ask the device why it woke from suspend.
> >   It has to wait for the device to tell it (in response to a regular
> >   'interrupt' poll I assume - but it isn't guaranteed to be reported on the
> >   first poll) - or timeout and give up waiting.
> maybe if you are unwilling to change the user mode stack that is reading
> these events.  But if you where they you shouldn't need time outs.

Remember, here we are talking about timeouts in the kernel stack, not
in the user-mode stack.

So consider this theoretical situation (which is not very different
from reality): The system gets a wakeup signal.  Sometime in the next
30 ms or less, there may or may not be an input event -- the kernel has
no way to tell other than wait and see.

The kernel could simply go right back to sleep without waiting, but if
it does and there is a pending input event, then very quickly it will
get another wakeup signal, and it'll be right back where it started -- 
trying to decide whether to stay awake for the next 30 ms.

Can you suggest a way to handle this other than using a timeout?

Alan Stern