[RFC][PATCH -mm 0/2] mm: shrink_all_memory improvements

[RFC][PATCH -mm 0/2] mm: shrink_all_memory improvements

[Rafael J. Wysocki]

Hi,

The following two patches are designed to improve the shrink_all_memory()
function used by swsusp and other pm functions.

The first patch makes shrink_all_memory() overflow-resistant.  The problem is
that, AFAICT, balance_pgdat(pgdat, nr_to_free, 0) may free more than nr_to_free
pages, in which case nr_to_free, being unsigned, will overflow (and obviously
it cannot be less than 0).  Also if the argument is too big, strange things may
happen.

The first patch adds a workaround of the problem that shrink_all_memory() may
return 0 even if there still are some pages to free.  WIth this patch applied
it sometimes frees 2 times as many pages as without it on my box.

Please have a look and comment.

Greetings,
Rafael


-- 
Beer is proof that God loves us and wants us to be happy - Benjamin Franklin

[RFC][PATCH -mm 1/2] mm: make shrink_all_memory overflow-resistant

[Rafael J. Wysocki]

Make shrink_all_memory() overflow-resistant.


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/swap.h |    2 +-
 mm/vmscan.c          |    9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

Index: linux-2.6.16-rc4-mm2/mm/vmscan.c
===================================================================
--- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
+++ linux-2.6.16-rc4-mm2/mm/vmscan.c
@@ -1785,18 +1785,19 @@ void wakeup_kswapd(struct zone *zone, in
  * Try to free `nr_pages' of memory, system-wide.  Returns the number of freed
  * pages.
  */
-int shrink_all_memory(unsigned long nr_pages)
+unsigned long shrink_all_memory(unsigned int nr_pages)
 {
 	pg_data_t *pgdat;
-	unsigned long nr_to_free = nr_pages;
-	int ret = 0;
+	long long nr_to_free = nr_pages;
+	unsigned long ret = 0;
 	struct reclaim_state reclaim_state = {
 		.reclaimed_slab = 0,
 	};
 
 	current->reclaim_state = &reclaim_state;
 	for_each_pgdat(pgdat) {
-		int freed;
+		unsigned long freed;
+
 		freed = balance_pgdat(pgdat, nr_to_free, 0);
 		ret += freed;
 		nr_to_free -= freed;
Index: linux-2.6.16-rc4-mm2/include/linux/swap.h
===================================================================
--- linux-2.6.16-rc4-mm2.orig/include/linux/swap.h
+++ linux-2.6.16-rc4-mm2/include/linux/swap.h
@@ -173,7 +173,7 @@ extern void swap_setup(void);
 
 /* linux/mm/vmscan.c */
 extern unsigned long try_to_free_pages(struct zone **, gfp_t);
-extern int shrink_all_memory(unsigned long nr_pages);
+extern unsigned long shrink_all_memory(unsigned int nr_pages);
 extern int vm_swappiness;
 
 #ifdef CONFIG_NUMA

Re: [RFC][PATCH -mm 1/2] mm: make shrink_all_memory overflow-resistant

[Jesper Juhl]

On 2/27/06, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Make shrink_all_memory() overflow-resistant.
>
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  include/linux/swap.h |    2 +-
>  mm/vmscan.c          |    9 +++++----
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> Index: linux-2.6.16-rc4-mm2/mm/vmscan.c
> ===================================================================
> --- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
> +++ linux-2.6.16-rc4-mm2/mm/vmscan.c
> @@ -1785,18 +1785,19 @@ void wakeup_kswapd(struct zone *zone, in
>   * Try to free `nr_pages' of memory, system-wide.  Returns the number of freed
>   * pages.
>   */
> -int shrink_all_memory(unsigned long nr_pages)
> +unsigned long shrink_all_memory(unsigned int nr_pages)

What about the callers of shrink_all_memory() who currently expect it
to return an int, have you checked how they will react to you changing
the return type (and signedness) ?

>  {
>         pg_data_t *pgdat;
> -       unsigned long nr_to_free = nr_pages;
> -       int ret = 0;
> +       long long nr_to_free = nr_pages;

'nr_pages' passed to the function is an unsigned int, why this change?
also, nr_to_free is later passed to balance_pgdat() as the second
argument and balance_pgdat() expects to be passed an int.


> +       unsigned long ret = 0;
>         struct reclaim_state reclaim_state = {
>                 .reclaimed_slab = 0,
>         };
>
>         current->reclaim_state = &reclaim_state;
>         for_each_pgdat(pgdat) {
> -               int freed;
> +               unsigned long freed;

balance_pgdat() returns a plain int, so what's the point of making
'freed' an unsigned long?


> +
>                 freed = balance_pgdat(pgdat, nr_to_free, 0);
>                 ret += freed;
>                 nr_to_free -= freed;
> Index: linux-2.6.16-rc4-mm2/include/linux/swap.h
> ===================================================================
> --- linux-2.6.16-rc4-mm2.orig/include/linux/swap.h
> +++ linux-2.6.16-rc4-mm2/include/linux/swap.h
> @@ -173,7 +173,7 @@ extern void swap_setup(void);
>
>  /* linux/mm/vmscan.c */
>  extern unsigned long try_to_free_pages(struct zone **, gfp_t);
> -extern int shrink_all_memory(unsigned long nr_pages);
> +extern unsigned long shrink_all_memory(unsigned int nr_pages);
>  extern int vm_swappiness;
>
>  #ifdef CONFIG_NUMA
>


This patch may be correct or it may be wrong, I've not spend enough
time staring at it and follow the code it calls and gets called by to
say which, but I for one would like an explanation of why these
changes are made and why they are correct.
There's a distinct lack of a changelog/explanation with this patch.
Or maybe I'm just dense and can't see the obvious correctness, but if
that's the case I'd still like to be enlightened :)


--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [RFC][PATCH -mm 1/2] mm: make shrink_all_memory overflow-resistant

[Pavel Machek]

On Po 27-02-06 19:53:47, Jesper Juhl wrote:
> On 2/27/06, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Make shrink_all_memory() overflow-resistant.
> >
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  include/linux/swap.h |    2 +-
> >  mm/vmscan.c          |    9 +++++----
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> >
> > Index: linux-2.6.16-rc4-mm2/mm/vmscan.c
> > ===================================================================
> > --- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
> > +++ linux-2.6.16-rc4-mm2/mm/vmscan.c
> > @@ -1785,18 +1785,19 @@ void wakeup_kswapd(struct zone *zone, in
> >   * Try to free `nr_pages' of memory, system-wide.  Returns the number of freed
> >   * pages.
> >   */
> > -int shrink_all_memory(unsigned long nr_pages)
> > +unsigned long shrink_all_memory(unsigned int nr_pages)
> 
> What about the callers of shrink_all_memory() who currently expect it
> to return an int, have you checked how they will react to you changing
> the return type (and signedness) ?

That's okay, we checked all 3 callers :-).
									Pavel
-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

Re: [RFC][PATCH -mm 1/2] mm: make shrink_all_memory overflow-resistant

[Jesper Juhl]

On 2/27/06, Pavel Machek <pavel@suse.cz> wrote:
> On Po 27-02-06 19:53:47, Jesper Juhl wrote:
> > On 2/27/06, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > Make shrink_all_memory() overflow-resistant.
> > >
> > >
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >  include/linux/swap.h |    2 +-
> > >  mm/vmscan.c          |    9 +++++----
> > >  2 files changed, 6 insertions(+), 5 deletions(-)
> > >
> > > Index: linux-2.6.16-rc4-mm2/mm/vmscan.c
> > > ===================================================================
> > > --- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
> > > +++ linux-2.6.16-rc4-mm2/mm/vmscan.c
> > > @@ -1785,18 +1785,19 @@ void wakeup_kswapd(struct zone *zone, in
> > >   * Try to free `nr_pages' of memory, system-wide.  Returns the number of freed
> > >   * pages.
> > >   */
> > > -int shrink_all_memory(unsigned long nr_pages)
> > > +unsigned long shrink_all_memory(unsigned int nr_pages)
> >
> > What about the callers of shrink_all_memory() who currently expect it
> > to return an int, have you checked how they will react to you changing
> > the return type (and signedness) ?
>
> That's okay, we checked all 3 callers :-).

I'm sure you did, I'm not saying you didn't.
All I'm asking for is a short explanation of why the changes the patch
makes are correct since that's not obvious to me, and I'd like to
understand the patch.

--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [RFC][PATCH -mm 1/2] mm: make shrink_all_memory overflow-resistant

[Rafael J. Wysocki]

Hi,

On Monday 27 February 2006 19:53, Jesper Juhl wrote:
> On 2/27/06, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Make shrink_all_memory() overflow-resistant.
> >
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  include/linux/swap.h |    2 +-
> >  mm/vmscan.c          |    9 +++++----
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> >
> > Index: linux-2.6.16-rc4-mm2/mm/vmscan.c
> > ===================================================================
> > --- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
> > +++ linux-2.6.16-rc4-mm2/mm/vmscan.c
> > @@ -1785,18 +1785,19 @@ void wakeup_kswapd(struct zone *zone, in
> >   * Try to free `nr_pages' of memory, system-wide.  Returns the number of freed
> >   * pages.
> >   */
> > -int shrink_all_memory(unsigned long nr_pages)
> > +unsigned long shrink_all_memory(unsigned int nr_pages)
> 
> What about the callers of shrink_all_memory() who currently expect it
> to return an int, have you checked how they will react to you changing
> the return type (and signedness) ?

AFAICT, there are not any.

> >  {
> >         pg_data_t *pgdat;
> > -       unsigned long nr_to_free = nr_pages;
> > -       int ret = 0;
> > +       long long nr_to_free = nr_pages;
> 
> 'nr_pages' passed to the function is an unsigned int, why this change?
> also, nr_to_free is later passed to balance_pgdat() as the second
> argument and balance_pgdat() expects to be passed an int.

First, the balance_pgdat() in the current -mm expects to be passed
unsigned long here.

Second, say you pass 100 and balance_pgdat() returns 110.  Then if
nr_to_free is unsigned, it will become a huge positive number instead of
a negative number, as it should be.  Thus it has to be signed IMO.

It could be long, but on i386 long is the same as int.

> > +       unsigned long ret = 0;
> >         struct reclaim_state reclaim_state = {
> >                 .reclaimed_slab = 0,
> >         };
> >
> >         current->reclaim_state = &reclaim_state;
> >         for_each_pgdat(pgdat) {
> > -               int freed;
> > +               unsigned long freed;
> 
> balance_pgdat() returns a plain int, so what's the point of making
> 'freed' an unsigned long?

Well, at least the balance_pgdat() I'm looking at now returns
unsigned long.

> > +
> >                 freed = balance_pgdat(pgdat, nr_to_free, 0);
> >                 ret += freed;
> >                 nr_to_free -= freed;
> > Index: linux-2.6.16-rc4-mm2/include/linux/swap.h
> > ===================================================================
> > --- linux-2.6.16-rc4-mm2.orig/include/linux/swap.h
> > +++ linux-2.6.16-rc4-mm2/include/linux/swap.h
> > @@ -173,7 +173,7 @@ extern void swap_setup(void);
> >
> >  /* linux/mm/vmscan.c */
> >  extern unsigned long try_to_free_pages(struct zone **, gfp_t);
> > -extern int shrink_all_memory(unsigned long nr_pages);
> > +extern unsigned long shrink_all_memory(unsigned int nr_pages);
> >  extern int vm_swappiness;
> >
> >  #ifdef CONFIG_NUMA
> >
> 
> 
> This patch may be correct or it may be wrong, I've not spend enough
> time staring at it and follow the code it calls and gets called by to
> say which, but I for one would like an explanation of why these
> changes are made and why they are correct.
> There's a distinct lack of a changelog/explanation with this patch.
> Or maybe I'm just dense and can't see the obvious correctness, but if
> that's the case I'd still like to be enlightened :)

Perhaps I should have explained it in a more detailed way in the
changelog.

The problem is that, AFAICT, balance_pgdat(pgdat, nr_to_free, 0) may free
more than nr_to_free pages, in which case nr_to_free, being unsigned, will
overflow (and obviously it cannot be less than 0).  Also if nr_pages is
too big, strange things may happen (without the patch, that is).

Greetings,
Rafael

Re: [RFC][PATCH -mm 1/2] mm: make shrink_all_memory overflow-resistant

[Andrew Morton]

"Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> Make shrink_all_memory() overflow-resistant.
> 

As you've probably noticed, I'm moving toward making all the scalars which
hold counts of pages in the VM toward unsigned long.  For uniformity, and
because there is a small risk that a huge machine with 4k pages could
actually overflow a 32-bit counter.

> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  include/linux/swap.h |    2 +-
>  mm/vmscan.c          |    9 +++++----
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> Index: linux-2.6.16-rc4-mm2/mm/vmscan.c
> ===================================================================
> --- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
> +++ linux-2.6.16-rc4-mm2/mm/vmscan.c
> @@ -1785,18 +1785,19 @@ void wakeup_kswapd(struct zone *zone, in
>   * Try to free `nr_pages' of memory, system-wide.  Returns the number of freed
>   * pages.
>   */
> -int shrink_all_memory(unsigned long nr_pages)
> +unsigned long shrink_all_memory(unsigned int nr_pages)

So nr_pages should remain unsigned long.

> +	long long nr_to_free = nr_pages;

As should nr_to_free.

The actual bug is the (unsigned <= 0) thing.   So how's about this?

--- devel/include/linux/swap.h~vmscan-use-unsigned-longs-shrink_all_memory-fix	2006-02-27 19:13:30.000000000 -0800
+++ devel-akpm/include/linux/swap.h	2006-02-27 19:13:30.000000000 -0800
@@ -173,7 +173,7 @@ extern void swap_setup(void);
 
 /* linux/mm/vmscan.c */
 extern unsigned long try_to_free_pages(struct zone **, gfp_t);
-extern int shrink_all_memory(unsigned long nr_pages);
+extern unsigned long shrink_all_memory(unsigned long nr_pages);
 extern int vm_swappiness;
 
 #ifdef CONFIG_NUMA
--- devel/mm/vmscan.c~vmscan-use-unsigned-longs-shrink_all_memory-fix	2006-02-27 19:13:30.000000000 -0800
+++ devel-akpm/mm/vmscan.c	2006-02-27 19:13:30.000000000 -0800
@@ -1779,22 +1779,23 @@ void wakeup_kswapd(struct zone *zone, in
  * Try to free `nr_pages' of memory, system-wide.  Returns the number of freed
  * pages.
  */
-int shrink_all_memory(unsigned long nr_pages)
+unsigned long shrink_all_memory(unsigned long nr_pages)
 {
 	pg_data_t *pgdat;
 	unsigned long nr_to_free = nr_pages;
-	int ret = 0;
+	unsigned long ret = 0;
 	struct reclaim_state reclaim_state = {
 		.reclaimed_slab = 0,
 	};
 
 	current->reclaim_state = &reclaim_state;
 	for_each_pgdat(pgdat) {
-		int freed;
+		unsigned long freed;
+
 		freed = balance_pgdat(pgdat, nr_to_free, 0);
 		ret += freed;
 		nr_to_free -= freed;
-		if (nr_to_free <= 0)
+		if ((long)nr_to_free <= 0)
 			break;
 	}
 	current->reclaim_state = NULL;
_

Re: [RFC][PATCH -mm 1/2] mm: make shrink_all_memory overflow-resistant

[Rafael J. Wysocki]

On Tuesday 28 February 2006 04:16, Andrew Morton wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> >
> > Make shrink_all_memory() overflow-resistant.
> > 
> 
> As you've probably noticed, I'm moving toward making all the scalars which
> hold counts of pages in the VM toward unsigned long.  For uniformity, and
> because there is a small risk that a huge machine with 4k pages could
> actually overflow a 32-bit counter.
> 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  include/linux/swap.h |    2 +-
> >  mm/vmscan.c          |    9 +++++----
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > Index: linux-2.6.16-rc4-mm2/mm/vmscan.c
> > ===================================================================
> > --- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
> > +++ linux-2.6.16-rc4-mm2/mm/vmscan.c
> > @@ -1785,18 +1785,19 @@ void wakeup_kswapd(struct zone *zone, in
> >   * Try to free `nr_pages' of memory, system-wide.  Returns the number of freed
> >   * pages.
> >   */
> > -int shrink_all_memory(unsigned long nr_pages)
> > +unsigned long shrink_all_memory(unsigned int nr_pages)
> 
> So nr_pages should remain unsigned long.
> 
> > +	long long nr_to_free = nr_pages;
> 
> As should nr_to_free.

OK

> The actual bug is the (unsigned <= 0) thing.   So how's about this?

I like it (if my opinion matters here ;-)).  Thanks a lot.

[RFC][PATCH -mm 2/2] mm: make shrink_all_memory try harder

[Rafael J. Wysocki]

Make shrink_all_memory() repeat the attempts to free more memory if there
seems to be no pages to free.


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 mm/vmscan.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

Index: linux-2.6.16-rc4-mm2/mm/vmscan.c
===================================================================
--- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
+++ linux-2.6.16-rc4-mm2/mm/vmscan.c
@@ -33,6 +33,7 @@
 #include <linux/cpuset.h>
 #include <linux/notifier.h>
 #include <linux/rwsem.h>
+#include <linux/delay.h>
 
 #include <asm/tlbflush.h>
 #include <asm/div64.h>
@@ -1793,17 +1794,24 @@ unsigned long shrink_all_memory(unsigned
 	struct reclaim_state reclaim_state = {
 		.reclaimed_slab = 0,
 	};
+	int retry = 2;
 
 	current->reclaim_state = &reclaim_state;
-	for_each_pgdat(pgdat) {
-		unsigned long freed;
+	do {
+		for_each_pgdat(pgdat) {
+			unsigned long freed;
 
-		freed = balance_pgdat(pgdat, nr_to_free, 0);
-		ret += freed;
-		nr_to_free -= freed;
-		if (nr_to_free <= 0)
+			freed = balance_pgdat(pgdat, nr_to_free, 0);
+			ret += freed;
+			nr_to_free -= freed;
+			if (nr_to_free <= 0)
+				break;
+		}
+		if (ret > 0)
 			break;
-	}
+		if (retry)
+			msleep_interruptible(100);
+	} while (retry--);
 	current->reclaim_state = NULL;
 	return ret;
 }

Re: [RFC][PATCH -mm 2/2] mm: make shrink_all_memory try harder

[Andrew Morton]

"Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> Make shrink_all_memory() repeat the attempts to free more memory if there
> seems to be no pages to free.
> 

This description doesn't describe what the problem is, not how the patch
fixes it.  So I'm kinda left guessing.

> ===================================================================
> --- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
> +++ linux-2.6.16-rc4-mm2/mm/vmscan.c
> @@ -33,6 +33,7 @@
>  #include <linux/cpuset.h>
>  #include <linux/notifier.h>
>  #include <linux/rwsem.h>
> +#include <linux/delay.h>
>  
>  #include <asm/tlbflush.h>
>  #include <asm/div64.h>
> @@ -1793,17 +1794,24 @@ unsigned long shrink_all_memory(unsigned
>  	struct reclaim_state reclaim_state = {
>  		.reclaimed_slab = 0,
>  	};
> +	int retry = 2;
>  
>  	current->reclaim_state = &reclaim_state;
> -	for_each_pgdat(pgdat) {
> -		unsigned long freed;
> +	do {
> +		for_each_pgdat(pgdat) {
> +			unsigned long freed;
>  
> -		freed = balance_pgdat(pgdat, nr_to_free, 0);
> -		ret += freed;
> -		nr_to_free -= freed;
> -		if (nr_to_free <= 0)
> +			freed = balance_pgdat(pgdat, nr_to_free, 0);
> +			ret += freed;
> +			nr_to_free -= freed;
> +			if (nr_to_free <= 0)
> +				break;
> +		}
> +		if (ret > 0)
>  			break;
> -	}
> +		if (retry)
> +			msleep_interruptible(100);

TASK_INTERRUPTIBLE sleeps won't do anything if someone has sent this
process a signal.    They should be used with caution.


Something like the below, I guess.  But it's hard to fix something when you
don't know what you're fixing.

swsusp should call drop_pagecache() and then drop_slab() before trying to
use shrink_all_memory(), btw.


diff -puN mm/vmscan.c~mm-make-shrink_all_memory-try-harder mm/vmscan.c
--- devel/mm/vmscan.c~mm-make-shrink_all_memory-try-harder	2006-02-27 19:17:29.000000000 -0800
+++ devel-akpm/mm/vmscan.c	2006-02-27 19:21:08.000000000 -0800
@@ -33,6 +33,7 @@
 #include <linux/cpuset.h>
 #include <linux/notifier.h>
 #include <linux/rwsem.h>
+#include <linux/delay.h>
 
 #include <asm/tlbflush.h>
 #include <asm/div64.h>
@@ -1783,11 +1784,13 @@ unsigned long shrink_all_memory(unsigned
 	pg_data_t *pgdat;
 	unsigned long nr_to_free = nr_pages;
 	unsigned long ret = 0;
+	unsigned retry = 2;
 	struct reclaim_state reclaim_state = {
 		.reclaimed_slab = 0,
 	};
 
 	current->reclaim_state = &reclaim_state;
+repeat:
 	for_each_pgdat(pgdat) {
 		unsigned long freed;
 
@@ -1797,6 +1800,10 @@ unsigned long shrink_all_memory(unsigned
 		if ((long)nr_to_free <= 0)
 			break;
 	}
+	if (retry-- && ret < nr_pages) {
+		blk_congestion_wait(WRITE, HZ/5);
+		goto repeat;
+	}
 	current->reclaim_state = NULL;
 	return ret;
 }
_

But then, the above could be implemented by the caller, so I don't know
what's going on..

Re: [RFC][PATCH -mm 2/2] mm: make shrink_all_memory try harder

[Rafael J. Wysocki]

On Tuesday 28 February 2006 04:25, Andrew Morton wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> >
> > Make shrink_all_memory() repeat the attempts to free more memory if there
> > seems to be no pages to free.
> > 
> 
> This description doesn't describe what the problem is, not how the patch
> fixes it.  So I'm kinda left guessing.

I have described it in the 0/0 message, but I should have repeated that in the
changelog, sorry.

The probelm is that shrink_all_memory() sometimes returns 0 when there are
some freeable pages left.  In swsusp we stop freeing memory when
shrink_all_memory() returns 0, so this is a problem to us.

> > ===================================================================
> > --- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
> > +++ linux-2.6.16-rc4-mm2/mm/vmscan.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/cpuset.h>
> >  #include <linux/notifier.h>
> >  #include <linux/rwsem.h>
> > +#include <linux/delay.h>
> >  
> >  #include <asm/tlbflush.h>
> >  #include <asm/div64.h>
> > @@ -1793,17 +1794,24 @@ unsigned long shrink_all_memory(unsigned
> >  	struct reclaim_state reclaim_state = {
> >  		.reclaimed_slab = 0,
> >  	};
> > +	int retry = 2;
> >  
> >  	current->reclaim_state = &reclaim_state;
> > -	for_each_pgdat(pgdat) {
> > -		unsigned long freed;
> > +	do {
> > +		for_each_pgdat(pgdat) {
> > +			unsigned long freed;
> >  
> > -		freed = balance_pgdat(pgdat, nr_to_free, 0);
> > -		ret += freed;
> > -		nr_to_free -= freed;
> > -		if (nr_to_free <= 0)
> > +			freed = balance_pgdat(pgdat, nr_to_free, 0);
> > +			ret += freed;
> > +			nr_to_free -= freed;
> > +			if (nr_to_free <= 0)
> > +				break;
> > +		}
> > +		if (ret > 0)
> >  			break;
> > -	}
> > +		if (retry)
> > +			msleep_interruptible(100);
> 
> TASK_INTERRUPTIBLE sleeps won't do anything if someone has sent this
> process a signal.    They should be used with caution.
> 
> 
> Something like the below, I guess.  But it's hard to fix something when you
> don't know what you're fixing.

Sorry again.

> swsusp should call drop_pagecache() and then drop_slab() before trying to
> use shrink_all_memory(), btw.

Well, sometimes we don't need to free a lot of memory.

> diff -puN mm/vmscan.c~mm-make-shrink_all_memory-try-harder mm/vmscan.c
> --- devel/mm/vmscan.c~mm-make-shrink_all_memory-try-harder	2006-02-27 19:17:29.000000000 -0800
> +++ devel-akpm/mm/vmscan.c	2006-02-27 19:21:08.000000000 -0800
> @@ -33,6 +33,7 @@
>  #include <linux/cpuset.h>
>  #include <linux/notifier.h>
>  #include <linux/rwsem.h>
> +#include <linux/delay.h>
>  
>  #include <asm/tlbflush.h>
>  #include <asm/div64.h>
> @@ -1783,11 +1784,13 @@ unsigned long shrink_all_memory(unsigned
>  	pg_data_t *pgdat;
>  	unsigned long nr_to_free = nr_pages;
>  	unsigned long ret = 0;
> +	unsigned retry = 2;
>  	struct reclaim_state reclaim_state = {
>  		.reclaimed_slab = 0,
>  	};
>  
>  	current->reclaim_state = &reclaim_state;
> +repeat:
>  	for_each_pgdat(pgdat) {
>  		unsigned long freed;
>  
> @@ -1797,6 +1800,10 @@ unsigned long shrink_all_memory(unsigned
>  		if ((long)nr_to_free <= 0)
>  			break;
>  	}
> +	if (retry-- && ret < nr_pages) {
> +		blk_congestion_wait(WRITE, HZ/5);
> +		goto repeat;
> +	}

I'd like to do this only if ret is 0.

>  	current->reclaim_state = NULL;
>  	return ret;
>  }
> _
> 
> But then, the above could be implemented by the caller, so I don't know
> what's going on..

If 0 is returned, the caller assumes there are no more freeable pages.

Re: [RFC][PATCH -mm 2/2] mm: make shrink_all_memory try harder

[Andrew Morton]

"Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> On Tuesday 28 February 2006 04:25, Andrew Morton wrote:
> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > >
> > > Make shrink_all_memory() repeat the attempts to free more memory if there
> > > seems to be no pages to free.
> > > 
> > 
> > This description doesn't describe what the problem is, not how the patch
> > fixes it.  So I'm kinda left guessing.
> 
> I have described it in the 0/0 message, but I should have repeated that in the
> changelog, sorry.

Actually these [patch 0/n] emails are a nuisance - some poor schmuck just
has to copy-n-paste that text into the fist patch's changelog anwyay.

> > swsusp should call drop_pagecache() and then drop_slab() before trying to
> > use shrink_all_memory(), btw.
> 
> Well, sometimes we don't need to free a lot of memory.

OK.  But if clean pagecache and reclaimable slabs are left in memory,
they'll have to be written to swap, won't they?

It could well be more efficent to restore them from swap.  Slower suspend,
faster resume.

> > +	if (retry-- && ret < nr_pages) {
> > +		blk_congestion_wait(WRITE, HZ/5);
> > +		goto repeat;
> > +	}
> 
> I'd like to do this only if ret is 0.

Well I figured that this was a more general approach: we were _asked_ to
free that many pages.  If we haven't done that yet, keep trying.  Can you
test that code please?

Re: [RFC][PATCH -mm 2/2] mm: make shrink_all_memory try harder

[Rafael J. Wysocki]

On Tuesday 28 February 2006 19:46, Andrew Morton wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> >
> > On Tuesday 28 February 2006 04:25, Andrew Morton wrote:
> > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > > >
> > > > Make shrink_all_memory() repeat the attempts to free more memory if there
> > > > seems to be no pages to free.
> > > > 
> > > 
> > > This description doesn't describe what the problem is, not how the patch
> > > fixes it.  So I'm kinda left guessing.
> > 
> > I have described it in the 0/0 message, but I should have repeated that in the
> > changelog, sorry.
> 
> Actually these [patch 0/n] emails are a nuisance - some poor schmuck just
> has to copy-n-paste that text into the fist patch's changelog anwyay.
> 
> > > swsusp should call drop_pagecache() and then drop_slab() before trying to
> > > use shrink_all_memory(), btw.
> > 
> > Well, sometimes we don't need to free a lot of memory.
> 
> OK.  But if clean pagecache and reclaimable slabs are left in memory,
> they'll have to be written to swap, won't they?

Yes, but then we write them more or less linearly and we can also compress
them. :-)
 
> It could well be more efficent to restore them from swap.  Slower suspend,
> faster resume.
> 
> > > +	if (retry-- && ret < nr_pages) {
> > > +		blk_congestion_wait(WRITE, HZ/5);
> > > +		goto repeat;
> > > +	}
> > 
> > I'd like to do this only if ret is 0.
> 
> Well I figured that this was a more general approach: we were _asked_ to
> free that many pages.  If we haven't done that yet, keep trying.  Can you
> test that code please?

So far, it works just fine.  Thanks.

[Tested on 2.6.16-rc4-mm2, because -rc5-mm1 crashes on my system in a
spectacular way (already reported separately).]

Re: [RFC][PATCH -mm 0/2] mm: shrink_all_memory improvements

[Pavel Machek]

Hi!

> The following two patches are designed to improve the shrink_all_memory()
> function used by swsusp and other pm functions.
> 
> The first patch makes shrink_all_memory() overflow-resistant.  The problem is
> that, AFAICT, balance_pgdat(pgdat, nr_to_free, 0) may free more than nr_to_free
> pages, in which case nr_to_free, being unsigned, will overflow (and obviously
> it cannot be less than 0).  Also if the argument is too big, strange things may
> happen.
> 
> The first patch adds a workaround of the problem that shrink_all_memory() may
> return 0 even if there still are some pages to free.  WIth this patch applied
> it sometimes frees 2 times as many pages as without it on my box.
> 
> Please have a look and comment.

ACK on the first one, Andrew, please suggest how to do the second one
better.
								Pavel
-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...