Re: [PATCH v2] powerpc/numa: set node_possible_map to only node_online_map during boot

On 03/06/2015 10:57 AM, Nishanth Aravamudan wrote:

On 05.03.2015 [15:29:00 -0800], David Rientjes wrote:

quoted

On Thu, 5 Mar 2015, Nishanth Aravamudan wrote:

quoted

So if we compare to x86:

arch/x86/mm/numa.c::numa_init():

         nodes_clear(numa_nodes_parsed);
         nodes_clear(node_possible_map);
         nodes_clear(node_online_map);
	...
	numa_register_memblks(...);

arch/x86/mm/numa.c::numa_register_memblks():

	node_possible_map = numa_nodes_parsed;

Basically, it looks like x86 NUMA init clears out possible map and
online map, probably for a similar reason to what I gave in the
changelog that by default, the possible map seems to be based off
MAX_NUMNODES, rather than nr_node_ids or anything dynamic.

My patch was an attempt to emulate the same thing on powerpc. You are
right that there is a window in which the node_possible_map and
node_online_map are out of sync with my patch. It seems like it
shouldn't matter given how early in boot we are, but perhaps the
following would have been clearer:

diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 0257a7d659ef..1a118b08fad2 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c

@@ -958,6 +958,13 @@ void __init initmem_init(void)

         memblock_dump_all();

+       /*
+        * Reduce the possible NUMA nodes to the online NUMA nodes,
+        * since we do not support node hotplug. This ensures that  we
+        * lower the maximum NUMA node ID to what is actually present.
+        */
+       nodes_and(node_possible_map, node_possible_map, node_online_map);

If you don't support node hotplug, then a node should always be possible
if it's online unless there are other tricks powerpc plays with
node_possible_map.  Shouldn't this just be
node_possible_map = node_online_map?

Yeah, but I was too dumb to think of that before sending :)

Updated version follows...

-Nish

---8<---

Raghu noticed an issue with excessive memory allocation on power with a
simple cgroup test, specifically, in mem_cgroup_css_alloc ->
for_each_node -> alloc_mem_cgroup_per_zone_info(), which ends up blowing
up the kmalloc-2048 slab (to the order of 200MB for 400 cgroup
directories).

should we also add after this patch it has reduced to around 2MB?

The underlying issue is that NODES_SHIFT on power is 8 (256 NUMA nodes
possible), which defines node_possible_map, which in turn defines the
value of nr_node_ids in setup_nr_node_ids and the iteration of
for_each_node.

In practice, we never see a system with 256 NUMA nodes, and in fact, we
do not support node hotplug on power in the first place, so the nodes
that are online when we come up are the nodes that will be present for
the lifetime of this kernel. So let's, at least, drop the NUMA possible
map down to the online map at runtime. This is similar to what x86 does
in its initialization routines.

mem_cgroup_css_alloc should also be fixed to only iterate over
memory-populated nodes and handle hotplug, but that is a separate
change.

Maybe we could fomally add
Reported-by: Raghavendra K T <redacted>

  Hope we remember this change when we add hotplug :)

+	node_possible_map = node_online_map;
+
  	for_each_online_node(nid) {
  		unsigned long start_pfn, end_pfn;