Re: [RFC][PATCH 6/8] PM / Sleep: Implement opportunistic sleep

[NeilBrown <neilb@suse.de>]

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

On Sun, 22 Apr 2012 23:23:23 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> From: "Rafael J. Wysocki" <rjw@sisk.pl>
> To: Linux PM list <linux-pm@vger.kernel.org>
> Cc: LKML <linux-kernel@vger.kernel.org>, Magnus Damm <magnus.damm@gmail.com>, markgross@thegnar.org, Matthew Garrett <mjg@redhat.com>, Greg KH <gregkh@linuxfoundation.org>, Arve Hjønnevåg <arve@android.com>, John Stultz <john.stultz@linaro.org>, Brian Swetland <swetland@google.com>, Neil Brown <neilb@suse.de>, Alan Stern <stern@rowland.harvard.edu>, Dmitry Torokhov <dmitry.torokhov@gmail.com>, "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [RFC][PATCH 6/8] PM / Sleep: Implement opportunistic sleep
> Date: Sun, 22 Apr 2012 23:23:23 +0200
> Sender: linux-kernel-owner@vger.kernel.org
> User-Agent: KMail/1.13.6 (Linux/3.4.0-rc3+; KDE/4.6.0; x86_64; ; )
> 
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Introduce a mechanism by which the kernel can trigger global
> transitions to a sleep state chosen by user space if there are no
> active wakeup sources.

Hi Rafael,
 just a few little issues below.  Over all I think that if we have to have
 auto-sleep in the kernel, then this is a good way to do it.

> +static void try_to_suspend(struct work_struct *work)
> +{
> +	unsigned int initial_count, final_count;
> +
> +	if (!pm_get_wakeup_count(&initial_count, true))
> +		goto out;
> +
> +	mutex_lock(&autosleep_lock);
> +
> +	if (!pm_save_wakeup_count(initial_count)) {
> +		mutex_unlock(&autosleep_lock);
> +		goto out;
> +	}
> +
> +	if (autosleep_state == PM_SUSPEND_ON) {
> +		mutex_unlock(&autosleep_lock);
> +		return;
> +	}
> +	if (autosleep_state >= PM_SUSPEND_MAX)
> +		hibernate();
> +	else
> +		pm_suspend(autosleep_state);
> +
> +	mutex_unlock(&autosleep_lock);
> +
> +	if (!pm_get_wakeup_count(&final_count, false))
> +		goto out;
> +
> +	if (final_count == initial_count)
> +		schedule_timeout(HZ / 2);

This doesn't do what you seem to expect it to do.
You need to set current->state to something like TASK_UNINTERRUPTIBLE
before calling schedule_timeout, otherwise it is effectily a no-op.
schedule_timeout_uninterruptible(), for example, will do this for you.

However the value of this isn't clear to me, so a comment would probably be a
good thing.
This continue presumably fires if we wake up without any wakeup sources
being activated.  In that case you want to delay for 500ms - presumably to
avoid a tight suspend/resume loop if something goes wrong?

I have occasionally seen a stray/uninteresting interrupt wake from suspend
immediately after entering suspend and the next attempt succeeds.  Maybe this
is a bug in some driver somewhere, but not a big one.  I think I would rather
in that case that we attempt to re-enter suspend immediately.  Maybe after a
few failed attempts it makes sense to back off.

The other question is: if we want to back-off, is 500ms really enough?  What
will be gained by, or could be achieved in, that time?  An exponential
back-off might be defensible, but I can't see the value of a 500ms fixed
back-off.
However if you can, I'd love to see a comment in there explaining it.


> +
> + out:
> +	queue_up_suspend_work();
> +}
> +


> +
> +int pm_autosleep_set_state(suspend_state_t state)
> +{
> +
> +#ifndef CONFIG_HIBERNATION
> +	if (state >= PM_SUSPEND_MAX)
> +		return -EINVAL;
> +#endif
> +
> +	__pm_stay_awake(autosleep_ws);
> +
> +	mutex_lock(&autosleep_lock);
> +
> +	autosleep_state = state;
> +
> +	__pm_relax(autosleep_ws);

I'm struggling to see the point of the autosleep_ws.

A suspend cannot actually happen while this code is running (can it?) because
it will wait for the process to enter the freezer.
So the only effect of this is:
  1/ cause the current auto-sleep cycle to abort and
  2/ maybe add some accounting number is the autosleep_ws.
Is that right?
Which of these is needed?

I would imagine that any process writing to /sys/power/autosleep would be
holding a wakelock, and if it didn't it should expect things to be racy...

Am I missing something?


> +
> +	if (state > PM_SUSPEND_ON)
> +		queue_up_suspend_work();

The test here is superfluous as queue_up_suspend_work() itself tests that
'state' is > PM_SUSPEND_ON.  However maybe it is more readable this way, so I
won't object it you like it.


> +
> +	mutex_unlock(&autosleep_lock);
> +	return 0;
> +}


> @@ -339,7 +359,8 @@ static ssize_t wakeup_count_show(struct
>  {
>  	unsigned int val;
>  
> -	return pm_get_wakeup_count(&val) ? sprintf(buf, "%u\n", val) : -EINTR;
> +	return pm_get_wakeup_count(&val, true) ?
> +		sprintf(buf, "%u\n", val) : -EINTR;
>  }

I think it would be really nice for user-space auto-suspend if the 'block'
flag to be settable from the O_NONBLOCK setting.  And for poll() to work
on /sys/power/wakeup-count.  However this would require a bit of surgery on
sysfs.  So that is a "maybe later", but having the 'block' flag in there is
a step in the right direction.


>  
>  static ssize_t wakeup_count_store(struct kobject *kobj,
> @@ -347,15 +368,69 @@ static ssize_t wakeup_count_store(struct
>  				const char *buf, size_t n)
>  {
>  	unsigned int val;
> +	int error;
> +
> +	error = pm_autosleep_lock();
> +	if (error)
> +		return error;
> +
> +	if (pm_autosleep_state() > PM_SUSPEND_ON) {
> +		error = -EBUSY;
> +		goto out;
> +	}
>  
>  	if (sscanf(buf, "%u", &val) == 1) {
>  		if (pm_save_wakeup_count(val))
>  			return n;

You need a 'pm_autosleep_unlock() in there - or possibly
  error = n; goto out;


>  	}
> -	return -EINVAL;
> +	error = -EINVAL;
> +
> + out:
> +	pm_autosleep_unlock();
> +	return error;
>  }

>  core_initcall(pm_init);
> Index: linux/drivers/base/power/wakeup.c
> ===================================================================
> --- linux.orig/drivers/base/power/wakeup.c
> +++ linux/drivers/base/power/wakeup.c
> @@ -498,8 +498,10 @@ static void wakeup_source_deactivate(str
>  	trace_wakeup_source_deactivate(ws->name, cec);
>  
>  	split_counters(&cnt, &inpr);
> -	if (!inpr && waitqueue_active(&wakeup_count_wait_queue))
> +	if (!inpr && waitqueue_active(&wakeup_count_wait_queue)) {
>  		wake_up(&wakeup_count_wait_queue);
> +		queue_up_suspend_work();
> +	}

This doesn't look right.  suspend_work always requeues itself unless
autosleep_state == PM_SUSPEND_ON, and whenver autosleep_state is set we
already call queue_up_suspend_work().  So there is no need to call it here.

> Index: linux/Documentation/ABI/testing/sysfs-power
> ===================================================================
> --- linux.orig/Documentation/ABI/testing/sysfs-power
> +++ linux/Documentation/ABI/testing/sysfs-power
> @@ -172,3 +172,20 @@ Description:
>  
>  		Reading from this file will display the current value, which is
>  		set to 1 MB by default.
> +
> +What:		/sys/power/autosleep
> +Date:		February 2012
> +Contact:	Rafael J. Wysocki <rjw@sisk.pl>
> +Description:
> +		The /sys/power/autosleep file can be written one of the strings

"To the .. file can be written..." or
"The .. file can have written ..." or
"One of the strings returned by (reads from) /sys/power/state can be written
to the file ..."
??
> +		returned by reads from /sys/power/state.  If that happens, a
> +		work item attempting to trigger a transition of the system to
> +		the sleep state represented by that string is queued up.  This
> +		attempt will only succeed if there are no active wakeup sources
> +		in the system at that time.  After evey execution, regardless
                                                   ^^^^
  "every"

> +		of whether or not the attempt to put the system to sleep has
> +		succeeded, the work item requeues itself until user space
> +		writes "off" to /sys/power/autosleep.
> +
> +		Reading from this file causes the last string successfully
> +		written to it to be displayed.
                                    ^^^^^^^^^  "returned".


Thanks,
NeilBrown

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