From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [PATCH V4] suspend: enable freeze timeout configuration through sys Date: Sun, 03 Feb 2013 14:41:27 +0100 Message-ID: <1919836.fQMoKOYYSd@vostro.rjw.lan> References: <1359428300.3211.3.camel@fli24-HP-Compaq-8100-Elite-CMT-PC> <1359608132.7918.2.camel@fli24-HP-Compaq-8100-Elite-CMT-PC> <1359708963.7240.3.camel@fli24-HP-Compaq-8100-Elite-CMT-PC> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7Bit Return-path: Received: from hydra.sisk.pl ([212.160.235.94]:55093 "EHLO hydra.sisk.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752258Ab3BCNfM (ORCPT ); Sun, 3 Feb 2013 08:35:12 -0500 In-Reply-To: <1359708963.7240.3.camel@fli24-HP-Compaq-8100-Elite-CMT-PC> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: fli24 Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, chuansheng.liu@intel.com On Friday, February 01, 2013 04:56:03 PM fli24 wrote: The code looks OK, but I'm still having a more fundamental problem with this change, because -> > At present, the value of timeout for freezing is 20s, which is > meaningless in case that one thread is frozen with mutex locked > and another thread is trying to lock the mutex, as this time of > freezing will fail unavoidably. -> the situation described above shouldn't happen and if it does, then there is a bug that needs to be fixed. The change you're proposing seems to be targeted at hiding those bugs rather than at fixing them. I wonder why we should hide those bugs instead of fixing them? Rafael > And if there is no new wakeup event registered, the system will > waste at most 20s for such meaningless trying of freezing. > > With this patch, the value of timeout can be configured to smaller > value, so such meaningless trying of freezing will be aborted in > earlier time, and later freezing can be also triggered in earlier > time. And more power will be saved. > In normal case on mobile phone, it costs real little time to freeze > processes. On some platform, it only costs about 20ms to freeze > user space processes and 10ms to freeze kernel freezable threads. > > Signed-off-by: Liu Chuansheng > Signed-off-by: Li Fei > --- > Documentation/power/freezing-of-tasks.txt | 5 +++++ > include/linux/freezer.h | 5 +++++ > kernel/power/main.c | 27 +++++++++++++++++++++++++++ > kernel/power/process.c | 4 ++-- > 4 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/Documentation/power/freezing-of-tasks.txt b/Documentation/power/freezing-of-tasks.txt > index 6ec291e..85894d8 100644 > --- a/Documentation/power/freezing-of-tasks.txt > +++ b/Documentation/power/freezing-of-tasks.txt > @@ -223,3 +223,8 @@ since they ask the freezer to skip freezing this task, since it is anyway > only after the entire suspend/hibernation sequence is complete. > So, to summarize, use [un]lock_system_sleep() instead of directly using > mutex_[un]lock(&pm_mutex). That would prevent freezing failures. > + > +V. Miscellaneous > +/sys/power/pm_freeze_timeout controls how long it will cost at most to freeze > +all user space processes or all freezable kernel threads, in unit of millisecond. > +The default value is 20000, with range of unsigned integer. > diff --git a/include/linux/freezer.h b/include/linux/freezer.h > index e4238ce..e70df40 100644 > --- a/include/linux/freezer.h > +++ b/include/linux/freezer.h > @@ -13,6 +13,11 @@ extern bool pm_freezing; /* PM freezing in effect */ > extern bool pm_nosig_freezing; /* PM nosig freezing in effect */ > > /* > + * Timeout for stopping processes > + */ > +extern unsigned int freeze_timeout_msecs; > + > +/* > * Check if a process has been frozen > */ > static inline bool frozen(struct task_struct *p) > diff --git a/kernel/power/main.c b/kernel/power/main.c > index 1c16f91..3e1c9da 100644 > --- a/kernel/power/main.c > +++ b/kernel/power/main.c > @@ -553,6 +553,30 @@ power_attr(pm_trace_dev_match); > > #endif /* CONFIG_PM_TRACE */ > > +#ifdef CONFIG_FREEZER > +static ssize_t pm_freeze_timeout_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + return sprintf(buf, "%u\n", freeze_timeout_msecs); > +} > + > +static ssize_t pm_freeze_timeout_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t n) > +{ > + unsigned long val; > + > + if (kstrtoul(buf, 10, &val)) > + return -EINVAL; > + > + freeze_timeout_msecs = val; > + return n; > +} > + > +power_attr(pm_freeze_timeout); > + > +#endif /* CONFIG_FREEZER*/ > + > static struct attribute * g[] = { > &state_attr.attr, > #ifdef CONFIG_PM_TRACE > @@ -576,6 +600,9 @@ static struct attribute * g[] = { > &pm_print_times_attr.attr, > #endif > #endif > +#ifdef CONFIG_FREEZER > + &pm_freeze_timeout_attr.attr, > +#endif > NULL, > }; > > diff --git a/kernel/power/process.c b/kernel/power/process.c > index d5a258b..98088e0 100644 > --- a/kernel/power/process.c > +++ b/kernel/power/process.c > @@ -21,7 +21,7 @@ > /* > * Timeout for stopping processes > */ > -#define TIMEOUT (20 * HZ) > +unsigned int __read_mostly freeze_timeout_msecs = 20 * MSEC_PER_SEC; > > static int try_to_freeze_tasks(bool user_only) > { > @@ -36,7 +36,7 @@ static int try_to_freeze_tasks(bool user_only) > > do_gettimeofday(&start); > > - end_time = jiffies + TIMEOUT; > + end_time = jiffies + msecs_to_jiffies(freeze_timeout_msecs); > > if (!user_only) > freeze_workqueues_begin(); > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.