> -----Original Message----- > From: Rafael J. Wysocki [mailto:r...@sisk.pl] > Sent: Friday, February 01, 2013 6:29 AM > To: anish singh; Li, Fei > Cc: Yasuaki Ishimatsu; a...@linux-foundation.org; > linux-kernel@vger.kernel.org; > linux...@vger.kernel.org; Liu, Chuansheng > Subject: Re: [PATCH V3] suspend: enable freeze timeout configuration through > sys > > On Thursday, January 31, 2013 03:22:25 PM anish singh wrote: > > On Thu, Jan 31, 2013 at 2:43 PM, Li, Fei <fei...@intel.com> wrote: > > >> -----Original Message----- > > >> From: Yasuaki Ishimatsu [mailto:isimatu.yasu...@jp.fujitsu.com] > > >> Sent: Thursday, January 31, 2013 3:30 PM > > >> To: Li, Fei > > >> Cc: r...@sisk.pl; a...@linux-foundation.org; > > >> linux-kernel@vger.kernel.org; > > >> linux...@vger.kernel.org; Liu, Chuansheng > > >> Subject: Re: [PATCH V3] suspend: enable freeze timeout configuration > through > > >> sys > > >> > > >> 2013/01/31 13:55, fli24 wrote: > > >> > > > >> > 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. > > >> > 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 <chuansheng....@intel.com> > > >> > Signed-off-by: Li Fei <fei...@intel.com> > > >> > --- > > >> > 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..5a24a33 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 sys_freeze_process_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..453ead1 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", sys_freeze_process_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; > > >> > + > > >> > + sys_freeze_process_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..ba45a26 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 sys_freeze_process_timeout_msecs = > 20000; > > >> > > >> 20000 does not mean 20 seconds since we can select HZ other than 1000. > > >> So (20 * HZ) is better than 20000. > > >> > > > [Li, Fei] > > > Are you sure about this, (20*HZ) better than 20000, or you mean 20 * > MSEC_PER_SEC? > > Yasuaki mean HZ value will not always be 1000.The value of HZ differs for > each > > supported architecture. In fact, on some supported architectures, > > it even differs between machine types. > > When writing kernel code, never assume that HZ has any given value. > > Right now you are assuming that the delay will be always 20 seconds because > of > > your assumption of HZ. > > That's correct, the initial value should be 20 * HZ (i.e. as before). [Li, Fei] Yes, you are right, and IMHO it's already as this in the patch, as 20 * HZ == msecs_to_jiffies(20000), with the current definition MSEC_PER_SEC of 1000L. I'll update the default value as 20 * MSEC_PER_SEC in patch V4.
> Besides, the name of the variable doesn't need to be _that_ long. > What about freeze_timeout_msecs? [Li, Fei] Agree with you, and will update it in patch V4. Thanks and Regards, Li Fei > Rafael > > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center.
