memory hotplug: fix a double register section info bug

PATCH RESEND In review qiuxishi 5 participants 8 messages Sep 14, 2012 – Sep 18, 2012

Subsystems (2)

Memory management Memory hot(un)plug

lore

Share & tools

lore

qiuxishi

author

2012-09-14 03:42 UTC #0 lore raw

PATCH RESEND 1/1 memory hotplug: fix a double register section info bug

There may be a bug when registering section info. For example, on my Itanium platform, the pfn range of node0 includes the other nodes, so other nodes' section info will be double registered, and memmap's page count will equal to 3.

node0: start_pfn=0x100, spanned_pfn=0x20fb00, present_pfn=0x7f8a3, => 0x000100-0x20fc00
node1: start_pfn=0x80000, spanned_pfn=0x80000, present_pfn=0x80000, => 0x080000-0x100000
node2: start_pfn=0x100000, spanned_pfn=0x80000, present_pfn=0x80000, => 0x100000-0x180000
node3: start_pfn=0x180000, spanned_pfn=0x80000, present_pfn=0x80000, => 0x180000-0x200000

free_all_bootmem_node()
register_page_bootmem_info_node()
register_page_bootmem_info_section()

When hot remove memory, we can't free the memmap's page because page_count() is 2 after put_page_bootmem().

sparse_remove_one_section()
free_section_usemap()
free_map_bootmem()
put_page_bootmem()

Signed-off-by Xishi Qiu <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

---
 mm/memory_hotplug.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

mm/memory_hotplug.c +4 -6

index 2adbcac..cf493c7 100644
@@ -126,9 +126,6 @@ static void register_page_bootmem_info_section(unsigned long start_pfn)
 	struct mem_section *ms;
 	struct page *page, *memmap;
 
-	if (!pfn_valid(start_pfn))
-		return;
-
 	section_nr = pfn_to_section_nr(start_pfn);
 	ms = __nr_to_section(section_nr);

@@ -187,9 +184,10 @@ void register_page_bootmem_info_node(struct pglist_data *pgdat)
 	end_pfn = pfn + pgdat->node_spanned_pages;
 
 	/* register_section info */
-	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION)
-		register_page_bootmem_info_section(pfn);
-
+	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+		if (pfn_valid(pfn) && (pfn_to_nid(pfn) == node))
+			register_page_bootmem_info_section(pfn);
+	}
 }
 #endif /* !CONFIG_SPARSEMEM_VMEMMAP */

Signature · 7 lines

1.7.1

--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mel Gorman

Acked-by on v1 1/1

2012-09-14 09:52 UTC #1 lore raw

Quoted 10 lines · qiuxishi

There may be a bug when registering section info. For example, on my Itanium platform, the pfn range of node0 includes the other nodes, so other nodes' section info will be double registered, and memmap's page count will equal to 3.
node0: start_pfn=0x100, spanned_pfn=0x20fb00, present_pfn=0x7f8a3, => 0x000100-0x20fc00
node1: start_pfn=0x80000, spanned_pfn=0x80000, present_pfn=0x80000, => 0x080000-0x100000
node2: start_pfn=0x100000, spanned_pfn=0x80000, present_pfn=0x80000, => 0x100000-0x180000
node3: start_pfn=0x180000, spanned_pfn=0x80000, present_pfn=0x80000, => 0x180000-0x200000

This is an unusual configuration but it's not unheard of. PPC64 in rare (and usually broken) configurations can have one node span another. Tony should know if such a configuration is normally allowed on Itanium or if this should be considered a platform bug. Tony?

Quoted 33 lines

free_all_bootmem_node()
register_page_bootmem_info_node()
register_page_bootmem_info_section()
When hot remove memory, we can't free the memmap's page because page_count() is 2 after put_page_bootmem().
sparse_remove_one_section()
free_section_usemap()
free_map_bootmem()
put_page_bootmem()
Signed-off-by Xishi Qiu <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
---
 mm/memory_hotplug.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
mm/memory_hotplug.c +0 -3
index 2adbcac..cf493c7 100644
@@ -126,9 +126,6 @@ static void register_page_bootmem_info_section(unsigned long start_pfn)
 	struct mem_section *ms;
 	struct page *page, *memmap;
 
-	if (!pfn_valid(start_pfn))
-		return;
-
 	section_nr = pfn_to_section_nr(start_pfn);
 	ms = __nr_to_section(section_nr);
 

mm/memory_hotplug.c void register_page_bootmem_info_node(struct pglist_data *pgdat) · line 190

Quoted 10 lines

mm/memory_hotplug.c +4 -3

@@ -187,9 +184,10 @@ void register_page_bootmem_info_node(struct pglist_data *pgdat)
 	end_pfn = pfn + pgdat->node_spanned_pages;
 
 	/* register_section info */
-	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION)
-		register_page_bootmem_info_section(pfn);
-
+	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+		if (pfn_valid(pfn) && (pfn_to_nid(pfn) == node))
+			register_page_bootmem_info_section(pfn);
+	}

Functionally what the patch does is check if the PFN is both valid *and* belongs to the expected node to catch a situation where nodes overlap. As there are no other callers of register_page_bootmem_info_section() this patch seems reasonable to me so

Acked-by Mel Gorman <[email protected]>

I think it would also be ok to consider this a -stable candidate.

Quoted 5 lines

mm/memory_hotplug.c +0 -0
 }
 #endif /* !CONFIG_SPARSEMEM_VMEMMAP */
 
Signature · 1 line
1.7.1

Signature · 8 lines

Mel Gorman
SUSE Labs

--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Luck, Tony

2012-09-14 16:24 UTC #2 lore raw

Quoted 4 lines

This is an unusual configuration but it's not unheard of. PPC64 in rare (and usually broken) configurations can have one node span another. Tony should know if such a configuration is normally allowed on Itanium or if this should be considered a platform bug. Tony?

We definitely have platforms where the physical memory on node 0 that we skipped to leave physical address space for PCI mem mapped devices gets tagged back at the very top of memory, after other nodes.

E.g. A 2-node system with 8G on each might look like this:

0-2G RAM on node 0
2G-4G PCI map space
4G-8G RAM on node 0
8G-16GRAM on node 1
16G-18G RAM on node 0

Is this the situation that we are talking about? Or something different?

-Tony

--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mel Gorman

2012-09-17 14:21 UTC #3 lore raw

Quoted 11 lines · Tony

Quoted 4 lines
This is an unusual configuration but it's not unheard of. PPC64 in rare (and usually broken) configurations can have one node span another. Tony should know if such a configuration is normally allowed on Itanium or if this should be considered a platform bug. Tony?
We definitely have platforms where the physical memory on node 0 that we skipped to leave physical address space for PCI mem mapped devices gets tagged back at the very top of memory, after other nodes.
E.g. A 2-node system with 8G on each might look like this:

Quoted 8 lines

0-2G RAM on node 0
2G-4G PCI map space
4G-8G RAM on node 0
8G-16GRAM on node 1
16G-18G RAM on node 0
Is this the situation that we are talking about? Or something different?

This is the type of situation we are talking about. The spanned range of node 0 includes node 1. The patch needs another revision with a comment explaining the situation included but otherwise the patch should be fine.

Signature · 8 lines

Mel Gorman
SUSE Labs

--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Yasuaki Ishimatsu

2012-09-14 11:00 UTC #4 lore raw

In reply to qiuxishi

HiXishi,

Quoted 43 lines · qiuxishi

There may be a bug when registering section info. For example, on my Itanium platform, the pfn range of node0 includes the other nodes, so other nodes' section info will be double registered, and memmap's page count will equal to 3.
node0: start_pfn=0x100, spanned_pfn=0x20fb00, present_pfn=0x7f8a3, => 0x000100-0x20fc00
node1: start_pfn=0x80000, spanned_pfn=0x80000, present_pfn=0x80000, => 0x080000-0x100000
node2: start_pfn=0x100000, spanned_pfn=0x80000, present_pfn=0x80000, => 0x100000-0x180000
node3: start_pfn=0x180000, spanned_pfn=0x80000, present_pfn=0x80000, => 0x180000-0x200000
free_all_bootmem_node()
register_page_bootmem_info_node()
register_page_bootmem_info_section()
When hot remove memory, we can't free the memmap's page because page_count() is 2 after put_page_bootmem().
sparse_remove_one_section()
free_section_usemap()
free_map_bootmem()
put_page_bootmem()
Signed-off-by Xishi Qiu <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
---
  mm/memory_hotplug.c |   10 ++++------
  1 files changed, 4 insertions(+), 6 deletions(-)
mm/memory_hotplug.c +0 -3
index 2adbcac..cf493c7 100644
@@ -126,9 +126,6 @@ static void register_page_bootmem_info_section(unsigned long start_pfn)
  	struct mem_section *ms;
  	struct page *page, *memmap;
 
-	if (!pfn_valid(start_pfn))
-		return;
-
  	section_nr = pfn_to_section_nr(start_pfn);
  	ms = __nr_to_section(section_nr);
 

mm/memory_hotplug.c void register_page_bootmem_info_node(struct pglist_data *pgdat) · line 188

Quoted 8 lines

mm/memory_hotplug.c +2 -3

@@ -187,9 +184,10 @@ void register_page_bootmem_info_node(struct pglist_data *pgdat)
  	end_pfn = pfn + pgdat->node_spanned_pages;
 
  	/* register_section info */
-	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION)
-		register_page_bootmem_info_section(pfn);
-
+	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+		if (pfn_valid(pfn) && (pfn_to_nid(pfn) == node))

I cannot judge whether your configuration is correct or not. Thus if it is correct, I want a comment of why the node check is needed. In usual configuration, a node does not span the other one. So it is natural that "pfn_to_nid(pfn) is same as "pgdat->node_id". Thus we may remove the node check in the future.

Thanks,
Yasuaki Ishimatsu

mm/memory_hotplug.c void register_page_bootmem_info_node(struct pglist_data *pgdat) · line 193

Quoted 5 lines

mm/memory_hotplug.c +2 -0

+			register_page_bootmem_info_section(pfn);
+	}
  }
  #endif /* !CONFIG_SPARSEMEM_VMEMMAP */

Andrew Morton

maintainer

2012-09-14 20:14 UTC #5 lore raw

Quoted 15 lines · On Fri, 14 Sep 2012 20:00:09 +0900 Yasuaki Ishimatsu…

Review void register_page_bootmem_info_node(struct pglist_data *pgdat) · line 188
Quoted 9 lines
(quoted diff) +2 -3
@@ -187,9 +184,10 @@ void register_page_bootmem_info_node(struct pglist_data *pgdat)
  	end_pfn = pfn + pgdat->node_spanned_pages;
 
  	/* register_section info */
-	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION)
-		register_page_bootmem_info_section(pfn);
-
+	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+		if (pfn_valid(pfn) && (pfn_to_nid(pfn) == node))
I cannot judge whether your configuration is correct or not. Thus if it is correct, I want a comment of why the node check is needed. In usual configuration, a node does not span the other one. So it is natural that "pfn_to_nid(pfn) is same as "pgdat->node_id". Thus we may remove the node check in the future.

yup. How does this look?

mm/memory_hotplug.c renamed from mm/memory_hotplug.c~memory-hotplug-fix-a-double-register-section-info-bug-fix +6 -0

@@ -185,6 +185,12 @@ void register_page_bootmem_info_node(str
 
 	/* register_section info */
 	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+		/*
+		 * Some platforms can assign the same pfn to multiple nodes - on
+		 * node0 as well as nodeN.  To avoid registering a pfn against
+		 * multiple nodes we check that this pfn does not already
+		 * reside in some other node.
+		 */
 		if (pfn_valid(pfn) && (pfn_to_nid(pfn) == node))
 			register_page_bootmem_info_section(pfn);
 	}

xishi qiu

author

2012-09-16 10:14 UTC #6 lore raw

Quoted 21 lines · Andrew Morton

Quoted 1 line · On Fri, 14 Sep 2012 20:00:09 +0900 Yasuaki Ishimatsu…
Review void register_page_bootmem_info_node(struct
Quoted 1 line
(quoted diff) +0 -0
@@ -187,9 +184,10 @@ void register_page_bootmem_info_node(struct
pglist_data *pgdat)
Quoted 14 lines
Review void register_page_bootmem_info_node(struct · line 188
Quoted 8 lines
(quoted diff) +2 -3
    end_pfn = pfn + pgdat->node_spanned_pages;
 
    /* register_section info */
-   for (; pfn < end_pfn; pfn += PAGES_PER_SECTION)
-           register_page_bootmem_info_section(pfn);
-
+   for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+           if (pfn_valid(pfn) && (pfn_to_nid(pfn) == node))
I cannot judge whether your configuration is correct or not. Thus if it is correct, I want a comment of why the node check is needed. In usual configuration, a node does not span the other one. So it is natural that "pfn_to_nid(pfn) is same as "pgdat->node_id". Thus we may remove the node check in the future.

yup. How does this look?

Quoted 1 line

It looks fine to me. Some platforms can have this unusual configuration, not only in IA64.
And register_page_bootmem_info_section() is only called by register_page_bootmem_info_node(),
so we can delete pfn_valid() check in register_page_bootmem_info_section()

Thanks
Xishi Qiu

Quoted 21 lines

---
a/mm/memory_hotplug.c~memory-hotplug-fix-a-double-register-section-info-bug-fix +++ a/mm/memory_hotplug.c
mm/memory_hotplug.c renamed from mm/memory_hotplug.c~memory-hotplug-fix-a-double-register-section-info-bug-fix +2 -0
@@ -185,6 +185,12 @@ void register_page_bootmem_info_node(str
 
        /* register_section info */
        for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+               /*
+                * Some platforms can assign the same pfn to multiple
nodes - on
+ * node0 as well as nodeN. To avoid registering a pfn
against
mm/memory_hotplug.c renamed from mm/memory_hotplug.c~memory-hotplug-fix-a-double-register-section-info-bug-fix +3 -0
+                * multiple nodes we check that this pfn does not already
+                * reside in some other node.
+                */
                if (pfn_valid(pfn) && (pfn_to_nid(pfn) == node))
                        register_page_bootmem_info_section(pfn);
        }
_

Yasuaki Ishimatsu

Reviewed-by

2012-09-18 00:12 UTC #7 lore raw

In reply to Andrew Morton

Quoted 19 lines · Andrew Morton

Quoted 15 lines · On Fri, 14 Sep 2012 20:00:09 +0900 Yasuaki Ishimatsu…
Review void register_page_bootmem_info_node(struct pglist_data *pgdat) · line 188
Quoted 9 lines
(quoted diff) +2 -3
@@ -187,9 +184,10 @@ void register_page_bootmem_info_node(struct pglist_data *pgdat)
   	end_pfn = pfn + pgdat->node_spanned_pages;
 
   	/* register_section info */
-	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION)
-		register_page_bootmem_info_section(pfn);
-
+	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+		if (pfn_valid(pfn) && (pfn_to_nid(pfn) == node))
I cannot judge whether your configuration is correct or not. Thus if it is correct, I want a comment of why the node check is needed. In usual configuration, a node does not span the other one. So it is natural that "pfn_to_nid(pfn) is same as "pgdat->node_id". Thus we may remove the node check in the future.

Quoted 1 line

yup. How does this look?

Looks good to me.

Reviewed-by Yasuaki Ishimatsu <[email protected]>

Quoted 18 lines

mm/memory_hotplug.c renamed from mm/memory_hotplug.c~memory-hotplug-fix-a-double-register-section-info-bug-fix +6 -0

@@ -185,6 +185,12 @@ void register_page_bootmem_info_node(str
 
  	/* register_section info */
  	for (; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+		/*
+		 * Some platforms can assign the same pfn to multiple nodes - on
+		 * node0 as well as nodeN.  To avoid registering a pfn against
+		 * multiple nodes we check that this pfn does not already
+		 * reside in some other node.
+		 */
  		if (pfn_valid(pfn) && (pfn_to_nid(pfn) == node))
  			register_page_bootmem_info_section(pfn);
  	}

Quoted 6 lines

--
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/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>