x86/mm/srat_64.c: nodes_parsed should include all nodes detected by ACPI.

This is to fix the bug discussed in email thread: http://patchwork.kernel.org/patch/69499/.

Currently node_possible_map won't include the offlined node that has neither CPU onlined nor MEM onlined at booting time. As a result, nr_node_ids won't be equal to possible nodes.

CC Thomas Gleixner <[email protected]>

CC Ingo Molnar <[email protected]>

CC H. Peter Anvin <[email protected]>

CC Andi Kleen <[email protected]>

Signed-off-by Haicheng Li <[email protected]>

---
 arch/x86/mm/srat_64.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

arch/x86/mm/srat_64.c +1 -1

index a271241..a5bc297 100644
@@ -238,7 +238,7 @@ update_nodes_add(int node, unsigned long start, 
 unsigned long end)
 void __init
 acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 {
-    struct bootnode *nd, oldnode;
+    struct bootnode *nd;
     unsigned long start, end;
     int node, pxm;

     int i;

arch/x86/mm/srat_64.c +0 -1

@@ -277,7 +277,6 @@ acpi_numa_memory_affinity_init(struct 
 acpi_srat_mem_affinity *ma)
         return;
     }
     nd = &nodes[node];
-    oldnode = *nd;
     if (!node_test_and_set(node, nodes_parsed)) {
         nd->start = start;

         nd->end = end;

arch/x86/mm/srat_64.c +1 -6

@@ -291,13 +290,8 @@ acpi_numa_memory_affinity_init(struct 
 acpi_srat_mem_affinity *ma)
     printk(KERN_INFO "SRAT: Node %u PXM %u %lx-%lx\n", node, pxm,
            start, end);
 
-    if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
+    if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)
         update_nodes_add(node, start, end);
-        /* restore nodes[node] */
-        *nd = oldnode;
-        if ((nd->start | nd->end) == 0)
-            node_clear(node, nodes_parsed);
-    }
 
     node_memblk_range[num_node_memblks].start = start;

     node_memblk_range[num_node_memblks].end = end;

Signature · 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

This is to fix the bug discussed in email thread:
http://patchwork.kernel.org/patch/69499/.

Currently node_possible_map won't include the offlined node that has neither CPU onlined nor MEM onlined at booting time. As a result, nr_node_ids won't be equal to possible nodes.

CC Thomas Gleixner <[email protected]>

CC Ingo Molnar <[email protected]>

CC H. Peter Anvin <[email protected]>

CC Andi Kleen <[email protected]>

Signed-off-by Haicheng Li <[email protected]>

---
 arch/x86/mm/srat_64.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

arch/x86/mm/srat_64.c +1 -1

index a271241..a5bc297 100644
@@ -238,7 +238,7 @@ update_nodes_add(int node, unsigned long start, unsigned
 long end)
 void __init
 acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
 {
-	struct bootnode *nd, oldnode;
+	struct bootnode *nd;
 	unsigned long start, end;
 	int node, pxm;

arch/x86/mm/srat_64.c +0 -1

 	int i;

@@ -277,7 +277,6 @@ acpi_numa_memory_affinity_init(struct
 acpi_srat_mem_affinity *ma)
 		return;
 	}
 	nd = &nodes[node];
-	oldnode = *nd;
 	if (!node_test_and_set(node, nodes_parsed)) {
 		nd->start = start;

arch/x86/mm/srat_64.c +1 -6

 		nd->end = end;

@@ -291,13 +290,8 @@ acpi_numa_memory_affinity_init(struct
 acpi_srat_mem_affinity *ma)
 	printk(KERN_INFO "SRAT: Node %u PXM %u %lx-%lx\n", node, pxm,
 	       start, end);
 
-	if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
+	if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)
 		update_nodes_add(node, start, end);
-		/* restore nodes[node] */
-		*nd = oldnode;
-		if ((nd->start | nd->end) == 0)
-			node_clear(node, nodes_parsed);
-	}
 
 	node_memblk_range[num_node_memblks].start = start;

node_memblk_range[num_node_memblks].end = end;

Quoted 3 lines · Haicheng Li

This is to fix the bug discussed in email thread:
http://patchwork.kernel.org/patch/69499/.

It would be better to include a brief synopsis of the problem in the changelog instead of a citation, otherwise it obfuscates the issue it solves.

Quoted 50 lines

Currently node_possible_map won't include the offlined node that has neither CPU onlined nor MEM onlined at booting time. As a result, nr_node_ids won't be equal to possible nodes.

CC Thomas Gleixner <[email protected]>

CC Ingo Molnar <[email protected]>

CC H. Peter Anvin <[email protected]>

CC Andi Kleen <[email protected]>

Signed-off-by Haicheng Li <[email protected]>

---
 arch/x86/mm/srat_64.c |   10 ++--------

 1 files changed, 2 insertions(+), 8 deletions(-)

arch/x86/mm/srat_64.c +0 -0

index a271241..a5bc297 100644
@@ -238,7 +238,7 @@ update_nodes_add(int node, unsigned long start, unsigned
 long end)

 void __init
 acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma) {
-     struct bootnode *nd, oldnode;
+     struct bootnode *nd;
      unsigned long start, end;
      int node, pxm;
      int i;

arch/x86/mm/srat_64.c +0 -1

@@ -277,7 +277,6 @@ acpi_numa_memory_affinity_init(struct
 acpi_srat_mem_affinity *ma)
               return;
       }
       nd = &nodes[node];
-     oldnode = *nd;
       if (!node_test_and_set(node, nodes_parsed)) {
               nd->start = start;

              nd->end = end;

arch/x86/mm/srat_64.c +1 -6

@@ -291,13 +290,8 @@ acpi_numa_memory_affinity_init(struct
 acpi_srat_mem_affinity *ma)
       printk(KERN_INFO "SRAT: Node %u PXM %u %lx-%lx\n", node, pxm,
              start, end);
 
-     if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
+     if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)
               update_nodes_add(node, start, end);
-             /* restore nodes[node] */
-             *nd = oldnode;
-             if ((nd->start | nd->end) == 0)
-                     node_clear(node, nodes_parsed);
-     }
 

Quoted 2 lines

arch/x86/mm/srat_64.c +0 -0

       node_memblk_range[num_node_memblks].start = start;

      node_memblk_range[num_node_memblks].end = end;

There're a couple of issues with this patch:

 - it breaks CONFIG_MEMORY_HOTPLUG_SPARSE kernels when the SRAT reports more than one entry for a single node id and the later entry does not specify an address range, and

 - carrying the bit for the node over in nodes_parsed is inappropriate since other x86 code depends on that map including only nodes with memory such as e820_register_active_regions() and nodes_cover_memory().

Quoted 5 lines

There're a couple of issues with this patch:

- it breaks CONFIG_MEMORY_HOTPLUG_SPARSE kernels when the SRAT reports more than one entry for a single node id and the later entry does not specify an address range, and

not sure this one.

Quoted 3 lines

- carrying the bit for the node over in nodes_parsed is inappropriate since other x86 code depends on that map including only nodes with memory such as e820_register_active_regions() and nodes_cover_memory().

assume if e820 is right, even srat table have a range for hot plug aka the e820 have hole already. then e820_register_active_regions() and nodes_cover_memory
should be ok even bootnode have some pre reserved range.

Quoted 7 lines · Yinghai Lu

Quoted 5 lines

There're a couple of issues with this patch:

- it breaks CONFIG_MEMORY_HOTPLUG_SPARSE kernels when the SRAT reports more than one entry for a single node id and the later entry does not specify an address range, and

not sure this one.

The old code would preserve the address range from the preceding SRAT entry for that node to pass to e820_register_active_regions() when CONFIG_MEMORY_HOTPLUG_SPARSE is enabled, this patch incorrectly clears that data.

Quoted 8 lines

Quoted 3 lines

- carrying the bit for the node over in nodes_parsed is inappropriate since other x86 code depends on that map including only nodes with memory such as e820_register_active_regions() and nodes_cover_memory().

assume if e820 is right, even srat table have a range for hot plug aka the e820 have hole already. then e820_register_active_regions() and nodes_cover_memory
should be ok even bootnode have some pre reserved range.

cpu_nodes_parsed handles nodes without memory, there's no reason why a bit should be set in nodes_parsed if its corresponding node does not have a valid address range.

We have a reasonable expectation that nodes_parsed represents memory nodes given its use for e820_register_active_regions() and nodes_cover_memory() as well as acpi_get_nodes() for NUMA emulation, for example, which would be broken with this patch. See dc0985519.

- it recovers node with oldnode as long as current entry is
HOT_PLUGGABLE. so it handles the recover issue. but I think following patch can simply fix it as well.

arch/x86/mm/srat_64.c +0 -0

index dbb5381..fdf067f 100644

arch/x86/mm/srat_64.c +0 -0

         if (start < nd->start)
             nd->start = start;

         if (nd->end < end)

Quoted 4 lines

The old code would preserve the address range from the preceding SRAT entry for that node to pass to e820_register_active_regions() when CONFIG_MEMORY_HOTPLUG_SPARSE is enabled, this patch incorrectly clears that data.

To fully understand what could happen with old code, I write a debug patch like following:

arch/x86/mm/srat_64.c +5 -1

index dbb5381..f7118d1 100644
@@ -290,6 +290,8 @@ acpi_numa_memory_affinity_init(struct
 acpi_srat_mem_affinity *ma)
 
 	printk(KERN_INFO "SRAT: Node %u PXM %u %lx-%lx\n", node, pxm,
 	       start, end);
+	printk(KERN_INFO "SRAT: Node %u oldnode: %lx-%lx\n", node,
+	       oldnode.start, oldnode.end);
 	e820_register_active_regions(node, start >> PAGE_SHIFT,
 				     end >> PAGE_SHIFT);

@@ -297,8 +299,10 @@ acpi_numa_memory_affinity_init(struct
 acpi_srat_mem_affinity *ma)
 		update_nodes_add(node, start, end);
 		/* restore nodes[node] */
 		*nd = oldnode;
-		if ((nd->start | nd->end) == 0)
+		if ((nd->start | nd->end) == 0) {
 			node_clear(node, nodes_parsed);
+			printk("node-clear: %u\n", node);
+		}
 	}
 

node_memblk_range[num_node_memblks].start = start;

Then on a system with Node0 on, but Node1 off (both cpu and memory are off), the booting log is like:

[    0.000000] SRAT: Node 0 PXM 0 0-80000000
[    0.000000] SRAT: Node 0 oldnode: 0-0
[    0.000000] SRAT: Node 0 PXM 0 100000000-280000000
[    0.000000] SRAT: Node 0 oldnode: 0-80000000
[    0.000000] SRAT: Node 0 PXM 0 300000000-2300000000
[    0.000000] SRAT: Node 0 oldnode: 0-280000000
[    0.000000] SRAT: hot plug zone found 300000000 - 2300000000
[    0.000000] SRAT: Node 0 PXM 0 2300000000-4300000000
[    0.000000] SRAT: Node 0 oldnode: 0-280000000
[    0.000000] SRAT: hot plug zone found 300000000 - 4300000000

[    0.000000] SRAT: Node 1 PXM 1 4300000000-6300000000
[    0.000000] SRAT: Node 1 oldnode: 0-0
[    0.000000] SRAT: hot plug zone found 4300000000 - 6300000000
[    0.000000] node-clear: 1
[    0.000000] SRAT: Node 1 PXM 1 6300000000-8300000000
[    0.000000] SRAT: Node 1 oldnode: 0-0
[    0.000000] SRAT: hot plug zone found 4300000000 - 8300000000
[    0.000000] node-clear: 1

David, per my understanding, your concern should be like, with this fix, if 3rd or 4th entry of Node0 has no address range, then Node0 won't be recoverd with oldnode and won't be cleared in nodes_parsed. But how is it handled by old code?

- it recovers node with oldnode as long as current entry is HOT_PLUGGABLE. so it handles the recover issue. but I think following patch can simply fix it as well.

Quoted 3 lines

cpu_nodes_parsed handles nodes without memory, there's no reason why a bit should be set in nodes_parsed if its corresponding node does not have a valid address range.

For a node has _NOT_ either CPU or Memory like Node1, cpu_nodes_parsed cannot handle it.

Quoted 5 lines

We have a reasonable expectation that nodes_parsed represents memory nodes given its use for e820_register_active_regions() and nodes_cover_memory() as well as acpi_get_nodes() for NUMA emulation, for example, which would be broken with this patch. See dc0985519.

either nodes_cover_memory() or e820_register_active_regions() or acpi_get_nodes(), they all have node-addr-range check code, if the node-addr-range is invalid, they won't be harmed.

Quoted 5 lines · Haicheng Li

David, per my understanding, your concern should be like, with this fix, if 3rd or 4th entry of Node0 has no address range, then Node0 won't be recoverd with oldnode and won't be cleared in nodes_parsed. But how is it handled by old code?

It's not evident with your machine because you do not have two SRAT entries for the same node id, one without ACPI_SRAT_MEM_HOT_PLUGGABLE and other with ACPI_SRAT_MEM_HOT_PLUGGABLE.

The old code would preserve the address range for the former in oldnode and then reset its data in the struct bootnode since nodes_parsed has a bit set for that node. That's needed by later code that I've mentioned:
acpi_get_nodes(), specifically, which breaks with your patch in addition to nodes_cover_memory() and e820_register_active_regions().

Only when the previous oldnode entry does not have a valid address range, meaning it is [0, 0), does the bit get cleared in nodes_parsed.

Quoted 4 lines

- it recovers node with oldnode as long as current entry is HOT_PLUGGABLE. so it handles the recover issue. but I think following patch can simply fix it as well.

If it's not ACPI_SRAT_MEM_HOT_PLUGGABLE, we know the address range is already valid given the sanity checks that it has successfully passed through in acpi_numa_memory_affinity_init(), so we require no further checking. However, your patch will not reset the previous address range when a ACPI_SRAT_MEM_HOT_PLUGGABLE entry is found for the same address range and you're leaving the bit set in nodes_parsed.

Quoted 6 lines

Quoted 3 lines

cpu_nodes_parsed handles nodes without memory, there's no reason why a bit should be set in nodes_parsed if its corresponding node does not have a valid address range.

For a node has _NOT_ either CPU or Memory like Node1, cpu_nodes_parsed cannot handle it.

It most certainly can since its sole purpose is to include memoryless nodes in node_possible_map. It has no other use case that would break as the result of adding hotpluggable nodes, hence the reason I suggested renaming it no_mem_nodes_parsed.

Quoted 9 lines

Quoted 5 lines

We have a reasonable expectation that nodes_parsed represents memory nodes given its use for e820_register_active_regions() and nodes_cover_memory() as well as acpi_get_nodes() for NUMA emulation, for example, which would be broken with this patch. See dc0985519.

either nodes_cover_memory() or e820_register_active_regions() or acpi_get_nodes(), they all have node-addr-range check code, if the node-addr-range is invalid, they won't be harmed.

Wrong, acpi_get_nodes() does not have such a check it only iterates over nodes_parsed. In other words, you'd be starting a new requirement for nodes_parsed with your patch: it would now be necessary to check for a valid (non-zero) address range for each set bit. Instead, I'm suggesting the nodes_parsed represents only nodes with valid memory, which is a reasonable expectation given the semantics of both it and cpu_nodes_parsed to handle their memoryless counterparts.

In other words, the following should easily fix the issue without breaking the existing logic that preserves the old address range for node ids that have SRAT entries both with and without ACPI_SRAT_MEM_HOT_PLUGGABLE. Could you give it a try?

let's add more changes to fix naming issue as well since it's too confusing for people to understand
the code logic. how about below patch?

Quoted 3 lines · Haicheng Li

let's add more changes to fix naming issue as well since it's too confusing for people to understand
the code logic. how about below patch?

That should be a seperate change; there's a bugfix here (my patch) and then a cleanup patch that you could make incrementally on mine. I don't personally like the name "rest_nodes_parsed" since it's poor English, I suggest renaming nodes_parsed to mem_nodes_parsed as I originally asked and then cpu_nodes_parsed to acpi_nodes_parsed or something similiar.

Ingo, the following is my bugfix patch that addresses the issue at
http://patchwork.kernel.org/patch/69499. Haicheng, can we add your tested-by line?

x86: set hotpluggable nodes in nodes_possible_map

nodes_possible_map does not currently include nodes that have SRAT entries that are all ACPI_SRAT_MEM_HOT_PLUGGABLE since the bit is cleared in nodes_parsed if it does not have an online address range.

Unequivocally setting the bit in nodes_parsed is insufficient since existing code, such as acpi_get_nodes(), assumes all nodes in the map have online address ranges. In fact, all code using nodes_parsed assumes such nodes represent an address range of online memory.

nodes_possible_map is created by unioning nodes_parsed and cpu_nodes_parsed; the former represents nodes with online memory and the latter represents memoryless nodes. We now set the bit for hotpluggable nodes in cpu_nodes_parsed so that it also gets set in nodes_possible_map.

Signed-off-by David Rientjes <[email protected]>

---
 arch/x86/mm/srat_64.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

arch/x86/mm/srat_64.c +0 -0

@@ -229,9 +229,11 @@ update_nodes_add(int node, unsigned long start, unsigned long end)
 			printk(KERN_ERR "SRAT: Hotplug zone not continuous. Partly ignored\n");
 	}

Quoted 25 lines

nodes_possible_map does not currently include nodes that have SRAT entries that are all ACPI_SRAT_MEM_HOT_PLUGGABLE since the bit is cleared in nodes_parsed if it does not have an online address range.

Unequivocally setting the bit in nodes_parsed is insufficient since existing code, such as acpi_get_nodes(), assumes all nodes in the map have online address ranges. In fact, all code using nodes_parsed assumes such nodes represent an address range of online memory.

nodes_possible_map is created by unioning nodes_parsed and cpu_nodes_parsed; the former represents nodes with online memory and the latter represents memoryless nodes. We now set the bit for hotpluggable nodes in cpu_nodes_parsed so that it also gets set in nodes_possible_map.

Signed-off-by David Rientjes <[email protected]>

---
 arch/x86/mm/srat_64.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

arch/x86/mm/srat_64.c +0 -0

@@ -229,9 +229,11 @@ update_nodes_add(int node, unsigned long start, unsigned long end)
 			printk(KERN_ERR "SRAT: Hotplug zone not continuous. Partly ignored\n");
 	}

arch/x86/mm/srat_64.c update_nodes_add(int node, unsigned long start, unsigned long end) · line 239

Quoted 10 lines

arch/x86/mm/srat_64.c +3 -1

 
-	if (changed)
+	if (changed) {
+		node_set(node, cpu_nodes_parsed);
 		printk(KERN_INFO "SRAT: hot plug zone found %Lx - %Lx\n",
 				 nd->start, nd->end);
+	}
 }
 
 /* Callback for parsing of the Proximity Domain <-> Memory Area mappings */

        if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
                update_nodes_add(node, start, end);
                /* restore nodes[node] */
                *nd = oldnode;
                if ((nd->start | nd->end) == 0)
                        node_clear(node, nodes_parsed);
        }

removing clearing with nodes_parsed is not working?

Quoted 3 lines · Haicheng Li

let's add more changes to fix naming issue as well since it's too confusing for people to understand
the code logic. how about below patch?

That should be a seperate change; there's a bugfix here (my patch) and then a cleanup patch that you could make incrementally on mine. I don't personally like the name "rest_nodes_parsed" since it's poor English, I suggest renaming nodes_parsed to mem_nodes_parsed as I originally asked and then cpu_nodes_parsed to acpi_nodes_parsed or something similiar.

Ingo, the following is my bugfix patch that addresses the issue at http://patchwork.kernel.org/patch/69499.

        if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
                update_nodes_add(node, start, end);
                /* restore nodes[node] */
                *nd = oldnode;
                if ((nd->start | nd->end) == 0)
                        node_clear(node, nodes_parsed);
        }

removing clearing with nodes_parsed is not working?

YH

arch/x86/mm/srat_64.c +13 -1

index a271241..c5552ae 100644
@@ -27,8 +27,18 @@ int acpi_numa __initdata;
 
 static struct acpi_table_slit *acpi_slit;
 
+/* nodes_parsed:
+ *  - nodes with memory on
+ * cpu_nodes_parsed:
+ *  - nodes with cpu on
+ * hp_nodes_parsed:
+ *  - nodes with hotpluggable memory region
+ *
+ * We union these tree nodemasks to get node_possible_map.
+ */
 static nodemask_t nodes_parsed __initdata;
 static nodemask_t cpu_nodes_parsed __initdata;
+static nodemask_t hp_nodes_parsed __initdata;
 static struct bootnode nodes[MAX_NUMNODES] __initdata;
 static struct bootnode nodes_add[MAX_NUMNODES];
 

@@ -229,9 +239,11 @@ update_nodes_add(int node, unsigned long start, unsigned
 long end)
 			printk(KERN_ERR "SRAT: Hotplug zone not continuous.
 Partly ignored\n");
 	}
 
-	if (changed)
+	if (changed) {
+		node_set(node, hp_nodes_parsed);
 		printk(KERN_INFO "SRAT: hot plug zone found %Lx - %Lx\n",
 				 nd->start, nd->end);
+	}
 }

/* Callback for parsing of the Proximity Domain <-> Memory Area mappings */

arch/x86/mm/srat_64.c +3 -1

@@ -380,8 +392,10 @@ int __init acpi_scan_nodes(unsigned long start, unsigned
 long end)
 		return -1;
 	}
 
-	/* Account for nodes with cpus and no memory */
+	/* Account for nodes with memory and cpus */
 	nodes_or(node_possible_map, nodes_parsed, cpu_nodes_parsed);
+	/* Account for nodes with hotpluggable memory regions */
+	nodes_or(node_possible_map, node_possible_map, hp_nodes_parsed);
 
 	/* Finally register nodes */

for_each_node_mask(i, node_possible_map)

Quoted 49 lines · Haicheng Li

arch/x86/mm/srat_64.c +13 -1

index a271241..c5552ae 100644
@@ -27,8 +27,18 @@ int acpi_numa __initdata;
 
 static struct acpi_table_slit *acpi_slit;
 
+/* nodes_parsed:
+ *  - nodes with memory on
+ * cpu_nodes_parsed:
+ *  - nodes with cpu on
+ * hp_nodes_parsed:
+ *  - nodes with hotpluggable memory region
+ *
+ * We union these tree nodemasks to get node_possible_map.
+ */
 static nodemask_t nodes_parsed __initdata;
 static nodemask_t cpu_nodes_parsed __initdata;
+static nodemask_t hp_nodes_parsed __initdata;
 static struct bootnode nodes[MAX_NUMNODES] __initdata;
 static struct bootnode nodes_add[MAX_NUMNODES];
 

@@ -229,9 +239,11 @@ update_nodes_add(int node, unsigned long start, unsigned
 long end)
 			printk(KERN_ERR "SRAT: Hotplug zone not continuous.
 Partly ignored\n");
 	}
 
-	if (changed)
+	if (changed) {
+		node_set(node, hp_nodes_parsed);
 		printk(KERN_INFO "SRAT: hot plug zone found %Lx - %Lx\n",
 				 nd->start, nd->end);
+	}
 }

/* Callback for parsing of the Proximity Domain <-> Memory Area mappings */

arch/x86/mm/srat_64.c +3 -1

@@ -380,8 +392,10 @@ int __init acpi_scan_nodes(unsigned long start, unsigned
 long end)
 		return -1;
 	}
 
-	/* Account for nodes with cpus and no memory */
+	/* Account for nodes with memory and cpus */
 	nodes_or(node_possible_map, nodes_parsed, cpu_nodes_parsed);
+	/* Account for nodes with hotpluggable memory regions */
+	nodes_or(node_possible_map, node_possible_map, hp_nodes_parsed);
 

Quoted 4 lines

arch/x86/mm/srat_64.c +0 -0

 	/* Finally register nodes */

for_each_node_mask(i, node_possible_map)

Nack, we don't need to add yet another nodemask because you're having trouble finding a new name for a cpu_nodes_parsed. It would be perfectly

acceptable to rename nodes_parsed to mem_nodes and cpu_nodes_parsed to no_mem_nodes, which are even shorter. But we definitely don't need another nodemask and I think we're convoluting the bug fix here way too much. Regardless of the future nomenclature, let's please merge my patch to fix the outstanding kernel issue and then propose an alternative naming

Quoted 2 lines

Nack, we don't need to add yet another nodemask because you're having trouble finding a new name for a cpu_nodes_parsed. It would be perfectly

Hey Dave, why do you think it's just a naming issue?

What I'm concerning is that your assumption of cpu_nodes_parsed use is wrong, cpu_nodes_parsed is needed anyway since its semantics represent the node with cpu affinity rather than memless node, that's also why I originally doubted cpu_node_parsed cannot handle hotplug node.

we also need hp_nodes_parsed to represent the node with hotpluggable memory region, just like why we need nodes_parsed to repsent node with mem on.

Quoted 8 lines · Haicheng Li

Quoted 2 lines

Nack, we don't need to add yet another nodemask because you're having trouble finding a new name for a cpu_nodes_parsed. It would be perfectly

Hey Dave, why do you think it's just a naming issue?

What I'm concerning is that your assumption of cpu_nodes_parsed use is wrong, cpu_nodes_parsed is needed anyway since its semantics represent the node with cpu affinity rather than memless node, that's also why I originally doubted cpu_node_parsed cannot handle hotplug node.

Wrong, cpu_nodes_parsed (despite its name) solely represents nodes that do not have online address memory ranges. That's it. Nothing more, nothing less. That's why I suggest you rename it to no_mem_nodes or something similar. Look at the single time that the nodemask is used: to set cleared bits in node_possible_map that were not set in nodes_parsed because THEY DO NOT HAVE ASSOCIATED ONLINE MEMORY RANGES, the _only_ time a node gets set in nodes_parsed.

Once you rename nodes_parsed to mem_nodes and cpu_nodes_parsed to no_mem_nodes, it may become clearer.

Quoted 3 lines

we also need hp_nodes_parsed to represent the node with hotpluggable memory region, just like why we need nodes_parsed to repsent node with mem on.

It's pointless to add another nodemask, the semantics of cpu_nodes_parsed is perfectly legitimate for hotpluggable nodes as well. Instead of fixating on the name, look at the code that uses it.

Quoted 3 lines

It's pointless to add another nodemask, the semantics of cpu_nodes_parsed is perfectly legitimate for hotpluggable nodes as well. Instead of fixating on the name, look at the code that uses it.

I'm not meant to be rude, but it's illogical excuse. that it is now used by a single function doesn't mean that it will never be used by others in future or it is only useful for that single function.

You've already tested my patch that this thread was restarted with and it works, so let's fix the bug. Then, later, you can rename cpu_nodes_parsed to no_mems_nodes, which I'd agree with. You may even try to seperate the hotpluggable nodes out into their own nodemask, but I trust that the x86 maintainers will be looking for some rationale behind that other than "it may one day be useful."

getting _very_ late in the 2.6.33 release cycle. Do you expect Ingo to push your fix to Linus with the rationale that "maybe someday we'll use this new nodemask even though it may be rc5 and nobody knows what we'd ever use it for"? Is that appropriate for -stable candidates as well?

Quoted 2 lines · David Rientjes

You've already tested my patch that this thread was restarted with and it works, so let's fix the bug. Then, later, you can rename

cpu_nodes_parsed

Quoted 4 lines

to no_mems_nodes, which I'd agree with. You may even try to seperate the hotpluggable nodes out into their own nodemask, but I trust that the x86 maintainers will be looking for some rationale behind that other than "it may one day be useful."

David, you are misleading people to fix the BUG with a logically problematic patch. I don't want such fixing to possibly bother other people someday, please let's avoid it in review stage.

Quoted 4 lines

getting _very_ late in the 2.6.33 release cycle. Do you expect Ingo to push your fix to Linus with the rationale that "maybe someday we'll use this new nodemask even though it may be rc5 and nobody knows what we'd ever use it for"? Is that appropriate for -stable candidates as well?

Don't speak for any other people. Let maintainers themselves decide if my patch is ugly or acceptable. I don't want to argue with you anymore if you cannot find any true problem from my recent patch.

Below is my updated patch (in fact, it's v2 for the patch I sent out for review in http://lkml.org/lkml/2010/1/15/9).

I have to say I agree with David Rientjes that we need the minimal patch for upstream and stable. If you need the additional bitmask in the future it should be added later.

Haicheng, would you be willing to prepare a minimal patch so we can close the issue in the release trees as quickly as possible?

Quoted 4 lines · H. Peter Anvin

I have to say I agree with David Rientjes that we need the minimal patch for upstream and stable. If you need the additional bitmask in the future it should be added later.

Quoted 2 lines

Haicheng, would you be willing to prepare a minimal patch so we can close the issue in the release trees as quickly as possible?

Peter,

Okay, let's close it. then please take the patch pasted below, which is the one without additional bitmask added.

Quoted 7 lines · Haicheng Li

Quoted 6 lines · H. Peter Anvin

I have to say I agree with David Rientjes that we need the minimal patch for upstream and stable. If you need the additional bitmask in the future it should be added later.

Haicheng, would you be willing to prepare a minimal patch so we can close the issue in the release trees as quickly as possible?

Quoted 5 lines

Peter,

Okay, let's close it. then please take the patch pasted below, which is the one without additional bitmask added.

As David asked, OK to add your Tested-by: line?