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@xxxxxxxxxxx>
> ---
>  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();
>  }
>