[PATCH -mm 0/3] Freezer: Use wait queue instead of busy looping

[PATCH -mm 0/3] Freezer: Use wait queue instead of busy looping

[Rafael J. Wysocki]

Hi,

The patches in the next three messages do the following:
* make the freezer a bit more verbose
* make try_to_freeze_tasks() go to sleep while waiting for tasks to enter
  the refrigerator instead of busy looping
* make try_to_freeze_tasks() measure the time of freezing, regardless of
  whether or not it is successful

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

[PATCH -mm 1/3] Freezer: Be more verbose

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Increase the freezer's verbosity a bit, so that it's easier to read problem
reports related to it.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Nigel Cunningham <nigel@nigel.suspend2.net>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 kernel/power/process.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Index: linux-2.6.23-rc1/kernel/power/process.c
===================================================================
--- linux-2.6.23-rc1.orig/kernel/power/process.c	2007-07-24 00:13:46.000000000 +0200
+++ linux-2.6.23-rc1/kernel/power/process.c	2007-07-24 00:13:58.000000000 +0200
@@ -227,18 +227,21 @@ int freeze_processes(void)
 {
 	int error;
 
-	printk("Stopping tasks ... ");
+	printk("Freezing user space processes ... ");
 	error = try_to_freeze_tasks(FREEZER_USER_SPACE);
 	if (error)
-		return error;
+		goto Exit;
+	printk("done.\n");
 
+	printk("Freezing remaining freezable tasks ... ");
 	error = try_to_freeze_tasks(FREEZER_KERNEL_THREADS);
 	if (error)
-		return error;
-
-	printk("done.\n");
+		goto Exit;
+	printk("done.");
+ Exit:
 	BUG_ON(in_atomic());
-	return 0;
+	printk("\n");
+	return error;
 }
 
 static void thaw_tasks(int thaw_user_space)

[PATCH -mm 2/3] Freezer: Use wait queue instead of busy looping

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Use the observation that try_to_freeze_tasks() need not loop while waiting for
the freezing tasks to enter the refrigerator and make it use a wait queue.

The idea is that after sending freeze requests to the tasks regarded as
freezable try_to_freeze_tasks() can go to sleep and wait until at least one task
enters the refrigerator.  The first task that does it wakes up
try_to_freeze_tasks() and the procedure is repeated.  If the refrigerator is not
entered by any tasks before TIMEOUT expires, the freezing of tasks fails.

This way, try_to_freeze_tasks() doesn't occupy the CPU unnecessarily when some
freezing tasks are waiting for I/O to complete.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 kernel/power/process.c |   34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

Index: linux-2.6.23-rc1/kernel/power/process.c
===================================================================
--- linux-2.6.23-rc1.orig/kernel/power/process.c	2007-07-31 21:58:32.000000000 +0200
+++ linux-2.6.23-rc1/kernel/power/process.c	2007-07-31 23:00:43.000000000 +0200
@@ -19,6 +19,12 @@
  */
 #define TIMEOUT	(20 * HZ)
 
+/*
+ * Time to wait until one or more tasks enter the refrigerator after sending
+ * freeze requests to them.
+ */
+#define WAIT_TIME (HZ / 5)
+
 #define FREEZER_KERNEL_THREADS 0
 #define FREEZER_USER_SPACE 1
 
@@ -43,6 +49,18 @@ static inline void frozen_process(void)
 	clear_freeze_flag(current);
 }
 
+/*
+ * Wait queue head used by try_to_freeze_tasks() to wait for tasks to enter the
+ * refrigerator.
+ */
+static DECLARE_WAIT_QUEUE_HEAD(refrigerator_waitq);
+
+/*
+ * Used to notify try_to_freeze_tasks() that the refrigerator has been entered
+ * by a task.
+ */
+static int refrigerator_called;
+
 /* Refrigerator is place where frozen processes are stored :-). */
 void refrigerator(void)
 {
@@ -58,6 +76,10 @@ void refrigerator(void)
 		task_unlock(current);
 		return;
 	}
+
+	refrigerator_called = 1;
+	wake_up(&refrigerator_waitq);
+
 	save = current->state;
 	pr_debug("%s entered refrigerator\n", current->comm);
 
@@ -170,6 +192,7 @@ static int try_to_freeze_tasks(int freez
 	unsigned int todo;
 
 	end_time = jiffies + TIMEOUT;
+	refrigerator_called = 0;
 	do {
 		todo = 0;
 		read_lock(&tasklist_lock);
@@ -189,7 +212,16 @@ static int try_to_freeze_tasks(int freez
 				todo++;
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
-		yield();			/* Yield is okay here */
+
+		if (todo) {
+			unsigned long ret;
+
+			ret = wait_event_timeout(refrigerator_waitq,
+					refrigerator_called, WAIT_TIME);
+			if (ret)
+				refrigerator_called = 0;
+		}
+
 		if (time_after(jiffies, end_time))
 			break;
 	} while (todo);

Re: [PATCH -mm 2/3] Freezer: Use wait queue instead of busy looping

[Andrew Morton]

On Wed, 1 Aug 2007 23:32:48 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> +/*
> + * Used to notify try_to_freeze_tasks() that the refrigerator has been entered
> + * by a task.
> + */
> +static int refrigerator_called;

this is rather icky.

Re: [PATCH -mm 2/3] Freezer: Use wait queue instead of busy looping

[Rafael J. Wysocki]

On Thursday, 2 August 2007 01:48, Andrew Morton wrote:
> On Wed, 1 Aug 2007 23:32:48 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > +/*
> > + * Used to notify try_to_freeze_tasks() that the refrigerator has been entered
> > + * by a task.
> > + */
> > +static int refrigerator_called;
> 
> this is rather icky.

Well, the alternative would be to open code something like wait_event_timeout.

[PATCH -mm 2/3] Freezer: Use wait queue instead of busy looping (updated)

[Rafael J. Wysocki]

On Thursday, 2 August 2007 13:15, Rafael J. Wysocki wrote:
> On Thursday, 2 August 2007 01:48, Andrew Morton wrote:
> > On Wed, 1 Aug 2007 23:32:48 +0200
> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > > +/*
> > > + * Used to notify try_to_freeze_tasks() that the refrigerator has been entered
> > > + * by a task.
> > > + */
> > > +static int refrigerator_called;
> > 
> > this is rather icky.
> 
> Well, the alternative would be to open code something like wait_event_timeout.

Still, having considered this for a while, I think it may be a good idea to
actually try this.

<modifies the patch, tests>

OK, here it goes (I hope I didn't messed up anything).  Please replace the
previous version with this patch if you prefer it. :-)

Greetings,
Rafael


---
From: Rafael J. Wysocki <rjw@sisk.pl>

Use the observation that try_to_freeze_tasks() need not loop while waiting for
the freezing tasks to enter the refrigerator and make it use a wait queue.

The idea is that after sending freeze requests to the tasks regarded as
freezable try_to_freeze_tasks() can go to sleep and wait until at least one task
enters the refrigerator.  The first task that does it wakes up
try_to_freeze_tasks() and the procedure is repeated.  If the refrigerator is not
entered by any tasks before TIMEOUT expires, the freezing of tasks fails.

This way, try_to_freeze_tasks() doesn't occupy the CPU unnecessarily when some
freezing tasks are waiting for I/O to complete.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/process.c |   26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Index: linux-2.6.23-rc1-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.23-rc1-mm2.orig/kernel/power/process.c
+++ linux-2.6.23-rc1-mm2/kernel/power/process.c
@@ -19,6 +19,12 @@
  */
 #define TIMEOUT	(20 * HZ)
 
+/*
+ * Time to wait until one or more tasks enter the refrigerator after sending
+ * freeze requests to them.
+ */
+#define WAIT_TIME (HZ / 5)
+
 #define FREEZER_KERNEL_THREADS 0
 #define FREEZER_USER_SPACE 1
 
@@ -43,6 +49,12 @@ static inline void frozen_process(void)
 	clear_freeze_flag(current);
 }
 
+/*
+ * Wait queue head used by try_to_freeze_tasks() to wait for tasks to enter the
+ * refrigerator.
+ */
+static DECLARE_WAIT_QUEUE_HEAD(refrigerator_waitq);
+
 /* Refrigerator is place where frozen processes are stored :-). */
 void refrigerator(void)
 {
@@ -58,6 +70,9 @@ void refrigerator(void)
 		task_unlock(current);
 		return;
 	}
+
+	wake_up(&refrigerator_waitq);
+
 	save = current->state;
 	pr_debug("%s entered refrigerator\n", current->comm);
 
@@ -171,6 +186,10 @@ static int try_to_freeze_tasks(int freez
 
 	end_time = jiffies + TIMEOUT;
 	do {
+		DEFINE_WAIT(wait);
+
+		add_wait_queue(&refrigerator_waitq, &wait);
+
 		todo = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
@@ -189,7 +208,12 @@ static int try_to_freeze_tasks(int freez
 				todo++;
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
-		yield();			/* Yield is okay here */
+
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		if (todo && !list_empty_careful(&wait.task_list))
+			schedule_timeout(WAIT_TIME);
+		finish_wait(&refrigerator_waitq, &wait);
+
 		if (time_after(jiffies, end_time))
 			break;
 	} while (todo);

Re: [PATCH -mm 2/3] Freezer: Use wait queue instead of busy looping (updated)

[Oleg Nesterov]

On 08/02, Rafael J. Wysocki wrote:
>
> @@ -171,6 +186,10 @@ static int try_to_freeze_tasks(int freez
>  
>  	end_time = jiffies + TIMEOUT;
>  	do {
> +		DEFINE_WAIT(wait);
> +
> +		add_wait_queue(&refrigerator_waitq, &wait);

Hmm. In that case I'd sugest to use prepare_to_wait(). This means that
multiple wakeups from refrigerator() won't do unnecessary work, and

> +
>  		todo = 0;
>  		read_lock(&tasklist_lock);
>  		do_each_thread(g, p) {
> @@ -189,7 +208,12 @@ static int try_to_freeze_tasks(int freez
>  				todo++;
>  		} while_each_thread(g, p);
>  		read_unlock(&tasklist_lock);
> -		yield();			/* Yield is okay here */
> +
> +		set_current_state(TASK_UNINTERRUPTIBLE);
> +		if (todo && !list_empty_careful(&wait.task_list))
> +			schedule_timeout(WAIT_TIME);

we don't need to check list_empty_careful() before schedule, prepare_to_wait()
sets TASK_UNINTERRUPTIBLE under wait_queue_head_t->lock.

Still, I personally agree with Pavel. Perhaps it is better to just replace
yield() with schedule_timeout(a_bit).

Oleg.

Re: [PATCH -mm 2/3] Freezer: Use wait queue instead of busy looping (updated)

[Rafael J. Wysocki]

On Thursday, 2 August 2007 20:40, Oleg Nesterov wrote:
> On 08/02, Rafael J. Wysocki wrote:
> >
> > @@ -171,6 +186,10 @@ static int try_to_freeze_tasks(int freez
> >  
> >  	end_time = jiffies + TIMEOUT;
> >  	do {
> > +		DEFINE_WAIT(wait);
> > +
> > +		add_wait_queue(&refrigerator_waitq, &wait);
> 
> Hmm. In that case I'd sugest to use prepare_to_wait(). This means that
> multiple wakeups from refrigerator() won't do unnecessary work,

I'm not sure what you mean.

Do you mean that if we are TASK_UNINTERRUPTIBLE, then the first wake up
should remove us from the queue?

> and 
> 
> > +
> >  		todo = 0;
> >  		read_lock(&tasklist_lock);
> >  		do_each_thread(g, p) {
> > @@ -189,7 +208,12 @@ static int try_to_freeze_tasks(int freez
> >  				todo++;
> >  		} while_each_thread(g, p);
> >  		read_unlock(&tasklist_lock);
> > -		yield();			/* Yield is okay here */
> > +
> > +		set_current_state(TASK_UNINTERRUPTIBLE);
> > +		if (todo && !list_empty_careful(&wait.task_list))
> > +			schedule_timeout(WAIT_TIME);
> 
> we don't need to check list_empty_careful() before schedule, prepare_to_wait()
> sets TASK_UNINTERRUPTIBLE under wait_queue_head_t->lock.

Yes.

> Still, I personally agree with Pavel. Perhaps it is better to just replace
> yield() with schedule_timeout(a_bit).

Hmm, I think that we shouldn't wait if that's not necessary.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH -mm 2/3] Freezer: Use wait queue instead of busy looping (updated)

[Oleg Nesterov]

On 08/02, Rafael J. Wysocki wrote:
>
> On Thursday, 2 August 2007 20:40, Oleg Nesterov wrote:
> > On 08/02, Rafael J. Wysocki wrote:
> > >
> > > @@ -171,6 +186,10 @@ static int try_to_freeze_tasks(int freez
> > >  
> > >  	end_time = jiffies + TIMEOUT;
> > >  	do {
> > > +		DEFINE_WAIT(wait);
> > > +
> > > +		add_wait_queue(&refrigerator_waitq, &wait);
> > 
> > Hmm. In that case I'd sugest to use prepare_to_wait(). This means that
> > multiple wakeups from refrigerator() won't do unnecessary work,
> 
> I'm not sure what you mean.
> 
> Do you mean that if we are TASK_UNINTERRUPTIBLE, then the first wake up
> should remove us from the queue?

No, not because we are TASK_UNINTERRUPTIBLE, but yes, first wake up will
remove us because DEFINE_WAIT() uses autoremove_wake_function().

Oleg.

Re: [PATCH -mm 2/3] Freezer: Use wait queue instead of busy looping (updated)

[Rafael J. Wysocki]

On Thursday, 2 August 2007 23:23, Oleg Nesterov wrote:
> On 08/02, Rafael J. Wysocki wrote:
> >
> > On Thursday, 2 August 2007 20:40, Oleg Nesterov wrote:
> > > On 08/02, Rafael J. Wysocki wrote:
> > > >
> > > > @@ -171,6 +186,10 @@ static int try_to_freeze_tasks(int freez
> > > >  
> > > >  	end_time = jiffies + TIMEOUT;
> > > >  	do {
> > > > +		DEFINE_WAIT(wait);
> > > > +
> > > > +		add_wait_queue(&refrigerator_waitq, &wait);
> > > 
> > > Hmm. In that case I'd sugest to use prepare_to_wait(). This means that
> > > multiple wakeups from refrigerator() won't do unnecessary work,
> > 
> > I'm not sure what you mean.
> > 
> > Do you mean that if we are TASK_UNINTERRUPTIBLE, then the first wake up
> > should remove us from the queue?
> 
> No, not because we are TASK_UNINTERRUPTIBLE, but yes, first wake up will
> remove us because DEFINE_WAIT() uses autoremove_wake_function().

Yes, it does, but the prepare_to_wait() version would only cause current to
become TASK_UNINTERRUPTIBLE before it sends freezing requests, the other
differences don't seem to matter.  I'm trying to understand why it would change
the behavior in the way you have described.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [PATCH -mm 2/3] Freezer: Use wait queue instead of busy looping (updated)

[Oleg Nesterov]

On 08/02, Rafael J. Wysocki wrote:
>
> On Thursday, 2 August 2007 23:23, Oleg Nesterov wrote:
> > On 08/02, Rafael J. Wysocki wrote:
> > >
> > > On Thursday, 2 August 2007 20:40, Oleg Nesterov wrote:
> > > > On 08/02, Rafael J. Wysocki wrote:
> > > > >
> > > > > @@ -171,6 +186,10 @@ static int try_to_freeze_tasks(int freez
> > > > >  
> > > > >  	end_time = jiffies + TIMEOUT;
> > > > >  	do {
> > > > > +		DEFINE_WAIT(wait);
> > > > > +
> > > > > +		add_wait_queue(&refrigerator_waitq, &wait);
> > > > 
> > > > Hmm. In that case I'd sugest to use prepare_to_wait(). This means that
> > > > multiple wakeups from refrigerator() won't do unnecessary work,
> > > 
> > > I'm not sure what you mean.
> > > 
> > > Do you mean that if we are TASK_UNINTERRUPTIBLE, then the first wake up
> > > should remove us from the queue?
> > 
> > No, not because we are TASK_UNINTERRUPTIBLE, but yes, first wake up will
> > remove us because DEFINE_WAIT() uses autoremove_wake_function().
> 
> Yes, it does, but the prepare_to_wait() version would only cause current to
> become TASK_UNINTERRUPTIBLE before it sends freezing requests, the other
> differences don't seem to matter.  I'm trying to understand why it would change
> the behavior in the way you have described.

Ugh, this is not the first time when I didn't read your patch carefully, sorry!

I missed that your patch already uses DEFINE_WAIT(), and I was confused by the
add_wait_queue() which is usually used along with DECLARE_WAITQUEUE().

Oleg.

[PATCH -mm 3/3] Freezer: Measure freezing time

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Measure the time of the freezing of tasks, even if it doesn't fail.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 kernel/power/process.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Index: linux-2.6.23-rc1/kernel/power/process.c
===================================================================
--- linux-2.6.23-rc1.orig/kernel/power/process.c	2007-07-31 23:00:43.000000000 +0200
+++ linux-2.6.23-rc1/kernel/power/process.c	2007-07-31 23:00:52.000000000 +0200
@@ -190,6 +190,11 @@ static int try_to_freeze_tasks(int freez
 	struct task_struct *g, *p;
 	unsigned long end_time;
 	unsigned int todo;
+	struct timeval start, end;
+	s64 elapsed_csecs64;
+	unsigned int elapsed_csecs;
+
+	do_gettimeofday(&start);
 
 	end_time = jiffies + TIMEOUT;
 	refrigerator_called = 0;
@@ -226,6 +231,11 @@ static int try_to_freeze_tasks(int freez
 			break;
 	} while (todo);
 
+	do_gettimeofday(&end);
+	elapsed_csecs64 = timeval_to_ns(&end) - timeval_to_ns(&start);
+	do_div(elapsed_csecs64, NSEC_PER_SEC / 100);
+	elapsed_csecs = elapsed_csecs64;
+
 	if (todo) {
 		/* This does not unfreeze processes that are already frozen
 		 * (we have slightly ugly calling convention in that respect,
@@ -233,10 +243,9 @@ static int try_to_freeze_tasks(int freez
 		 * but it cleans up leftover PF_FREEZE requests.
 		 */
 		printk("\n");
-		printk(KERN_ERR "Freezing of %s timed out after %d seconds "
+		printk(KERN_ERR "Freezing of tasks failed after %d.%02d seconds "
 				"(%d tasks refusing to freeze):\n",
-				freeze_user_space ? "user space " : "tasks ",
-				TIMEOUT / HZ, todo);
+				elapsed_csecs / 100, elapsed_csecs % 100, todo);
 		show_state();
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
@@ -247,6 +256,9 @@ static int try_to_freeze_tasks(int freez
 			task_unlock(p);
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
+	} else {
+		printk("(elapsed %d.%02d seconds) ", elapsed_csecs / 100,
+			elapsed_csecs % 100);
 	}
 
 	return todo ? -EBUSY : 0;

Re: [PATCH -mm 3/3] Freezer: Measure freezing time

[Andrew Morton]

On Wed, 1 Aug 2007 23:36:39 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> +	do_gettimeofday(&end);
> +	elapsed_csecs64 = timeval_to_ns(&end) - timeval_to_ns(&start);
> +	do_div(elapsed_csecs64, NSEC_PER_SEC / 100);
> +	elapsed_csecs = elapsed_csecs64;

I'd have thought that we had enough timeval library code by now to
not need to open-code things like this.

<looks around>

No, it seems that we don't.  So people keep on open-coding the same
thing, or inventing private code which shouldn't be.

<notices net/dccp/dccp.h>

What the hell is all that stuff doing in there?