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

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Wednesday, 25 July 2007 16:24, Oleg Nesterov wrote:
> On 07/25, Rafael J. Wysocki wrote:
> >
> > On Wednesday, 25 July 2007 15:29, Oleg Nesterov wrote:
> > > On 07/25, Rafael J. Wysocki wrote:
> > > >
> > > >  void refrigerator(void)
> > > >  {
> > > > @@ -50,6 +73,9 @@ void refrigerator(void)
> > > >  	   processes around? */
> > > >  	long save;
> > > >
> > > > +	refrigerator_called = 1;
> > > > +	wake_up(&refrigerator_waitq);
> > > > +
> > > 
> > > This is a bit racy. Unless I missed something, the task should not set
> > > refrigerator_called == 1 until it has PF_FROZEN.
> > 
> > No, it's just to signal that the task has entered the refrigerator, not that
> > it has actually frozen.
> 
> Yes, I see.
> 
> > > Otherwise, try_to_freeze_tasks() can set refrigerator_called == 0 after
> > > refrigerator() sets it == 1, the the main loop notices this unfrozen task,
> > > and goes to sleep.
> > 
> > refrigerator_called is only reset after try_to_freeze_tasks() has found it
> > equal to one.  There is only a small window between checking it in
> > wait_event_timeout() and resetting it,
> 
> Yes.
> 
> > but then we go to send freeze requests
> > to the remaining tasks and we count 'todo' from the start, so that shouldn't
> > be a problem.
> 
> ... and we find the task which is not frozen() yet, but which has already passed
> the "set condition and wakeup", increment todo, and wait for the event. If it was
> the last task, we will sleep until timeout.
> 
> I agree, this is not fatal and unlikely, but still it is a race. I think it is
> better to move this code down, after frozen_process().

OK, I see your point.  The updated patch is appended.

> (offtopic: strictly speaking, we don't even need the "refrigerator_called", we
>  only need the wait_queue_head_t. try_to_freeze_tasks() just adds the "current"
>  to wq at the very start of the main loop).

Hmm, yes, I think so.

Greetings,
Rafael


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

Use the observation that try_to_freeze() 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() 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()
and the procedure is repeated.  If the refrigerator is not entered by any tasks
before TIMEOUT expires, try_to_freeze() increases the counter of expired
timeouts and sends freeze requests to the remaining tasks.  If the number of
expired timeouts becomes greater than MAX_WAITS, the freezing of tasks fails
(the counter of expired timeouts is reset whenever a task enters the
refrigerator).

This way, try_to_freeze() doesn't occupy the CPU unnecessarily when some
freezing tasks are waiting for I/O to complete and we have more fine grained
control over the freezing procedure.

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

Index: linux-2.6.23-rc1/kernel/power/process.c
===================================================================
--- linux-2.6.23-rc1.orig/kernel/power/process.c
+++ linux-2.6.23-rc1/kernel/power/process.c
@@ -13,11 +13,22 @@
 #include <linux/module.h>
 #include <linux/syscalls.h>
 #include <linux/freezer.h>
+#include <linux/time.h>
 
 /* 
- * Timeout for stopping processes
+ * Time to wait until one or more tasks enter the refrigerator after sending
+ * freeze requests to them.
  */
-#define TIMEOUT	(20 * HZ)
+#define TIMEOUT (HZ / 5)
+
+/*
+ * Each time after sending freeze requests to tasks the freezer will wait until
+ * some of them enter the refrigerater, but no longer than TIMEOUT.  If TIMEOUT
+ * has been exceeded, the freezer increases the number of waits by one and
+ * repeats.  If the number of waits becomes greater than MAX_WAITS, the
+ * freezing fails.
+ */
+#define MAX_WAITS 5
 
 #define FREEZER_KERNEL_THREADS 0
 #define FREEZER_USER_SPACE 1
@@ -43,6 +54,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 signal 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 +81,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);
 
@@ -166,10 +193,16 @@ static void cancel_freezing(struct task_
 static int try_to_freeze_tasks(int freeze_user_space)
 {
 	struct task_struct *g, *p;
-	unsigned long end_time;
-	unsigned int todo;
+	unsigned int todo, waits;
+	unsigned long ret;
+	struct timeval start, end;
+	s64 elapsed_csecs64;
+	unsigned int elapsed_csecs;
+
+	do_gettimeofday(&start);
 
-	end_time = jiffies + TIMEOUT;
+	refrigerator_called = 0;
+	waits = 0;
 	do {
 		todo = 0;
 		read_lock(&tasklist_lock);
@@ -189,11 +222,25 @@ static int try_to_freeze_tasks(int freez
 				todo++;
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
-		yield();			/* Yield is okay here */
-		if (time_after(jiffies, end_time))
-			break;
+
+		if (todo) {
+			ret = wait_event_timeout(refrigerator_waitq,
+						refrigerator_called, TIMEOUT);
+			if (!ret) {
+				if (++waits > MAX_WAITS)
+					break;
+			} else {
+				refrigerator_called = 0;
+				waits = 0;
+			}
+		}
 	} 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,
@@ -201,10 +248,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.%d 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) {
@@ -215,6 +261,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.%d seconds) ", elapsed_csecs / 100,
+			elapsed_csecs % 100);
 	}
 
 	return todo ? -EBUSY : 0;