[PATCH][mm][Fix] swsusp: fix counting of highmem pages

[PATCH][mm][Fix] swsusp: fix counting of highmem pages

[Rafael J. Wysocki]

Hi,

The following patch fixes a problem with swsusp that causes suspend to
fail on systems with the highmem zone, if many highmem pages are in use.

It makes swsusp count the non-free highmem pages in a correct way
and, consequently, release a sufficient amount of memory before suspend.

Please apply (Pavel, please ack if you think the patch is ok).

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 kernel/power/snapshot.c |   41 ++++++++++++++++++++++++++++-------------
 kernel/power/swsusp.c   |    3 ++-
 2 files changed, 30 insertions(+), 14 deletions(-)

Index: linux-2.6.15-rc3-mm1/kernel/power/snapshot.c
===================================================================
--- linux-2.6.15-rc3-mm1.orig/kernel/power/snapshot.c	2005-12-03 00:14:49.000000000 +0100
+++ linux-2.6.15-rc3-mm1/kernel/power/snapshot.c	2005-12-03 00:40:33.000000000 +0100
@@ -37,6 +37,22 @@
 unsigned int nr_copy_pages;
 
 #ifdef CONFIG_HIGHMEM
+struct highmem_page {
+	char *data;
+	struct page *page;
+	struct highmem_page *next;
+};
+
+static inline unsigned int get_kmalloc_size(void)
+{
+#define CACHE(x) \
+	if (sizeof(struct highmem_page) <= x) \
+		return x;
+#include <linux/kmalloc_sizes.h>
+#undef CACHE
+	return sizeof(struct highmem_page);
+}
+
 unsigned int count_highmem_pages(void)
 {
 	struct zone *zone;
@@ -52,22 +68,15 @@
 				if (!pfn_valid(pfn))
 					continue;
 				page = pfn_to_page(pfn);
-				if (PageReserved(page))
-					continue;
-				if (PageNosaveFree(page))
-					continue;
-				n++;
+				if (!PageNosaveFree(page) && !PageReserved(page))
+					n++;
 			}
 		}
+	if (n > 0)
+		n += (n * get_kmalloc_size() + PAGE_SIZE - 1) / PAGE_SIZE + 1;
 	return n;
 }
 
-struct highmem_page {
-	char *data;
-	struct page *page;
-	struct highmem_page *next;
-};
-
 static struct highmem_page *highmem_copy;
 
 static int save_highmem_zone(struct zone *zone)
@@ -437,8 +446,14 @@
 
 static int enough_free_mem(unsigned int nr_pages)
 {
-	pr_debug("swsusp: available memory: %u pages\n", nr_free_pages());
-	return nr_free_pages() > (nr_pages + PAGES_FOR_IO +
+	struct zone *zone;
+	unsigned int n = 0;
+
+	for_each_zone (zone)
+		if (!is_highmem(zone))
+			n += zone->free_pages;
+	pr_debug("swsusp: available memory: %u pages\n", n);
+	return n > (nr_pages + PAGES_FOR_IO +
 		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
 }
 
Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
===================================================================
--- linux-2.6.15-rc3-mm1.orig/kernel/power/swsusp.c	2005-12-03 00:14:49.000000000 +0100
+++ linux-2.6.15-rc3-mm1/kernel/power/swsusp.c	2005-12-03 21:25:07.000000000 +0100
@@ -635,7 +635,8 @@
 	printk("Shrinking memory...  ");
 	do {
 #ifdef FAST_FREE
-		tmp = count_data_pages() + count_highmem_pages();
+		tmp = 2 * count_highmem_pages();
+		tmp += tmp / 50 + count_data_pages();
 		tmp += (tmp + PBES_PER_PAGE - 1) / PBES_PER_PAGE +
 			PAGES_FOR_IO;
 		for_each_zone (zone)

Re: [PATCH][mm][Fix] swsusp: fix counting of highmem pages

[Pavel Machek]

Hi!

> The following patch fixes a problem with swsusp that causes suspend to
> fail on systems with the highmem zone, if many highmem pages are in use.
> 
> It makes swsusp count the non-free highmem pages in a correct way
> and, consequently, release a sufficient amount of memory before suspend.
> 
> Please apply (Pavel, please ack if you think the patch is ok).

Please don't, it's way too complex in my eyes. Sorry, result of
misscomunication between me and Rafael.

> +static inline unsigned int get_kmalloc_size(void)
> +{
> +#define CACHE(x) \
> +	if (sizeof(struct highmem_page) <= x) \
> +		return x;
> +#include <linux/kmalloc_sizes.h>
> +#undef CACHE
> +	return sizeof(struct highmem_page);
> +}
> +

Can we get rid of this uglyness...

> @@ -437,8 +446,14 @@
>  
>  static int enough_free_mem(unsigned int nr_pages)
>  {
> -	pr_debug("swsusp: available memory: %u pages\n", nr_free_pages());
> -	return nr_free_pages() > (nr_pages + PAGES_FOR_IO +
> +	struct zone *zone;
> +	unsigned int n = 0;
> +
> +	for_each_zone (zone)
> +		if (!is_highmem(zone))
> +			n += zone->free_pages;
> +	pr_debug("swsusp: available memory: %u pages\n", n);
> +	return n > (nr_pages + PAGES_FOR_IO +
>  		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
>  }
>  

And just use 2% approximation here, too?

> Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
> ===================================================================
> --- linux-2.6.15-rc3-mm1.orig/kernel/power/swsusp.c	2005-12-03 00:14:49.000000000 +0100
> +++ linux-2.6.15-rc3-mm1/kernel/power/swsusp.c	2005-12-03 21:25:07.000000000 +0100
> @@ -635,7 +635,8 @@
>  	printk("Shrinking memory...  ");
>  	do {
>  #ifdef FAST_FREE
> -		tmp = count_data_pages() + count_highmem_pages();
> +		tmp = 2 * count_highmem_pages();
> +		tmp += tmp / 50 + count_data_pages();
>  		tmp += (tmp + PBES_PER_PAGE - 1) / PBES_PER_PAGE +
>  			PAGES_FOR_IO;
>  		for_each_zone (zone)

This part is okay. Just make enough_free_mem use similar code. (If
possible, share the code, it is really computing the same thing).

								Pavel
-- 
Thanks, Sharp!

Re: [PATCH][mm][Fix] swsusp: fix counting of highmem pages

[Rafael J. Wysocki]

Hi,

[dropped Andrew from the CC list]

On Saturday, 3 of December 2005 22:40, Pavel Machek wrote:
> Hi!
> 
> > The following patch fixes a problem with swsusp that causes suspend to
> > fail on systems with the highmem zone, if many highmem pages are in use.
> > 
> > It makes swsusp count the non-free highmem pages in a correct way
> > and, consequently, release a sufficient amount of memory before suspend.
> > 
> > Please apply (Pavel, please ack if you think the patch is ok).
> 
> Please don't, it's way too complex in my eyes. Sorry, result of
> misscomunication between me and Rafael.
> 
> > +static inline unsigned int get_kmalloc_size(void)
> > +{
> > +#define CACHE(x) \
> > +	if (sizeof(struct highmem_page) <= x) \
> > +		return x;
> > +#include <linux/kmalloc_sizes.h>
> > +#undef CACHE
> > +	return sizeof(struct highmem_page);
> > +}
> > +
> 
> Can we get rid of this uglyness...

Sure, we can.

> > @@ -437,8 +446,14 @@
> >  
> >  static int enough_free_mem(unsigned int nr_pages)
> >  {
> > -	pr_debug("swsusp: available memory: %u pages\n", nr_free_pages());
> > -	return nr_free_pages() > (nr_pages + PAGES_FOR_IO +
> > +	struct zone *zone;
> > +	unsigned int n = 0;
> > +
> > +	for_each_zone (zone)
> > +		if (!is_highmem(zone))
> > +			n += zone->free_pages;
> > +	pr_debug("swsusp: available memory: %u pages\n", n);
> > +	return n > (nr_pages + PAGES_FOR_IO +
> >  		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
> >  }
> >  
> 
> And just use 2% approximation here, too?

Well, I don't think so.  It's checking free memory _after_ the highmem
pages have been "saved" (ie we are ready to create the image and just
check if there are enough non-highmem pages to do this).  Here we _know_
exactly how many pages are needed for the image, so we don't need to use
any "safety margins".

> > Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
> > ===================================================================
> > --- linux-2.6.15-rc3-mm1.orig/kernel/power/swsusp.c	2005-12-03 00:14:49.000000000 +0100
> > +++ linux-2.6.15-rc3-mm1/kernel/power/swsusp.c	2005-12-03 21:25:07.000000000 +0100
> > @@ -635,7 +635,8 @@
> >  	printk("Shrinking memory...  ");
> >  	do {
> >  #ifdef FAST_FREE
> > -		tmp = count_data_pages() + count_highmem_pages();
> > +		tmp = 2 * count_highmem_pages();
> > +		tmp += tmp / 50 + count_data_pages();
> >  		tmp += (tmp + PBES_PER_PAGE - 1) / PBES_PER_PAGE +
> >  			PAGES_FOR_IO;
> >  		for_each_zone (zone)
> 
> This part is okay. Just make enough_free_mem use similar code. (If
> possible, share the code, it is really computing the same thing).

enough_free_mem() must not take highmem into account, so it has
to use different code.  IOW, the current implementation is buggy,
so I'm trying to change it.

Greetings,
Rafael

Re: [PATCH][mm][Fix] swsusp: fix counting of highmem pages

[Pavel Machek]

Hi!

> > > +static inline unsigned int get_kmalloc_size(void)
> > > +{
> > > +#define CACHE(x) \
> > > +	if (sizeof(struct highmem_page) <= x) \
> > > +		return x;
> > > +#include <linux/kmalloc_sizes.h>
> > > +#undef CACHE
> > > +	return sizeof(struct highmem_page);
> > > +}
> > > +
> > 
> > Can we get rid of this uglyness...
> 
> Sure, we can.

Good.

> > > @@ -437,8 +446,14 @@
> > >  
> > >  static int enough_free_mem(unsigned int nr_pages)
> > >  {
> > > -	pr_debug("swsusp: available memory: %u pages\n", nr_free_pages());
> > > -	return nr_free_pages() > (nr_pages + PAGES_FOR_IO +
> > > +	struct zone *zone;
> > > +	unsigned int n = 0;
> > > +
> > > +	for_each_zone (zone)
> > > +		if (!is_highmem(zone))
> > > +			n += zone->free_pages;
> > > +	pr_debug("swsusp: available memory: %u pages\n", n);
> > > +	return n > (nr_pages + PAGES_FOR_IO +
> > >  		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
> > >  }
> > >  
> > 
> > And just use 2% approximation here, too?
> 
> Well, I don't think so.  It's checking free memory _after_ the highmem
> pages have been "saved" (ie we are ready to create the image and just
> check if there are enough non-highmem pages to do this).  Here we _know_
> exactly how many pages are needed for the image, so we don't need to use
> any "safety margins".

Ah, okay, I see. As long as the include hack is gone, its okay with me.

> > > Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
> > > ===================================================================
> > > --- linux-2.6.15-rc3-mm1.orig/kernel/power/swsusp.c	2005-12-03 00:14:49.000000000 +0100
> > > +++ linux-2.6.15-rc3-mm1/kernel/power/swsusp.c	2005-12-03 21:25:07.000000000 +0100
> > > @@ -635,7 +635,8 @@
> > >  	printk("Shrinking memory...  ");
> > >  	do {
> > >  #ifdef FAST_FREE
> > > -		tmp = count_data_pages() + count_highmem_pages();
> > > +		tmp = 2 * count_highmem_pages();
> > > +		tmp += tmp / 50 + count_data_pages();
> > >  		tmp += (tmp + PBES_PER_PAGE - 1) / PBES_PER_PAGE +
> > >  			PAGES_FOR_IO;
> > >  		for_each_zone (zone)
> > 
> > This part is okay. Just make enough_free_mem use similar code. (If
> > possible, share the code, it is really computing the same thing).
> 
> enough_free_mem() must not take highmem into account, so it has
> to use different code.  IOW, the current implementation is buggy,
> so I'm trying to change it.

Ok, sorry, I did not notice that.
								Pavel
-- 
Thanks, Sharp!

Re: [PATCH][mm][Fix] swsusp: fix counting of highmem pages

[Rafael J. Wysocki]

Hi,

On Sunday, 4 of December 2005 00:50, Pavel Machek wrote:
}-- snip --{
> 
> Ah, okay, I see. As long as the include hack is gone, its okay with me.

All right.  Appended is the latest version.

Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 kernel/power/snapshot.c |   25 ++++++++++++++++++-------
 kernel/power/swsusp.c   |    3 ++-
 2 files changed, 20 insertions(+), 8 deletions(-)

Index: linux-2.6.15-rc3-mm1/kernel/power/snapshot.c
===================================================================
--- linux-2.6.15-rc3-mm1.orig/kernel/power/snapshot.c	2005-12-03 00:14:49.000000000 +0100
+++ linux-2.6.15-rc3-mm1/kernel/power/snapshot.c	2005-12-04 00:35:14.000000000 +0100
@@ -37,6 +37,12 @@
 unsigned int nr_copy_pages;
 
 #ifdef CONFIG_HIGHMEM
+#if (PAGE_SIZE == 4096)
+#define KMALLOC_SIZE	32
+#else
+#define KMALLOC_SIZE	64
+#endif
+
 unsigned int count_highmem_pages(void)
 {
 	struct zone *zone;
@@ -52,13 +58,12 @@
 				if (!pfn_valid(pfn))
 					continue;
 				page = pfn_to_page(pfn);
-				if (PageReserved(page))
-					continue;
-				if (PageNosaveFree(page))
-					continue;
-				n++;
+				if (!PageNosaveFree(page) && !PageReserved(page))
+					n++;
 			}
 		}
+	if (n > 0)
+		n += (n * KMALLOC_SIZE + PAGE_SIZE - 1) / PAGE_SIZE + 1;
 	return n;
 }
 
@@ -437,8 +442,14 @@
 
 static int enough_free_mem(unsigned int nr_pages)
 {
-	pr_debug("swsusp: available memory: %u pages\n", nr_free_pages());
-	return nr_free_pages() > (nr_pages + PAGES_FOR_IO +
+	struct zone *zone;
+	unsigned int n = 0;
+
+	for_each_zone (zone)
+		if (!is_highmem(zone))
+			n += zone->free_pages;
+	pr_debug("swsusp: available memory: %u pages\n", n);
+	return n > (nr_pages + PAGES_FOR_IO +
 		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
 }
 
Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
===================================================================
--- linux-2.6.15-rc3-mm1.orig/kernel/power/swsusp.c	2005-12-03 00:14:49.000000000 +0100
+++ linux-2.6.15-rc3-mm1/kernel/power/swsusp.c	2005-12-03 21:25:07.000000000 +0100
@@ -635,7 +635,8 @@
 	printk("Shrinking memory...  ");
 	do {
 #ifdef FAST_FREE
-		tmp = count_data_pages() + count_highmem_pages();
+		tmp = 2 * count_highmem_pages();
+		tmp += tmp / 50 + count_data_pages();
 		tmp += (tmp + PBES_PER_PAGE - 1) / PBES_PER_PAGE +
 			PAGES_FOR_IO;
 		for_each_zone (zone)

Re: [PATCH][mm][Fix] swsusp: fix counting of highmem pages

[Pavel Machek]

Hi!

> > Ah, okay, I see. As long as the include hack is gone, its okay with me.
> 
> All right.  Appended is the latest version.

Okay, seems I'll need to get latest mm version, because this changed a
lot. Sorry, that will be tommorow afternoon.

> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
>  kernel/power/snapshot.c |   25 ++++++++++++++++++-------
>  kernel/power/swsusp.c   |    3 ++-
>  2 files changed, 20 insertions(+), 8 deletions(-)
> 
> Index: linux-2.6.15-rc3-mm1/kernel/power/snapshot.c
> ===================================================================
> --- linux-2.6.15-rc3-mm1.orig/kernel/power/snapshot.c	2005-12-03 00:14:49.000000000 +0100
> +++ linux-2.6.15-rc3-mm1/kernel/power/snapshot.c	2005-12-04 00:35:14.000000000 +0100
> @@ -37,6 +37,12 @@
> @@ -52,13 +58,12 @@
>  				if (!pfn_valid(pfn))
>  					continue;
>  				page = pfn_to_page(pfn);
> -				if (PageReserved(page))
> -					continue;
> -				if (PageNosaveFree(page))
> -					continue;
> -				n++;
> +				if (!PageNosaveFree(page) && !PageReserved(page))
> +					n++;
>  			}

As far as I can see, this does not change anything. Can you keep it
out?

>  		}
> +	if (n > 0)
> +		n += (n * KMALLOC_SIZE + PAGE_SIZE - 1) / PAGE_SIZE + 1;
>  	return n;
>  }

Can't you just n += n/50 here? Playing with KMALLOC_SIZE knows way too
much about memory allocation details.

> @@ -437,8 +442,14 @@
>  
>  static int enough_free_mem(unsigned int nr_pages)
>  {
> -	pr_debug("swsusp: available memory: %u pages\n", nr_free_pages());
> -	return nr_free_pages() > (nr_pages + PAGES_FOR_IO +
> +	struct zone *zone;
> +	unsigned int n = 0;
> +
> +	for_each_zone (zone)
> +		if (!is_highmem(zone))
> +			n += zone->free_pages;
> +	pr_debug("swsusp: available memory: %u pages\n", n);
> +	return n > (nr_pages + PAGES_FOR_IO +
>  		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
>  }
>  

Ok, so this is bugfix for different problem.

> Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
> ===================================================================
> --- linux-2.6.15-rc3-mm1.orig/kernel/power/swsusp.c	2005-12-03 00:14:49.000000000 +0100
> +++ linux-2.6.15-rc3-mm1/kernel/power/swsusp.c	2005-12-03 21:25:07.000000000 +0100
> @@ -635,7 +635,8 @@
>  	printk("Shrinking memory...  ");
>  	do {
>  #ifdef FAST_FREE
> -		tmp = count_data_pages() + count_highmem_pages();
> +		tmp = 2 * count_highmem_pages();
> +		tmp += tmp / 50 + count_data_pages();
>  		tmp += (tmp + PBES_PER_PAGE - 1) / PBES_PER_PAGE +
>  			PAGES_FOR_IO;
>  		for_each_zone (zone)

And this is simple enough...
							Pavel
-- 
Thanks, Sharp!

Re: [PATCH][mm][Fix] swsusp: fix counting of highmem pages

[Rafael J. Wysocki]

Hi,

On Sunday, 4 of December 2005 01:10, Pavel Machek wrote:
> > > Ah, okay, I see. As long as the include hack is gone, its okay with me.
> > 
> > All right.  Appended is the latest version.
> 
> Okay, seems I'll need to get latest mm version, because this changed a
> lot. Sorry, that will be tommorow afternoon.

OK

> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> >  kernel/power/snapshot.c |   25 ++++++++++++++++++-------
> >  kernel/power/swsusp.c   |    3 ++-
> >  2 files changed, 20 insertions(+), 8 deletions(-)
> > 
> > Index: linux-2.6.15-rc3-mm1/kernel/power/snapshot.c
> > ===================================================================
> > --- linux-2.6.15-rc3-mm1.orig/kernel/power/snapshot.c	2005-12-03 00:14:49.000000000 +0100
> > +++ linux-2.6.15-rc3-mm1/kernel/power/snapshot.c	2005-12-04 00:35:14.000000000 +0100
> > @@ -37,6 +37,12 @@
> > @@ -52,13 +58,12 @@
> >  				if (!pfn_valid(pfn))
> >  					continue;
> >  				page = pfn_to_page(pfn);
> > -				if (PageReserved(page))
> > -					continue;
> > -				if (PageNosaveFree(page))
> > -					continue;
> > -				n++;
> > +				if (!PageNosaveFree(page) && !PageReserved(page))
> > +					n++;
> >  			}
> 
> As far as I can see, this does not change anything. Can you keep it
> out?

OK

> >  		}
> > +	if (n > 0)
> > +		n += (n * KMALLOC_SIZE + PAGE_SIZE - 1) / PAGE_SIZE + 1;
> >  	return n;
> >  }
> 
> Can't you just n += n/50 here? Playing with KMALLOC_SIZE knows way too
> much about memory allocation details.

I do the "n + n/50" later on, so I can just drop all of the above changes
if they are too complicated.

Greetings,
Rafael

Re: [PATCH][mm][Fix] swsusp: fix counting of highmem pages

[Pavel Machek]

Hi!

> > >  		}
> > > +	if (n > 0)
> > > +		n += (n * KMALLOC_SIZE + PAGE_SIZE - 1) / PAGE_SIZE + 1;
> > >  	return n;
> > >  }
> > 
> > Can't you just n += n/50 here? Playing with KMALLOC_SIZE knows way too
> > much about memory allocation details.
> 
> I do the "n + n/50" later on, so I can just drop all of the above changes
> if they are too complicated.

Yes, that would be nice.
								Pavel
-- 
Thanks, Sharp!

Re: [PATCH][mm][Fix] swsusp: fix counting of highmem pages

[Rafael J. Wysocki]

Hi,

On Sunday, 4 of December 2005 01:35, Pavel Machek wrote:
> > > >  		}
> > > > +	if (n > 0)
> > > > +		n += (n * KMALLOC_SIZE + PAGE_SIZE - 1) / PAGE_SIZE + 1;
> > > >  	return n;
> > > >  }
> > > 
> > > Can't you just n += n/50 here? Playing with KMALLOC_SIZE knows way too
> > > much about memory allocation details.
> > 
> > I do the "n + n/50" later on, so I can just drop all of the above changes
> > if they are too complicated.
> 
> Yes, that would be nice.

OK

In which case I'll have only two changes fixing two different problems,
so I think they should go each in a separate patch.  Would that be ok?

Rafael