cpumask_of_node() should handle -1 as a node

A number of places in the kernel use cpumask_of_node(-1), but most arch NUMA implementations are broken. The following patches fix this.

Apologies for the resend, somehow quilt mail screwed up the message ids.

pcibus_to_node can return -1 if we cannot determine which node a pci bus is on. If passed -1, cpumask_of_node will negatively index the lookup array and pull in random data:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
00000000,00000003,00000000,00000000
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
64-65

Change cpumask_of_node to check for -1 and return cpu_all_mask in this case:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
ffffffff,ffffffff,ffffffff,ffffffff
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
0-127

---

Index: linux-cpumask/arch/powerpc/include/asm/topology.h
===================================================================
@@ -17,7 +17,9 @@ static inline int cpu_to_node(int cpu)
 
 #define parent_node(node)	(node)
 
-#define cpumask_of_node(node) (&numa_cpumask_lookup_table[node])
+#define cpumask_of_node(node) ((node) == -1 ?				\
+			       cpu_all_mask :				\
+			       &numa_cpumask_lookup_table[node])
 
 int of_node_to_nid(struct device_node *device);
 

pcibus_to_node can return -1 if we cannot determine which node a pci bus is on. If passed -1, cpumask_of_node will negatively index the lookup array and pull in random data:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
00000000,00000003,00000000,00000000
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
64-65

Change cpumask_of_node to check for -1 and return cpu_all_mask in this case:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
ffffffff,ffffffff,ffffffff,ffffffff
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
0-127

---

Index: linux-cpumask/arch/alpha/include/asm/topology.h
===================================================================
@@ -28,6 +28,9 @@ static const struct cpumask *cpumask_of_
 {
 	int cpu;
 
+	if (node == -1)
+		return cpu_all_mask;
+
 	cpumask_clear(&node_to_cpumask_map[node]);
 
 	for_each_online_cpu(cpu) {

pcibus_to_node can return -1 if we cannot determine which node a pci bus is on. If passed -1, cpumask_of_node will negatively index the lookup array and pull in random data:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
00000000,00000003,00000000,00000000
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
64-65

Change cpumask_of_node to check for -1 and return cpu_all_mask in this case:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
ffffffff,ffffffff,ffffffff,ffffffff
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
0-127

---

Index: linux-cpumask/arch/ia64/include/asm/topology.h
=================================--- linux-cpumask.orig/arch/ia64/include/asm/topology.h 2010-01-06 15:20:22.832584260 +1100
+++ linux-cpumask/arch/ia64/include/asm/topology.h 2010-01-06 15:26:46.620650121 +1100
@@ -33,7 +33,9 @@
/*
* Returns a bitmask of CPUs on Node 'node'. */
-#define cpumask_of_node(node) (&node_to_cpu_mask[node])
+#define cpumask_of_node(node) ((node) = -1 ? \
+ cpu_all_mask : \
+ &node_to_cpu_mask[node])

/*
* Returns the number of the node containing Node 'nid'.

pcibus_to_node can return -1 if we cannot determine which node a pci bus is on. If passed -1, cpumask_of_node will negatively index the lookup array and pull in random data:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
00000000,00000003,00000000,00000000
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
64-65

Change cpumask_of_node to check for -1 and return cpu_all_mask in this case:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
ffffffff,ffffffff,ffffffff,ffffffff
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
0-127

---

Index: linux-cpumask/arch/mips/include/asm/mach-ip27/topology.h
===================================================================
@@ -24,7 +24,9 @@ extern struct cpuinfo_ip27 sn_cpu_info[N
 
 #define cpu_to_node(cpu)	(sn_cpu_info[(cpu)].p_nodeid)
 #define parent_node(node)	(node)
-#define cpumask_of_node(node)	(&hub_data(node)->h_cpus)
+#define cpumask_of_node(node)	((node) == -1 ?				\
+				 cpu_all_mask :				\
+				 &hub_data(node)->h_cpus)
 struct pci_bus;
 extern int pcibus_to_node(struct pci_bus *);
 

Thanks, applied!

Ralf

pcibus_to_node can return -1 if we cannot determine which node a pci bus is on. If passed -1, cpumask_of_node will negatively index the lookup array and pull in random data:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
00000000,00000003,00000000,00000000
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
64-65

Change cpumask_of_node to check for -1 and return cpu_all_mask in this case:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
ffffffff,ffffffff,ffffffff,ffffffff
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
0-127

---

Index: linux-cpumask/arch/sparc/include/asm/topology_64.h
=================================--- linux-cpumask.orig/arch/sparc/include/asm/topology_64.h 2010-01-06 15:20:22.912584116 +1100
+++ linux-cpumask/arch/sparc/include/asm/topology_64.h 2010-01-06 15:27:17.209419879 +1100
@@ -12,7 +12,9 @@ static inline int cpu_to_node(int cpu)

#define parent_node(node) (node)

-#define cpumask_of_node(node) (&numa_cpumask_lookup_table[node])
+#define cpumask_of_node(node) ((node) = -1 ? \
+ cpu_all_mask : \
+ &numa_cpumask_lookup_table[node])

struct pci_bus;
#ifdef CONFIG_PCI

From: Anton Blanchard <[email protected]>
Date: Wed, 06 Jan 2010 15:55:14 +1100

pcibus_to_node can return -1 if we cannot determine which node a pci bus is on. If passed -1, cpumask_of_node will negatively index the lookup array and pull in random data:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
00000000,00000003,00000000,00000000
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
64-65

Change cpumask_of_node to check for -1 and return cpu_all_mask in this case:

# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpus
ffffffff,ffffffff,ffffffff,ffffffff
# cat /sys/devices/pci0000:00/0000:00:01.0/local_cpulist
0-127

---

index c5087d7..cc4f136 100644
@@ -99,7 +99,8 @@ extern const struct cpumask *cpumask_of_node(int node);
 /* Returns a pointer to the cpumask of CPUs on Node 'node'. */
 static inline const struct cpumask *cpumask_of_node(int node)
 {
-	return node_to_cpumask_map[node];
+	return (node == -1) ? cpu_all_mask :
+		node_to_cpumask_map[node];
 }
 #endif
 

index 550df48..e57a7f5 100644
@@ -47,6 +47,9 @@ void __init setup_node_to_cpumask_map(void)
  */
 const struct cpumask *cpumask_of_node(int node)
 {
+	if (node == -1)
+		return cpu_all_mask;
+
 	if (node >= nr_node_ids) {
 		printk(KERN_WARNING
 			"cpumask_of_node(%d): node > nr_node_ids(%d)\n",

If you grep for the callers, you'll see those which do this (now-obsolete) check. One more patch :)

Thanks,
Rusty.

Do we really want to do this? A nid of -1 is undefined, so the result of cpumask_of_node(-1) should be undefined; there's no formal definition that a nid of -1 follows the semantics that we use for x86 pci buses, for example, where it implies no NUMA locality in all cases.

I think a better fix would be to define -1 where appropriate to either be an invalid node, cpu_all_mask, or whatever in the caller:

	mask = (nid != -1) ? cpumask_of_node(nid) : cpu_all_mask;

Hi David,

I don't like the use of -1 as a node, but it's much more widespread than x86; including sh, powerpc, sparc and the generic topology code. eg:

#fdef CONFIG_PCI
extern int pcibus_to_node(struct pci_bus *pbus); #else
static inline int pcibus_to_node(struct pci_bus *pbus)

{
        return -1;
}

It would be nice to get rid of this special case but I suspect that's not 2.6.33 material.

Speaking of invalid node ids, I also noticed the scheduler isn't using node iterators:

        for (i = 0; i < nr_node_ids; i++) {

which should be fixed at some stage too since it doesn't allow us to allocate the node structures sparsely.

Anton

This seems to be the same semantics that NUMA_NO_NODE was defined for, it's not necessarily a special case.

Regardless, the result of cpumask_of_node(NUMA_NO_NODE) should be undefined as it currently is unless you want to obsolete NUMA_NO_NODE entirely which is much more work. In other words, special-casing a nid of -1 to mean no affinity is inappropriate if NUMA_NO_NODE represents an invalid nid.

If x86 pci buses want to use -1 to imply that meaning, that's fine, but it shouldn't be coded in a generic interface such as cpumask_of_node(). Does that make sense?

That loop has nothing to do with the allocation of a node structure, it's quite plausible that it checks for various states such as node_online(i) while looping and doing something else interesting for those that are offline. Keep in mind that this isn't equivalent to using for_each_node() since that only iterates over N_POSSIBLE which is architecture specific.

Hi David,

I wasn't using the example to strengthen the case of the -1 behaviour, but to highlight that a complete fix would be more work and risk not making it back to -stable.

I'm all for removing the special case as a followon patch.

Yeah I understand it isn't the same thing, but the scheduler oopses in a number of places with CPUMASK_OFFSTACK and sparse node ids, so things that can be switched to node iterators should and node_online() checks should be added elsewhere.

Anton

I don't think that we should defer a complete fix to the callers because it's "more work." If you've identified places where -1 is passed to cpumask_of_node() without being checked, I think those would be fairly obvious -stable candidates themselves instead of this series.

It's widespread, and we've just had another bug due to pcibus_to_node handling -1 and cpumask_of_node not. (Search lkml for subject "[Regression] 2.6.33-rc2
- pci: Commit e0cd516 causes OOPS").

So I think the evidence is in favor of just handling -1.

Cheers,
Rusty.

That's similiar to the problem in cpumask_of_pcibus() that I fixed with 7715a1e back in September. The difference is that I isolated my fix to the pci bus implementation that defined the nid of -1 to mean no NUMA affinity, whereas generic kernel code can use that value for any (or no) definition and returning cpu_all_mask may not apply. We know it does for pcibus, but not for generic NUMA node ids that happen to be invald.

The hope is that eventually we can remove many dependencies on node ids for these purposes; buses with no affinity are not actually members of any NUMA node. I had a proposal for a generic kernel interface that is based on ACPI system localities that would define the proximity of any system entity (of which node is only a type defining "memory") to each other. I'm waiting for enough time to work on that project.

NUMA is special because a single cpu is always a member of a single node, so we're violating the bidirectional mapping by saying that node -1 maps to all cpus while all cpus don't map to node -1. In other words, I think we should take this as an opportunity to find and fix broken callers as we've done both by my patch from September and by your aforementioned case. In this particular case, it would be a matter of doing:

	mask = (nid != -1) ? cpumask_of_node(nid) : cpu_all_mask;

I'm hoping that Ingo will weigh in on this topic with his taste and vision for how we should decouple device locality information from entities that are not members of any NUMA node if we continue to "special case" these things.

Hi Rusty,

Good point. I was hoping to have a chance at a -stable backport, so I'll do this cleanup in a follow up patch.

Anton

So.. what's happening here? Nobody has applied the patches anywhere, you later said "the scheduler oopses in a number of places with CPUMASK_OFFSTACK and sparse node ids" and mention of a -stable backport was mentioned.

Hi Andrew,

That scheduler issue doesn't appear in mainline as no one is allocating sparse node IDs yet.

There is still the opportunity for an Oops with CPUMASK_OFFSTACK and /sys/.../local_cpus and even with CPUMASK_OFFSTACK disabled it will return random data.

Ralph has picked up the mips bit, Ben is sucking in the powerpc bit. Davem has acked the sparc bit. I'll roll up a -stable patch once they hit.

No answer from alpha and ia64.

Anton

Hi,

Just checked and it's already upstream on ia64. That just leaves alpha.

Anton