Skip to contentSkip to conversation

x86_64: Fix ACPI NUMA parsing

PATCH In review Andi Kleen 2 participants 3 messages Jan 12, 2005 – Jan 13, 2005 Bitmap API

lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/4 x86_64: Fix ACPI NUMA parsing

Fix SRAT NUMA parsing

Fix fallout from the recent nodemask_t changes. The node ids assigned in the SRAT parser were off by one.

I added a new first_unset_node() function to nodemask.h to allocate IDs sanely.

Signed-off-by Andi Kleen <[email protected]>
arch/x86_64/mm/srat.c +10 -7
Index: linux/arch/x86_64/mm/srat.c
===================================================================
@@ -20,17 +20,20 @@
 
 static struct acpi_table_slit *acpi_slit;
 
-static DECLARE_BITMAP(nodes_parsed, MAX_NUMNODES) __initdata;
+static nodemask_t nodes_parsed __initdata;
+static nodemask_t nodes_found __initdata;
 static struct node nodes[MAX_NUMNODES] __initdata;
 static __u8  pxm2node[256] __initdata = { [0 ... 255] = 0xff };
 
 static __init int setup_node(int pxm)
 {
-	if (pxm2node[pxm] == 0xff) {
-		if (num_online_nodes() >= MAX_NUMNODES)
+	unsigned node = pxm2node[pxm];
+	if (node == 0xff) {
+		if (nodes_weight(nodes_found) >= MAX_NUMNODES)
 			return -1;
-		pxm2node[pxm] = num_online_nodes();
-		node_set_online(num_online_nodes());
+		node = first_unset_node(nodes_found); 
+		node_set(node, nodes_found);
+		pxm2node[pxm] = node;
 	}
 	return pxm2node[pxm];
 }

@@ -140,7 +143,7 @@
 		return;
 	}
 	nd = &nodes[node];
-	if (!test_and_set_bit(node, &nodes_parsed)) {
+	if (!node_test_and_set(node, nodes_parsed)) {
 		nd->start = start;
 		nd->end = end;
 	} else {

@@ -171,7 +174,7 @@
 		return -1;
 	}
 	for (i = 0; i < MAX_NUMNODES; i++) {
-		if (!test_bit(i, &nodes_parsed))
+		if (!node_isset(i, nodes_parsed))
 			continue;
 		cutoff_node(i, start, end);
 		if (nodes[i].start == nodes[i].end)
include/linux/nodemask.h +9 -0
Index: linux/include/linux/nodemask.h
===================================================================
@@ -38,6 +38,8 @@
  *
  * int first_node(mask)			Number lowest set bit, or MAX_NUMNODES
  * int next_node(node, mask)		Next node past 'node', or MAX_NUMNODES
+ * int first_unset_node(mask)		First node not set in mask, or 
+ *					MAX_NUMNODES.
  *
  * nodemask_t nodemask_of_node(node)	Return nodemask with bit 'node' set
  * NODE_MASK_ALL			Initializer - all bits set

@@ -238,6 +240,13 @@
 	m;								\
 })
 
+#define first_unset_node(mask) __first_unset_node(&(mask))
+static inline int __first_unset_node(const nodemask_t *maskp)
+{
+	return min_t(int,MAX_NUMNODES,
+			find_first_zero_bit(maskp->bits, MAX_NUMNODES));
+}
+
 #define NODE_MASK_LAST_WORD BITMAP_LAST_WORD_MASK(MAX_NUMNODES)
 
 #if MAX_NUMNODES <= BITS_PER_LONG
#1 lore raw
Quoted 30 lines · Andi Kleen

Fix SRAT NUMA parsing

Fix fallout from the recent nodemask_t changes. The node ids assigned in the SRAT parser were off by one.

I added a new first_unset_node() function to nodemask.h to allocate IDs sanely.

Signed-off-by Andi Kleen <[email protected]>

Index: linux/arch/x86_64/mm/srat.c
===================================================================

arch/x86_64/mm/srat.c +4 -3
@@ -20,17 +20,20 @@
 
 static struct acpi_table_slit *acpi_slit;
 
-static DECLARE_BITMAP(nodes_parsed, MAX_NUMNODES) __initdata;
+static nodemask_t nodes_parsed __initdata;
+static nodemask_t nodes_found __initdata;
 static struct node nodes[MAX_NUMNODES] __initdata;
 static __u8  pxm2node[256] __initdata = { [0 ... 255] = 0xff };
 
 static __init int setup_node(int pxm)
 {
-	if (pxm2node[pxm] == 0xff) {
-		if (num_online_nodes() >= MAX_NUMNODES)
+	unsigned node = pxm2node[pxm];
+	if (node == 0xff) {

arch/x86_64/mm/srat.c line 39

Quoted 10 lines
arch/x86_64/mm/srat.c +4 -2
+		if (nodes_weight(nodes_found) >= MAX_NUMNODES)
 			return -1;
-		pxm2node[pxm] = num_online_nodes();
-		node_set_online(num_online_nodes());
+		node = first_unset_node(nodes_found); 
+		node_set(node, nodes_found);
+		pxm2node[pxm] = node;
 	}
 	return pxm2node[pxm];
 }

This snippet looks incorrect because first_unset_node() can return MAX_NUMNODES, which isn't a valid node number, nor is it valid to set a bit >= MAX_NUMNODES in a nodemask. Although, this is init code, so there shouldn't be other CPUs messing with the nodes_found mask at the same time, and you do check that there are unset bits in nodes_found before the first_unset_node() call, so I'm probably wrong. Is there any reason you don't just do all these checks on node_online_map? ie:

	if (nodes_weight(node_online_map) >= MAX_NUMNODES)
		return -1;
	node = first_unset_node(node_online_map); 
	node_set(node, nodes_found);
	pxm2node[pxm] = node;

The extra nodes_found bitmask seems unnecessary? I like the idea of the first_unset_node() function, though.

-Matt

author
#2 lore raw
Quoted 37 lines · Matthew Dobson
Quoted 36 lines · Andi Kleen

Fix SRAT NUMA parsing

Fix fallout from the recent nodemask_t changes. The node ids assigned in the SRAT parser were off by one.

I added a new first_unset_node() function to nodemask.h to allocate IDs sanely.

Signed-off-by Andi Kleen <[email protected]>

Index: linux/arch/x86_64/mm/srat.c
===================================================================

arch/x86_64/mm/srat.c +7 -5
@@ -20,17 +20,20 @@
 
 static struct acpi_table_slit *acpi_slit;
 
-static DECLARE_BITMAP(nodes_parsed, MAX_NUMNODES) __initdata;
+static nodemask_t nodes_parsed __initdata;
+static nodemask_t nodes_found __initdata;
 static struct node nodes[MAX_NUMNODES] __initdata;
 static __u8  pxm2node[256] __initdata = { [0 ... 255] = 0xff };
 
 static __init int setup_node(int pxm)
 {
-	if (pxm2node[pxm] == 0xff) {
-		if (num_online_nodes() >= MAX_NUMNODES)
+	unsigned node = pxm2node[pxm];
+	if (node == 0xff) {
+		if (nodes_weight(nodes_found) >= MAX_NUMNODES)
 			return -1;
-		pxm2node[pxm] = num_online_nodes();
-		node_set_online(num_online_nodes());
+		node = first_unset_node(nodes_found); 
+		node_set(node, nodes_found);
Quoted 8 lines

arch/x86_64/mm/srat.c line 39

Quoted 4 lines
arch/x86_64/mm/srat.c +1 -0
+		pxm2node[pxm] = node;
 	}
 	return pxm2node[pxm];
 }

This snippet looks incorrect because first_unset_node() can return MAX_NUMNODES, which isn't a valid node number, nor is it valid to set a bit >= MAX_NUMNODES in a nodemask. Although, this is init code, so

The nodes_weight check handles this.

Quoted 5 lines

there shouldn't be other CPUs messing with the nodes_found mask at the same time, and you do check that there are unset bits in nodes_found before the first_unset_node() call, so I'm probably wrong. Is there any reason you don't just do all these checks on node_online_map? ie:

Quoted 8 lines
	if (nodes_weight(node_online_map) >= MAX_NUMNODES)
		return -1;
	node = first_unset_node(node_online_map); 
	node_set(node, nodes_found);
	pxm2node[pxm] = node;

The extra nodes_found bitmask seems unnecessary? I like the idea of the first_unset_node() function, though.

It keeps clean global state in case there is an error and we fall back to non NUMA.

-Andi