swsusp: fix counting of highmem pages

PATCH In review Rafael J. Wysocki 2 participants 9 messages Dec 3, 2005 – Dec 4, 2005

Subsystems (2)

Suspend to RAM Hibernation

lore

Share & tools

lore

Rafael J. Wysocki

author

2005-12-03 20:52 UTC #0 lore raw

PATCH 1/1 swsusp: fix counting of highmem pages

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 <[email protected]>

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

kernel/power/snapshot.c +28 ~~-13~~

Index: linux-2.6.15-rc3-mm1/kernel/power/snapshot.c
===================================================================
@@ -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);
 }

kernel/power/swsusp.c +2 -1

Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
===================================================================
@@ -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)

Pavel Machek

2005-12-03 21:40 UTC #1 lore raw

Hi!

Quoted 7 lines

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.

kernel/power/snapshot.c line 55

Quoted 10 lines

kernel/power/snapshot.c +10 -0

+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...

Quoted 7 lines

kernel/power/snapshot.c +1 -2

@@ -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;

kernel/power/snapshot.c line 459

Quoted 10 lines

kernel/power/snapshot.c +7 -0

+	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?

Quoted 5 lines

Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
===================================================================

kernel/power/swsusp.c line 642

Quoted 9 lines

kernel/power/swsusp.c +2 -1

@@ -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

Signature · 1 line

Thanks, Sharp!

Rafael J. Wysocki

author

2005-12-03 23:10 UTC #2 lore raw

Hi,

[dropped Andrew from the CC list]

Quoted 13 lines · Pavel Machek

Hi!
Quoted 7 lines
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.

Quoted 12 lines

kernel/power/snapshot.c line 55

Quoted 10 lines

kernel/power/snapshot.c +10 -0

+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.

Quoted 11 lines

kernel/power/snapshot.c +5 -2

@@ -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))

Quoted 8 lines

kernel/power/snapshot.c line 459
Quoted 6 lines
kernel/power/snapshot.c +3 -0
+			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".

Quoted 9 lines

Quoted 9 lines
Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
===================================================================
kernel/power/swsusp.c +0 -1
@@ -635,7 +635,8 @@
 	printk("Shrinking memory...  ");
 	do {
 #ifdef FAST_FREE
-		tmp = count_data_pages() + count_highmem_pages();

Quoted 8 lines

kernel/power/swsusp.c line 642
Quoted 5 lines
kernel/power/swsusp.c +2 -0
+		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

Pavel Machek

2005-12-03 23:51 UTC #3 lore raw

Hi!

Quoted 13 lines

Quoted 4 lines
Quoted 4 lines
kernel/power/snapshot.c +4 -0
+static inline unsigned int get_kmalloc_size(void)
+{
+#define CACHE(x) \
+	if (sizeof(struct highmem_page) <= x) \
Quoted 8 lines
kernel/power/snapshot.c line 55
Quoted 6 lines
kernel/power/snapshot.c +6 -0
+		return x;
+#include <linux/kmalloc_sizes.h>
+#undef CACHE
+	return sizeof(struct highmem_page);
+}
+
Can we get rid of this uglyness...

Quoted 1 line

Sure, we can.

Good.

Quoted 20 lines

Quoted 11 lines

kernel/power/snapshot.c +5 -2

@@ -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))

Quoted 8 lines

kernel/power/snapshot.c line 459
Quoted 6 lines
kernel/power/snapshot.c +3 -0
+			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?

Quoted 5 lines

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.

Quoted 18 lines

Quoted 9 lines
Quoted 9 lines
Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
===================================================================
kernel/power/swsusp.c +0 -1
@@ -635,7 +635,8 @@
 	printk("Shrinking memory...  ");
 	do {
 #ifdef FAST_FREE
-		tmp = count_data_pages() + count_highmem_pages();
Quoted 8 lines
kernel/power/swsusp.c line 642
Quoted 5 lines
kernel/power/swsusp.c +2 -0
+		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).

Quoted 3 lines

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

Signature · 1 line

Thanks, Sharp!

Rafael J. Wysocki

author

2005-12-04 00:01 UTC #4 lore raw

Hi,

On Sunday, 4 of December 2005 00:50, Pavel Machek wrote:
}-- snip --{

Quoted 2 lines

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 <[email protected]>

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

kernel/power/snapshot.c +18 -7

Index: linux-2.6.15-rc3-mm1/kernel/power/snapshot.c
===================================================================
@@ -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);
 }

kernel/power/swsusp.c +2 -1

Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
===================================================================
@@ -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)

Pavel Machek

2005-12-04 00:11 UTC #5 lore raw

Hi!

Quoted 3 lines

Quoted 1 line
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.

Quoted 13 lines

Signed-off-by Rafael J. Wysocki <[email protected]>
 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
===================================================================
kernel/power/snapshot.c +0 -0
@@ -37,6 +37,12 @@
@@ -52,13 +58,12 @@
 				if (!pfn_valid(pfn))

kernel/power/snapshot.c line 63

Quoted 10 lines

kernel/power/snapshot.c +2 -5

 					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?

kernel/power/snapshot.c line 68

Quoted 5 lines

kernel/power/snapshot.c +2 -0

 		}
+	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.

Quoted 7 lines

kernel/power/snapshot.c +1 -2

@@ -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;

kernel/power/snapshot.c line 455

Quoted 10 lines

kernel/power/snapshot.c +7 -0

+	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.

Quoted 5 lines

Index: linux-2.6.15-rc3-mm1/kernel/power/swsusp.c
===================================================================

kernel/power/swsusp.c line 642

Quoted 9 lines

kernel/power/swsusp.c +2 -1

@@ -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

Signature · 1 line

Thanks, Sharp!

Rafael J. Wysocki

author

2005-12-04 00:25 UTC #6 lore raw

Hi,

Quoted 6 lines · Pavel Machek

Quoted 3 lines
Quoted 1 line
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.

Quoted 18 lines

Quoted 18 lines
Signed-off-by Rafael J. Wysocki <[email protected]>
 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
===================================================================
kernel/power/snapshot.c +0 -3
@@ -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))

Quoted 8 lines

kernel/power/snapshot.c line 63
Quoted 5 lines
kernel/power/snapshot.c +2 -2
-					continue;
-				n++;
+				if (!PageNosaveFree(page) && !PageReserved(page))
+					n++;
 			}
As far as I can see, this does not change anything. Can you keep it out?

Quoted 8 lines

kernel/power/snapshot.c line 68
Quoted 5 lines
kernel/power/snapshot.c +2 -0
 		}
+	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

Pavel Machek

2005-12-04 00:36 UTC #7 lore raw

Hi!

Quoted 11 lines

Quoted 8 lines
kernel/power/snapshot.c line 68
Quoted 5 lines
kernel/power/snapshot.c +2 -0
 		}
+	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

Signature · 1 line

Thanks, Sharp!

Rafael J. Wysocki

author

2005-12-04 00:56 UTC #8 lore raw

Hi,

Quoted 9 lines · Pavel Machek

Quoted 9 lines
Quoted 8 lines
kernel/power/snapshot.c line 68
Quoted 5 lines
kernel/power/snapshot.c +2 -0
 		}
+	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.

Quoted 4 lines

Quoted 2 lines
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.

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