Message-ID: <AANLkTimdGWKbUTkgTZEUGNPrXVzZ6CYwbbKQ7C3X7Ek_@mail.gmail.com>
Date:	Tue, 25 May 2010 16:54:34 -0700
From:	Arve Hjønnevåg <arve@...roid.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	Alan Stern <stern@...land.harvard.edu>,
	Linux-pm mailing list <linux-pm@...ts.linux-foundation.org>,
	Kernel development list <linux-kernel@...r.kernel.org>,
	"Rafael J. Wysocki" <rjw@...k.pl>, Len Brown <len.brown@...el.com>,
	Pavel Machek <pavel@....cz>,
	Randy Dunlap <rdunlap@...otime.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <ak@...ux.intel.com>,
	Cornelia Huck <cornelia.huck@...ibm.com>,
	Tejun Heo <tj@...nel.org>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Nigel Cunningham <nigel@...onice.net>,
	Ming Lei <tom.leiming@...il.com>,
	Wu Fengguang <fengguang.wu@...el.com>,
	Maxim Levitsky <maximlevitsky@...il.com>,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH 1/8] PM: Opportunistic suspend support.

2010/5/25 Dmitry Torokhov <dmitry.torokhov@...il.com>:
> On Tue, May 25, 2010 at 04:13:35PM -0700, Arve Hjønnevåg wrote:
>> 2010/5/25 Dmitry Torokhov <dmitry.torokhov@...il.com>:
>> > On Tue, May 25, 2010 at 03:37:48PM -0700, Arve Hjønnevåg wrote:
>> >> 2010/5/25 Dmitry Torokhov <dmitry.torokhov@...il.com>:
>> >> > On Tue, May 25, 2010 at 03:23:23PM -0700, Arve Hjønnevåg wrote:
>> >> >> On Tue, May 25, 2010 at 11:47 AM, Dmitry Torokhov
>> >> >> <dmitry.torokhov@...il.com> wrote:
>> >> >> > On Tue, May 25, 2010 at 02:35:17PM -0400, Alan Stern wrote:
>> >> >> >> On Tue, 25 May 2010, Dmitry Torokhov wrote:
>> >> >> >>
>> >> >> >> > > Here's the scenario:
>> >> >> >> > >
>> >> >> >> > > The system is awake, and the user presses a key. The keyboard driver
>> >> >> >> > > processes the keystroke and puts it in an input queue.  A user process
>> >> >> >> > > reads it from the event queue, thereby emptying the queue.
>> >> >> >> > >
>> >> >> >> > > At that moment, the system decides to go into opportunistic suspend.
>> >> >> >> > > Since the input queue is empty, there's nothing to stop it.  As the
>> >> >> >> > > first step, userspace is frozen -- before the process has a chance to
>> >> >> >> > > do anything with the keystroke it just read.  As a result, the system
>> >> >> >> > > stays asleep until something else wakes it up, even though the
>> >> >> >> > > keystroke was important and should have prevented it from sleeping.
>> >> >> >> > >
>> >> >> >> > > Suspend blockers protect against this scenario.  Here's how:
>> >> >> >> > >
>> >> >> >> > > The user process doesn't read the input queue directly; instead it
>> >> >> >> > > does a select or poll.  When it sees there is data in the queue, it
>> >> >> >> > > first acquires a suspend blocker and then reads the data.
>> >> >> >> > >
>> >> >> >> > > Now the system _can't_ go into opportunistic suspend, because a suspend
>> >> >> >> > > blocker is active.  The user process can do whatever it wants with the
>> >> >> >> > > keystroke.  When it is finished, it releases the suspend blocker and
>> >> >> >> > > loops back to the select/poll call.
>> >> >> >> > >
>> >> >> >> >
>> >> >> >> > What you describe can be done in userspace though, via a "suspend manager"
>> >> >> >> > process. Tasks reading input events will post "busy" events to stop the
>> >> >> >> > manager process from sending system into suspend. But this can be confined to
>> >> >> >> > Android userspace, leaving the kernel as is (well, kernel needs to be modified
>> >> >> >> > to not go into suspend with full queues, but that is using existing kernel
>> >> >> >> > APIs).
>> >> >> >>
>> >> >> >> I think that could be made to work.  And it might remove the need for
>> >> >> >> the userspace suspend-blocker API, which would be an advantage.  It
>> >> >> >> could even remove the need for the opportunistic-suspend workqueue --
>> >> >> >> opportunistic suspends would be initiated by the "suspend manager"
>> >> >> >> process instead of by the kernel.
>> >> >> >>
>> >> >> >> However you still have the issue of modifying the kernel drivers to
>> >> >> >> disallow opportunistic suspend if their queues are non-empty.  Doing
>> >> >> >> that is more or less equivalent to implementing kernel-level suspend
>> >> >> >> blockers.  (The suspend blocker approach is slightly more efficient,
>> >> >> >> because it will prevent a suspend from starting if a queue is
>> >> >> >> non-empty, instead of allowing the suspend to start and then aborting
>> >> >> >> it partway through.)
>> >> >> >>
>> >> >> >> Maybe I'm missing something here...  No doubt someone will point it out
>> >> >> >> if I am.
>> >> >> >>
>> >> >> >
>> >> >> > Well, from my perspective that would limit changes to the evdev driver
>> >> >> > (well, limited input core plumbing will be needed) but that is using the
>> >> >> > current PM infrastructure. The HW driver changes will be limited to what
>> >> >> > you described "type 2" in your other e-mail.
>> >> >> >
>> >> >> > Also, not suspending while events are in progress) is probably
>> >> >> > beneficial for platforms other than Android as well. So unless I am
>> >> >> > missing something this sounds like a win.
>> >> >> >
>> >> >>
>> >> >> How would this limit the changes you need in the evdev driver? It need
>> >> >> to block suspend when there are unprocessed events in some queues.
>> >> >> Suspend blockers gives you an api to do this, without it, you check
>> >> >> the queues in your suspend hook and abort suspend if they are not
>> >> >> empty. Without suspend blockers you have no api to signal that it is
>> >> >> OK to suspend again, so you are forcing the thread that tried to
>> >> >> suspend to poll until you stop aborting suspend.
>> >> >
>> >> > No, you do not need to poll. You just set a timeout (short or long,
>> >> > depending on your needs) and if no userspace task blocked suspend
>> >> > durng that time you attempt to initiate suspend from your manager
>> >> > process. If it succeeds - good, if not that means that more events came
>> >> > your way and you have to do it later.
>> >> >
>> >>
>> >> How is that not polling? If the user is holding down a key, the keypad
>> >> driver has to block suspend, and user space will try to suspend again
>> >> and again and again...
>> >>
>> >
>> > If your userpsace is that stupid - sure. However, you can:
>> >
>> > 1. Notify the suspend manager process that he rest of your userspace is
>> > busy handling keystrokes so that it does not try to suspend while there
>> > are events pending.
>>
>> You are missing the point. There are no event pending. The kernel
>> reported the key down event, it was handled, but the keypad driver is
>> still scanning to see if the user presses another key,
>
> Employ reasonable timeout.

Timeout for what? Stop trying to suspend altogether, stop scanning for
key changes, or a more "reasonable" poll interval?

>
>> or releases the
>> currently held key.
>>
>
> Userspace consumer should wait for the key release and retract "busy"
> once event is received and handled.
>

No it should not. User-space does not know if the key is coming from a
keypad driver that needs to actively scan the matrix while keys are
pressed.

>> >
>> > 2. Wait a tiny bit after last application notified you that it finished
>> > processing events.
>> >
>> > So basically the difference is that with in-kernel suspend blockers,
>> > there is a tiny window where we haven't started the suspend yet but are
>> > about to the driver has a chance to prevent entire system from starting
>> > sleep.
>>
>> No, the difference is that if a driver needs to prevent suspend for an
>> extended period of time, you don't have user space continuously
>> polling to see if it can suspend.
>
> Why would a driver, on its own, prevent suspend for extended periods of
> time? I think that the decision should originate from userspace, kernel
> is here just to serve the requests.
>

A driver prevents suspend if suspend would prevent it from working.
For instance, the gpio keypad matrix code prevents suspend when a key
is help down, since it has to activly scan the keypad for changes.
Only no-keys-pressed versus one-or-more-keys-pressed can be detected
with an interrupt.

>>
>> >
>> > Without the blocker we may start suspending and will stop midcycle. We
>> > may be even better off in the end since we could leave some devices
>> > still powered down after aborting system-wide suspend.
>> >
>>
>> That does not sound right.
>
> Why doesn't it? If a device implements runtime PM it may chose remain in
> powered-down mode even if system is awake.
>

If the device implements runtime PM it should already be powered-down
if it is not in use.

-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists