| Message ID | 1359428300.3211.3.camel@fli24-HP-Compaq-8100-Elite-CMT-PC (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Tuesday, January 29, 2013 10:58:20 AM fli24 wrote: > > At present, the timeout value for freezing tasks is fixed as 20s, > which is too long for handheld device usage, especially for mobile > phone. > > In order to improve user experience, we enable freeze timeout > configuration through sysctl, so that we can tune the value easily > for concrete usage, such as smaller value for handheld device such > as mobile phone. Well, I'd argue that you shouldn't see freeze problems on such systems. If you're seeing them, it's better to fix them than to try to hide them from users (they are problems after all). Do you have a specific example in which that new knob will be useful? Why do you want to do that through sysctl and not sysfs? Rafael > Signed-off-by: Liu Chuansheng <chuansheng.liu@intel.com> > Signed-off-by: Li Fei <fei.li@intel.com> > --- > include/linux/freezer.h | 5 +++++ > kernel/power/process.c | 4 ++-- > kernel/sysctl.c | 12 ++++++++++++ > 3 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/include/linux/freezer.h b/include/linux/freezer.h > index e4238ce..f37b3be 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 sysctl_freeze_process_timeout_secs; > + > +/* > * Check if a process has been frozen > */ > static inline bool frozen(struct task_struct *p) > diff --git a/kernel/power/process.c b/kernel/power/process.c > index d5a258b..f7eb7c9 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 sysctl_freeze_process_timeout_secs = 20; > > 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 + sysctl_freeze_process_timeout_secs * HZ; > > if (!user_only) > freeze_workqueues_begin(); > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index c88878d..f88bcb9 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -90,6 +90,9 @@ > #include <linux/nmi.h> > #endif > > +#ifdef CONFIG_FREEZER > +#include <linux/freezer.h> > +#endif > > #if defined(CONFIG_SYSCTL) > > @@ -1047,6 +1050,15 @@ static struct ctl_table kern_table[] = { > .proc_handler = proc_dointvec, > }, > #endif > +#ifdef CONFIG_FREEZER > + { > + .procname = "freeze_process_timeout_secs", > + .data = &sysctl_freeze_process_timeout_secs, > + .maxlen = sizeof(unsigned int), > + .mode = 0644, > + .proc_handler = proc_dointvec, > + }, > +#endif > { } > }; > >
On Tue, 29 Jan 2013 10:58:20 +0800 fli24 <fei.li@intel.com> wrote: > At present, the timeout value for freezing tasks is fixed as 20s, > which is too long for handheld device usage, especially for mobile > phone. > > In order to improve user experience, we enable freeze timeout > configuration through sysctl, so that we can tune the value easily > for concrete usage, such as smaller value for handheld device such > as mobile phone. > > ... > The patch looks nice - it does everything right in places where things are frequently done wrongly. Except.. It forgot to document the sysctl. Documentation/sysctl/kernel.txt, I guess. Is /proc/sys/kernel the most appropriate place for this? Perhaps a PM-specific place would be better. Maybe not. > --- 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 sysctl_freeze_process_timeout_secs; I suggest the use of milliseconds here. Someone might want a half-second timeout and it's pretty pointless to design the interface in a way which rules that out. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Rafael, Thanks for your feedback, and my understanding is interleaved in your email as below. Best Regards, Li Fei > -----Original Message----- > From: Rafael J. Wysocki [mailto:rjw@sisk.pl] > Sent: Tuesday, January 29, 2013 7:42 PM > To: Li, Fei > Cc: akpm@linux-foundation.org; linux-kernel@vger.kernel.org; > linux-pm@vger.kernel.org; Liu, Chuansheng > Subject: Re: [PATCH] suspend: enable freeze timeout configuration through sysctl > > On Tuesday, January 29, 2013 10:58:20 AM fli24 wrote: > > > > At present, the timeout value for freezing tasks is fixed as 20s, > > which is too long for handheld device usage, especially for mobile > > phone. > > > > In order to improve user experience, we enable freeze timeout > > configuration through sysctl, so that we can tune the value easily > > for concrete usage, such as smaller value for handheld device such > > as mobile phone. > > Well, I'd argue that you shouldn't see freeze problems on such systems. > If you're seeing them, it's better to fix them than to try to hide them > from users (they are problems after all). [Li, Fei] Thanks for your opinion. Indeed, I see such freeze problems on mobile phone system using fuse file system. The scenario is as below: Thread A with i_mutex held is frozen during waiting for feedback from fuse daemon; Thread B is trying to lock i_mutex and can't be frozen. In the case above, 20s waiting is needless, as freezing will fail unavoidably. I agree with you that we'd better fix them from the root, which may need solution of long term. I also saw some related discussion on Linux community as below, without final conclusion: http://lists.linux-foundation.org/pipermail/linux-pm/2008-October/018774.html So I think if we can enable freezing timeout configuration, it will improve such issue. > Do you have a specific example in which that new knob will be useful? [Li, Fei] As the scenario stated above, if we can configure the value of timeout to 10s or other small value, this time of freezing will be aborted in earlier time, and after i_mutex is released during thread A restarting, the next time of suspend/freeze may succeed in relatively earlier time. > Why do you want to do that through sysctl and not sysfs? [Li, Fei] Thanks for your suggestion, sysfs is more suitable, and I'll use sysfs in patch V2. > Rafael > > > > Signed-off-by: Liu Chuansheng <chuansheng.liu@intel.com> > > Signed-off-by: Li Fei <fei.li@intel.com> > > --- > > include/linux/freezer.h | 5 +++++ > > kernel/power/process.c | 4 ++-- > > kernel/sysctl.c | 12 ++++++++++++ > > 3 files changed, 19 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/freezer.h b/include/linux/freezer.h > > index e4238ce..f37b3be 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 sysctl_freeze_process_timeout_secs; > > + > > +/* > > * Check if a process has been frozen > > */ > > static inline bool frozen(struct task_struct *p) > > diff --git a/kernel/power/process.c b/kernel/power/process.c > > index d5a258b..f7eb7c9 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 sysctl_freeze_process_timeout_secs = 20; > > > > 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 + sysctl_freeze_process_timeout_secs * HZ; > > > > if (!user_only) > > freeze_workqueues_begin(); > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > > index c88878d..f88bcb9 100644 > > --- a/kernel/sysctl.c > > +++ b/kernel/sysctl.c > > @@ -90,6 +90,9 @@ > > #include <linux/nmi.h> > > #endif > > > > +#ifdef CONFIG_FREEZER > > +#include <linux/freezer.h> > > +#endif > > > > #if defined(CONFIG_SYSCTL) > > > > @@ -1047,6 +1050,15 @@ static struct ctl_table kern_table[] = { > > .proc_handler = proc_dointvec, > > }, > > #endif > > +#ifdef CONFIG_FREEZER > > + { > > + .procname = "freeze_process_timeout_secs", > > + .data = &sysctl_freeze_process_timeout_secs, > > + .maxlen = sizeof(unsigned int), > > + .mode = 0644, > > + .proc_handler = proc_dointvec, > > + }, > > +#endif > > { } > > }; > > > > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center.
Hello Andrew, Thanks for your feedback. Your suggestion are all accepted, and will be updated in V2 as below: 1> The newly added attribute will be /sys/power/pm_freeze_timeout; 2> Documentation/power/freezing-of-tasks.txt will be updated to record such attribute; 3> Unit of millisecond will be used for the attribute. Best Regards, Li Fei -----Original Message----- From: Andrew Morton [mailto:akpm@linux-foundation.org] Sent: Wednesday, January 30, 2013 8:37 AM To: Li, Fei Cc: rjw@sisk.pl; linux-kernel@vger.kernel.org; linux-pm@vger.kernel.org; Liu, Chuansheng Subject: Re: [PATCH] suspend: enable freeze timeout configuration through sysctl On Tue, 29 Jan 2013 10:58:20 +0800 fli24 <fei.li@intel.com> wrote: > At present, the timeout value for freezing tasks is fixed as 20s, > which is too long for handheld device usage, especially for mobile > phone. > > In order to improve user experience, we enable freeze timeout > configuration through sysctl, so that we can tune the value easily > for concrete usage, such as smaller value for handheld device such > as mobile phone. > > ... > The patch looks nice - it does everything right in places where things are frequently done wrongly. Except.. It forgot to document the sysctl. Documentation/sysctl/kernel.txt, I guess. Is /proc/sys/kernel the most appropriate place for this? Perhaps a PM-specific place would be better. Maybe not. > --- 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 sysctl_freeze_process_timeout_secs; I suggest the use of milliseconds here. Someone might want a half-second timeout and it's pretty pointless to design the interface in a way which rules that out. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/freezer.h b/include/linux/freezer.h index e4238ce..f37b3be 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 sysctl_freeze_process_timeout_secs; + +/* * Check if a process has been frozen */ static inline bool frozen(struct task_struct *p) diff --git a/kernel/power/process.c b/kernel/power/process.c index d5a258b..f7eb7c9 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 sysctl_freeze_process_timeout_secs = 20; 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 + sysctl_freeze_process_timeout_secs * HZ; if (!user_only) freeze_workqueues_begin(); diff --git a/kernel/sysctl.c b/kernel/sysctl.c index c88878d..f88bcb9 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -90,6 +90,9 @@ #include <linux/nmi.h> #endif +#ifdef CONFIG_FREEZER +#include <linux/freezer.h> +#endif #if defined(CONFIG_SYSCTL) @@ -1047,6 +1050,15 @@ static struct ctl_table kern_table[] = { .proc_handler = proc_dointvec, }, #endif +#ifdef CONFIG_FREEZER + { + .procname = "freeze_process_timeout_secs", + .data = &sysctl_freeze_process_timeout_secs, + .maxlen = sizeof(unsigned int), + .mode = 0644, + .proc_handler = proc_dointvec, + }, +#endif { } };