From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758453Ab0EYW7V (ORCPT ); Tue, 25 May 2010 18:59:21 -0400 Received: from mail-pv0-f174.google.com ([74.125.83.174]:41478 "EHLO mail-pv0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751024Ab0EYW7T convert rfc822-to-8bit (ORCPT ); Tue, 25 May 2010 18:59:19 -0400 To: Arve =?iso-8859-1?Q?Hj=F8nnev=E5g?= Cc: Dmitry Torokhov , Alan Stern , Linux-pm mailing list , Kernel development list , "Rafael J. Wysocki" , Len Brown , Pavel Machek , Randy Dunlap , Andrew Morton , Andi Kleen , Cornelia Huck , Tejun Heo , Jesse Barnes , Nigel Cunningham , Ming Lei , Wu Fengguang , Maxim Levitsky , linux-doc@vger.kernel.org Subject: Re: [PATCH 1/8] PM: Opportunistic suspend support. References: <201005251124.58017.dmitry.torokhov@gmail.com> <20100525184740.GA4248@core.coreip.homeip.net> <20100525223220.GC4928@core.coreip.homeip.net> From: Kevin Hilman Organization: Deep Root Systems, LLC Date: Tue, 25 May 2010 15:59:15 -0700 In-Reply-To: ("Arve =?iso-8859-1?Q?Hj=F8nnev=E5g=22's?= message of "Tue\, 25 May 2010 15\:37\:48 -0700") Message-ID: <87ljb7io58.fsf@deeprootsystems.com> User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Arve Hjønnevåg writes: > 2010/5/25 Dmitry Torokhov : >> 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 >>> 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... Then the userspace suspend manager should be a little more clever and should not blindly retry continuously. It should be more like a governor which makes some simple decisions based on previous events, simple heuristics, uses timeouts etc., Kevin