Software suspend testing in 2.6.0-test1

I have done some testing of the software suspend function in 2.6.0-test1. It works mostly very well, but I have found two problems.

The first problem is that software suspend fails if a process is stopped before you invoke suspend. (For example, by starting cat from the shell and pressing ctrl-z.) When the processes are woken up again, the cat process is stuck in the schedule loop in refrigerator(), sucking up all available cpu time.

The second problem is that freeing memory seems to be much slower than it has to be. It appears to be caused by the call to blk_congestion_wait() in balance_pgdat(). The patch below makes page freeing much faster, although I'm quite sure the patch is not correct.

How can we fix this properly? The disk is mostly idle during page freeing, but it looks like blk_congestion_wait still doesn't return until the timeout expires. I tried HZ/2 and that made the page freeing extremely slow.

mm/vmscan.c line 936

Quoted 12 lines

mm/vmscan.c renamed from mm/vmscan.c.old +1 -1

@@ -930,7 +930,7 @@
 		}
 		if (all_zones_ok)
 			break;
-		blk_congestion_wait(WRITE, HZ/10);
+		blk_congestion_wait(WRITE, HZ/50);
 	}
 	return nr_pages - to_free;
 }

This is certainly not okay. Andrew, you know more about vm internals... What does this ugly constant mean?

Quoted 15 lines · Pavel Machek

mm/vmscan.c line 936

Quoted 12 lines

mm/vmscan.c renamed from mm/vmscan.c.old +1 -1

@@ -930,7 +930,7 @@
 		}
 		if (all_zones_ok)
 			break;
-		blk_congestion_wait(WRITE, HZ/10);
+		blk_congestion_wait(WRITE, HZ/50);
 	}
 	return nr_pages - to_free;
 }

This is certainly not okay. Andrew, you know more about vm internals... What does this ugly constant mean?

Most of the time the timeout is a "can't happen" - blk_congestion_wait() is terminated by completion of writeout. The timeout is mainly there to prevent hangs if weird and rare races happen. Otherwise we'd need lots more locking.

I don't think we want to be calling it at all if reclaim is working well. Something like this.

If this patch is an acceptable approach to fix the problem,

the balance_pgdat function should probably be cleaned up.

Quoted 1 line

If this patch is an acceptable approach to fix the problem,

Seems reasonable.

Quoted 1 line

the balance_pgdat function should probably be cleaned up.

Well it was rather bolted on the side of the kswapd code. But from an API perspective, being able to tell it how many page to free is a bit more flexible. Minor point.

However I'm trying to remember why the code exists at all. Why doesn't swsusp just allocate lots of pages then free them again?

Something like:

	LIST_HEAD(list);
	int sleep_count = 0;

	while (sleep_count < 10) {
		page = __alloc_pages(0, GFP_ATOMIC);
		if (page) {
			list_add(&page->list, &list);
		} else {
			blk_congestion_wait(WRITE, HZ/20);
			sleep_count++;
		}
	}
	<free all the pages on `list'>

Quoted 2 lines

However I'm trying to remember why the code exists at all. Why doesn't swsusp just allocate lots of pages then free them again?

Because that either

a) does not free enough pages or

b) triggers OOM killer.

It was actually your idea, IIRC ;-).

Ahha, you seem to be addressing that in your code. Peter, perhaps you want to test that one?

I tried the patch below, but it didn't work. Nothing (or very little) was swapped out to disk. I also tried using GFP_KERNEL, but that seemed to cause a deadlock. (Maybe it would have gone OOM if I had waited long enough). I think the problem is that pdflush and friends are already frozen when this code runs.

Quoted 5 lines

I tried the patch below, but it didn't work. Nothing (or very little) was swapped out to disk. I also tried using GFP_KERNEL, but that seemed to cause a deadlock. (Maybe it would have gone OOM if I had waited long enough). I think the problem is that pdflush and friends are already frozen when this code runs.

Oh, we shouldn't be doing this sort of thing when the kernel threads are refrigerated. We do need kswapd services for the trick you tried.

And all flavours of ext3_writepage() can block on kjournald activity, so if kjournald is refrigerated during the memory shrink the machine can deadlock.

It would be much better to freeze kernel threads _after_ doing the big memory shrink.

I wanted to avoid that: we do want user threads refrigerated at that point so that we know noone is allocating memory as we are trying to do memory shrink. I'd like to avoid having refrigerator run in two phases....

Quoted 4 lines

I wanted to avoid that: we do want user threads refrigerated at that point so that we know noone is allocating memory as we are trying to do memory shrink. I'd like to avoid having refrigerator run in two phases....

But we should be the only process running, and we can guarantee that by not sleeping and doing preempt_disable() when we begin. Especially if we start the refrigeration sequence after we shrink memory. Right?

Quoted 3 lines

It would be much better to freeze kernel threads _after_ doing the big memory shrink.

I wanted to avoid that: we do want user threads refrigerated at that point so that we know noone is allocating memory as we are trying to do memory shrink.

I'd like to avoid having refrigerator run in two phases....

Quoted 6 lines · Peter Osterlund

I tried the patch below, but it didn't work. Nothing (or very little) was swapped out to disk. I also tried using GFP_KERNEL, but that seemed to cause a deadlock. (Maybe it would have gone OOM if I had waited long enough). I think the problem is that pdflush and friends are already frozen when this code runs.

Oh, we shouldn't be doing this sort of thing when the kernel threads are refrigerated. We do need kswapd services for the trick you tried.

And all flavours of ext3_writepage() can block on kjournald activity, so if kjournald is refrigerated during the memory shrink the machine can deadlock.

It would be much better to freeze kernel threads _after_ doing the big memory shrink.

The patch below works, but doesn't really solve anything, because it is just as slow as the original code.

This is not surprising because
it is still balance_pgdat() that does the page freeing, the only difference being that it is now called from kswapd which is woken up by the alloc_page() call.

Note that I had to use HZ/5 in free_some_memory() or else the loop would terminate too early. The main problem seems to be that balance_pgdat() is too slow when freeing memory. The function can fail to free memory in the inner loop either because the disk is congested or because too few pages are scanned, but in both cases the function calls blk_congestion_wait(), and in the second case the disk may be idle and then blk_congestion_wait() doesn't return until the timeout expires.

Oh, we shouldn't be doing this sort of thing when the kernel threads are refrigerated. We do need kswapd services for the trick you tried.

And all flavours of ext3_writepage() can block on kjournald activity, so if kjournald is refrigerated during the memory shrink the machine can deadlock.

It would be much better to freeze kernel threads _after_ doing the big memory shrink.

Hi!

Quoted 9 lines

Quoted 4 lines

I wanted to avoid that: we do want user threads refrigerated at that point so that we know noone is allocating memory as we are trying to do memory shrink. I'd like to avoid having refrigerator run in two phases....

But we should be the only process running, and we can guarantee that by not sleeping and doing preempt_disable() when we begin. Especially if we start the refrigeration sequence after we shrink memory. Right?

If we refrigerate after we shrink, userspace can allocate everything just after shrink.
Pavel

Quoted 2 lines

The patch below works, but doesn't really solve anything, because it is just as slow as the original code.

That's OK - it's a step in the right direction.

Yup. Probaby we should not be throttling kswapd if it is making good progress, but this needs broad testing and thought. Something like this, untested:

mm/vmscan.c renamed from mm/vmscan.c~less-kswapd-throttling +2 -1

@@ -930,7 +930,8 @@ static int balance_pgdat(pg_data_t *pgda
 		}
 		if (all_zones_ok)
 			break;
-		blk_congestion_wait(WRITE, HZ/10);
+		if (to_free)
+			blk_congestion_wait(WRITE, HZ/10);
 	}
 	return nr_pages - to_free;
 }

Quoted 10 lines

Note that I had to use HZ/5 in free_some_memory() or else the loop would terminate too early. The main problem seems to be that balance_pgdat() is too slow when freeing memory. The function can fail to free memory in the inner loop either because the disk is congested or because too few pages are scanned, but in both cases the function calls blk_congestion_wait(), and in the second case the disk may be idle and then blk_congestion_wait() doesn't return until the timeout expires.

hm, perhaps because kswapd is throttled rather than doing work.

Try the above change, see if you still need the extended timeout in free_some_memory().

Also, play around with using __GFP_WAIT|__GFP_HIGH|__GFP_NORETRY rather than GFP_ATOMIC.

I have tried the change, but the writeout is still very slow. (Maybe somewhat faster than the original code, but far from being limited by disk bandwidth.)

Quoted 4 lines · Peter Osterlund

I have tried the change, but the writeout is still very slow. (Maybe somewhat faster than the original code, but far from being limited by disk bandwidth.)

Did you fix swsusp to leave kswapd unrefrigerated during the shrink? If not, the change wouldn't make any difference.

Quoted 8 lines

I have done some testing of the software suspend function in 2.6.0-test1. It works mostly very well, but I have found two problems.

The first problem is that software suspend fails if a process is stopped before you invoke suspend. (For example, by starting cat from the shell and pressing ctrl-z.) When the processes are woken up again, the cat process is stuck in the schedule loop in refrigerator(), sucking up all available cpu time.

Thanks for a report. If it came with a patch it would be better ;-). I'll take a look.

Quoted 2 lines

Thanks for a report. If it came with a patch it would be better ;-). I'll take a look.

Here is a patch to make software suspend correctly handle stopped processes. It should apply cleanly on top of the patch you sent to Linus to decouple SOFTWARE_SUSPEND and ACPI_SLEEP.

kernel/suspend.c renamed from linux-stopped/kernel/suspend.c +3 -1

@@ -164,14 +164,18 @@
  * Refrigerator and related stuff
  */
 
-#define INTERESTING(p) > -			/* We don't want to touch kernel_threads..*/ > -			if (p->flags & PF_IOTHREAD) > -				continue; > -			if (p == current) > -				continue; > -			if (p->state == TASK_ZOMBIE) > -				continue;
+/* 0 = Ignore this process when freezing/thawing, 1 = freeze/thaw this process */
+static inline int interesting_process(struct task_struct *p)
+{

kernel/suspend.c renamed from linux-stopped/kernel/suspend.c +0 -0

@@ -214,9 +218,12 @@
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			unsigned long flags;

kernel/suspend.c renamed from linux-stopped/kernel/suspend.c +0 -5

 	read_lock(&tasklist_lock);
 	do_each_thread(g, p) {
-		INTERESTING(p);
-		
-		if (p->flags & PF_FROZEN) p->flags &= ~PF_FROZEN;
-		else
-			printk(KERN_INFO " Strange, %s not stopped\n", p->comm );

Quoted 12 lines

kernel/suspend.c renamed from linux-stopped/kernel/suspend.c +4 -1

@@ -214,9 +218,12 @@
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			unsigned long flags;
-			INTERESTING(p);
+			if (!interesting_process(p))
+				continue;
 			if (p->flags & PF_FROZEN)
 				continue;
+			if (p->state == TASK_STOPPED)
+				continue;
 

kernel/suspend.c line 229

Quoted 2 lines

kernel/suspend.c renamed from linux-stopped/kernel/suspend.c +0 -0

 			/* FIXME: smp problem here: we may not access other process' flags
 			   without locking */

No need to handle TASK_STOPPED tasks, as they are "frozen" already. Ok.

Quoted 12 lines

kernel/suspend.c renamed from linux-stopped/kernel/suspend.c +4 -6

 	read_lock(&tasklist_lock);
 	do_each_thread(g, p) {
-		INTERESTING(p);
-		
-		if (p->flags & PF_FROZEN) p->flags &= ~PF_FROZEN;
-		else
-			printk(KERN_INFO " Strange, %s not stopped\n", p->comm );
-		wake_up_process(p);
+		if (!interesting_process(p))
+			continue;
+
+		p->flags &= ~PF_FREEZE;

kernel/suspend.c

Quoted 5 lines

kernel/suspend.c renamed from linux-stopped/kernel/suspend.c +5 -0

+		if (p->flags & PF_FROZEN) {
+			p->flags &= ~PF_FROZEN;
+			wake_up_process(p);
+		} else
+			

But why do you touch PF_FROZEN here? Refrigerator should do that. And wake_up_process should not be needed...
If it is in refrigerator, it polls PF_FREEZE...

Quoted 3 lines

But why do you touch PF_FROZEN here? Refrigerator should do that. And wake_up_process should not be needed...
If it is in refrigerator, it polls PF_FREEZE...

Note that the old code always called wake_up_process(), which is necessary to make the process run one more iteration in refrigerator() and relize that it is time to unfreeze.

The patch changes things so that wake_up_process() is NOT called if the process is stopped at some other place than in refrigerator(). This ensures that processes that were stopped before we invoked swsusp are still stopped after resume.

I manually clear PF_FREEZE here in an attempt to handle a race condition, but I realize I need to understand more of the scheduler and signal code before I know for sure if this is necessary and/or sufficient.

Quoted 4 lines

Quoted 3 lines

But why do you touch PF_FROZEN here? Refrigerator should do that. And wake_up_process should not be needed...
If it is in refrigerator, it polls PF_FREEZE...

Quoted 8 lines

Note that the old code always called wake_up_process(), which is necessary to make the process run one more iteration in refrigerator() and relize that it is time to unfreeze.

The patch changes things so that wake_up_process() is NOT called if the process is stopped at some other place than in refrigerator(). This ensures that processes that were stopped before we invoked swsusp are still stopped after resume.

Yes, but you still print warning for them. I hopefully killed that.

usr/src/linux/kernel/suspend.c renamed from usr/src/tmp/linux/kernel/suspend.c +4 -0

+static inline int interesting_process(struct task_struct *p)
+{
+	if (p->flags & PF_IOTHREAD)
+		return 0;

Quoted 14 lines

Quoted 4 lines

Quoted 3 lines

But why do you touch PF_FROZEN here? Refrigerator should do that. And wake_up_process should not be needed...
If it is in refrigerator, it polls PF_FREEZE...

Quoted 8 lines

Note that the old code always called wake_up_process(), which is necessary to make the process run one more iteration in refrigerator() and relize that it is time to unfreeze.

The patch changes things so that wake_up_process() is NOT called if the process is stopped at some other place than in refrigerator(). This ensures that processes that were stopped before we invoked swsusp are still stopped after resume.

Yes, but you still print warning for them. I hopefully killed that.

...

Quoted 4 lines

usr/src/linux/kernel/suspend.c renamed from usr/src/tmp/linux/kernel/suspend.c +4 -0

+static inline int interesting_process(struct task_struct *p)
+{
+	if (p->flags & PF_IOTHREAD)
+		return 0;

usr/src/linux/kernel/suspend.c line 180

Quoted 10 lines

usr/src/linux/kernel/suspend.c renamed from usr/src/tmp/linux/kernel/suspend.c +10 -0

+	if (p == current)
+		return 0;
+	if ((p->state == TASK_ZOMBIE) || (p->state == TASK_DEAD))
+		return 0;
+	if (p->state == TASK_STOPPED)
+		return 0;
+
+
+	return 1;
+}

But this doesn't work. We can't skip stopped tasks in the thaw_processes() function, because frozen tasks are also stopped tasks. Therefore nothing will be woken up during resume.

It's probably best to just delete the printk("strange,...") line.