Date:	Wed, 12 May 2010 21:35:30 -0600 (MDT)
From:	Paul Walmsley <paul@...an.com>
To:	Arve Hjønnevåg <arve@...roid.com>
cc:	linux-pm@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
	Tejun Heo <tj@...nel.org>, Oleg Nesterov <oleg@...hat.com>,
	Tony Lindgren <tony@...mide.com>,
	Kevin Hilman <khilman@...prootsystems.com>,
	Alan Stern <stern@...land.harvard.edu>, magnus.damm@...il.com,
	Theodore Ts'o <tytso@....edu>,
	mark gross <mgross@...ux.intel.com>,
	Arjan van de Ven <arjan@...radead.org>,
	Geoff Smith <geoffx.smith@...el.com>,
	Brian Swetland <swetland@...gle.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Matthew Garrett <mjg@...hat.com>,
	Benoît Cousson <b-cousson@...com>,
	linux-omap@...r.kernel.org, Vitaly Wool <vitalywool@...il.com>,
	Linus Walleij <linus.walleij@...csson.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Liam Girdwood <lrg@...mlogic.co.uk>
Subject: Re: [linux-pm] [PATCH 0/8] Suspend block api (version 6)

Hello,

Some general comments on the suspend blockers/wakelock/opportunistic
suspend v6 patch series, posted here:

    https://lists.linux-foundation.org/pipermail/linux-pm/2010-April/025146.html

The comments below are somewhat telegraphic in the interests of 
readability - more specific comments to follow in later E-mails. I am 
indebted to those of us who discussed these issues at LPC last year and 
ELC this year for several stimulating discussions.

There are several general problems with the design of opportunistic
suspend and suspend-blocks.

1. The opportunistic suspend code bypasses existing Linux kernel code,
   such as timers and the scheduler, that indicates when code
   needs to run, and when the system is idle.  This causes two problems:

   a. When opportunistic suspend is enabled, the default mode is to
      break all timers and scheduling on the system.  This isn't
      right: the default mode should be to preserve standard Linux
      behavior.  Exceptions can then be added for process groups that
      should run with the non-standard timer and scheduler behavior.

   b. The series introduces a de novo kernel API and userspace API
      that are unrelated to timers and the scheduler, but if the point
      is to modify the behavior of timers or the scheduler, the
      existing timer or scheduler APIs should be extended.  Any new
      APIs will need to be widely spread throughout the kernel and
      userspace.

2. The suspend-block kernel API tells the kernel _how_ to accomplish a
   goal, rather than telling the kernel _what_ the goal is.  This
   results in layering violations, unstated assumptions, and is too
   coarse-grained.  These problems in turn will cause fragile kernel
   code, kernel code with userspace dependencies, and power management
   problems on modern hardware.  Code should ask for what it wants.
   For example, if a driver needs to place an upper bound on its
   device wakeup latency, or if it needs to place an upper bound on
   interrupt response latency, that is what it should request.  Driver
   and subsystem code should not care how the kernel implements those
   requests, since the implementation can differ on different hardware
   and even on different use-cases with the same hardware.
   
3. Similarly, the suspend-block userspace API tells the kernel how to
   accomplish a goal, rather than telling the kernel what the goal is.
   Userspace processes should ask the kernel for what they really
   want.  If a process' timers should be disabled upon entering
   suspend, or the timer durations should have a lower bound, that's
   what the API should request.

Merging this series as currently designed and implemented will cause 
problems.  Suspend-blocks introduce a second, separate idle management 
approach in the Linux kernel.  The existing approach is the familiar timer 
and scheduler based approach.  The new approach is one where timers and 
runqueues no longer matter: the system is always at risk of entering 
suspend at any moment, with only suspend-blocks to stop it. Driver authors 
will effectively have to implement both approaches in their code.

Once merged, it will be nearly impossible to remove this code in favor 
of a cleaner approach.  Suspend-block calls are likely to spread 
throughout the kernel and drivers.  Patches 6, 7, and 8 are the leading 
edge of this - a quick grep through the Android common kernel at

    git://android.git.kernel.org/kernel/common.git 

shows wakelocks in the following drivers:

    drivers/input/evdev.c
    drivers/input/misc/gpio_input.c
    drivers/input/misc/gpio_matrix.c
    drivers/mmc/core/core.c
    drivers/rtc/alarm.c
    drivers/usb/gadget/f_mass_storage.c

Suspend-blocks will be difficult to convert to a finer-grained approach 
later.  The API design problems, mentioned above in points 2 and 3, will 
make it very difficult to determine what a driver author's or modifier's 
intention was when adding the suspend-block.  Also, patches 2 and 7 
introduce userspace APIs.  We will undoubtedly wish to avoid removing a 
userspace API once it is merged.  It will be quite difficult to implement 
such a general directive ("block system suspend") on a future kernel that 
may have a much finer-grained notion of low-power system modes, indeed 
that may have no useful notion of "system suspend."

...

The opportunistic suspend patches try to solve at least two real problems, 
that should be resolved in some way.  First, some types of userspace 
processes can unintentionally block system power management.  Second, the 
kernel is missing a system-wide form of CPUIdle.  This patch series, 
though, isn't the right way to solve either of these problems.  Let's 
figure out a different approach.

Figuring out a different way to do this should not limit Android at all, 
since Google can do what other Linux distributions do and continue to 
patch opportunistic suspend/suspend-block calls into their kernels as 
needed to ship devices, while contributing towards a different solution to 
the problem.

 
regards,

- Paul

(Linux-OMAP co-maintainer, focusing mostly on power management and 
software architecture issues)

--
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