PROBLEM: __offline_isolated_pages may offline too many pages

PATCH New Ingo Korb 3 participants 6 messages Dec 14, 2010 – Dec 17, 2010

Subsystems (2)

Memory management Memory hot(un)plug

lore

Share & tools

lore

Ingo Korb

author

2010-12-14 15:01 UTC #0 lore raw

Hi!

[1.] One line summary of the problem:
__offline_isolated_pages may isolate too many pages

[2.] Full description of the problem/report:
While experimenting with remove_memory/online_pages, removing as few pages as possible (pageblock_nr_pages, 512 on my box) I noticed that the number of pages marked "reserved" increased even though both functions did not indicate an error. Following the code it was clear that __offline_isolated_pages marked twice as many pages as it should:

=== start paste (from dmesg) ===
Offlined Pages 512
remove from free list c00 1024 e00
=== end paste ===

The issue seems to be that __offline_isolated_pages blindly uses page_order() to determine how many pages it should mark as reserved in the current loop iteration, without checking if this would exceed the limit set by end_pfn.

I'm not sure what the correct way to fix this would be - is memory isolation supposed to touch the order of a page if it crosses the end (or beginning!) of the range of pages to be isolated?

[3.] Keywords (i.e., modules, networking, kernel):
kernel mm memory-hotplug

[4.] Kernel information
[4.1.] Kernel version (from /proc/version):
Linux version 2.6.35-00002-g76c52bb (ingo@memtester) (gcc version 4.4.5 (Debian 4.4.5-6) ) #7 SMP Tue Dec 14 14:28:17 CET 2010

The diff between vanilla 2.6.35 and this version is available at http://akana.de/memtest35.diff - the only changes are a reduced timeout in remove_memory and a bunch of additional exported symbols.

[4.2.] Kernel .config file:
http://akana.de/config-memtest35

[5.] Most recent kernel version which did not have the bug:
Probably none

[8.] Environment
[8.1.] Software (add the output of the ver_linux script here)
Linux memtester 2.6.35-00002-g76c52bb #7 SMP Tue Dec 14 14:28:17 CET 2010 x86_64 GNU/Linux

Gnu C                  4.4.5
Gnu make               3.81
binutils               2.20.1
util-linux             (no fdformat on the system)
mount                  support
module-init-tools      found
Linux C Library        2.11.2
Dynamic linker (ldd)   2.11.2
Procps                 3.2.8
Kbd                    1.15.2
Sh-utils               8.5
Modules Loaded         phys_mem ipv6 pcspkr i2c_piix4 i2c_core shpchp e1000

Distribution is Debian testing if it matters

[8.2.] Processor information (from /proc/cpuinfo):
AMD Phenom 9650, but the system is running inside a VMWare Player instance with just a single virtual CPU

[8.3.] Module information (from /proc/modules):
phys_mem 15068 0 - Live 0xffffffffa00d1000 ipv6 340746 24 - Live 0xffffffffa0068000 pcspkr 2022 0 - Live 0xffffffffa0062000 i2c_piix4 13334 0 - Live 0xffffffffa0059000 i2c_core 28244 1 i2c_piix4, Live 0xffffffffa004b000 shpchp 35612 0 - Live 0xffffffffa003b000 e1000 164575 0 - Live 0xffffffffa0000000

[8.4.] Loaded driver and hardware information (/proc/ioports, /proc/iomem) [8.5.] PCI information ('lspci -vvv' as root)
[8.6.] SCSI information (from /proc/scsi/scsi)
As far as I can tell irrelevant to this problem?
(forgot to copy those, will add later if neccessary)

-ik

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

KAMEZAWA Hiroyuki

2010-12-15 00:27 UTC #1 lore raw

Quoted 5 lines · On Tue, 14 Dec 2010 16:01:39 +0100 Ingo Korb <[email protected]> wrote:

Hi!
[1.] One line summary of the problem:
__offline_isolated_pages may isolate too many pages

Quoted 7 lines

[2.] Full description of the problem/report:
While experimenting with remove_memory/online_pages, removing as few pages as possible (pageblock_nr_pages, 512 on my box) I noticed that the number of pages marked "reserved" increased even though both functions did not indicate an error. Following the code it was clear that __offline_isolated_pages marked twice as many pages as it should:

It's designed for offline memory section > MAX_ORDER. pageblock_nr_pages is tend to be smaller than that.

Do you see the problem with _exsisting_ user interface of memory hotplug ? I think we have no control other than memory section.

Quoted 10 lines

=== start paste (from dmesg) ===
Offlined Pages 512
remove from free list c00 1024 e00
=== end paste ===
The issue seems to be that __offline_isolated_pages blindly uses page_order() to determine how many pages it should mark as reserved in the current loop iteration, without checking if this would exceed the limit set by end_pfn.

It's because designed to work under memory section, it's aligned to MAX_ORDER. Its blindness works correctly.

Quoted 4 lines

I'm not sure what the correct way to fix this would be - is memory isolation supposed to touch the order of a page if it crosses the end (or beginning!) of the range of pages to be isolated?

Nothing to be fixed. If you _need_ another functionality, please add a new feature. But, in theory, memory offline doesn't work in the range smaller than MAX_ORDER because of buddy allocator.

Thanks,
-Kame

Ingo Korb

author

2010-12-15 09:26 UTC #2 lore raw

Quoted 5 lines · KAMEZAWA Hiroyuki

It's designed for offline memory section> MAX_ORDER. pageblock_nr_pages is tend to be smaller than that.
Do you see the problem with _exsisting_ user interface of memory hotplug ? I think we have no control other than memory section.

The existing, exported interface (remove_memory() - the check itself is in offline_pages()) only checks if both start and end of the to-be-removed block are aligned to pageblock_nr_pages. As you noted the actual size and alignment requirements in __offline_isolated_pages can be larger that that, so I think the checks in offline_pages() should be changed (if 1<<MAX_ORDER is always >= pageblock_nr_pages) or extended (if there can be any relation between the two).

-ik

KAMEZAWA Hiroyuki

2010-12-16 00:13 UTC #3 lore raw

Quoted 8 lines · On Wed, 15 Dec 2010 10:25:51 +0100 Ingo Korb <[email protected]> wrote:

Quoted 5 lines · KAMEZAWA Hiroyuki
It's designed for offline memory section> MAX_ORDER. pageblock_nr_pages is tend to be smaller than that.
Do you see the problem with _exsisting_ user interface of memory hotplug ? I think we have no control other than memory section.

Quoted 8 lines

The existing, exported interface (remove_memory() - the check itself is in offline_pages()) only checks if both start and end of the to-be-removed block are aligned to pageblock_nr_pages. As you noted the actual size and alignment requirements in __offline_isolated_pages can be larger that that, so I think the checks in offline_pages() should be changed (if 1<<MAX_ORDER is always >= pageblock_nr_pages) or extended (if there can be any relation between the two).

Ok, maybe my mistake. This is a fix. Thank you for reporting. ==

offline_pages()'s sanity check of given range is wrong. It should be aligned to MAX_ORDER. Current exsiting caller uses SECTION_SIZE alignment, so this change has no influence to exsisting callers.

Reported-by Ingo Korb <[email protected]>

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 mm/memory_hotplug.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

mm/memory_hotplug.c +7 -3

Index: linux-2.6.37-rc5/mm/memory_hotplug.c
===================================================================
@@ -798,10 +798,14 @@ static int offline_pages(unsigned long s
 	struct memory_notify arg;
 
 	BUG_ON(start_pfn >= end_pfn);
-	/* at least, alignment against pageblock is necessary */
-	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
+	/*
+	 * Considering buddy allocator which joins nearby pages, the range
+	 * in offline should be aligned to MAX_ORDER. If not, isolated
+	 * page will be joined to other (not isolated) pages.
+	 */
+	if (!IS_ALIGNED(start_pfn, MAX_ORDER_NR_PAGES))
 		return -EINVAL;
-	if (!IS_ALIGNED(end_pfn, pageblock_nr_pages))
+	if (!IS_ALIGNED(end_pfn, MAX_ORDER_NR_PAGES))
 		return -EINVAL;
 	/* This makes hotplug much easier...and readable.
 	   we assume this for now. .*/

Mel Gorman

Acked-by

2010-12-16 10:27 UTC #4 lore raw

Quoted 23 lines · KAMEZAWA Hiroyuki

Quoted 8 lines · On Wed, 15 Dec 2010 10:25:51 +0100 Ingo Korb <[email protected]> wrote:
Quoted 5 lines · KAMEZAWA Hiroyuki
It's designed for offline memory section> MAX_ORDER. pageblock_nr_pages is tend to be smaller than that.
Do you see the problem with _exsisting_ user interface of memory hotplug ? I think we have no control other than memory section.
Quoted 8 lines
The existing, exported interface (remove_memory() - the check itself is in offline_pages()) only checks if both start and end of the to-be-removed block are aligned to pageblock_nr_pages. As you noted the actual size and alignment requirements in __offline_isolated_pages can be larger that that, so I think the checks in offline_pages() should be changed (if 1<<MAX_ORDER is always >= pageblock_nr_pages) or extended (if there can be any relation between the two).
Ok, maybe my mistake. This is a fix. Thank you for reporting. ==

Quoted 6 lines

offline_pages()'s sanity check of given range is wrong. It should be aligned to MAX_ORDER. Current exsiting caller uses SECTION_SIZE alignment, so this change has no influence to exsisting callers.
Reported-by Ingo Korb <[email protected]>
Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

Other than the spelling mistakes in the changelog and the lack of a subject;

Acked-by Mel Gorman <[email protected]>

Quoted 27 lines

---
 mm/memory_hotplug.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Index: linux-2.6.37-rc5/mm/memory_hotplug.c
===================================================================

mm/memory_hotplug.c +7 -3

@@ -798,10 +798,14 @@ static int offline_pages(unsigned long s
 	struct memory_notify arg;
 
 	BUG_ON(start_pfn >= end_pfn);
-	/* at least, alignment against pageblock is necessary */
-	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
+	/*
+	 * Considering buddy allocator which joins nearby pages, the range
+	 * in offline should be aligned to MAX_ORDER. If not, isolated
+	 * page will be joined to other (not isolated) pages.
+	 */
+	if (!IS_ALIGNED(start_pfn, MAX_ORDER_NR_PAGES))
 		return -EINVAL;
-	if (!IS_ALIGNED(end_pfn, pageblock_nr_pages))
+	if (!IS_ALIGNED(end_pfn, MAX_ORDER_NR_PAGES))
 		return -EINVAL;
 	/* This makes hotplug much easier...and readable.
 	   we assume this for now. .*/

Signature · 3 lines

Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

KAMEZAWA Hiroyuki

2010-12-17 00:03 UTC #5 lore raw

PATCH 1/1 memory hotplug: fix alignment check (Was Re: PROBLEM: __offline_isolated_pages may offline too many pages

Quoted 5 lines · On Thu, 16 Dec 2010 10:26:41 +0000 Mel Gorman <[email protected]> wrote:

Other than the spelling mistakes in the changelog and the lack of a subject;
Acked-by Mel Gorman <[email protected]>

Thank you. fixed one.
==

offline_pages()'s sanity check of given range is wrong. It should be aligned to MAX_ORDER. Current existing caller uses SECTION_SIZE alignment, so this change has no influence to existing callers.

Reported-by Ingo Korb <[email protected]>

Acked-by Mel Gorman <[email protected]>

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 mm/memory_hotplug.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

mm/memory_hotplug.c +7 -3

Index: linux-2.6.37-rc5/mm/memory_hotplug.c
===================================================================
@@ -798,10 +798,14 @@ static int offline_pages(unsigned long s
 	struct memory_notify arg;
 
 	BUG_ON(start_pfn >= end_pfn);
-	/* at least, alignment against pageblock is necessary */
-	if (!IS_ALIGNED(start_pfn, pageblock_nr_pages))
+	/*
+	 * Considering Buddy system which joins nearby pages, the range
+	 * in offline should be aligned to MAX_ORDER. If not, isolated
+	 * page will be joined to other (not isolated) pages.
+	 */
+	if (!IS_ALIGNED(start_pfn, MAX_ORDER_NR_PAGES))
 		return -EINVAL;
-	if (!IS_ALIGNED(end_pfn, pageblock_nr_pages))
+	if (!IS_ALIGNED(end_pfn, MAX_ORDER_NR_PAGES))
 		return -EINVAL;
 	/* This makes hotplug much easier...and readable.
 	   we assume this for now. .*/