Messages in this thread Patch in this message | | | From | "Pandruvada, Srinivas" <> | Subject | Re: [RFC PATCH] suspend/resume performance improvement | Date | Tue, 26 May 2015 20:12:40 +0000 |
| |
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(); > } >
| |