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