New: Kernel panic when boot the 2.6.39+ kernel based off of 2.6.32 kernel

https://bugzilla.kernel.org/show_bug.cgi?id=36192

Summary: Kernel panic when boot the 2.6.39+ kernel based off of
2.6.32 kernel
Product: Memory Management
Version: 2.5
Kernel Version: 2.6.39+
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: Page Allocator
AssignedTo: [email protected]
ReportedBy: [email protected]
Regression: Yes

Created an attachment (id=60012)
--> (https://bugzilla.kernel.org/attachment.cgi?id=60012)
kernel panic console output

When I updated the kernel from 2.6.32 to 2.6.39+ on a server with AMD Magny-Cours CPU, the server can not boot the 2.6.39+ kernel successfully. The console ouput showed 'Kernel panic - not syncing: Attempted to kill the idle task!' I have tried to set the kernel parameter idle=poll in the grub file. It still failed to reboot due to the same error. But it can reboot successfully on the server with Intel CPU. The full console output is attached.

Steps to reproduce:
1. install the 2.6.32 kernel
2. compile and install the kernel 2.6.39+
3. reboot

BUG: unable to handle kernel paging request at 0000000000001c08
IP: [<ffffffff811076cc>] __alloc_pages_nodemask+0x7c/0x1f0
PGD 0 
Oops: 0000 [#1] SMP 
last sysfs file: 
CPU 0 
Modules linked in:

Pid: 0, comm: swapper Not tainted 2.6.39+ #1 AMD DRACHMA/DRACHMA

RIP: 0010:[<ffffffff811076cc>]  [<ffffffff811076cc>] __alloc_pages_nodemask+0x7c/0x1f0
RSP: 0000:ffffffff81a01e48  EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000008 RDI: 00000000000002d0
RBP: ffffffff81a01ea8 R08: ffffffff81c03680 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000001 R12: 00000000000002d0
R13: 0000000000001c00 R14: ffffffff81a01fa8 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff880437800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000001c08 CR3: 0000000001a03000 CR4: 00000000000006b0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 0, threadinfo ffffffff81a00000, task ffffffff81a0b020)
Stack:
 0000000000000000 0000000000000000 ffffffff81a01eb8 000002d000000008
 ffffffff00000020 ffffffff81a01ec8 ffffffff81a01e88 0000000000000008
 0000000000100000 0000000000000000 ffffffff81a01fa8 0000000000093cf0
Call Trace:
 [<ffffffff81107d7f>] alloc_pages_exact_nid+0x5f/0xc0
 [<ffffffff814b2dea>] alloc_page_cgroup+0x2a/0x80
 [<ffffffff814b2ece>] init_section_page_cgroup+0x8e/0x110
 [<ffffffff81c4a2f1>] page_cgroup_init+0x6e/0xa7
 [<ffffffff81c22de4>] start_kernel+0x2ae/0x366
 [<ffffffff81c22346>] x86_64_start_reservations+0x131/0x135
 [<ffffffff81c2244d>] x86_64_start_kernel+0x103/0x112
Code: e0 08 83 f8 01 44 89 e0 19 db c1 e8 13 f7 d3 83 e0 01 83 e3 02 09 c3 8b 05 22 e5 af 00 44 21 e0 a8 10 89 45 bc 0f 85 c4 00 00 00 
 83 7d 08 00 0f 84 dd 00 00 00 65 4c 8b 34 25 c0 cc 00 00 41 
RIP  [<ffffffff811076cc>] __alloc_pages_nodemask+0x7c/0x1f0
 RSP <ffffffff81a01e48>

CR2: 0000000000001c08

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 30 lines · On Mon, 30 May 2011 02:38:33 GMT [email protected] wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=36192

Summary: Kernel panic when boot the 2.6.39+ kernel based off of
2.6.32 kernel
Product: Memory Management
Version: 2.5
Kernel Version: 2.6.39+
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: Page Allocator
AssignedTo: [email protected]
ReportedBy: [email protected]
Regression: Yes

Created an attachment (id=60012)
--> (https://bugzilla.kernel.org/attachment.cgi?id=60012)
kernel panic console output

When I updated the kernel from 2.6.32 to 2.6.39+ on a server with AMD Magny-Cours CPU, the server can not boot the 2.6.39+ kernel successfully. The console ouput showed 'Kernel panic - not syncing: Attempted to kill the idle task!' I have tried to set the kernel parameter idle=poll in the grub file. It still failed to reboot due to the same error. But it can reboot successfully on the server with Intel CPU. The full console output is attached.

Quoted 5 lines

Steps to reproduce:
1. install the 2.6.32 kernel
2. compile and install the kernel 2.6.39+
3. reboot

hm, this is not good. Might be memcg-related?

Quoted 31 lines

BUG: unable to handle kernel paging request at 0000000000001c08
IP: [<ffffffff811076cc>] __alloc_pages_nodemask+0x7c/0x1f0
PGD 0 
Oops: 0000 [#1] SMP 
last sysfs file: 
CPU 0 
Modules linked in:

Pid: 0, comm: swapper Not tainted 2.6.39+ #1 AMD DRACHMA/DRACHMA

RIP: 0010:[<ffffffff811076cc>]  [<ffffffff811076cc>] __alloc_pages_nodemask+0x7c/0x1f0
RSP: 0000:ffffffff81a01e48  EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000008 RDI: 00000000000002d0
RBP: ffffffff81a01ea8 R08: ffffffff81c03680 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000001 R12: 00000000000002d0
R13: 0000000000001c00 R14: ffffffff81a01fa8 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff880437800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000001c08 CR3: 0000000001a03000 CR4: 00000000000006b0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 0, threadinfo ffffffff81a00000, task ffffffff81a0b020)
Stack:
 0000000000000000 0000000000000000 ffffffff81a01eb8 000002d000000008
 ffffffff00000020 ffffffff81a01ec8 ffffffff81a01e88 0000000000000008
 0000000000100000 0000000000000000 ffffffff81a01fa8 0000000000093cf0
Call Trace:
 [<ffffffff81107d7f>] alloc_pages_exact_nid+0x5f/0xc0
 [<ffffffff814b2dea>] alloc_page_cgroup+0x2a/0x80
 [<ffffffff814b2ece>] init_section_page_cgroup+0x8e/0x110
 [<ffffffff81c4a2f1>] page_cgroup_init+0x6e/0xa7

Quoted 8 lines

 [<ffffffff81c22de4>] start_kernel+0x2ae/0x366
 [<ffffffff81c22346>] x86_64_start_reservations+0x131/0x135
 [<ffffffff81c2244d>] x86_64_start_kernel+0x103/0x112
Code: e0 08 83 f8 01 44 89 e0 19 db c1 e8 13 f7 d3 83 e0 01 83 e3 02 09 c3 8b 05 22 e5 af 00 44 21 e0 a8 10 89 45 bc 0f 85 c4 00 00 00 
 83 7d 08 00 0f 84 dd 00 00 00 65 4c 8b 34 25 c0 cc 00 00 41 
RIP  [<ffffffff811076cc>] __alloc_pages_nodemask+0x7c/0x1f0
 RSP <ffffffff81a01e48>

CR2: 0000000000001c08

Quoted 42 lines · On Sun, 29 May 2011 23:19:48 -0700 Andrew Morton <[email protected]>…

(switched to email.  Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 35 lines · On Mon, 30 May 2011 02:38:33 GMT [email protected] wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=36192

           Summary: Kernel panic when boot the 2.6.39+ kernel based off of 2.6.32 kernel
           Product: Memory Management
           Version: 2.5
    Kernel Version: 2.6.39+
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: Page Allocator
        AssignedTo: [email protected]
        ReportedBy: [email protected]
        Regression: Yes

Created an attachment (id=60012)
 --> (https://bugzilla.kernel.org/attachment.cgi?id=60012)
kernel panic console output

When I updated the kernel from 2.6.32 to 2.6.39+ on a server with AMD Magny-Cours CPU, the server can not boot the 2.6.39+ kernel successfully. The console ouput showed 'Kernel panic - not syncing: Attempted to kill the idle task!' I have tried to set the kernel parameter idle=poll in the grub file. It still failed to reboot due to the same error. But it can reboot successfully on the server with Intel CPU. The full console output is attached.

Steps to reproduce:
1. install the 2.6.32 kernel
2. compile and install the kernel 2.6.39+
3. reboot

Quoted 2 lines

hm, this is not good.  Might be memcg-related?

yes, and the system may be able to boot with a boot option of cgroup_disable=memory. but the problem happens in __alloc_pages_nodemask with NULL pointer access. Hmm, doesn't this imply some error in building zone/pgdat ?

Thanks,
-Kame

Quoted 31 lines

Quoted 31 lines

BUG: unable to handle kernel paging request at 0000000000001c08
IP: [<ffffffff811076cc>] __alloc_pages_nodemask+0x7c/0x1f0
PGD 0
Oops: 0000 [#1] SMP
last sysfs file:
CPU 0
Modules linked in:

Pid: 0, comm: swapper Not tainted 2.6.39+ #1 AMD DRACHMA/DRACHMA

RIP: 0010:[<ffffffff811076cc>]  [<ffffffff811076cc>] __alloc_pages_nodemask+0x7c/0x1f0

RSP: 0000:ffffffff81a01e48  EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000008 RDI: 00000000000002d0
RBP: ffffffff81a01ea8 R08: ffffffff81c03680 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000001 R12: 00000000000002d0
R13: 0000000000001c00 R14: ffffffff81a01fa8 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff880437800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000001c08 CR3: 0000000001a03000 CR4: 00000000000006b0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process swapper (pid: 0, threadinfo ffffffff81a00000, task ffffffff81a0b020)

Stack:
 0000000000000000 0000000000000000 ffffffff81a01eb8 000002d000000008
 ffffffff00000020 ffffffff81a01ec8 ffffffff81a01e88 0000000000000008
 0000000000100000 0000000000000000 ffffffff81a01fa8 0000000000093cf0
Call Trace:
 [<ffffffff81107d7f>] alloc_pages_exact_nid+0x5f/0xc0
 [<ffffffff814b2dea>] alloc_page_cgroup+0x2a/0x80
 [<ffffffff814b2ece>] init_section_page_cgroup+0x8e/0x110
 [<ffffffff81c4a2f1>] page_cgroup_init+0x6e/0xa7

Quoted 10 lines

Quoted 8 lines

 [<ffffffff81c22de4>] start_kernel+0x2ae/0x366
 [<ffffffff81c22346>] x86_64_start_reservations+0x131/0x135
 [<ffffffff81c2244d>] x86_64_start_kernel+0x103/0x112
Code: e0 08 83 f8 01 44 89 e0 19 db c1 e8 13 f7 d3 83 e0 01 83 e3 02 09 c3 8b 05 22 e5 af 00 44 21 e0 a8 10 89 45 bc 0f 85 c4 00 00 00

 83 7d 08 00 0f 84 dd 00 00 00 65 4c 8b 34 25 c0 cc 00 00 41 RIP  [<ffffffff811076cc>] __alloc_pages_nodemask+0x7c/0x1f0 RSP <ffffffff81a01e48>
CR2: 0000000000001c08

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 42 lines · On Sun, 29 May 2011 23:19:48 -0700 Andrew Morton <[email protected]>…

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 35 lines · On Mon, 30 May 2011 02:38:33 GMT [email protected] wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=36192

Summary: Kernel panic when boot the 2.6.39+ kernel based off of
2.6.32 kernel
Product: Memory Management
Version: 2.5
Kernel Version: 2.6.39+
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: Page Allocator
AssignedTo: [email protected]
ReportedBy: [email protected]
Regression: Yes

Created an attachment (id=60012)
--> (https://bugzilla.kernel.org/attachment.cgi?id=60012)
kernel panic console output

When I updated the kernel from 2.6.32 to 2.6.39+ on a server with AMD Magny-Cours CPU, the server can not boot the 2.6.39+ kernel successfully. The console ouput showed 'Kernel panic - not syncing: Attempted to kill the idle task!' I have tried to set the kernel parameter idle=poll in the grub file. It still failed to reboot due to the same error. But it can reboot successfully on the server with Intel CPU. The full console output is attached.

Steps to reproduce:
1. install the 2.6.32 kernel
2. compile and install the kernel 2.6.39+
3. reboot

Quoted 2 lines

hm, this is not good. Might be memcg-related?

yes, and the system may be able to boot with a boot option of cgroup_disable=memory. but the problem happens in __alloc_pages_nodemask with NULL pointer access. Hmm, doesn't this imply some error in building zone/pgdat ?

On Mon, 30 May 2011 16:01:14 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:

I want to see .config and 2.6.32's boot log (dmesg) and 2.6.39+'s boot log if possible.

On Mon, 30 May 2011 16:29:04 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:

SRAT: Node 1 PXM 1 0-a0000
SRAT: Node 1 PXM 1 100000-c8000000
SRAT: Node 1 PXM 1 100000000-438000000
SRAT: Node 3 PXM 3 438000000-838000000
SRAT: Node 5 PXM 5 838000000-c38000000
SRAT: Node 7 PXM 7 c38000000-1038000000

Initmem setup node 1 0000000000000000-0000000438000000
NODE_DATA [0000000437fd9000 - 0000000437ffffff]
Initmem setup node 3 0000000438000000-0000000838000000
NODE_DATA [0000000837fd9000 - 0000000837ffffff]
Initmem setup node 5 0000000838000000-0000000c38000000
NODE_DATA [0000000c37fd9000 - 0000000c37ffffff]
Initmem setup node 7 0000000c38000000-0000001038000000
NODE_DATA [0000001037fd7000 - 0000001037ffdfff]
[ffffea000ec40000-ffffea000edfffff] potential offnode page_structs [ffffea001cc40000-ffffea001cdfffff] potential offnode page_structs [ffffea002ac40000-ffffea002adfffff] potential offnode page_structs ==

Hmm..there are four nodes 1,3,5,7 but....no memory on node 0 hmm ?

Quoted 23 lines · On Mon, 30 May 2011 16:54:53 +0900 KAMEZAWA Hiroyuki…

On Mon, 30 May 2011 16:29:04 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:

SRAT: Node 1 PXM 1 0-a0000
SRAT: Node 1 PXM 1 100000-c8000000
SRAT: Node 1 PXM 1 100000000-438000000
SRAT: Node 3 PXM 3 438000000-838000000
SRAT: Node 5 PXM 5 838000000-c38000000
SRAT: Node 7 PXM 7 c38000000-1038000000

Initmem setup node 1 0000000000000000-0000000438000000
NODE_DATA [0000000437fd9000 - 0000000437ffffff]
Initmem setup node 3 0000000438000000-0000000838000000
NODE_DATA [0000000837fd9000 - 0000000837ffffff]
Initmem setup node 5 0000000838000000-0000000c38000000
NODE_DATA [0000000c37fd9000 - 0000000c37ffffff]
Initmem setup node 7 0000000c38000000-0000001038000000
NODE_DATA [0000001037fd7000 - 0000001037ffdfff]
[ffffea000ec40000-ffffea000edfffff] potential offnode page_structs [ffffea001cc40000-ffffea001cdfffff] potential offnode page_structs [ffffea002ac40000-ffffea002adfffff] potential offnode page_structs ==

Quoted 2 lines

Hmm..there are four nodes 1,3,5,7 but....no memory on node 0 hmm ?

I think I found a reason and this is a possible fix. But need to be tested. And suggestion for better fix rather than this band-aid is appreciated.

==

Quoted 1 line

From b95edcf43619312f72895476c3e6ef46079bb05f Mon Sep 17 00:00:00 2001

From: KAMEZAWA Hiroyuki <[email protected]>
Date: Mon, 30 May 2011 16:49:59 +0900
Subject: [PATCH][BUGFIX] fallbacks at page_cgroup allocation.

Under SPARSEMEM, the page_struct is allocated per section. Then, pfn_valid() for the whole section is "true" and there are page structs. But, it's not related to valid range of [start_pfn, end_pfn) and some page structs may not be initialized collectly because it's not a valid pages.
(memmap_init_zone() skips a page which is not correct in
early_node_map[] and page->flags is initialized to be 0.)

In this case, a page->flags can be '0'. Assume a case where node 0 has no memory....

page_cgroup is allocated onto the node

- page_to_nid(head of section pfn)

Head's pfn will be valid (struct page exists) but page->flags is 0 and contains node_id:0. This causes allocation onto NODE_DATA(0) and cause panic.

This patch makes page_cgroup to use alloc_pages_exact() only when NID is N_NORMAL_MEMORY.

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

---
 mm/page_cgroup.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

mm/page_cgroup.c +0 -0

index 74ccff6..3b0c8f2 100644
@@ -134,7 +134,10 @@ static void *__meminit alloc_page_cgroup(size_t size, int nid)
 {

Cc Mel for memory model

Quoted 58 lines · KAMEZAWA Hiroyuki

Quoted 23 lines · On Mon, 30 May 2011 16:54:53 +0900 KAMEZAWA Hiroyuki…

On Mon, 30 May 2011 16:29:04 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:

SRAT: Node 1 PXM 1 0-a0000
SRAT: Node 1 PXM 1 100000-c8000000
SRAT: Node 1 PXM 1 100000000-438000000
SRAT: Node 3 PXM 3 438000000-838000000
SRAT: Node 5 PXM 5 838000000-c38000000
SRAT: Node 7 PXM 7 c38000000-1038000000

Initmem setup node 1 0000000000000000-0000000438000000
NODE_DATA [0000000437fd9000 - 0000000437ffffff]
Initmem setup node 3 0000000438000000-0000000838000000
NODE_DATA [0000000837fd9000 - 0000000837ffffff]
Initmem setup node 5 0000000838000000-0000000c38000000
NODE_DATA [0000000c37fd9000 - 0000000c37ffffff]
Initmem setup node 7 0000000c38000000-0000001038000000
NODE_DATA [0000001037fd7000 - 0000001037ffdfff]
[ffffea000ec40000-ffffea000edfffff] potential offnode page_structs [ffffea001cc40000-ffffea001cdfffff] potential offnode page_structs [ffffea002ac40000-ffffea002adfffff] potential offnode page_structs ==

Quoted 2 lines

Hmm..there are four nodes 1,3,5,7 but....no memory on node 0 hmm ?

I think I found a reason and this is a possible fix. But need to be tested. And suggestion for better fix rather than this band-aid is appreciated.

==

Quoted 1 line

From b95edcf43619312f72895476c3e6ef46079bb05f Mon Sep 17 00:00:00 2001

From: KAMEZAWA Hiroyuki <[email protected]>
Date: Mon, 30 May 2011 16:49:59 +0900
Subject: [PATCH][BUGFIX] fallbacks at page_cgroup allocation.

Under SPARSEMEM, the page_struct is allocated per section. Then, pfn_valid() for the whole section is "true" and there are page structs. But, it's not related to valid range of [start_pfn, end_pfn) and some page structs may not be initialized collectly because it's not a valid pages.
(memmap_init_zone() skips a page which is not correct in
early_node_map[] and page->flags is initialized to be 0.)

In this case, a page->flags can be '0'. Assume a case where node 0 has no memory....

page_cgroup is allocated onto the node

- page_to_nid(head of section pfn)

Head's pfn will be valid (struct page exists) but page->flags is 0 and contains node_id:0. This causes allocation onto NODE_DATA(0) and cause panic.

This patch makes page_cgroup to use alloc_pages_exact() only when NID is N_NORMAL_MEMORY.

I don't like this much as it essentially will allocate the array from a (semantically) random node, as long as it has memory.

IMO, the problem is either 1) looking at PFNs outside known node ranges, or 2) having present/valid sections partially outside of node ranges. I am leaning towards 2), so I am wondering about the following fix:

---
From: Johannes Weiner <[email protected]>
Subject: [patch] sparse: only mark sections present when fully covered by memory

When valid memory ranges are to be registered with sparsemem, make sure that only fully covered sections are marked as present.

Otherwise we end up with PFN ranges that are reported present and valid but are actually backed by uninitialized mem map.

The page_cgroup allocator relies on pfn_present() being reliable for all PFNs between 0 and max_pfn, then retrieve the node id stored in the corresponding page->flags to allocate the per-section page_cgroup arrays on the local node.

This lead to at least one crash in the page allocator on a system where the uninitialized page struct returned the id for node 0, which had no memory itself.

Reported-by [email protected]

Debugged-by KAMEZAWA Hiroyuki <[email protected]>

Not-Yet-Signed-off-by Johannes Weiner <[email protected]>

---

mm/sparse.c +0 -0

index aa64b12..a4fbeb8 100644

Quoted 61 lines · On Mon, 6 Jun 2011 14:54:21 +0200 Johannes Weiner <[email protected]> wrote:

Cc Mel for memory model

Quoted 58 lines · KAMEZAWA Hiroyuki

Quoted 23 lines · On Mon, 30 May 2011 16:54:53 +0900 KAMEZAWA Hiroyuki…

On Mon, 30 May 2011 16:29:04 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:

SRAT: Node 1 PXM 1 0-a0000
SRAT: Node 1 PXM 1 100000-c8000000
SRAT: Node 1 PXM 1 100000000-438000000
SRAT: Node 3 PXM 3 438000000-838000000
SRAT: Node 5 PXM 5 838000000-c38000000
SRAT: Node 7 PXM 7 c38000000-1038000000

Initmem setup node 1 0000000000000000-0000000438000000
NODE_DATA [0000000437fd9000 - 0000000437ffffff]
Initmem setup node 3 0000000438000000-0000000838000000
NODE_DATA [0000000837fd9000 - 0000000837ffffff]
Initmem setup node 5 0000000838000000-0000000c38000000
NODE_DATA [0000000c37fd9000 - 0000000c37ffffff]
Initmem setup node 7 0000000c38000000-0000001038000000
NODE_DATA [0000001037fd7000 - 0000001037ffdfff]
[ffffea000ec40000-ffffea000edfffff] potential offnode page_structs [ffffea001cc40000-ffffea001cdfffff] potential offnode page_structs [ffffea002ac40000-ffffea002adfffff] potential offnode page_structs ==

Quoted 2 lines

Hmm..there are four nodes 1,3,5,7 but....no memory on node 0 hmm ?

I think I found a reason and this is a possible fix. But need to be tested. And suggestion for better fix rather than this band-aid is appreciated.

==

Quoted 1 line

From b95edcf43619312f72895476c3e6ef46079bb05f Mon Sep 17 00:00:00 2001

From: KAMEZAWA Hiroyuki <[email protected]>
Date: Mon, 30 May 2011 16:49:59 +0900
Subject: [PATCH][BUGFIX] fallbacks at page_cgroup allocation.

Under SPARSEMEM, the page_struct is allocated per section. Then, pfn_valid() for the whole section is "true" and there are page structs. But, it's not related to valid range of [start_pfn, end_pfn) and some page structs may not be initialized collectly because it's not a valid pages.
(memmap_init_zone() skips a page which is not correct in
early_node_map[] and page->flags is initialized to be 0.)

In this case, a page->flags can be '0'. Assume a case where node 0 has no memory....

page_cgroup is allocated onto the node

- page_to_nid(head of section pfn)

Head's pfn will be valid (struct page exists) but page->flags is 0 and contains node_id:0. This causes allocation onto NODE_DATA(0) and cause panic.

This patch makes page_cgroup to use alloc_pages_exact() only when NID is N_NORMAL_MEMORY.

fyi, the reporter has gone in via the bugzilla UI and says he has tested the patch and it worked well.

Please don't do that! See this?

: (switched to email. Please respond via emailed reply-to-all, not via the : bugzilla web interface).

So we have a tested-by if we use this patch.

Quoted 40 lines

I don't like this much as it essentially will allocate the array from a (semantically) random node, as long as it has memory.

IMO, the problem is either 1) looking at PFNs outside known node ranges, or 2) having present/valid sections partially outside of node ranges. I am leaning towards 2), so I am wondering about the following fix:

---
From: Johannes Weiner <[email protected]>
Subject: [patch] sparse: only mark sections present when fully covered by memory

When valid memory ranges are to be registered with sparsemem, make sure that only fully covered sections are marked as present.

Otherwise we end up with PFN ranges that are reported present and valid but are actually backed by uninitialized mem map.

The page_cgroup allocator relies on pfn_present() being reliable for all PFNs between 0 and max_pfn, then retrieve the node id stored in the corresponding page->flags to allocate the per-section page_cgroup arrays on the local node.

This lead to at least one crash in the page allocator on a system where the uninitialized page struct returned the id for node 0, which had no memory itself.

Reported-by [email protected]

Debugged-by KAMEZAWA Hiroyuki <[email protected]>

Not-Yet-Signed-off-by Johannes Weiner <[email protected]>

---

mm/sparse.c +0 -0

index aa64b12..a4fbeb8 100644
@@ -182,7 +182,9 @@ void __init memory_present(int nid, unsigned long start, unsigned long end)
 {
 	unsigned long pfn;
 

mm/sparse.c void __init memory_present(int nid, unsigned long start, unsigned long end) · line 190

Quoted 8 lines

mm/sparse.c +3 -1

-	start &= PAGE_SECTION_MASK;
+	start = ALIGN(start, PAGES_PER_SECTION);
+	end &= PAGE_SECTION_MASK;
+
 	mminit_validate_memmodel_limits(&start, &end);
 	for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {
 		unsigned long section = pfn_to_section_nr(pfn);

Hopefully he can test this one for us as well, thanks.

Quoted 115 lines · On Mon, 6 Jun 2011 14:45:19 -0700 Andrew Morton <[email protected]>…

Quoted 61 lines · On Mon, 6 Jun 2011 14:54:21 +0200 Johannes Weiner <[email protected]> wrote:

Cc Mel for memory model

Quoted 58 lines · KAMEZAWA Hiroyuki

Quoted 23 lines · On Mon, 30 May 2011 16:54:53 +0900 KAMEZAWA Hiroyuki…

On Mon, 30 May 2011 16:29:04 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:

SRAT: Node 1 PXM 1 0-a0000
SRAT: Node 1 PXM 1 100000-c8000000
SRAT: Node 1 PXM 1 100000000-438000000
SRAT: Node 3 PXM 3 438000000-838000000
SRAT: Node 5 PXM 5 838000000-c38000000
SRAT: Node 7 PXM 7 c38000000-1038000000

Initmem setup node 1 0000000000000000-0000000438000000
NODE_DATA [0000000437fd9000 - 0000000437ffffff]
Initmem setup node 3 0000000438000000-0000000838000000
NODE_DATA [0000000837fd9000 - 0000000837ffffff]
Initmem setup node 5 0000000838000000-0000000c38000000
NODE_DATA [0000000c37fd9000 - 0000000c37ffffff]
Initmem setup node 7 0000000c38000000-0000001038000000
NODE_DATA [0000001037fd7000 - 0000001037ffdfff]
[ffffea000ec40000-ffffea000edfffff] potential offnode page_structs [ffffea001cc40000-ffffea001cdfffff] potential offnode page_structs [ffffea002ac40000-ffffea002adfffff] potential offnode page_structs ==

Quoted 2 lines

Hmm..there are four nodes 1,3,5,7 but....no memory on node 0 hmm ?

I think I found a reason and this is a possible fix. But need to be tested. And suggestion for better fix rather than this band-aid is appreciated.

==

Quoted 1 line

From b95edcf43619312f72895476c3e6ef46079bb05f Mon Sep 17 00:00:00 2001

From: KAMEZAWA Hiroyuki <[email protected]>
Date: Mon, 30 May 2011 16:49:59 +0900
Subject: [PATCH][BUGFIX] fallbacks at page_cgroup allocation.

Under SPARSEMEM, the page_struct is allocated per section. Then, pfn_valid() for the whole section is "true" and there are page structs. But, it's not related to valid range of [start_pfn, end_pfn) and some page structs may not be initialized collectly because it's not a valid pages.
(memmap_init_zone() skips a page which is not correct in
early_node_map[] and page->flags is initialized to be 0.)

In this case, a page->flags can be '0'. Assume a case where node 0 has no memory....

page_cgroup is allocated onto the node

- page_to_nid(head of section pfn)

Head's pfn will be valid (struct page exists) but page->flags is 0 and contains node_id:0. This causes allocation onto NODE_DATA(0) and cause panic.

This patch makes page_cgroup to use alloc_pages_exact() only when NID is N_NORMAL_MEMORY.

fyi, the reporter has gone in via the bugzilla UI and says he has tested the patch and it worked well.

Please don't do that! See this?

: (switched to email. Please respond via emailed reply-to-all, not via the : bugzilla web interface).

So we have a tested-by if we use this patch.

Quoted 40 lines

I don't like this much as it essentially will allocate the array from a (semantically) random node, as long as it has memory.

IMO, the problem is either 1) looking at PFNs outside known node ranges, or 2) having present/valid sections partially outside of node ranges. I am leaning towards 2), so I am wondering about the following fix:

---
From: Johannes Weiner <[email protected]>
Subject: [patch] sparse: only mark sections present when fully covered by memory

When valid memory ranges are to be registered with sparsemem, make sure that only fully covered sections are marked as present.

Otherwise we end up with PFN ranges that are reported present and valid but are actually backed by uninitialized mem map.

The page_cgroup allocator relies on pfn_present() being reliable for all PFNs between 0 and max_pfn, then retrieve the node id stored in the corresponding page->flags to allocate the per-section page_cgroup arrays on the local node.

This lead to at least one crash in the page allocator on a system where the uninitialized page struct returned the id for node 0, which had no memory itself.

Reported-by [email protected]

Debugged-by KAMEZAWA Hiroyuki <[email protected]>

Not-Yet-Signed-off-by Johannes Weiner <[email protected]>

---

mm/sparse.c +0 -0

index aa64b12..a4fbeb8 100644
@@ -182,7 +182,9 @@ void __init memory_present(int nid, unsigned long start, unsigned long end)
 {
 	unsigned long pfn;
 

Quoted 11 lines

mm/sparse.c void __init memory_present(int nid, unsigned long start, unsigned long end) · line 190

Quoted 8 lines

mm/sparse.c +3 -1

-	start &= PAGE_SECTION_MASK;
+	start = ALIGN(start, PAGES_PER_SECTION);
+	end &= PAGE_SECTION_MASK;
+
 	mminit_validate_memmodel_limits(&start, &end);
 	for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {
 		unsigned long section = pfn_to_section_nr(pfn);

Hopefully he can test this one for us as well, thanks.

My concern is ARM. I know ARM unmaps 'struct page' even if pages are in existing section.

Quoted 131 lines · KAMEZAWA Hiroyuki

Quoted 126 lines · On Mon, 6 Jun 2011 14:45:19 -0700 Andrew Morton <[email protected]>…

Quoted 61 lines · On Mon, 6 Jun 2011 14:54:21 +0200 Johannes Weiner <[email protected]> wrote:

Cc Mel for memory model

Quoted 58 lines · KAMEZAWA Hiroyuki

Quoted 23 lines · On Mon, 30 May 2011 16:54:53 +0900 KAMEZAWA Hiroyuki…

On Mon, 30 May 2011 16:29:04 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:

SRAT: Node 1 PXM 1 0-a0000
SRAT: Node 1 PXM 1 100000-c8000000
SRAT: Node 1 PXM 1 100000000-438000000
SRAT: Node 3 PXM 3 438000000-838000000
SRAT: Node 5 PXM 5 838000000-c38000000
SRAT: Node 7 PXM 7 c38000000-1038000000

Initmem setup node 1 0000000000000000-0000000438000000
NODE_DATA [0000000437fd9000 - 0000000437ffffff]
Initmem setup node 3 0000000438000000-0000000838000000
NODE_DATA [0000000837fd9000 - 0000000837ffffff]
Initmem setup node 5 0000000838000000-0000000c38000000
NODE_DATA [0000000c37fd9000 - 0000000c37ffffff]
Initmem setup node 7 0000000c38000000-0000001038000000
NODE_DATA [0000001037fd7000 - 0000001037ffdfff]
[ffffea000ec40000-ffffea000edfffff] potential offnode page_structs [ffffea001cc40000-ffffea001cdfffff] potential offnode page_structs [ffffea002ac40000-ffffea002adfffff] potential offnode page_structs ==

Quoted 2 lines

Hmm..there are four nodes 1,3,5,7 but....no memory on node 0 hmm ?

I think I found a reason and this is a possible fix. But need to be tested. And suggestion for better fix rather than this band-aid is appreciated.

==

Quoted 1 line

From b95edcf43619312f72895476c3e6ef46079bb05f Mon Sep 17 00:00:00 2001

From: KAMEZAWA Hiroyuki <[email protected]>
Date: Mon, 30 May 2011 16:49:59 +0900
Subject: [PATCH][BUGFIX] fallbacks at page_cgroup allocation.

Under SPARSEMEM, the page_struct is allocated per section. Then, pfn_valid() for the whole section is "true" and there are page structs. But, it's not related to valid range of [start_pfn, end_pfn) and some page structs may not be initialized collectly because it's not a valid pages.
(memmap_init_zone() skips a page which is not correct in
early_node_map[] and page->flags is initialized to be 0.)

In this case, a page->flags can be '0'. Assume a case where node 0 has no memory....

page_cgroup is allocated onto the node

- page_to_nid(head of section pfn)

Head's pfn will be valid (struct page exists) but page->flags is 0 and contains node_id:0. This causes allocation onto NODE_DATA(0) and cause panic.

This patch makes page_cgroup to use alloc_pages_exact() only when NID is N_NORMAL_MEMORY.

fyi, the reporter has gone in via the bugzilla UI and says he has tested the patch and it worked well.

Please don't do that! See this?

: (switched to email. Please respond via emailed reply-to-all, not via the : bugzilla web interface).

So we have a tested-by if we use this patch.

Quoted 38 lines

I don't like this much as it essentially will allocate the array from a (semantically) random node, as long as it has memory.

IMO, the problem is either 1) looking at PFNs outside known node ranges, or 2) having present/valid sections partially outside of node ranges. I am leaning towards 2), so I am wondering about the following fix:

---
From: Johannes Weiner <[email protected]>
Subject: [patch] sparse: only mark sections present when fully covered by memory

When valid memory ranges are to be registered with sparsemem, make sure that only fully covered sections are marked as present.

Otherwise we end up with PFN ranges that are reported present and valid but are actually backed by uninitialized mem map.

The page_cgroup allocator relies on pfn_present() being reliable for all PFNs between 0 and max_pfn, then retrieve the node id stored in the corresponding page->flags to allocate the per-section page_cgroup arrays on the local node.

This lead to at least one crash in the page allocator on a system where the uninitialized page struct returned the id for node 0, which had no memory itself.

Reported-by [email protected]

Debugged-by KAMEZAWA Hiroyuki <[email protected]>

Not-Yet-Signed-off-by Johannes Weiner <[email protected]>

---

mm/sparse.c +0 -0

index aa64b12..a4fbeb8 100644

mm/sparse.c void __init memory_present(int nid, unsigned long start, unsigned long end) · line 190

Quoted 10 lines

mm/sparse.c +3 -1

@@ -182,7 +182,9 @@ void __init memory_present(int nid, unsigned long start, unsigned long end)
 {
 	unsigned long pfn;
 
-	start &= PAGE_SECTION_MASK;
+	start = ALIGN(start, PAGES_PER_SECTION);
+	end &= PAGE_SECTION_MASK;
+
 	mminit_validate_memmodel_limits(&start, &end);
 	for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {
 		unsigned long section = pfn_to_section_nr(pfn);

Hopefully he can test this one for us as well, thanks.

Quoted 2 lines

My concern is ARM. I know ARM unmaps 'struct page' even if pages are in existing section.

Yes, but not outside zone boundaries. The problem for ARM is having zones unaligned to sections. The struct pages for the non-resident memory gets unmapped. This is a problem for linear PFN walkers that align to boundaries unrelated to the zone such as to MAX_ORDER_NR_PAGES or pageblock_nr_pages.

Quoted 132 lines · On Tue, 7 Jun 2011 09:45:30 +0100 Mel Gorman <[email protected]> wrote:

Quoted 131 lines · KAMEZAWA Hiroyuki

Quoted 126 lines · On Mon, 6 Jun 2011 14:45:19 -0700 Andrew Morton <[email protected]>…

Quoted 61 lines · On Mon, 6 Jun 2011 14:54:21 +0200 Johannes Weiner <[email protected]> wrote:

Cc Mel for memory model

Quoted 58 lines · KAMEZAWA Hiroyuki

Quoted 23 lines · On Mon, 30 May 2011 16:54:53 +0900 KAMEZAWA Hiroyuki…

On Mon, 30 May 2011 16:29:04 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:

SRAT: Node 1 PXM 1 0-a0000
SRAT: Node 1 PXM 1 100000-c8000000
SRAT: Node 1 PXM 1 100000000-438000000
SRAT: Node 3 PXM 3 438000000-838000000
SRAT: Node 5 PXM 5 838000000-c38000000
SRAT: Node 7 PXM 7 c38000000-1038000000

Initmem setup node 1 0000000000000000-0000000438000000
NODE_DATA [0000000437fd9000 - 0000000437ffffff]
Initmem setup node 3 0000000438000000-0000000838000000
NODE_DATA [0000000837fd9000 - 0000000837ffffff]
Initmem setup node 5 0000000838000000-0000000c38000000
NODE_DATA [0000000c37fd9000 - 0000000c37ffffff]
Initmem setup node 7 0000000c38000000-0000001038000000
NODE_DATA [0000001037fd7000 - 0000001037ffdfff]
[ffffea000ec40000-ffffea000edfffff] potential offnode page_structs [ffffea001cc40000-ffffea001cdfffff] potential offnode page_structs [ffffea002ac40000-ffffea002adfffff] potential offnode page_structs ==

Quoted 2 lines

Hmm..there are four nodes 1,3,5,7 but....no memory on node 0 hmm ?

I think I found a reason and this is a possible fix. But need to be tested. And suggestion for better fix rather than this band-aid is appreciated.

==

Quoted 1 line

From b95edcf43619312f72895476c3e6ef46079bb05f Mon Sep 17 00:00:00 2001

From: KAMEZAWA Hiroyuki <[email protected]>
Date: Mon, 30 May 2011 16:49:59 +0900
Subject: [PATCH][BUGFIX] fallbacks at page_cgroup allocation.

Under SPARSEMEM, the page_struct is allocated per section. Then, pfn_valid() for the whole section is "true" and there are page structs. But, it's not related to valid range of [start_pfn, end_pfn) and some page structs may not be initialized collectly because it's not a valid pages.
(memmap_init_zone() skips a page which is not correct in
early_node_map[] and page->flags is initialized to be 0.)

In this case, a page->flags can be '0'. Assume a case where node 0 has no memory....

page_cgroup is allocated onto the node

- page_to_nid(head of section pfn)

Head's pfn will be valid (struct page exists) but page->flags is 0 and contains node_id:0. This causes allocation onto NODE_DATA(0) and cause panic.

This patch makes page_cgroup to use alloc_pages_exact() only when NID is N_NORMAL_MEMORY.

fyi, the reporter has gone in via the bugzilla UI and says he has tested the patch and it worked well.

Please don't do that! See this?

: (switched to email. Please respond via emailed reply-to-all, not via the : bugzilla web interface).

So we have a tested-by if we use this patch.

Quoted 38 lines

I don't like this much as it essentially will allocate the array from a (semantically) random node, as long as it has memory.

IMO, the problem is either 1) looking at PFNs outside known node ranges, or 2) having present/valid sections partially outside of node ranges. I am leaning towards 2), so I am wondering about the following fix:

---
From: Johannes Weiner <[email protected]>
Subject: [patch] sparse: only mark sections present when fully covered by memory

When valid memory ranges are to be registered with sparsemem, make sure that only fully covered sections are marked as present.

Otherwise we end up with PFN ranges that are reported present and valid but are actually backed by uninitialized mem map.

The page_cgroup allocator relies on pfn_present() being reliable for all PFNs between 0 and max_pfn, then retrieve the node id stored in the corresponding page->flags to allocate the per-section page_cgroup arrays on the local node.

This lead to at least one crash in the page allocator on a system where the uninitialized page struct returned the id for node 0, which had no memory itself.

Reported-by [email protected]

Debugged-by KAMEZAWA Hiroyuki <[email protected]>

Not-Yet-Signed-off-by Johannes Weiner <[email protected]>

---

mm/sparse.c +0 -0

index aa64b12..a4fbeb8 100644

mm/sparse.c void __init memory_present(int nid, unsigned long start, unsigned long end) · line 190

Quoted 10 lines

mm/sparse.c +3 -1

@@ -182,7 +182,9 @@ void __init memory_present(int nid, unsigned long start, unsigned long end)
 {
 	unsigned long pfn;
 
-	start &= PAGE_SECTION_MASK;
+	start = ALIGN(start, PAGES_PER_SECTION);
+	end &= PAGE_SECTION_MASK;
+
 	mminit_validate_memmodel_limits(&start, &end);
 	for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {
 		unsigned long section = pfn_to_section_nr(pfn);

Hopefully he can test this one for us as well, thanks.

Quoted 9 lines

Quoted 2 lines

My concern is ARM. I know ARM unmaps 'struct page' even if pages are in existing section.

Yes, but not outside zone boundaries. The problem for ARM is having zones unaligned to sections. The struct pages for the non-resident memory gets unmapped. This is a problem for linear PFN walkers that align to boundaries unrelated to the zone such as to MAX_ORDER_NR_PAGES or pageblock_nr_pages.

zone boundary is not problem. If memmap for head of section is unmapped and reused, we'll see wrong node because page->flags is broken.

I should have said "nodes" even though the end result is the same. The problem at the moment is cgroup initialisation is checking PFNs outside node boundaries. It should be ensuring that the start and end PFNs it uses are within boundaries.

Quoted 5 lines · On Tue, 7 Jun 2011 10:09:00 +0100 Mel Gorman <[email protected]> wrote:

I should have said "nodes" even though the end result is the same. The problem at the moment is cgroup initialisation is checking PFNs outside node boundaries. It should be ensuring that the start and end PFNs it uses are within boundaries.

Maybe you like this kind of fix. Yes, this can fix the problem on bugzilla. My concern is this will not work for ARM.

This patch (and all other patch) works on my test host. ==
make sparsemem's page_cgroup_init to be node aware.

With sparsemem, page_cgroup_init scans pfn from 0 to max_pfn. But this may scan a pfn which is not on any node and can access memmap which is not initialized.

This makes page_cgroup_init() for SPARSEMEM node aware.

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

---
 mm/page_cgroup.c |   26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

Index: linux-3.0-rc1/mm/page_cgroup.c
===================================================================

mm/page_cgroup.c +4 -4

@@ -285,14 +285,32 @@ void __init page_cgroup_init(void)
 {
 	unsigned long pfn;
 	int fail = 0;
+	int node;
 
 	if (mem_cgroup_disabled())
 		return;
 
-	for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
-		if (!pfn_present(pfn))
-			continue;
-		fail = init_section_page_cgroup(pfn);
+	for_each_node_state(node, N_HIGH_MEMORY) {
+		unsigned long start_pfn, end_pfn;
+

Quoted 46 lines · KAMEZAWA Hiroyuki

Quoted 5 lines · On Tue, 7 Jun 2011 10:09:00 +0100 Mel Gorman <[email protected]> wrote:

I should have said "nodes" even though the end result is the same. The problem at the moment is cgroup initialisation is checking PFNs outside node boundaries. It should be ensuring that the start and end PFNs it uses are within boundaries.

Maybe you like this kind of fix. Yes, this can fix the problem on bugzilla. My concern is this will not work for ARM.

This patch (and all other patch) works on my test host. ==
make sparsemem's page_cgroup_init to be node aware.

With sparsemem, page_cgroup_init scans pfn from 0 to max_pfn. But this may scan a pfn which is not on any node and can access memmap which is not initialized.

This makes page_cgroup_init() for SPARSEMEM node aware.

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

---
 mm/page_cgroup.c |   26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

Index: linux-3.0-rc1/mm/page_cgroup.c
===================================================================

mm/page_cgroup.c +4 -4

@@ -285,14 +285,32 @@ void __init page_cgroup_init(void)
 {
 	unsigned long pfn;
 	int fail = 0;
+	int node;
 
 	if (mem_cgroup_disabled())
 		return;
 
-	for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
-		if (!pfn_present(pfn))
-			continue;
-		fail = init_section_page_cgroup(pfn);
+	for_each_node_state(node, N_HIGH_MEMORY) {
+		unsigned long start_pfn, end_pfn;
+

mm/page_cgroup.c void __init page_cgroup_init(void) · line 305

Quoted 10 lines

mm/page_cgroup.c +10 -0

+		start_pfn = NODE_DATA(node)->node_start_pfn;
+		end_pfn = start_pfn + NODE_DATA(node)->node_spanned_pages;
+		/*
+		 * This calculation makes sure that this nid detection for
+		 * section can work even if node->start_pfn is not aligned to
+		 * section. For sections on not-node-boundary, we see head
+		 * page of sections.
+		 */
+		for (pfn = start_pfn;
+		     !fail & (pfn < end_pfn);

&& instead of & there?

mm/page_cgroup.c void __init page_cgroup_init(void) · line 313

Quoted 8 lines

mm/page_cgroup.c +8 -0

+		     pfn = ALIGN(pfn + PAGES_PER_SECTION, PAGES_PER_SECTION)) {
+			if (!pfn_present(pfn))
+				continue;
+			/* Nodes can be overlapped */
+			if (pfn_to_nid(pfn) != node)
+				continue;
+			fail = init_section_page_cgroup(pfn);
+		}

So this is finding the first valid PFN in a node. Even the overlapping problem should not be an issue here as unless node memory initialisation is overwriting flags belonging to other nodes. The paranoia does not hurt.

I also don't think the ARM punching holes in the memmap is a problem because we'd at least expect the start of the node to be valid.

Quoted 3 lines · On Tue, 7 Jun 2011 11:18:57 +0100 Mel Gorman <[email protected]> wrote:

I also don't think the ARM punching holes in the memmap is a problem because we'd at least expect the start of the node to be valid.

Ok, I'll post a fixed and cleaned one. (above patch has bug ;(

Thanks,
-Kame

On Wed, 8 Jun 2011 08:40:34 +0900
<SNIP>

With sparsemem, page_cgroup_init scans pfn from 0 to max_pfn. But this may scan a pfn which is not on any node and can access memmap which is not initialized.

This makes page_cgroup_init() for SPARSEMEM node aware and remove a code to get nid from page->flags. (Then, we'll use valid NID always.)

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

---
 mm/page_cgroup.c |   41 +++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

Index: linux-3.0-rc1/mm/page_cgroup.c
===================================================================

mm/page_cgroup.c +1 -1

@@ -162,21 +162,25 @@ static void free_page_cgroup(void *addr)
 }
 #endif
 
-static int __meminit init_section_page_cgroup(unsigned long pfn)
+static int __meminit init_section_page_cgroup(unsigned long pfn, int nid)
 {
 	struct page_cgroup *base, *pc;
 	struct mem_section *section;
 	unsigned long table_size;

mm/page_cgroup.c +1 -2

 	section = __nr_to_section(nr);
 
 	if (section->page_cgroup)
 		return 0;
 
-	nid = page_to_nid(pfn_to_page(pfn));
 	table_size = sizeof(struct page_cgroup) * PAGES_PER_SECTION;
 	base = alloc_page_cgroup(table_size, nid);
 

@@ -228,7 +232,7 @@ int __meminit online_page_cgroup(unsigne
 	for (pfn = start; !fail && pfn < end; pfn += PAGES_PER_SECTION) {
 		if (!pfn_present(pfn))
 			continue;
-		fail = init_section_page_cgroup(pfn);
+		fail = init_section_page_cgroup(pfn, nid);
 	}
 	if (!fail)
 		return 0;

mm/page_cgroup.c +4 -4

 	if (mem_cgroup_disabled())
 		return;
 
-	for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
-		if (!pfn_present(pfn))
-			continue;
-		fail = init_section_page_cgroup(pfn);
+	for_each_node_state(node, N_HIGH_MEMORY) {
+		unsigned long start_pfn, end_pfn;
+
+		start_pfn = NODE_DATA(node)->node_start_pfn;

mm/page_cgroup.c +3 -0

+			/*
+			 * Nodes can be overlapped
+			 * We know some arch can have nodes layout as

Quoted 2 lines · KAMEZAWA Hiroyuki

On Wed, 8 Jun 2011 08:40:34 +0900
<SNIP>

Missing a subject

Quoted 32 lines

With sparsemem, page_cgroup_init scans pfn from 0 to max_pfn. But this may scan a pfn which is not on any node and can access memmap which is not initialized.

This makes page_cgroup_init() for SPARSEMEM node aware and remove a code to get nid from page->flags. (Then, we'll use valid NID always.)

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

---
 mm/page_cgroup.c |   41 +++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

Index: linux-3.0-rc1/mm/page_cgroup.c
===================================================================

mm/page_cgroup.c +2 -2

@@ -162,21 +162,25 @@ static void free_page_cgroup(void *addr)
 }
 #endif
 
-static int __meminit init_section_page_cgroup(unsigned long pfn)
+static int __meminit init_section_page_cgroup(unsigned long pfn, int nid)
 {
 	struct page_cgroup *base, *pc;
 	struct mem_section *section;
 	unsigned long table_size;
 	unsigned long nr;
-	int nid, index;
+	int index;
 

mm/page_cgroup.c static void free_page_cgroup(void *addr) · line 178

Quoted 6 lines

mm/page_cgroup.c +5 -0

+	/*
+	 * Even if passed 'pfn' is not aligned to section, we need to align
+	 * it to section boundary because of SPARSEMEM pfn calculation.
+	 */
+	pfn = ALIGN(pfn, PAGES_PER_SECTION);
 	nr = pfn_to_section_nr(pfn);

This comment is a bit opaque and from the context of the patch, it's hard to know why the alignment is necessary. At least move the alignment to beside where section->page_cgroup is set because it'll be easier to understand what is going on and why.

Quoted 20 lines

mm/page_cgroup.c +1 -2

 	section = __nr_to_section(nr);
 
 	if (section->page_cgroup)
 		return 0;
 
-	nid = page_to_nid(pfn_to_page(pfn));
 	table_size = sizeof(struct page_cgroup) * PAGES_PER_SECTION;
 	base = alloc_page_cgroup(table_size, nid);
 

@@ -228,7 +232,7 @@ int __meminit online_page_cgroup(unsigne
 	for (pfn = start; !fail && pfn < end; pfn += PAGES_PER_SECTION) {
 		if (!pfn_present(pfn))
 			continue;
-		fail = init_section_page_cgroup(pfn);
+		fail = init_section_page_cgroup(pfn, nid);
 	}
 	if (!fail)
 		return 0;

@@ -285,14 +289,35 @@ void __init page_cgroup_init(void)
 {

mm/page_cgroup.c void __init page_cgroup_init(void) · line 293

Quoted 4 lines

mm/page_cgroup.c +1 -0

 	unsigned long pfn;
 	int fail = 0;
+	int node;
 

Very nit-picky but you sometimes use node and sometimes use nid. Personally, nid is my preferred choice of name as its meaning is unambigious.

Quoted 11 lines

mm/page_cgroup.c +4 -4

 	if (mem_cgroup_disabled())
 		return;
 
-	for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
-		if (!pfn_present(pfn))
-			continue;
-		fail = init_section_page_cgroup(pfn);
+	for_each_node_state(node, N_HIGH_MEMORY) {
+		unsigned long start_pfn, end_pfn;
+
+		start_pfn = NODE_DATA(node)->node_start_pfn;

mm/page_cgroup.c void __init page_cgroup_init(void) · line 310

Quoted 10 lines

mm/page_cgroup.c +10 -0

+		end_pfn = start_pfn + NODE_DATA(node)->node_spanned_pages;
+		/*
+		 * Because we cannot trust page->flags of page out of node
+		 * boundary, we skip pfn < start_pfn.
+		 */
+		for (pfn = start_pfn;
+		     !fail && (pfn < end_pfn);
+		     pfn = ALIGN(pfn + PAGES_PER_SECTION, PAGES_PER_SECTION)) {
+			if (!pfn_present(pfn))
+				continue;

Why did you not use pfn_valid()?

pfn_valid checks a section has SECTION_HAS_MEM_MAP pfn_present checks a section has SECTION_MARKED_PRESENT

SECTION_MARKED_PRESENT does not necessarily mean mem_map has been allocated although I admit that this is somewhat unlikely. I'm just curious if you had a reason for avoiding pfn_valid()?

Quoted 8 lines

mm/page_cgroup.c +8 -0

+			/*
+			 * Nodes can be overlapped
+			 * We know some arch can have nodes layout as
+			 * -------------pfn-------------->
+			 * N0 | N1 | N2 | N0 | N1 | N2 |.....
+			 */
+			if (pfn_to_nid(pfn) != node)
+				continue;

mm/page_cgroup.c void __init page_cgroup_init(void) · line 323

Quoted 6 lines

mm/page_cgroup.c +2 -0

+			fail = init_section_page_cgroup(pfn, node);
+		}
 	}
 	if (fail) {
 		printk(KERN_CRIT "try 'cgroup_disable=memory' boot option\n");

FWIW, overall I think this is heading in the right direction.

Quoted 9 lines

Quoted 9 lines

<SNIP>

mm/page_cgroup.c +8 -0

+			/*
+			 * Nodes can be overlapped
+			 * We know some arch can have nodes layout as
+			 * -------------pfn-------------->
+			 * N0 | N1 | N2 | N0 | N1 | N2 |.....
+			 */
+			if (pfn_to_nid(pfn) != node)
+				continue;

Quoted 9 lines

mm/page_cgroup.c void __init page_cgroup_init(void) · line 323

Quoted 6 lines

mm/page_cgroup.c +2 -0

+			fail = init_section_page_cgroup(pfn, node);
+		}
 	}
 	if (fail) {
 		printk(KERN_CRIT "try 'cgroup_disable=memory' boot option\n");

FWIW, overall I think this is heading in the right direction.

Thank you. and I noticed I misunderstood what ALIGN() does.

This patch is made agaisnt the latest mainline git tree.
Tested on my host, at least.
==
From 0485201fec6a9bbfabc4c2674756360c05080155 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <[email protected]>
Date: Wed, 8 Jun 2011 17:13:37 +0900
Subject: [PATCH] [BUGFIX] Avoid getting nid from invalid struct page at page_cgroup allocation.

With sparsemem, page_cgroup_init scans pfn from 0 to max_pfn. But this may scan a pfn which is not on any node and can access memmap which is not initialized.

This makes page_cgroup_init() for SPARSEMEM node aware and remove a code to get nid from page->flags. (Then, we'll use valid NID always.)

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

mm/page_cgroup.c +3 -4

@@ -285,14 +288,36 @@ void __init page_cgroup_init(void)
 {
 	unsigned long pfn;
 	int fail = 0;
+	int nid;
 
 	if (mem_cgroup_disabled())
 		return;
 
-	for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
-		if (!pfn_present(pfn))
-			continue;
-		fail = init_section_page_cgroup(pfn);
+	for_each_node_state(nid, N_HIGH_MEMORY) {
+		unsigned long start_pfn, end_pfn;

mm/page_cgroup.c static int __meminit init_section_page_cgroup(unsigned long pfn) · line 202

Quoted 10 lines · KAMEZAWA Hiroyuki

mm/page_cgroup.c +5 -1

@@ -196,7 +195,11 @@ static int __meminit init_section_page_cgroup(unsigned long pfn)
 		pc = base + index;
 		init_page_cgroup(pc, nr);
 	}
-
+	/*
+	 * Even if passed 'pfn' is not aligned to section, we need to align
+	 * it to section boundary because of SPARSEMEM pfn calculation.
+	 */
+	pfn = pfn & ~(PAGES_PER_SECTION - 1);

PAGE_SECTION_MASK?

Quoted 5 lines

mm/page_cgroup.c +0 -0

 	section->page_cgroup = base - pfn;
 	total_usage += table_size;
 	return 0;

@@ -228,7 +231,7 @@ int __meminit online_page_cgroup(unsigned long start_pfn,
 	for (pfn = start; !fail && pfn < end; pfn += PAGES_PER_SECTION) {

mm/page_cgroup.c int __meminit online_page_cgroup(unsigne · line 235

Quoted 4 lines

mm/page_cgroup.c +1 -1

 		if (!pfn_present(pfn))
 			continue;
-		fail = init_section_page_cgroup(pfn);
+		fail = init_section_page_cgroup(pfn, nid);

AFAICS, nid can be -1 in the hotplug callbacks when there is a new section added to a node that already has memory, and then the allocation will fall back to numa_node_id().

So I think we either need to trust start_pfn has valid mem map backing it (ARM has no memory hotplug support) and use pfn_to_nid(start_pfn), or find another way to the right node, no?

Quoted 15 lines

mm/page_cgroup.c +3 -4

@@ -285,14 +288,36 @@ void __init page_cgroup_init(void)
 {
 	unsigned long pfn;
 	int fail = 0;
+	int nid;
 
 	if (mem_cgroup_disabled())
 		return;
 
-	for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
-		if (!pfn_present(pfn))
-			continue;
-		fail = init_section_page_cgroup(pfn);
+	for_each_node_state(nid, N_HIGH_MEMORY) {
+		unsigned long start_pfn, end_pfn;

mm/page_cgroup.c void __init page_cgroup_init(void) · line 307

Quoted 10 lines

mm/page_cgroup.c +10 -0

+
+		start_pfn = NODE_DATA(nid)->node_start_pfn;
+		end_pfn = start_pfn + NODE_DATA(nid)->node_spanned_pages;
+		/*
+		 * Because we cannot trust page->flags of page out of node
+		 * boundary, we skip pfn < start_pfn.
+		 */
+		for (pfn = start_pfn;
+		     !fail && (pfn < end_pfn);
+		     pfn = ALIGN(pfn + 1, PAGES_PER_SECTION)) {

If we don't bother to align the pfn on the first iteration, I don't think we should for subsequent iterations. init_section_page_cgroup() has to be able to cope anyway. How about

	pfn += PAGES_PER_SECTION

instead?

On Wed, 8 Jun 2011 12:15:11 +0200
Johannes Weiner <[email protected]> wrote:

Thank you for review.

Hopefully he can test this one for us as well, thanks.

Quoted 2 lines · On Mon, 6 Jun 2011 14:45:19 -0700 Andrew Morton <[email protected]>…

Hopefully he can test this one for us as well, thanks.

A patch with better description (of mine) is here. Anyway, I felt I needed a fix for ARM special case.

fix-init-page_cgroup-for-sparsemem-taking-care-of-broken-page-flags.patch Even with SPARSEMEM, there are some magical memmap.

If a Node is not aligned to SECTION, memmap of pfn which is out of Node's range is not initialized. And page->flags contains 0.

If Node(0) doesn't exist, NODE_DATA(pfn_to_nid(pfn)) causes error.

In another case, for example, ARM frees memmap which is never be used even under SPARSEMEM. In that case, page->flags will contain broken value.

This patch does a strict check on nid which is obtained by pfn_to_page() and use proper NID for page_cgroup allocation.

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

---
 mm/page_cgroup.c |   36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

Index: linux-3.0-rc1/mm/page_cgroup.c
===================================================================

mm/page_cgroup.c +19 -0

@@ -168,6 +168,7 @@ static int __meminit init_section_page_c
 	struct mem_section *section;
 	unsigned long table_size;
 	unsigned long nr;
+	unsigned long tmp;
 	int nid, index;
 
 	nr = pfn_to_section_nr(pfn);

@@ -175,8 +176,41 @@ static int __meminit init_section_page_c
 
 	if (section->page_cgroup)
 		return 0;
+	/*
+	 * check Node-ID. Because we get 'pfn' which is obtained by calculation,
+	 * the pfn may "not exist" or "alreay freed". Even if pfn_valid() returns
+	 * true, page->flags may contain broken value and pfn_to_nid() returns
+	 * bad value.
+	 * (See CONFIG_ARCH_HAS_HOLES_MEMORYMODEL and ARM's free_memmap())
+	 * So, we need to do careful check, here.
+	 */
+	for (tmp = pfn;
+	     tmp < pfn + PAGES_PER_SECTION;
+	     tmp += MAX_ORDER_NR_PAGES, nid = -1) {
+		struct page *page;
+
+		if (!pfn_valid(tmp))
+			continue;
+
+		page = pfn_to_page(tmp);
+		nid = page_to_nid(page);

Quoted 8 lines · KAMEZAWA Hiroyuki

Quoted 2 lines · On Mon, 6 Jun 2011 14:45:19 -0700 Andrew Morton <[email protected]>…

Hopefully he can test this one for us as well, thanks.

A patch with better description (of mine) is here. Anyway, I felt I needed a fix for ARM special case.

It's a different issue that warrants a separate patch, I think.

Quoted 56 lines

fix-init-page_cgroup-for-sparsemem-taking-care-of-broken-page-flags.patch Even with SPARSEMEM, there are some magical memmap.

If a Node is not aligned to SECTION, memmap of pfn which is out of Node's range is not initialized. And page->flags contains 0.

If Node(0) doesn't exist, NODE_DATA(pfn_to_nid(pfn)) causes error.

In another case, for example, ARM frees memmap which is never be used even under SPARSEMEM. In that case, page->flags will contain broken value.

This patch does a strict check on nid which is obtained by pfn_to_page() and use proper NID for page_cgroup allocation.

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

---
 mm/page_cgroup.c |   36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

Index: linux-3.0-rc1/mm/page_cgroup.c
===================================================================

mm/page_cgroup.c +19 -0

@@ -168,6 +168,7 @@ static int __meminit init_section_page_c
 	struct mem_section *section;
 	unsigned long table_size;
 	unsigned long nr;
+	unsigned long tmp;
 	int nid, index;
 
 	nr = pfn_to_section_nr(pfn);

@@ -175,8 +176,41 @@ static int __meminit init_section_page_c
 
 	if (section->page_cgroup)
 		return 0;
+	/*
+	 * check Node-ID. Because we get 'pfn' which is obtained by calculation,
+	 * the pfn may "not exist" or "alreay freed". Even if pfn_valid() returns
+	 * true, page->flags may contain broken value and pfn_to_nid() returns
+	 * bad value.
+	 * (See CONFIG_ARCH_HAS_HOLES_MEMORYMODEL and ARM's free_memmap())
+	 * So, we need to do careful check, here.
+	 */
+	for (tmp = pfn;
+	     tmp < pfn + PAGES_PER_SECTION;
+	     tmp += MAX_ORDER_NR_PAGES, nid = -1) {
+		struct page *page;
+
+		if (!pfn_valid(tmp))
+			continue;
+
+		page = pfn_to_page(tmp);
+		nid = page_to_nid(page);
 

mm/page_cgroup.c static int __meminit init_section_page_c · line 205

Quoted 9 lines

mm/page_cgroup.c +8 -1

-	nid = page_to_nid(pfn_to_page(pfn));
+		/*
+		 * If 'page' isn't initialized or freed, it may contains broken
+		 * information.
+		 */
+		if (!node_state(nid, N_NORMAL_MEMORY))
+			continue;
+		if (page_to_pfn(pfn_to_page(tmp)) != tmp)
+			continue;

This looks quite elaborate just to figure out the node id.

Here is what I wrote before I went with the sparsemem model fix (with a modified changelog, because it also fixed the off-node range problem).

It just iterates nodes in the first place, so the node id is never a question. The memory hotplug callback still relies on pfn_to_nid() but ARM, at least as of now, does not support hotplug anyway.

What do you think?

---
From: Johannes Weiner <[email protected]>
Subject: [patch] page_cgroup: do not rely on memmap outside of node ranges

On ARM, memmap for present sections may partially be released.

As a consequence of this, a PFN walker like the page_cgroup array allocator may not rely on the struct page that corresponds to a pfn_present() PFN before it has been validated with something like memmap_valid_within().

However, since this code only requires the node ID from the PFN range, this patch changes it from a pure PFN walker to a PFN in nodes walker. The node ID information is then inherently available through the node that is currently being walked.

Signed-off-by Johannes Weiner <[email protected]>

---
 mm/page_cgroup.c |   41 +++++++++++++++++++++++++----------------
 1 files changed, 25 insertions(+), 16 deletions(-)

mm/page_cgroup.c +10 -16

index 74ccff6..46b6814 100644
@@ -162,13 +162,13 @@ static void free_page_cgroup(void *addr)
 }
 #endif
 
-static int __meminit init_section_page_cgroup(unsigned long pfn)
+static int __meminit init_section_page_cgroup(int nid, unsigned long pfn)
 {
 	struct page_cgroup *base, *pc;
 	struct mem_section *section;
 	unsigned long table_size;
 	unsigned long nr;
-	int nid, index;
+	int index;
 
 	nr = pfn_to_section_nr(pfn);
 	section = __nr_to_section(nr);

@@ -176,7 +176,6 @@ static int __meminit init_section_page_cgroup(unsigned long pfn)
 	if (section->page_cgroup)
 		return 0;
 
-	nid = page_to_nid(pfn_to_page(pfn));
 	table_size = sizeof(struct page_cgroup) * PAGES_PER_SECTION;
 	base = alloc_page_cgroup(table_size, nid);
 

@@ -222,13 +221,16 @@ int __meminit online_page_cgroup(unsigned long start_pfn,
 	unsigned long start, end, pfn;
 	int fail = 0;
 
+	if (nid < 0)
+		nid = pfn_to_nid(start_pfn);
+
 	start = start_pfn & ~(PAGES_PER_SECTION - 1);
 	end = ALIGN(start_pfn + nr_pages, PAGES_PER_SECTION);
 
 	for (pfn = start; !fail && pfn < end; pfn += PAGES_PER_SECTION) {
 		if (!pfn_present(pfn))
 			continue;
-		fail = init_section_page_cgroup(pfn);
+		fail = init_section_page_cgroup(nid, pfn);
 	}
 	if (!fail)
 		return 0;

@@ -283,23 +285,30 @@ static int __meminit page_cgroup_callback(struct notifier_block *self,
 
 void __init page_cgroup_init(void)
 {
-	unsigned long pfn;
-	int fail = 0;
+	pg_data_t *pgdat;
 
 	if (mem_cgroup_disabled())
 		return;
 
-	for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
-		if (!pfn_present(pfn))
-			continue;
-		fail = init_section_page_cgroup(pfn);
-	}
-	if (fail) {
-		printk(KERN_CRIT "try 'cgroup_disable=memory' boot option\n");
-		panic("Out of memory");
-	} else {
-		hotplug_memory_notifier(page_cgroup_callback, 0);
+	for_each_online_pgdat(pgdat) {
+		unsigned long start;
+		unsigned long end;

Quoted 30 lines · On Tue, 7 Jun 2011 09:51:31 +0200 Johannes Weiner <[email protected]> wrote:

mm/page_cgroup.c +10 -12

@@ -283,23 +285,30 @@ static int __meminit page_cgroup_callback(struct notifier_block *self,
 
 void __init page_cgroup_init(void)
 {
-	unsigned long pfn;
-	int fail = 0;
+	pg_data_t *pgdat;
 
 	if (mem_cgroup_disabled())
 		return;
 
-	for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
-		if (!pfn_present(pfn))
-			continue;
-		fail = init_section_page_cgroup(pfn);
-	}
-	if (fail) {
-		printk(KERN_CRIT "try 'cgroup_disable=memory' boot option\n");
-		panic("Out of memory");
-	} else {
-		hotplug_memory_notifier(page_cgroup_callback, 0);
+	for_each_online_pgdat(pgdat) {
+		unsigned long start;
+		unsigned long end;
+		unsigned long pfn;
+
+		start = pgdat->node_start_pfn & ~(PAGES_PER_SECTION - 1);
+		end = ALIGN(pgdat->node_start_pfn + pgdat->node_spanned_pages,
+			    PAGES_PER_SECTION);
+		for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {

mm/page_cgroup.c static int __meminit page_cgroup_callback(struct notifier_block *self, · line 305

Quoted 4 lines

mm/page_cgroup.c +4 -0

+			if (!pfn_present(pfn))
+				continue;
+			if (!init_section_page_cgroup(pgdat->node_id, pfn))
+				continue;

AFAIK, nodes can overlap. So, this [start, end) scan doesn't work. sections may be initizalised mulitple times ...in wrong way. At here, what we can trust is nid in page->flags or early_node_map[]?.

Quoted 32 lines · KAMEZAWA Hiroyuki

Quoted 30 lines · On Tue, 7 Jun 2011 09:51:31 +0200 Johannes Weiner <[email protected]> wrote:

mm/page_cgroup.c +10 -12

@@ -283,23 +285,30 @@ static int __meminit page_cgroup_callback(struct notifier_block *self,
 
 void __init page_cgroup_init(void)
 {
-	unsigned long pfn;
-	int fail = 0;
+	pg_data_t *pgdat;
 
 	if (mem_cgroup_disabled())
 		return;
 
-	for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
-		if (!pfn_present(pfn))
-			continue;
-		fail = init_section_page_cgroup(pfn);
-	}
-	if (fail) {
-		printk(KERN_CRIT "try 'cgroup_disable=memory' boot option\n");
-		panic("Out of memory");
-	} else {
-		hotplug_memory_notifier(page_cgroup_callback, 0);
+	for_each_online_pgdat(pgdat) {
+		unsigned long start;
+		unsigned long end;
+		unsigned long pfn;
+
+		start = pgdat->node_start_pfn & ~(PAGES_PER_SECTION - 1);
+		end = ALIGN(pgdat->node_start_pfn + pgdat->node_spanned_pages,
+			    PAGES_PER_SECTION);
+		for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {

Quoted 8 lines

mm/page_cgroup.c static int __meminit page_cgroup_callback(struct notifier_block *self, · line 305

Quoted 4 lines

mm/page_cgroup.c +4 -0

+			if (!pfn_present(pfn))
+				continue;
+			if (!init_section_page_cgroup(pgdat->node_id, pfn))
+				continue;

AFAIK, nodes can overlap. So, this [start, end) scan doesn't work. sections may be initizalised mulitple times ...in wrong way. At here, what we can trust is nid in page->flags or early_node_map[]?.

Sections are not be initialized multiple times. Once their page_cgroup array is allocated they are skipped if considered again later.

Second, even if there are two nodes backing the memory of a single section, there is still just a single page_cgroup array per section, we have to take the memory from one node or the other.

So if both node N1 and N2 fall into section SN, SN->page_cgroup will be an array of page_cgroup structures, allocated on N1, to represent the pages of SN.

The first section considered when walking the PFNs of N2 will be SN, which will be skipped because of !!SN->page_cgroup.

I do not see the problem.

Quoted 50 lines · On Tue, 7 Jun 2011 12:26:11 +0200 Johannes Weiner <[email protected]> wrote:

Quoted 32 lines · KAMEZAWA Hiroyuki

Quoted 30 lines · On Tue, 7 Jun 2011 09:51:31 +0200 Johannes Weiner <[email protected]> wrote:

mm/page_cgroup.c +10 -12

@@ -283,23 +285,30 @@ static int __meminit page_cgroup_callback(struct notifier_block *self,
 
 void __init page_cgroup_init(void)
 {
-	unsigned long pfn;
-	int fail = 0;
+	pg_data_t *pgdat;
 
 	if (mem_cgroup_disabled())
 		return;
 
-	for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
-		if (!pfn_present(pfn))
-			continue;
-		fail = init_section_page_cgroup(pfn);
-	}
-	if (fail) {
-		printk(KERN_CRIT "try 'cgroup_disable=memory' boot option\n");
-		panic("Out of memory");
-	} else {
-		hotplug_memory_notifier(page_cgroup_callback, 0);
+	for_each_online_pgdat(pgdat) {
+		unsigned long start;
+		unsigned long end;
+		unsigned long pfn;
+
+		start = pgdat->node_start_pfn & ~(PAGES_PER_SECTION - 1);
+		end = ALIGN(pgdat->node_start_pfn + pgdat->node_spanned_pages,
+			    PAGES_PER_SECTION);
+		for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {

Quoted 8 lines

mm/page_cgroup.c static int __meminit page_cgroup_callback(struct notifier_block *self, · line 305

Quoted 4 lines

mm/page_cgroup.c +4 -0

+			if (!pfn_present(pfn))
+				continue;
+			if (!init_section_page_cgroup(pgdat->node_id, pfn))
+				continue;

AFAIK, nodes can overlap. So, this [start, end) scan doesn't work. sections may be initizalised mulitple times ...in wrong way. At here, what we can trust is nid in page->flags or early_node_map[]?.

Sections are not be initialized multiple times. Once their page_cgroup array is allocated they are skipped if considered again later.

Second, even if there are two nodes backing the memory of a single section, there is still just a single page_cgroup array per section, we have to take the memory from one node or the other.

Quoted 9 lines

So if both node N1 and N2 fall into section SN, SN->page_cgroup will be an array of page_cgroup structures, allocated on N1, to represent the pages of SN.

The first section considered when walking the PFNs of N2 will be SN, which will be skipped because of !!SN->page_cgroup.

I do not see the problem.

Assume a host with node layout

N0 | N1 | N2 | N3 | N4 | N0

pages for all node will be allocated onto node-0.

Quoted 2 lines · On Mon, 6 Jun 2011 14:45:19 -0700 Andrew Morton <[email protected]>…

Hopefully he can test this one for us as well, thanks.

A patch with better description (of mine) is here. Anyway, I felt I needed a fix for ARM special case.

== fix-init-page_cgroup-for-sparsemem-taking-care-of-broken-page-flags.patch Even with SPARSEMEM, there are some magical memmap.

If a Node is not aligned to SECTION, memmap of pfn which is out of Node's range is not initialized. And page->flags contains 0.

If Node(0) doesn't exist, NODE_DATA(pfn_to_nid(pfn)) causes error.

In another case, for example, ARM frees memmap which is never be used even under SPARSEMEM. In that case, page->flags will contain broken value.

This patch does a strict check on nid which is obtained by pfn_to_page() and use proper NID for page_cgroup allocation.

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

---
 mm/page_cgroup.c |   36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

Index: linux-3.0-rc1/mm/page_cgroup.c
===================================================================

mm/page_cgroup.c +1 -0

@@ -168,6 +168,7 @@ static int __meminit init_section_page_c
 	struct mem_section *section;
 	unsigned long table_size;
 	unsigned long nr;
+	unsigned long tmp;
 	int nid, index;
 
 	nr = pfn_to_section_nr(pfn);

@@ -175,8 +176,41 @@ static int __meminit init_section_page_c
 

mm/page_cgroup.c +26 -1

+	for (tmp = pfn;
+	     tmp < pfn + PAGES_PER_SECTION;
+	     tmp += MAX_ORDER_NR_PAGES, nid = -1) {
+		struct page *page;
+
+		if (!pfn_valid(tmp))
+			continue;
+
+		page = pfn_to_page(tmp);
+		nid = page_to_nid(page);
 
-	nid = page_to_nid(pfn_to_page(pfn));
+		/*
+		 * If 'page' isn't initialized or freed, it may contains broken
+		 * information.
+		 */
+		if (!node_state(nid, N_NORMAL_MEMORY))
+			continue;
+		if (page_to_pfn(pfn_to_page(tmp)) != tmp)
+			continue;
+		/*
+		 * The page seems valid and this 'nid' is safe to access,
+ 		 * at least.
+ 		 */
+		break;
+	}
+	if (nid == -1)
+		return 0;
 	table_size = sizeof(struct page_cgroup) * PAGES_PER_SECTION;
 	base = alloc_page_cgroup(table_size, nid);
 

Quoted 9 lines · KAMEZAWA Hiroyuki

Quoted 2 lines · On Mon, 6 Jun 2011 14:45:19 -0700 Andrew Morton <[email protected]>…

Hopefully he can test this one for us as well, thanks.

A patch with better description (of mine) is here. Anyway, I felt I needed a fix for ARM special case.

Quoted 4 lines

== fix-init-page_cgroup-for-sparsemem-taking-care-of-broken-page-flags.patch Even with SPARSEMEM, there are some magical memmap.

Who wants to introduce SPARSEMEM_MAGICAL?

Quoted 3 lines

If a Node is not aligned to SECTION, memmap of pfn which is out of Node's range is not initialized. And page->flags contains 0.

This is tangential but it might be worth introducing CONFIG_DEBUG_MEMORY_MODEL that WARN_ONs page->flag == 0 in pfn_to_page() to catch some accesses outside node boundaries. Not for this bug though.

Quoted 2 lines

If Node(0) doesn't exist, NODE_DATA(pfn_to_nid(pfn)) causes error.

Well, not in itself. It causes a bug when we try allocate memory from node 0 but there is a subtle performance bug here as well. For unaligned nodes, the cgroup information can be allocated from node 0 instead of node-local.

Quoted 4 lines

In another case, for example, ARM frees memmap which is never be used even under SPARSEMEM. In that case, page->flags will contain broken value.

Again, not as such. In that case, struct page is not valid memory at all.

Quoted 24 lines

This patch does a strict check on nid which is obtained by pfn_to_page() and use proper NID for page_cgroup allocation.

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

---
 mm/page_cgroup.c |   36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

Index: linux-3.0-rc1/mm/page_cgroup.c
===================================================================

mm/page_cgroup.c +1 -0

@@ -168,6 +168,7 @@ static int __meminit init_section_page_c
 	struct mem_section *section;
 	unsigned long table_size;
 	unsigned long nr;
+	unsigned long tmp;
 	int nid, index;
 
 	nr = pfn_to_section_nr(pfn);

@@ -175,8 +176,41 @@ static int __meminit init_section_page_c
 

mm/page_cgroup.c static int __meminit init_section_page_c · line 186

Quoted 10 lines

mm/page_cgroup.c +8 -0

 	if (section->page_cgroup)
 		return 0;
+	/*
+	 * check Node-ID. Because we get 'pfn' which is obtained by calculation,
+	 * the pfn may "not exist" or "alreay freed". Even if pfn_valid() returns
+	 * true, page->flags may contain broken value and pfn_to_nid() returns
+	 * bad value.
+	 * (See CONFIG_ARCH_HAS_HOLES_MEMORYMODEL and ARM's free_memmap())
+	 * So, we need to do careful check, here.
+	 */

You don't really need to worry about ARM here as long as you stay within node boundaries and you only care about the first valid page in the node. Why not lookup NODE_DATA(nid) and make sure start and end are within the node boundaries?

Quoted 10 lines · On Tue, 7 Jun 2011 10:03:13 +0100 Mel Gorman <[email protected]> wrote:

Quoted 9 lines · KAMEZAWA Hiroyuki

Quoted 2 lines · On Mon, 6 Jun 2011 14:45:19 -0700 Andrew Morton <[email protected]>…

Hopefully he can test this one for us as well, thanks.

A patch with better description (of mine) is here. Anyway, I felt I needed a fix for ARM special case.

Quoted 7 lines

Quoted 4 lines

== fix-init-page_cgroup-for-sparsemem-taking-care-of-broken-page-flags.patch Even with SPARSEMEM, there are some magical memmap.

Who wants to introduce SPARSEMEM_MAGICAL?

ARM guys ;)

Quoted 4 lines

Quoted 3 lines

If a Node is not aligned to SECTION, memmap of pfn which is out of Node's range is not initialized. And page->flags contains 0.

Quoted 5 lines

This is tangential but it might be worth introducing CONFIG_DEBUG_MEMORY_MODEL that WARN_ONs page->flag == 0 in pfn_to_page() to catch some accesses outside node boundaries. Not for this bug though.

Hmm, buf if zone == 0 && section == 0 && nid == 0, page->flags is 0.

Quoted 8 lines

Quoted 2 lines

If Node(0) doesn't exist, NODE_DATA(pfn_to_nid(pfn)) causes error.

Well, not in itself. It causes a bug when we try allocate memory from node 0 but there is a subtle performance bug here as well. For unaligned nodes, the cgroup information can be allocated from node 0 instead of node-local.

Quoted 7 lines

Quoted 4 lines

In another case, for example, ARM frees memmap which is never be used even under SPARSEMEM. In that case, page->flags will contain broken value.

Again, not as such. In that case, struct page is not valid memory at all.

Hmm, IIUC, ARM's code frees memmap by free_bootmem().....so, memory used for 'struct page' is valid and can access (but it's not struct page.)

If my English sounds strange, I'm sorry. Hm

How about this ?
==
In another case, for example, ARM frees memmap which is never be used and reuse memory for memmap for other purpose. So, in that case, a page got by pfn_to_page(pfn) may not a struct page.
==

Quoted 26 lines

Quoted 26 lines

This patch does a strict check on nid which is obtained by pfn_to_page() and use proper NID for page_cgroup allocation.

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

---
 mm/page_cgroup.c |   36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

Index: linux-3.0-rc1/mm/page_cgroup.c
===================================================================

mm/page_cgroup.c +1 -0

@@ -168,6 +168,7 @@ static int __meminit init_section_page_c
 	struct mem_section *section;
 	unsigned long table_size;
 	unsigned long nr;
+	unsigned long tmp;
 	int nid, index;
 
 	nr = pfn_to_section_nr(pfn);

@@ -175,8 +176,41 @@ static int __meminit init_section_page_c
 

Quoted 16 lines

mm/page_cgroup.c static int __meminit init_section_page_c · line 186

Quoted 10 lines

mm/page_cgroup.c +8 -0

 	if (section->page_cgroup)
 		return 0;
+	/*
+	 * check Node-ID. Because we get 'pfn' which is obtained by calculation,
+	 * the pfn may "not exist" or "alreay freed". Even if pfn_valid() returns
+	 * true, page->flags may contain broken value and pfn_to_nid() returns
+	 * bad value.
+	 * (See CONFIG_ARCH_HAS_HOLES_MEMORYMODEL and ARM's free_memmap())
+	 * So, we need to do careful check, here.
+	 */

You don't really need to worry about ARM here as long as you stay within node boundaries and you only care about the first valid page in the node. Why not lookup NODE_DATA(nid) and make sure start and end are within the node boundaries?

I thought ARM's code just takes care of MAX_ORDER alignment..

Cc Mel for memory model

Quoted 53 lines · KAMEZAWA Hiroyuki

Quoted 23 lines · On Mon, 30 May 2011 16:54:53 +0900 KAMEZAWA Hiroyuki…

On Mon, 30 May 2011 16:29:04 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:

SRAT: Node 1 PXM 1 0-a0000
SRAT: Node 1 PXM 1 100000-c8000000
SRAT: Node 1 PXM 1 100000000-438000000
SRAT: Node 3 PXM 3 438000000-838000000
SRAT: Node 5 PXM 5 838000000-c38000000
SRAT: Node 7 PXM 7 c38000000-1038000000

Initmem setup node 1 0000000000000000-0000000438000000
NODE_DATA [0000000437fd9000 - 0000000437ffffff]
Initmem setup node 3 0000000438000000-0000000838000000
NODE_DATA [0000000837fd9000 - 0000000837ffffff]
Initmem setup node 5 0000000838000000-0000000c38000000
NODE_DATA [0000000c37fd9000 - 0000000c37ffffff]
Initmem setup node 7 0000000c38000000-0000001038000000
NODE_DATA [0000001037fd7000 - 0000001037ffdfff]
[ffffea000ec40000-ffffea000edfffff] potential offnode page_structs [ffffea001cc40000-ffffea001cdfffff] potential offnode page_structs [ffffea002ac40000-ffffea002adfffff] potential offnode page_structs ==

Quoted 2 lines

Hmm..there are four nodes 1,3,5,7 but....no memory on node 0 hmm ?

I think I found a reason and this is a possible fix. But need to be tested. And suggestion for better fix rather than this band-aid is appreciated.

==

Quoted 1 line

From b95edcf43619312f72895476c3e6ef46079bb05f Mon Sep 17 00:00:00 2001

From: KAMEZAWA Hiroyuki <[email protected]>
Date: Mon, 30 May 2011 16:49:59 +0900
Subject: [PATCH][BUGFIX] fallbacks at page_cgroup allocation.

Under SPARSEMEM, the page_struct is allocated per section. Then, pfn_valid() for the whole section is "true" and there are page structs. But, it's not related to valid range of [start_pfn, end_pfn) and some page structs may not be initialized collectly because it's not a valid pages.
(memmap_init_zone() skips a page which is not correct in
early_node_map[] and page->flags is initialized to be 0.)

In this case, a page->flags can be '0'. Assume a case where node 0 has no memory....

page_cgroup is allocated onto the node

- page_to_nid(head of section pfn)

Quoted 5 lines

Head's pfn will be valid (struct page exists) but page->flags is 0 and contains node_id:0. This causes allocation onto NODE_DATA(0) and cause panic.

This patch makes page_cgroup to use alloc_pages_exact() only when NID is N_NORMAL_MEMORY.

I don't like this much as it essentially will allocate the array from a (semantically) random node, as long as it has memory.

IMO, the problem is either 1) looking at PFNs outside known node ranges, or 2) having present/valid sections partially outside of node ranges.
I am leaning towards 2), so I am wondering about the following fix:

---
From: Johannes Weiner <[email protected]>
Subject: [patch] sparse: only mark sections present when fully covered by memory

When valid memory ranges are to be registered with sparsemem, make sure that only fully covered sections are marked as present.

Otherwise we end up with PFN ranges that are reported present and valid but are actually backed by uninitialized mem map.

The page_cgroup allocator relies on pfn_present() being reliable for all PFNs between 0 and max_pfn, then retrieve the node id stored in the corresponding page->flags to allocate the per-section page_cgroup arrays on the local node.

This lead to at least one crash in the page allocator on a system where the uninitialized page struct returned the id for node 0, which had no memory itself.

Reported-by [email protected]

Debugged-by KAMEZAWA Hiroyuki <[email protected]>

Not-Yet-Signed-off-by Johannes Weiner <[email protected]>

---

mm/sparse.c +0 -0

index aa64b12..a4fbeb8 100644