[PATCH] suspend: enable freeze timeout configuration through sysctl

[PATCH] suspend: enable freeze timeout configuration through sysctl

[fli24]

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.

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
 	{ }
 };
 
-- 
1.7.4.1

Re: [PATCH] suspend: enable freeze timeout configuration through sysctl

[Rafael J. Wysocki]

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
>  	{ }
>  };
>  
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH] suspend: enable freeze timeout configuration through sysctl

[Li, Fei]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4870 bytes --]

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.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] suspend: enable freeze timeout configuration through sysctl

[Andrew Morton]

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.

RE: [PATCH] suspend: enable freeze timeout configuration through sysctl

[Li, Fei]

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.

[PATCH V2] suspend: enable freeze timeout configuration through sys

[fli24]

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 sys, so that we can tune the value easily
for concrete usage, such as smaller value for handheld device such
as mobile phone.

Signed-off-by: Liu Chuansheng <chuansheng.liu@intel.com>
Signed-off-by: Li Fei <fei.li@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;
 
 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(sys_freeze_process_timeout_msecs);
 
 	if (!user_only)
 		freeze_workqueues_begin();
-- 
1.7.4.1

Re: [PATCH V2] suspend: enable freeze timeout configuration through sys

[Rafael J. Wysocki]

On Wednesday, January 30, 2013 02:23:39 PM 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 sys, so that we can tune the value easily
> for concrete usage, such as smaller value for handheld device such
> as mobile phone.

As I said, I'd like to know what's the exact problem you need to work around
using this.

Thanks,
Rafael


> Signed-off-by: Liu Chuansheng <chuansheng.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@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;
>  
>  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(sys_freeze_process_timeout_msecs);
>  
>  	if (!user_only)
>  		freeze_workqueues_begin();
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH V2] suspend: enable freeze timeout configuration through sys

[Li, Fei]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5849 bytes --]

Hello Rafael,
   I update it as below in V3, do you think it makes sense?

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.

Thanks a lot!

Best Regards,
Li Fei
TEL: +86 (0)21 6116 5140
Mob: +86 (0)139 1696 7435
Platform System Integration Shanghai



> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Wednesday, January 30, 2013 9:10 PM
> To: Li, Fei
> Cc: akpm@linux-foundation.org; linux-kernel@vger.kernel.org;
> linux-pm@vger.kernel.org; Liu, Chuansheng
> Subject: Re: [PATCH V2] suspend: enable freeze timeout configuration through
> sys
> 
> On Wednesday, January 30, 2013 02:23:39 PM 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 sys, so that we can tune the value easily
> > for concrete usage, such as smaller value for handheld device such
> > as mobile phone.
> 
> As I said, I'd like to know what's the exact problem you need to work around
> using this.
> 
> Thanks,
> Rafael
> 
> 
> > Signed-off-by: Liu Chuansheng <chuansheng.liu@intel.com>
> > Signed-off-by: Li Fei <fei.li@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;
> >
> >  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(sys_freeze_process_timeout_msecs);
> >
> >  	if (!user_only)
> >  		freeze_workqueues_begin();
> >
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH V3] suspend: enable freeze timeout configuration through sys

[fli24]

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.liu@intel.com>
Signed-off-by: Li Fei <fei.li@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;
 
 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(sys_freeze_process_timeout_msecs);
 
 	if (!user_only)
 		freeze_workqueues_begin();
-- 
1.7.4.1

Re: [PATCH V3] suspend: enable freeze timeout configuration through sys

[Yasuaki Ishimatsu]

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.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@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.

Thanks,
Yasuaki Ishimatsu

>
>   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(sys_freeze_process_timeout_msecs);
>
>   	if (!user_only)
>   		freeze_workqueues_begin();
>

RE: [PATCH V3] suspend: enable freeze timeout configuration through sys

[Li, Fei]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5741 bytes --]

> -----Original Message-----
> From: Yasuaki Ishimatsu [mailto:isimatu.yasuaki@jp.fujitsu.com]
> Sent: Thursday, January 31, 2013 3:30 PM
> To: Li, Fei
> Cc: rjw@sisk.pl; akpm@linux-foundation.org; linux-kernel@vger.kernel.org;
> linux-pm@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.liu@intel.com>
> > Signed-off-by: Li Fei <fei.li@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?
In fact, the usage of such variable has been changed as below in the patch:
   end_time = jiffies + msecs_to_jiffies(sys_freeze_process_timeout_msecs)
So I don't think it make sense to use msecs_to_jiffies(20*HZ).

If needed, I think we can refine it to use 20* MSEC_PER_SEC instead of 20000.
How about your opinion?

Thanks!
Best Regards,
Li Fei

> Thanks,
> Yasuaki Ishimatsu
>
> >
> >   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(sys_freeze_process_timeout_msecs);
> >
> >   	if (!user_only)
> >   		freeze_workqueues_begin();
> >
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH V3] suspend: enable freeze timeout configuration through sys

[anish singh]

On Thu, Jan 31, 2013 at 2:43 PM, Li, Fei <fei.li@intel.com> wrote:
>> -----Original Message-----
>> From: Yasuaki Ishimatsu [mailto:isimatu.yasuaki@jp.fujitsu.com]
>> Sent: Thursday, January 31, 2013 3:30 PM
>> To: Li, Fei
>> Cc: rjw@sisk.pl; akpm@linux-foundation.org; linux-kernel@vger.kernel.org;
>> linux-pm@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.liu@intel.com>
>> > Signed-off-by: Li Fei <fei.li@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.

> In fact, the usage of such variable has been changed as below in the patch:
>    end_time = jiffies + msecs_to_jiffies(sys_freeze_process_timeout_msecs)
> So I don't think it make sense to use msecs_to_jiffies(20*HZ).
>
> If needed, I think we can refine it to use 20* MSEC_PER_SEC instead of 20000.
> How about your opinion?
>
> Thanks!
> Best Regards,
> Li Fei
>
>> Thanks,
>> Yasuaki Ishimatsu
>>
>> >
>> >   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(sys_freeze_process_timeout_msecs);
>> >
>> >     if (!user_only)
>> >             freeze_workqueues_begin();
>> >
>>
>

Re: [PATCH V3] suspend: enable freeze timeout configuration through sys

[Rafael J. Wysocki]

On Thursday, January 31, 2013 03:22:25 PM anish singh wrote:
> On Thu, Jan 31, 2013 at 2:43 PM, Li, Fei <fei.li@intel.com> wrote:
> >> -----Original Message-----
> >> From: Yasuaki Ishimatsu [mailto:isimatu.yasuaki@jp.fujitsu.com]
> >> Sent: Thursday, January 31, 2013 3:30 PM
> >> To: Li, Fei
> >> Cc: rjw@sisk.pl; akpm@linux-foundation.org; linux-kernel@vger.kernel.org;
> >> linux-pm@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.liu@intel.com>
> >> > Signed-off-by: Li Fei <fei.li@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).

Besides, the name of the variable doesn't need to be _that_ long.
What about freeze_timeout_msecs?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH V3] suspend: enable freeze timeout configuration through sys

[Li, Fei]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 7267 bytes --]

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Friday, February 01, 2013 6:29 AM
> To: anish singh; Li, Fei
> Cc: Yasuaki Ishimatsu; akpm@linux-foundation.org; linux-kernel@vger.kernel.org;
> linux-pm@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.li@intel.com> wrote:
> > >> -----Original Message-----
> > >> From: Yasuaki Ishimatsu [mailto:isimatu.yasuaki@jp.fujitsu.com]
> > >> Sent: Thursday, January 31, 2013 3:30 PM
> > >> To: Li, Fei
> > >> Cc: rjw@sisk.pl; akpm@linux-foundation.org; linux-kernel@vger.kernel.org;
> > >> linux-pm@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.liu@intel.com>
> > >> > Signed-off-by: Li Fei <fei.li@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.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH V3] suspend: enable freeze timeout configuration through sys

[Yasuaki Ishimatsu]

2013/02/01 10:33, Li, Fei wrote:
>> -----Original Message-----
>> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
>> Sent: Friday, February 01, 2013 6:29 AM
>> To: anish singh; Li, Fei
>> Cc: Yasuaki Ishimatsu; akpm@linux-foundation.org; linux-kernel@vger.kernel.org;
>> linux-pm@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.li@intel.com> wrote:
>>>>> -----Original Message-----
>>>>> From: Yasuaki Ishimatsu [mailto:isimatu.yasuaki@jp.fujitsu.com]
>>>>> Sent: Thursday, January 31, 2013 3:30 PM
>>>>> To: Li, Fei
>>>>> Cc: rjw@sisk.pl; akpm@linux-foundation.org; linux-kernel@vger.kernel.org;
>>>>> linux-pm@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.liu@intel.com>
>>>>>> Signed-off-by: Li Fei <fei.li@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.

20 * MSEC_PER_SEC is not 20 seconds. In Linux, 1 * HZ is 1 seconds.
Thus,
  - If HZ is defined as 1000, 1000 is 1 seconds.
  - If HZ is defined as 250, 250 is 1 seconds.

20 * MSEC_PER_SEC is always 20000.
Thus,
  - If HZ is defined as 1000, 20 * MSEC_PER_SEC is 20 seconds.
  - If HZ is defined as 250, 20 * MSEC_PER_SEC is 80 seconds.

So you should use 20 * HZ if you define timeout at 20 seconds.

Thanks,
Yasuaki Ishimatsu.

>
>> 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.

RE: [PATCH V3] suspend: enable freeze timeout configuration through sys

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1917 bytes --]

> >>>>>> -#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.
> 
> 20 * MSEC_PER_SEC is not 20 seconds. In Linux, 1 * HZ is 1 seconds.
Sure.

> Thus,
>   - If HZ is defined as 1000, 1000 is 1 seconds.
>   - If HZ is defined as 250, 250 is 1 seconds.
> 
> 20 * MSEC_PER_SEC is always 20000.

> Thus,
>   - If HZ is defined as 1000, 20 * MSEC_PER_SEC is 20 seconds.
>   - If HZ is defined as 250, 20 * MSEC_PER_SEC is 80 seconds.
Could you check the patch again?
The patch are using msecs_to_jiffies(20 * MSEC_PER_SEC), not 20 * MSEC_PER_SEC.
msecs_to_jiffies() has done the conversion from MSEC_PER_SEC to HZ.

> 
> So you should use 20 * HZ if you define timeout at 20 seconds.
> 
> Thanks,
> Yasuaki Ishimatsu.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH V4] suspend: enable freeze timeout configuration through sys

[fli24]

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.liu@intel.com>
Signed-off-by: Li Fei <fei.li@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..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();
-- 
1.7.4.1

Re: [PATCH V4] suspend: enable freeze timeout configuration through sys

[Rafael J. Wysocki]

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 <chuansheng.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@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..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.

RE: [PATCH V4] suspend: enable freeze timeout configuration through sys

[Li, Fei]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5949 bytes --]

> 
> 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.

Yes, we agree with that it's a bug that needs to be fixed, and we are already done
for some cases. 
During the process, we realize that tuning the timeout value to smaller value will help
to expose freezing failure for some cases.

> 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

We understand your concern about hiding bugs, and we are not going to do this.
In contrast, setting timeout value to smaller value means more strict threshold, and
it will expose the potential issue more quickly with more chance.

Taking diversity of system into consideration, we think it's suitable to expose the interface
to support different configuration on different system for purpose of debugging and 
performance tuning.

Do you think it make sense for you?

Best Regards,
Li Fei

> 
> > 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.liu@intel.com>
> > Signed-off-by: Li Fei <fei.li@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..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.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH V4] suspend: enable freeze timeout configuration through sys

[Alan Stern]

On Mon, 4 Feb 2013, Li, Fei wrote:

> > -> the situation described above shouldn't happen and if it does, then there
> > is a bug that needs to be fixed.
> 
> Yes, we agree with that it's a bug that needs to be fixed, and we are already done
> for some cases. 
> During the process, we realize that tuning the timeout value to smaller value will help
> to expose freezing failure for some cases.

Besides, the underlying bug is well known (fuse filesystems hanging
because the fuse daemon gets frozen before the process doing file I/O,
or something like that) and so far nobody has a good idea of how to
fix it.

Alan Stern

RE: [PATCH V4] suspend: enable freeze timeout configuration through sys

[Liu, Chuansheng]

> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Monday, February 04, 2013 11:16 AM
> To: Li, Fei
> Cc: Rafael J. Wysocki; akpm@linux-foundation.org;
> linux-kernel@vger.kernel.org; linux-pm@vger.kernel.org; Liu, Chuansheng
> Subject: RE: [PATCH V4] suspend: enable freeze timeout configuration through
> sys
> 
> On Mon, 4 Feb 2013, Li, Fei wrote:
> 
> > > -> the situation described above shouldn't happen and if it does, then there
> > > is a bug that needs to be fixed.
> >
> > Yes, we agree with that it's a bug that needs to be fixed, and we are already
> done
> > for some cases.
> > During the process, we realize that tuning the timeout value to smaller value
> will help
> > to expose freezing failure for some cases.
> 
> Besides, the underlying bug is well known (fuse filesystems hanging
> because the fuse daemon gets frozen before the process doing file I/O,
> or something like that) and so far nobody has a good idea of how to
> fix it.
Indeed, we have some well-known deadlock issues existed there.
And once there is deadlock case, this patch is helpful to the user experience,
because 20s is pointless for those deadlock case.

Also even adjusting the 20s to 2s, we still can get to know the process stack who rejecting the freezing,
so this freezing configuration is really helpful to avoid 20s waiting for that deadlock case.
Alan and Rafael, could you consider this patch? Thanks.
> 
> Alan Stern

Re: [PATCH V4] suspend: enable freeze timeout configuration through sys

[Rafael J. Wysocki]

On Monday, February 04, 2013 04:32:11 AM Liu, Chuansheng wrote:
> 
> > -----Original Message-----
> > From: Alan Stern [mailto:stern@rowland.harvard.edu]
> > Sent: Monday, February 04, 2013 11:16 AM
> > To: Li, Fei
> > Cc: Rafael J. Wysocki; akpm@linux-foundation.org;
> > linux-kernel@vger.kernel.org; linux-pm@vger.kernel.org; Liu, Chuansheng
> > Subject: RE: [PATCH V4] suspend: enable freeze timeout configuration through
> > sys
> > 
> > On Mon, 4 Feb 2013, Li, Fei wrote:
> > 
> > > > -> the situation described above shouldn't happen and if it does, then there
> > > > is a bug that needs to be fixed.
> > >
> > > Yes, we agree with that it's a bug that needs to be fixed, and we are already
> > done
> > > for some cases.
> > > During the process, we realize that tuning the timeout value to smaller value
> > will help
> > > to expose freezing failure for some cases.
> > 
> > Besides, the underlying bug is well known (fuse filesystems hanging
> > because the fuse daemon gets frozen before the process doing file I/O,
> > or something like that) and so far nobody has a good idea of how to
> > fix it.
> Indeed, we have some well-known deadlock issues existed there.
> And once there is deadlock case, this patch is helpful to the user experience,
> because 20s is pointless for those deadlock case.
> 
> Also even adjusting the 20s to 2s, we still can get to know the process stack who rejecting the freezing,
> so this freezing configuration is really helpful to avoid 20s waiting for that deadlock case.
> Alan and Rafael, could you consider this patch? Thanks.

OK, I'll take it.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.