[RFC PATCH] suspend/resume performance improvement

[RFC PATCH] suspend/resume performance improvement

[이은택]

When a task that calls state_store() to suspend
the device has used up most of its time slice,
suspend sometimes take too long. (User noticeable)

Suspend/resume is a system wide operation.
So, instead of depending on a userspace task's time
slice, let kworker do the work to avoid a long wait
on the runqueue.

Signed-off-by: Eun Taik Lee <eun.taik.lee@samsung.com>
---
 kernel/power/Kconfig |  12 +++++
 kernel/power/main.c  | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 132 insertions(+), 1 deletion(-)

diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 7e01f78..7eef317 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -18,6 +18,18 @@ config SUSPEND_FREEZER
 
 	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
 
+config SUSPEND_HELPER
+	bool "Use kworker for suspend/resume"
+	depends on SUSPEND
+	default n
+	help
+	  Use kworker for suspend/resume functionality to decrease
+	  the suspend/resume time when the caller has used up too much
+	  of its time slice before calling.
+	  Use kworker for suspend/resume functionality to decrease
+	  the suspend/resume time when the caller has used up too much
+	  of its time slice before calling.
+
 config HIBERNATE_CALLBACKS
 	bool
 
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 86e8157..e782b86 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -15,7 +15,7 @@
 #include <linux/workqueue.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
-
+#include <linux/completion.h>
 #include "power.h"
 
 DEFINE_MUTEX(pm_mutex);
@@ -335,9 +335,33 @@ static suspend_state_t decode_state(const char *buf, size_t n)
 	return PM_SUSPEND_ON;
 }
 
+#ifdef CONFIG_SUSPEND_HELPER
+static struct workqueue_struct *suspend_helper_wq;
+struct state_store_params {
+	const char *buf;
+	size_t n;
+};
+
+struct suspend_helper_data {
+	struct work_struct work;
+	struct completion done;
+	struct state_store_params params;
+	int result;
+};
+struct suspend_helper_data suspend_helper_data;
+
+static ssize_t state_store_helper(struct kobject *kobj,
+		struct kobj_attribute *attr,
+		const char *buf, size_t n);
+#endif
+
 static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
 			   const char *buf, size_t n)
 {
+#ifdef CONFIG_SUSPEND_HELPER
+	pr_debug("%s: Let our helper do the real work!\n", __func__);
+	return state_store_helper(kobj, attr, buf, n);
+#else
 	suspend_state_t state;
 	int error;
 
@@ -361,10 +385,102 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
  out:
 	pm_autosleep_unlock();
 	return error ? error : n;
+#endif
 }
 
 power_attr(state);
 
+#ifdef CONFIG_SUSPEND_HELPER
+static void suspend_helper(struct work_struct *work)
+{
+	struct suspend_helper_data *data = (struct suspend_helper_data *)
+		container_of(work, struct suspend_helper_data, work);
+	const char *buf = data->params.buf;
+	size_t n = data->params.n;
+	suspend_state_t state;
+	int error = 0;
+
+	pr_debug("%s: start!\n", __func__);
+
+	error = pm_autosleep_lock();
+	if (error)
+		goto out_nolock;
+
+	if (pm_autosleep_state() > PM_SUSPEND_ON) {
+		error = -EBUSY;
+		goto out;
+	}
+
+	state = decode_state(buf, n);
+	if (state < PM_SUSPEND_MAX)
+		error = pm_suspend(state);
+	else if (state == PM_SUSPEND_MAX)
+		error = hibernate();
+	else
+		error = -EINVAL;
+
+out:
+	pm_autosleep_unlock();
+
+out_nolock:
+	/* set result and notify completion */
+	data->result = error;
+	complete(&data->done);
+
+	pr_debug("%s: result = %d\n", __func__, error);
+}
+
+static ssize_t state_store_helper(struct kobject *kobj,
+		struct kobj_attribute *attr,
+		const char *buf, size_t n)
+{
+	int error;
+	int freezable = 0;
+
+	/* we don't need to freeze. so tell the freezer */
+	if (!freezer_should_skip(current)) {
+		freezable = 1;
+		freezer_do_not_count();
+		pr_debug("%s: freezer should skip me (%s:%d)\n",
+			__func__, current->comm, current->pid);
+	}
+
+	suspend_helper_data.params.buf = buf;
+	suspend_helper_data.params.n = n;
+	init_completion(&suspend_helper_data.done);
+
+	/* use kworker for suspend resume */
+	queue_work(suspend_helper_wq, &suspend_helper_data.work);
+
+	/* wait for suspend/resume work to be complete */
+	wait_for_completion(&suspend_helper_data.done);
+
+	if (freezable) {
+		/* set ourself as freezable */
+		freezer_count();
+	}
+
+	error = suspend_helper_data.result;
+	pr_debug("%s: suspend_helper returned %d\n", __func__, error);
+
+	return error ? error : n;
+}
+
+static int suspend_helper_init(void)
+{
+	suspend_helper_wq = alloc_ordered_workqueue("suspend_helper", 0);
+	if (!suspend_helper_wq)
+		return -ENOMEM;
+
+	INIT_WORK(&suspend_helper_data.work, suspend_helper);
+	init_completion(&suspend_helper_data.done);
+
+	pr_debug("%s: init done\n", __func__);
+
+	return 0;
+}
+#endif /* CONFIG_SUSPEND_HELPER */
+
 #ifdef CONFIG_PM_SLEEP
 /*
  * The 'wakeup_count' attribute, along with the functions defined in
@@ -640,6 +756,9 @@ static int __init pm_init(void)
 	if (error)
 		return error;
 	pm_print_times_init();
+#ifdef CONFIG_SUSPEND_HELPER
+	suspend_helper_init();
+#endif
 	return pm_autosleep_init();
 }
 
-- 
1.9.1

Re: [RFC PATCH] suspend/resume performance improvement

[Pandruvada, Srinivas]

On Tue, 2015-05-26 at 14:05 +0000, 이은택 wrote:
> When a task that calls state_store() to suspend
> the device has used up most of its time slice,
> suspend sometimes take too long. (User noticeable)
> 
> Suspend/resume is a system wide operation.
> So, instead of depending on a userspace task's time
> slice, let kworker do the work to avoid a long wait
> on the runqueue.
> 
> Signed-off-by: Eun Taik Lee <eun.taik.lee@samsung.com>
> ---
>  kernel/power/Kconfig |  12 +++++
>  kernel/power/main.c  | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 132 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 7e01f78..7eef317 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -18,6 +18,18 @@ config SUSPEND_FREEZER
>  
>  	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
>  
> +config SUSPEND_HELPER
> +	bool "Use kworker for suspend/resume"
> +	depends on SUSPEND
> +	default n
> +	help
> +	  Use kworker for suspend/resume functionality to decrease
> +	  the suspend/resume time when the caller has used up too much
> +	  of its time slice before calling.
> +	  Use kworker for suspend/resume functionality to decrease
> +	  the suspend/resume time when the caller has used up too much
> +	  of its time slice before calling.
> +
>  config HIBERNATE_CALLBACKS
>  	bool
>  
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 86e8157..e782b86 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -15,7 +15,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/debugfs.h>
>  #include <linux/seq_file.h>
> -
> +#include <linux/completion.h>
>  #include "power.h"
>  
>  DEFINE_MUTEX(pm_mutex);
> @@ -335,9 +335,33 @@ static suspend_state_t decode_state(const char *buf, size_t n)
>  	return PM_SUSPEND_ON;
>  }
>  
> +#ifdef CONFIG_SUSPEND_HELPER
> +static struct workqueue_struct *suspend_helper_wq;
> +struct state_store_params {
> +	const char *buf;
> +	size_t n;
> +};
> +
> +struct suspend_helper_data {
> +	struct work_struct work;
> +	struct completion done;
> +	struct state_store_params params;
> +	int result;
> +};
> +struct suspend_helper_data suspend_helper_data;
> +
> +static ssize_t state_store_helper(struct kobject *kobj,
> +		struct kobj_attribute *attr,
> +		const char *buf, size_t n);
> +#endif
> +
>  static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  			   const char *buf, size_t n)
>  {
> +#ifdef CONFIG_SUSPEND_HELPER
> +	pr_debug("%s: Let our helper do the real work!\n", __func__);
> +	return state_store_helper(kobj, attr, buf, n);
> +#else
>  	suspend_state_t state;
>  	int error;
>  
> @@ -361,10 +385,102 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>   out:
>  	pm_autosleep_unlock();
>  	return error ? error : n;
> +#endif
>  }
>  
>  power_attr(state);
>  
> +#ifdef CONFIG_SUSPEND_HELPER
> +static void suspend_helper(struct work_struct *work)
> +{
> +	struct suspend_helper_data *data = (struct suspend_helper_data *)
> +		container_of(work, struct suspend_helper_data, work);
> +	const char *buf = data->params.buf;
> +	size_t n = data->params.n;
> +	suspend_state_t state;
> +	int error = 0;
> +
> +	pr_debug("%s: start!\n", __func__);
> +
> +	error = pm_autosleep_lock();
> +	if (error)
> +		goto out_nolock;
> +
> +	if (pm_autosleep_state() > PM_SUSPEND_ON) {
> +		error = -EBUSY;
> +		goto out;
> +	}
> +
> +	state = decode_state(buf, n);
> +	if (state < PM_SUSPEND_MAX)
> +		error = pm_suspend(state);
> +	else if (state == PM_SUSPEND_MAX)
> +		error = hibernate();
> +	else
> +		error = -EINVAL;
Instead of replicating code in state_store here, does it make sense to
move into a common part, so that fixes/optimization can be done in one
place?
Also why another config? If this has proven benefit then it can be
default path.

> +
> +out:
> +	pm_autosleep_unlock();
> +
> +out_nolock:
> +	/* set result and notify completion */
> +	data->result = error;
> +	complete(&data->done);
> +
> +	pr_debug("%s: result = %d\n", __func__, error);
> +}
> +
> +static ssize_t state_store_helper(struct kobject *kobj,
> +		struct kobj_attribute *attr,
> +		const char *buf, size_t n)
> +{
> +	int error;
> +	int freezable = 0;
> +
> +	/* we don't need to freeze. so tell the freezer */
> +	if (!freezer_should_skip(current)) {
> +		freezable = 1;
> +		freezer_do_not_count();
> +		pr_debug("%s: freezer should skip me (%s:%d)\n",
> +			__func__, current->comm, current->pid);
> +	}
> +
> +	suspend_helper_data.params.buf = buf;
> +	suspend_helper_data.params.n = n;
> +	init_completion(&suspend_helper_data.done);
> +
> +	/* use kworker for suspend resume */
> +	queue_work(suspend_helper_wq, &suspend_helper_data.work);
> +
> +	/* wait for suspend/resume work to be complete */
> +	wait_for_completion(&suspend_helper_data.done);
> +
> +	if (freezable) {
> +		/* set ourself as freezable */
> +		freezer_count();
> +	}
> +
> +	error = suspend_helper_data.result;
> +	pr_debug("%s: suspend_helper returned %d\n", __func__, error);
> +
> +	return error ? error : n;
> +}
> +
> +static int suspend_helper_init(void)
> +{
> +	suspend_helper_wq = alloc_ordered_workqueue("suspend_helper", 0);
> +	if (!suspend_helper_wq)
> +		return -ENOMEM;
> +
> +	INIT_WORK(&suspend_helper_data.work, suspend_helper);
> +	init_completion(&suspend_helper_data.done);
> +
> +	pr_debug("%s: init done\n", __func__);
> +
> +	return 0;
> +}
> +#endif /* CONFIG_SUSPEND_HELPER */
> +
>  #ifdef CONFIG_PM_SLEEP
>  /*
>   * The 'wakeup_count' attribute, along with the functions defined in
> @@ -640,6 +756,9 @@ static int __init pm_init(void)
>  	if (error)
>  		return error;
>  	pm_print_times_init();
> +#ifdef CONFIG_SUSPEND_HELPER
> +	suspend_helper_init();
> +#endif
>  	return pm_autosleep_init();
>  }
>  

Re: [RFC PATCH] suspend/resume performance improvement

[Rafael J. Wysocki]

On Tuesday, May 26, 2015 02:05:48 PM 이은택 wrote:
> When a task that calls state_store() to suspend
> the device has used up most of its time slice,
> suspend sometimes take too long. (User noticeable)
> 
> Suspend/resume is a system wide operation.
> So, instead of depending on a userspace task's time
> slice, let kworker do the work to avoid a long wait
> on the runqueue.

All of that seems to duplicate the existing autosleep.c functionality.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] suspend/resume performance improvement

[Pavel Machek]

On Tue 2015-05-26 14:05:47, 이은택 wrote:
> When a task that calls state_store() to suspend
> the device has used up most of its time slice,
> suspend sometimes take too long. (User noticeable)
> 
> Suspend/resume is a system wide operation.

It is also root-only.

> So, instead of depending on a userspace task's time
> slice, let kworker do the work to avoid a long wait
> on the runqueue.

...so if you really want high priority for that operation, just renice
yourself to higher priority or something...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html