Skip to contentSkip to conversation

x86, mm: clean up setup_node_bootmem

PATCH In review Yinghai Lu 3 participants 5 messages Mar 3, 2011

Subsystems (2)
x86 x86 MM
lore
Share & tools
lore

Yinghai Lu

author
#0 lore raw

PATCH 1/1 x86, mm: clean up setup_node_bootmem

only one user now, so change it to static

Also move validity checking into the fuction.

Signed-off-by Yinghai Lu <[email protected]>
---
 arch/x86/include/asm/numa_64.h |    2 --
 arch/x86/mm/numa_64.c          |   10 +++-------
 2 files changed, 3 insertions(+), 9 deletions(-)
arch/x86/include/asm/numa_64.h +0 -2
Index: linux-2.6/arch/x86/include/asm/numa_64.h
===================================================================
@@ -13,8 +13,6 @@ struct bootnode {
 extern int numa_off;
 
 extern unsigned long numa_free_all_bootmem(void);
-extern void setup_node_bootmem(int nodeid, unsigned long start,
-			       unsigned long end);
 
 #ifdef CONFIG_NUMA
 /*
arch/x86/mm/numa_64.c +3 -7
Index: linux-2.6/arch/x86/mm/numa_64.c
===================================================================
@@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
 }
 
 /* Initialize bootmem allocator for a node */
-void __init
+static void __init
 setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
 {
 	unsigned long start_pfn, last_pfn, nodedata_phys;
 	const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
 	int nid;
 
-	if (!end)
-		return;
-
 	/*
 	 * Don't confuse VM with a node that doesn't have the
 	 * minimum amount of memory:
 	 */
-	if (end && (end - start) < NODE_MIN_SIZE)
+	if (end < (start +  NODE_MIN_SIZE))
 		return;
 
 	start = roundup(start, ZONE_ALIGN);

@@ -557,8 +554,7 @@ static int __init numa_register_memblks(
 			end = max(mi->blk[i].end, end);
 		}
 
-		if (start < end)
-			setup_node_bootmem(nid, start, end);
+		setup_node_bootmem(nid, start, end);
 	}
 
 	return 0;
#1 lore raw
Quoted 1 line · Yinghai Lu

only one user now, so change it to static

Can you please try to use proper sentences? ie.

There is only one user left now, so make it static.

Quoted 3 lines

Also move validity checking into the fuction.

Signed-off-by Yinghai Lu <[email protected]>

...

Quoted 15 lines

Index: linux-2.6/arch/x86/mm/numa_64.c
===================================================================

arch/x86/mm/numa_64.c +1 -1
@@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
 }
 
 /* Initialize bootmem allocator for a node */
-void __init
+static void __init
 setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
 {
 	unsigned long start_pfn, last_pfn, nodedata_phys;
 	const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
 	int nid;

arch/x86/mm/numa_64.c int __init numa_add_memblk(int nid, u64 · line 251

Quoted 10 lines
arch/x86/mm/numa_64.c +1 -4
 
-	if (!end)
-		return;
-
 	/*
 	 * Don't confuse VM with a node that doesn't have the
 	 * minimum amount of memory:
 	 */
-	if (end && (end - start) < NODE_MIN_SIZE)
+	if (end < (start +  NODE_MIN_SIZE))

Please drop the parentheses.

Thanks.

Signature · 1 line
tejun
#2 lore raw
In reply to Yinghai Lu
Quoted 55 lines · Yinghai Lu

only one user now, so change it to static

Also move validity checking into the fuction.

Signed-off-by Yinghai Lu <[email protected]>
---
 arch/x86/include/asm/numa_64.h |    2 --
 arch/x86/mm/numa_64.c          |   10 +++-------
 2 files changed, 3 insertions(+), 9 deletions(-)

Index: linux-2.6/arch/x86/include/asm/numa_64.h
===================================================================

arch/x86/include/asm/numa_64.h +0 -2
@@ -13,8 +13,6 @@ struct bootnode {
 extern int numa_off;
 
 extern unsigned long numa_free_all_bootmem(void);
-extern void setup_node_bootmem(int nodeid, unsigned long start,
-			       unsigned long end);
 
 #ifdef CONFIG_NUMA
 /*

Index: linux-2.6/arch/x86/mm/numa_64.c
===================================================================

arch/x86/mm/numa_64.c +2 -5
@@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
 }
 
 /* Initialize bootmem allocator for a node */
-void __init
+static void __init
 setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
 {
 	unsigned long start_pfn, last_pfn, nodedata_phys;
 	const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
 	int nid;
 
-	if (!end)
-		return;
-
 	/*
 	 * Don't confuse VM with a node that doesn't have the
 	 * minimum amount of memory:
 	 */
-	if (end && (end - start) < NODE_MIN_SIZE)
+	if (end < (start +  NODE_MIN_SIZE))
 		return;
 
 	start = roundup(start, ZONE_ALIGN);

arch/x86/mm/numa_64.c static int __init numa_register_memblks( · line 560

Quoted 9 lines
arch/x86/mm/numa_64.c +1 -2
@@ -557,8 +554,7 @@ static int __init numa_register_memblks(
 			end = max(mi->blk[i].end, end);
 		}
 
-		if (start < end)
-			setup_node_bootmem(nid, start, end);
+		setup_node_bootmem(nid, start, end);
 	}
 
 	return 0;

Good catch on finding only the one caller of setup_node_bootmem().

I'd actually rather see the validity checking being done in numa_register_memblks(), though, because it's a bug for a node to be set in node_possible_map without having a valid [mi->blk[i].start, mi->blk[i].end) range.

So could this be

	BUG_ON(end < start);
	/*
	 * Don't confuse the VM with a node that doesn't have the minimum
	 * amount of memory.
	 */
	if (end < start + NODE_MIN_SIZE) {
		node_clear(nid, node_possible_map);
		continue;
	}
	setup_node_bootmem(nid, start, end);

instead?

author
#3 lore raw
Quoted 69 lines · David Rientjes
Quoted 55 lines · Yinghai Lu

only one user now, so change it to static

Also move validity checking into the fuction.

Signed-off-by Yinghai Lu <[email protected]>
---
 arch/x86/include/asm/numa_64.h |    2 --
 arch/x86/mm/numa_64.c          |   10 +++-------
 2 files changed, 3 insertions(+), 9 deletions(-)

Index: linux-2.6/arch/x86/include/asm/numa_64.h
===================================================================

arch/x86/include/asm/numa_64.h +0 -2
@@ -13,8 +13,6 @@ struct bootnode {
 extern int numa_off;
 
 extern unsigned long numa_free_all_bootmem(void);
-extern void setup_node_bootmem(int nodeid, unsigned long start,
-			       unsigned long end);
 
 #ifdef CONFIG_NUMA
 /*

Index: linux-2.6/arch/x86/mm/numa_64.c
===================================================================

arch/x86/mm/numa_64.c +2 -5
@@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
 }
 
 /* Initialize bootmem allocator for a node */
-void __init
+static void __init
 setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
 {
 	unsigned long start_pfn, last_pfn, nodedata_phys;
 	const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
 	int nid;
 
-	if (!end)
-		return;
-
 	/*
 	 * Don't confuse VM with a node that doesn't have the
 	 * minimum amount of memory:
 	 */
-	if (end && (end - start) < NODE_MIN_SIZE)
+	if (end < (start +  NODE_MIN_SIZE))
 		return;
 
 	start = roundup(start, ZONE_ALIGN);

arch/x86/mm/numa_64.c static int __init numa_register_memblks( · line 560

Quoted 9 lines
arch/x86/mm/numa_64.c +1 -2
@@ -557,8 +554,7 @@ static int __init numa_register_memblks(
 			end = max(mi->blk[i].end, end);
 		}
 
-		if (start < end)
-			setup_node_bootmem(nid, start, end);
+		setup_node_bootmem(nid, start, end);
 	}
 
 	return 0;

Good catch on finding only the one caller of setup_node_bootmem().

Quoted 8 lines

I'd actually rather see the validity checking being done in numa_register_memblks(), though, because it's a bug for a node to be set in node_possible_map without having a valid [mi->blk[i].start, mi->blk[i].end) range.

So could this be

	BUG_ON(end < start);

no, it could cause crash here

        /* Finally register nodes. */
        for_each_node_mask(nid, node_possible_map) {
                u64 start = (u64)max_pfn << PAGE_SHIFT;
                u64 end = 0;

                for (i = 0; i < mi->nr_blks; i++) {
                        if (nid != mi->blk[i].nid)
                                continue;
                        start = min(mi->blk[i].start, start);
                        end = max(mi->blk[i].end, end);
                }

could have some node without memory. and it could be set in node_possible_map, and it will have end = 0, and start max_pfn<<page_shift.

Quoted 6 lines
	/*
	 * Don't confuse the VM with a node that doesn't have the minimum
	 * amount of memory.
	 */
	if (end < start + NODE_MIN_SIZE) {
		node_clear(nid, node_possible_map);

why touch that possible_map ?
online_map is for that purpose that state node have ram.

Quoted 5 lines
		continue;
	}
	setup_node_bootmem(nid, start, end);

instead?

Thanks

Yinghai

#4 lore raw
Quoted 65 lines · Yinghai Lu
Quoted 42 lines
Quoted 30 lines

Index: linux-2.6/arch/x86/mm/numa_64.c
===================================================================

arch/x86/mm/numa_64.c +2 -5
@@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
 }
 
 /* Initialize bootmem allocator for a node */
-void __init
+static void __init
 setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
 {
 	unsigned long start_pfn, last_pfn, nodedata_phys;
 	const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
 	int nid;
 
-	if (!end)
-		return;
-
 	/*
 	 * Don't confuse VM with a node that doesn't have the
 	 * minimum amount of memory:
 	 */
-	if (end && (end - start) < NODE_MIN_SIZE)
+	if (end < (start +  NODE_MIN_SIZE))
 		return;
 
 	start = roundup(start, ZONE_ALIGN);

arch/x86/mm/numa_64.c static int __init numa_register_memblks( · line 560

Quoted 9 lines
arch/x86/mm/numa_64.c +1 -2
@@ -557,8 +554,7 @@ static int __init numa_register_memblks(
 			end = max(mi->blk[i].end, end);
 		}
 
-		if (start < end)
-			setup_node_bootmem(nid, start, end);
+		setup_node_bootmem(nid, start, end);
 	}
 
 	return 0;

Good catch on finding only the one caller of setup_node_bootmem().

Quoted 8 lines

I'd actually rather see the validity checking being done in numa_register_memblks(), though, because it's a bug for a node to be set in node_possible_map without having a valid [mi->blk[i].start, mi->blk[i].end) range.

So could this be

	BUG_ON(end < start);

no, it could cause crash here

        /* Finally register nodes. */
        for_each_node_mask(nid, node_possible_map) {
                u64 start = (u64)max_pfn << PAGE_SHIFT;
                u64 end = 0;

                for (i = 0; i < mi->nr_blks; i++) {
                        if (nid != mi->blk[i].nid)
                                continue;
                        start = min(mi->blk[i].start, start);
                        end = max(mi->blk[i].end, end);
                }
Quoted 2 lines

could have some node without memory. and it could be set in node_possible_map, and it will have end = 0, and start max_pfn<<page_shift.

Ok, I didn't realize this was being used for memoryless nodes.

Quoted 10 lines
Quoted 6 lines
	/*
	 * Don't confuse the VM with a node that doesn't have the minimum
	 * amount of memory.
	 */
	if (end < start + NODE_MIN_SIZE) {
		node_clear(nid, node_possible_map);

why touch that possible_map ?
online_map is for that purpose that state node have ram.

Hmm, that's the purpose of N_NORMAL_MEMORY, not node_online_map. I understand why we don't want to register bootmem for a node that has such little memory that we disregard it, but shouldn't we then consider it to no longer be possible?