Fake NUMA emulation for PowerPC (Take 2)

Changelog

1. Get rid of the constant 5 (based on comments from [email protected])
2. Implement suggestions from Olof Johannson
3. Check if cmdline is NULL in fake_numa_create_new_node()

Tested with additional parameters from Olof

numa=debug,fake=
numa=foo,fake=bar

Here's a dumb simple implementation of fake NUMA nodes for PowerPC. Fake NUMA nodes can be specified using the following command line option

numa=fake=<node range>

node range is of the format <range1>,<range2>,...<rangeN>

Each of the rangeX parameters is passed using memparse(). I find the patch useful for fake NUMA emulation on my simple PowerPC machine. I've tested it on a non-numa box with the following arguments

numa=fake=1G
numa=fake=1G,2G
name=fake=1G,512M,2G
numa=fake=1500M,2800M mem=3500M
numa=fake=1G mem=512M
numa=fake=1G mem=1G

This patch applies on top of 2.6.24-rc4.

All though I've tried my best to handle some of the architecture specific details of PowerPC, I might have overlooked something obvious, like the usage of an API or some architecture tweaks. The patch depends on CONFIG_NUMA and I decided against creating a separate config option for fake NUMA to keep the code simple.

Comments are as always welcome!

Signed-off-by Balbir Singh <[email protected]>

---

 arch/powerpc/mm/numa.c |   59 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 54 insertions(+), 5 deletions(-)

diff -puN arch/powerpc/mm/numa.c~ppc-fake-numa-easy arch/powerpc/mm/numa.c

arch/powerpc/mm/numa.c renamed from arch/powerpc/mm/numa.c~ppc-fake-numa-easy +54 -5

@@ -24,6 +24,8 @@
 
 static int numa_enabled = 1;
 
+static char *cmdline __initdata;
+
 static int numa_debug;
 #define dbg(args...) if (numa_debug) { printk(KERN_INFO args); }
 

@@ -39,6 +41,43 @@ static bootmem_data_t __initdata plat_no
 static int min_common_depth;
 static int n_mem_addr_cells, n_mem_size_cells;
 
+static int __cpuinit fake_numa_create_new_node(unsigned long end_pfn,
+						unsigned int *nid)
+{
+	unsigned long long mem;
+	char *p = cmdline;
+	static unsigned int fake_nid = 0;
+	static unsigned long long curr_boundary = 0;
+
+	*nid = fake_nid;
+	if (!p)
+		return 0;
+
+	mem = memparse(p, &p);
+	if (!mem)
+		return 0;
+
+	if (mem < curr_boundary)
+		return 0;
+
+	curr_boundary = mem;
+
+	if ((end_pfn << PAGE_SHIFT) > mem) {
+		/*
+		 * Skip commas and spaces
+		 */
+		while (*p == ',' || *p == ' ' || *p == '\t')
+			p++;
+
+		cmdline = p;
+		fake_nid++;
+		*nid = fake_nid;
+		dbg("created new fake_node with id %d\n", fake_nid);
+		return 1;
+	}
+	return 0;
+}
+
 static void __cpuinit map_cpu_to_node(int cpu, int node)
 {
 	numa_cpu_lookup_table[cpu] = node;

@@ -344,12 +383,14 @@ static void __init parse_drconf_memory(s
 			if (nid == 0xffff || nid >= MAX_NUMNODES)
 				nid = default_nid;
 		}
-		node_set_online(nid);
 
 		size = numa_enforce_memory_limit(start, lmb_size);
 		if (!size)
 			continue;
 
+		fake_numa_create_new_node(((start + size) >> PAGE_SHIFT), &nid);
+		node_set_online(nid);
+
 		add_active_range(nid, start >> PAGE_SHIFT,
 				 (start >> PAGE_SHIFT) + (size >> PAGE_SHIFT));
 	}

@@ -429,7 +470,6 @@ new_range:
 		nid = of_node_to_nid_single(memory);
 		if (nid < 0)
 			nid = default_nid;
-		node_set_online(nid);
 
 		if (!(size = numa_enforce_memory_limit(start, size))) {
 			if (--ranges)

@@ -438,6 +478,9 @@ new_range:
 				continue;
 		}
 
+		fake_numa_create_new_node(((start + size) >> PAGE_SHIFT), &nid);
+		node_set_online(nid);
+
 		add_active_range(nid, start >> PAGE_SHIFT,
 				(start >> PAGE_SHIFT) + (size >> PAGE_SHIFT));
 

@@ -461,7 +504,7 @@ static void __init setup_nonnuma(void)
 	unsigned long top_of_ram = lmb_end_of_DRAM();
 	unsigned long total_ram = lmb_phys_mem_size();
 	unsigned long start_pfn, end_pfn;
-	unsigned int i;
+	unsigned int i, nid = 0;
 
 	printk(KERN_DEBUG "Top of RAM: 0x%lx, Total RAM: 0x%lx\n",
 	       top_of_ram, total_ram);

@@ -471,9 +514,11 @@ static void __init setup_nonnuma(void)
 	for (i = 0; i < lmb.memory.cnt; ++i) {
 		start_pfn = lmb.memory.region[i].base >> PAGE_SHIFT;
 		end_pfn = start_pfn + lmb_size_pages(&lmb.memory, i);
-		add_active_range(0, start_pfn, end_pfn);
+
+		fake_numa_create_new_node(end_pfn, &nid);
+		add_active_range(nid, start_pfn, end_pfn);
+		node_set_online(nid);
 	}
-	node_set_online(0);
 }
 
 void __init dump_numa_cpu_topology(void)

@@ -702,6 +747,10 @@ static int __init early_numa(char *p)
 	if (strstr(p, "debug"))
 		numa_debug = 1;
 
+	p = strstr(p, "fake=");
+	if (p)
+		cmdline = p + strlen("fake=");
+
 	return 0;
 }
 early_param("numa", early_numa);

_

Signed-off-by Balbir Singh <[email protected]>

Changelog

1. Get rid of the constant 5 (based on comments from [email protected])
2. Implement suggestions from Olof Johannson
3. Check if cmdline is NULL in fake_numa_create_new_node()

Tested with additional parameters from Olof

numa=debug,fake=
numa=foo,fake=bar

Quoted 7 lines · Balbir Singh

Changelog

1. Get rid of the constant 5 (based on comments from [email protected])
2. Implement suggestions from Olof Johannson
3. Check if cmdline is NULL in fake_numa_create_new_node()

Quoted 4 lines

Tested with additional parameters from Olof

numa=debug,fake=
numa=foo,fake=bar

I'm not sure why yet, but git bisect tells me it's this patch that's causing the for-2.6.25 tree to explode on boot on cell machines.

Quoted 8 lines · Michael Ellerman

Quoted 7 lines · Balbir Singh

Changelog

1. Get rid of the constant 5 (based on comments from [email protected])
2. Implement suggestions from Olof Johannson
3. Check if cmdline is NULL in fake_numa_create_new_node()

Quoted 8 lines

Quoted 4 lines

Tested with additional parameters from Olof

numa=debug,fake=
numa=foo,fake=bar

I'm not sure why yet, but git bisect tells me it's this patch that's causing the for-2.6.25 tree to explode on boot on cell machines.

This fixes it, although I'm a little worried about some of the removals/movings of node_set_online() in the patch.

arch/powerpc/mm/numa.c +0 -1

index 1666e7d..dcedc26 100644
@@ -49,7 +49,6 @@ static int __cpuinit fake_numa_create_new_node(unsigned long end_pfn,
 	static unsigned int fake_nid = 0;
 	static unsigned long long curr_boundary = 0;
 
-	*nid = fake_nid;
 	if (!p)
 		return 0;
 

This fixes it, although I'm a little worried about some of the removals/movings of node_set_online() in the patch.

arch/powerpc/mm/numa.c +0 -1

index 1666e7d..dcedc26 100644
@@ -49,7 +49,6 @@ static int __cpuinit fake_numa_create_new_node(unsigned long end_pfn,
 	static unsigned int fake_nid = 0;
 	static unsigned long long curr_boundary = 0;
 
-	*nid = fake_nid;
 	if (!p)
 		return 0;
 

Here's a better and more complete fix for the problem. Could you please see if it works for you? I tested it on a real NUMA box and it seemed to work fine there.

Quoted 3 lines · Balbir Singh

Here's a better and more complete fix for the problem. Could you please see if it works for you? I tested it on a real NUMA box and it seemed to work fine there.

There are a couple of other changes in behaviour that your patch introduces, and I'd like to understand them better before taking the patch. First, with your patch we don't set nodes online if they end up having no memory in them because of the memory limit, whereas previously we did. Secondly, in the case where we don't have NUMA information, we now set node 0 online after adding each LMB, whereas previously we only set it online once.

If in fact these changes are benign, then your patch description should mention them and explain why they are benign.

Paul.
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Quoted 14 lines · Paul Mackerras

Quoted 3 lines · Balbir Singh

Here's a better and more complete fix for the problem. Could you please see if it works for you? I tested it on a real NUMA box and it seemed to work fine there.

There are a couple of other changes in behaviour that your patch introduces, and I'd like to understand them better before taking the patch. First, with your patch we don't set nodes online if they end up having no memory in them because of the memory limit, whereas previously we did. Secondly, in the case where we don't have NUMA information, we now set node 0 online after adding each LMB, whereas previously we only set it online once.

Quoted 3 lines

If in fact these changes are benign, then your patch description should mention them and explain why they are benign.

Yes, they are. I'll try and justify the changes with a good detailed changelog. If people prefer it, I can hide fake NUMA nodes under a config option, so that it does not come enabled by default.

Quoted 16 lines · Balbir Singh

Quoted 14 lines · Paul Mackerras

Quoted 3 lines · Balbir Singh

Here's a better and more complete fix for the problem. Could you please see if it works for you? I tested it on a real NUMA box and it seemed to work fine there.

There are a couple of other changes in behaviour that your patch introduces, and I'd like to understand them better before taking the patch. First, with your patch we don't set nodes online if they end up having no memory in them because of the memory limit, whereas previously we did. Secondly, in the case where we don't have NUMA information, we now set node 0 online after adding each LMB, whereas previously we only set it online once.

Quoted 7 lines

Quoted 3 lines

If in fact these changes are benign, then your patch description should mention them and explain why they are benign.

Yes, they are. I'll try and justify the changes with a good detailed changelog. If people prefer it, I can hide fake NUMA nodes under a config option, so that it does not come enabled by default.

Sigh, there already *is* a fake NUMA config option: CONFIG_NUMA_EMU.

"CONFIG_NUMA_EMU:
Enable NUMA emulation. A flat machine will be split into virtual nodes when booted with "numa=fake=N", where N is the number of nodes. This is only useful for debugging."

I have to assume your patch is implementing the same feature for powerpc (really just extending the x86_64 one), and thus should share the config option.

Any chance you can just make some of that code common? Maybe as a follow-on patch. I expect that some of Mel's (added to Cc) work to allow NUMA to be set on x86 more easily will flow quite simply into adding fake NUMA support there as well. So moving the code to a common place (at least the parsing) makes sense.

I also feel like you want to be able to online memoryless nodes -- that's where we've been hitting a number of bugs lately in the VM. I can't tell from Paul's comment if your patch prevents that from being faked or not.

Thanks,
Nish
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Quoted 3 lines · Balbir Singh

Here's a better and more complete fix for the problem. Could you please see if it works for you? I tested it on a real NUMA box and it seemed to work fine there.

There are a couple of other changes in behaviour that your patch introduces, and I'd like to understand them better before taking the patch. First, with your patch we don't set nodes online if they end up having no memory in them because of the memory limit, whereas previously we did. Secondly, in the case where we don't have NUMA information, we now set node 0 online after adding each LMB, whereas previously we only set it online once.

If in fact these changes are benign, then your patch description should mention them and explain why they are benign.

Paul.

Hi, Paul,

Here's version 3 of the patch. I've commented the side-effect of repeatedly setting node 0 online (as to why that is done) and I've removed the side effect of not creating memory less nodes (when we hit the memory limit).

I've described all my tests below

Changelog v3
1. Remove the side-effect of not setting nodes online if they end up having no memory in them because of the memory limit.

Changelog v2

1. Get rid of the constant 5 (based on comments from [email protected])
2. Implement suggestions from Olof Johannson
3. Check if cmdline is NULL in fake_numa_create_new_node()

diff -puN arch/powerpc/mm/numa.c~fakenumappc arch/powerpc/mm/numa.c

arch/powerpc/mm/numa.c renamed from arch/powerpc/mm/numa.c~fakenumappc +5 -0

@@ -39,6 +41,47 @@ static bootmem_data_t __initdata plat_no
 static int min_common_depth;
 static int n_mem_addr_cells, n_mem_size_cells;
 
+static int __cpuinit fake_numa_create_new_node(unsigned long end_pfn,
+						unsigned int *nid)
+{
+	unsigned long long mem;
+	char *p = cmdline;

Why do you check !p after assigning to nid? I assume it's because we might have reached the end of the command line, ie. p == NULL, but we're still adding memory to the last node? If so it's a it's a little subtle and deserves a comment I think.

Otherwise this looks pretty good.

cheers

Signature · 8 lines

Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

Why do you check !p after assigning to nid? I assume it's because we might have reached the end of the command line, ie. p == NULL, but we're still adding memory to the last node? If so it's a it's a little subtle and deserves a comment I think.

Otherwise this looks pretty good.

cheers

Quoted 7 lines · Balbir Singh

Changelog

1. Get rid of the constant 5 (based on comments from [email protected])
2. Implement suggestions from Olof Johannson
3. Check if cmdline is NULL in fake_numa_create_new_node()

Quoted 4 lines

Tested with additional parameters from Olof

numa=debug,fake=
numa=foo,fake=bar

I'm not sure why yet, but git bisect tells me it's this patch that's causing the for-2.6.25 tree to explode on boot on cell machines.

Here's a dumb simple implementation of fake NUMA nodes for PowerPC. Fake NUMA nodes can be specified using the following command line option

Comments are as always welcome!

arch/powerpc/mm/numa.c renamed from arch/powerpc/mm/numa.c~ppc-fake-numa-easy +28 -0

+	if (!p)
+		return 0;
+
+	mem = memparse(p, &p);
+	if (!mem)
+		return 0;
+
+	if (mem < curr_boundary)
+		return 0;
+
+	curr_boundary = mem;
+
+	if ((end_pfn << PAGE_SHIFT) > mem) {
+		/*
+		 * Skip commas and spaces
+		 */
+		while (*p == ',' || *p == ' ' || *p == '\t')
+			p++;
+
+		cmdline = p;
+		fake_nid++;
+		*nid = fake_nid;
+		dbg("created new fake_node with id %d\n", fake_nid);
+		return 1;
+	}
+	return 0;
+}
+
 static void __cpuinit map_cpu_to_node(int cpu, int node)
 {
 	numa_cpu_lookup_table[cpu] = node;

@@ -344,12 +383,14 @@ static void __init parse_drconf_memory(s
 			if (nid == 0xffff || nid >= MAX_NUMNODES)

arch/powerpc/mm/numa.c renamed from arch/powerpc/mm/numa.c~ppc-fake-numa-easy +0 -1

 		add_active_range(nid, start >> PAGE_SHIFT,
 				 (start >> PAGE_SHIFT) + (size >> PAGE_SHIFT));
 	}

@@ -429,7 +470,6 @@ new_range:
 		nid = of_node_to_nid_single(memory);
 		if (nid < 0)
 			nid = default_nid;
-		node_set_online(nid);
 
 		if (!(size = numa_enforce_memory_limit(start, size))) {
 			if (--ranges)

Quoted 6 lines · Balbir Singh

Here's a dumb simple implementation of fake NUMA nodes for PowerPC. Fake NUMA nodes can be specified using the following command line option

Comments are as always welcome!

Here's some :)

arch/powerpc/mm/numa.c line 27

Quoted 8 lines

diff -puN arch/powerpc/mm/numa.c~ppc-fake-numa-easy arch/powerpc/mm/numa.c

arch/powerpc/mm/numa.c renamed from arch/powerpc/mm/numa.c~ppc-fake-numa-easy +1 -0

@@ -24,6 +24,8 @@
 
 static int numa_enabled = 1;
 
+static char *cmdline __initdata;

Can you call this fake_numa_args or something, cmdline is a bit generic.

Quoted 5 lines

arch/powerpc/mm/numa.c renamed from arch/powerpc/mm/numa.c~ppc-fake-numa-easy +0 -0

@@ -39,6 +41,43 @@ static bootmem_data_t __initdata plat_no
 static int min_common_depth;
 static int n_mem_addr_cells, n_mem_size_cells;
 

arch/powerpc/mm/numa.c static bootmem_data_t __initdata plat_no · line 52

Quoted 9 lines

arch/powerpc/mm/numa.c renamed from arch/powerpc/mm/numa.c~ppc-fake-numa-easy +9 -0

+static int __cpuinit fake_numa_create_new_node(unsigned long end_pfn,
+						unsigned int *nid)
+{
+	unsigned long long mem;
+	char *p = cmdline;
+	static unsigned int fake_nid = 0;
+	static unsigned long long curr_boundary = 0;
+
+	*nid = fake_nid;

As I mentioned in my other email I think this is broken, you unconditionally overwrite *nid, even if no fake numa was specified?

Quoted 41 lines

arch/powerpc/mm/numa.c renamed from arch/powerpc/mm/numa.c~ppc-fake-numa-easy +28 -1

+	if (!p)
+		return 0;
+
+	mem = memparse(p, &p);
+	if (!mem)
+		return 0;
+
+	if (mem < curr_boundary)
+		return 0;
+
+	curr_boundary = mem;
+
+	if ((end_pfn << PAGE_SHIFT) > mem) {
+		/*
+		 * Skip commas and spaces
+		 */
+		while (*p == ',' || *p == ' ' || *p == '\t')
+			p++;
+
+		cmdline = p;
+		fake_nid++;
+		*nid = fake_nid;
+		dbg("created new fake_node with id %d\n", fake_nid);
+		return 1;
+	}
+	return 0;
+}
+
 static void __cpuinit map_cpu_to_node(int cpu, int node)
 {
 	numa_cpu_lookup_table[cpu] = node;

@@ -344,12 +383,14 @@ static void __init parse_drconf_memory(s
 			if (nid == 0xffff || nid >= MAX_NUMNODES)
 				nid = default_nid;
 		}
-		node_set_online(nid);
 
 		size = numa_enforce_memory_limit(start, lmb_size);
 		if (!size)
 			continue;
 

arch/powerpc/mm/numa.c static void __init parse_drconf_memory(s · line 392

Quoted 2 lines

arch/powerpc/mm/numa.c renamed from arch/powerpc/mm/numa.c~ppc-fake-numa-easy +2 -0

+		fake_numa_create_new_node(((start + size) >> PAGE_SHIFT), &nid);
+		node_set_online(nid);

I can't convince myself that this is 100% ok, the moving of node_set_online(). At the very least it's a change in behaviour, previously we would online the node regardless of the memory limit.

Quoted 15 lines

arch/powerpc/mm/numa.c renamed from arch/powerpc/mm/numa.c~ppc-fake-numa-easy +0 -1

 		add_active_range(nid, start >> PAGE_SHIFT,
 				 (start >> PAGE_SHIFT) + (size >> PAGE_SHIFT));
 	}

@@ -429,7 +470,6 @@ new_range:
 		nid = of_node_to_nid_single(memory);
 		if (nid < 0)
 			nid = default_nid;
-		node_set_online(nid);
 
 		if (!(size = numa_enforce_memory_limit(start, size))) {
 			if (--ranges)

@@ -438,6 +478,9 @@ new_range:
 				continue;
 		}
 

arch/powerpc/mm/numa.c new_range: · line 482

Quoted 2 lines

arch/powerpc/mm/numa.c renamed from arch/powerpc/mm/numa.c~ppc-fake-numa-easy +2 -0

+		fake_numa_create_new_node(((start + size) >> PAGE_SHIFT), &nid);
+		node_set_online(nid);

Ditto previous comment.

cheers

Signature · 8 lines

Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person