Re: Async suspend-resume patch w/ completions (was: Re: Async suspend-resume patch w/ rwsems)

Rafael J. Wysocki
2009-12-16T02:10:33Z

On Tuesday 15 December 2009, Linus Torvalds wrote:
>
> On Tue, 15 Dec 2009, Rafael J. Wysocki wrote:
> > >
> > > Give a real example that matters.
> >
> > I'll try. Let -> denote child-parent relationships and assume dpm_list looks
> > like this:
>
> No.
>
> I mean something real - something like
>
> - if you run on a non-PC with two USB buses behind non-PCI controllers.
>
> - device xyz.
>
> > If this applies to _resume_ only, then I agree, but the Arjan's data clearly
> > show that serio devices take much more time to suspend than USB.
>
> I mean in general - something where you actually have hard data that some
> device really needs anythign more than my one-liner, and really _needs_
> some complex infrastructure.
>
> Not "let's imagine a case like xyz".

As I said I would, I made some measurements.

I measured the total time of suspending and resuming devices as shown by the
code added by this patch:
http://git.kernel.org/?p=linux/kernel/git/rafael/suspend-2.6.git;a=commitdiff_plain;h=c1b8fc0a8bff7707c10f31f3d26bfa88e18ccd94;hp=087dbf5f079f1b55cbd3964c9ce71268473d5b67
on two boxes, HP nx6325 and MSI Wind U100 (hardware-wise they are quite
different and the HP was running 64-bit kernel and user space).

I took four cases into consideration:
(1) synchronous suspend and resume (/sys/power/pm_async = 0)
(2) asynchronous suspend and resume as introduced by the async branch at:
http://git.kernel.org/?p=linux/kernel/git/rafael/suspend-2.6.git;a=shortlog;h=refs/heads/async
(3) asynchronous suspend and resume like in (2), but with your one-liner setting
the power.async_suspend flag for PCI bridges on top
(4) asynchronous suspend and resume like in (2), but with an extra patch that
is appended on top

For those tests I set power.async_suspend for all USB devices, all serio input
devices, the ACPI battery and the USB PCI controllers (to see the impact of the
one-liner, if any).

I carried out 5 consecutive suspend-resume cycles (started from under X) on
each box in each case, and the raw data are here (all times in milliseconds):
http://www.sisk.pl/kernel/data/async-suspend.pdf

The summarized data are below (the "big" numbers are averages and the +/-
numbers are standard deviations, all in milliseconds):

HP nx6325 MSI Wind U100

sync suspend 1482 (+/- 40) 1180 (+/- 24)
sync resume 2955 (+/- 2) 3597 (+/- 25)

async suspend 1553 (+/- 49) 1177 (+/- 32)
async resume 2692 (+/- 326) 3556 (+/- 33)

async+one-liner suspend 1600 (+/- 39) 1212 (+/- 41)
async+one-liner resume 2692 (+/- 324) 3579 (+/- 24)

async+extra suspend 1496 (+/- 37) 1217 (+/- 38)
async+extra resume 1859 (+/- 114) 1923 (+/- 35)

So, in my opinion, with the above set of "async" devices, it doesn't
make sense to do async suspend at all, because the sync suspend is actually
the fastest on both machines.

However, it surely is worth doing async _resume_ with the extra patch appended
below, because that allows us to save 1 second or more on both machines with
respect to the sync case. The other variants of async resume also produce some
time savings, but (on the nx6325) at the expense of huge fluctuations from one
cycle to another (so they can actually be slower than the sync resume). Only
the async resume with the extra patch is consistently better than the sync one.
The impact of the one-liner is either negligible or slightly negative.

Now, what does the extra patch do? Exactly the thing I was talking about, it
starts all async suspends and resumes upfront.

So, it looks like we both were wrong. I was wrong, because I thought the
extra patch would help suspend, but not resume, while in fact it appears to
help resume big time. You were wrong, because you thought that the one-liner
would have positive impact, while in fact it doesn't.

Concluding, at this point I'd opt for implementing asynchronous resume alone,
_without_ asynchronous suspend, which is more complicated and doesn't really
give us any time savings. At the same time, I'd implement the asynchronous
resume in such a way that all of the async resume threads would be started
before the synchronous suspend thread, because that would give us the best
results.

Rafael

---
drivers/base/power/main.c | 48 +++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 17 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -523,14 +523,9 @@ static void async_resume(void *data, asy

static int device_resume(struct device *dev)
{
- INIT_COMPLETION(dev->power.completion);
-
- if (pm_async_enabled && dev->power.async_suspend
- && !pm_trace_is_enabled()) {
- get_device(dev);
- async_schedule(async_resume, dev);
+ if (dev->power.async_suspend && pm_async_enabled
+ && !pm_trace_is_enabled())
return 0;
- }

return __device_resume(dev, pm_transition, false);
}
@@ -545,14 +540,28 @@ static int device_resume(struct device *
static void dpm_resume(pm_message_t state)
{
struct list_head list;
+ struct device *dev;
ktime_t starttime = ktime_get();

INIT_LIST_HEAD(&list);
mutex_lock(&dpm_list_mtx);
pm_transition = state;
- while (!list_empty(&dpm_list)) {
- struct device *dev = to_device(dpm_list.next);

+ list_for_each_entry(dev, &dpm_list, power.entry) {
+ if (dev->power.status < DPM_OFF)
+ continue;
+
+ INIT_COMPLETION(dev->power.completion);
+
+ if (dev->power.async_suspend && pm_async_enabled
+ && !pm_trace_is_enabled()) {
+ get_device(dev);
+ async_schedule(async_resume, dev);
+ }
+ }
+
+ while (!list_empty(&dpm_list)) {
+ dev = to_device(dpm_list.next);
get_device(dev);
if (dev->power.status >= DPM_OFF) {
int error;
@@ -809,13 +818,8 @@ static void async_suspend(void *data, as

static int device_suspend(struct device *dev)
{
- INIT_COMPLETION(dev->power.completion);
-
- if (pm_async_enabled && dev->power.async_suspend) {
- get_device(dev);
- async_schedule(async_suspend, dev);
+ if (pm_async_enabled && dev->power.async_suspend)
return 0;
- }

return __device_suspend(dev, pm_transition, false);
}
@@ -827,6 +831,7 @@ static int device_suspend(struct device
static int dpm_suspend(pm_message_t state)
{
struct list_head list;
+ struct device *dev;
ktime_t starttime = ktime_get();
int error = 0;

@@ -834,9 +839,18 @@ static int dpm_suspend(pm_message_t stat
mutex_lock(&dpm_list_mtx);
pm_transition = state;
async_error = 0;
- while (!list_empty(&dpm_list)) {
- struct device *dev = to_device(dpm_list.prev);

+ list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
+ INIT_COMPLETION(dev->power.completion);
+
+ if (pm_async_enabled && dev->power.async_suspend) {
+ get_device(dev);
+ async_schedule(async_suspend, dev);
+ }
+ }
+
+ while (!list_empty(&dpm_list)) {
+ dev = to_device(dpm_list.prev);
get_device(dev);
mutex_unlock(&dpm_list_mtx);