check the node id consistently across different arches

According to Section 6.2.14 from ACPI spec 6.3 [1], the setting of proximity domain is optional, as below:

This optional object is used to describe proximity domain associations within a machine. _PXM evaluates to an integer that identifies a device as belonging to a Proximity Domain defined in the System Resource Affinity Table (SRAT).

This patch checks node id with the below case before returning node_to_cpumask_map[node]:
1. if node_id >= nr_node_ids, return cpu_none_mask
2. if node_id < 0, return cpu_online_mask
3. if node_to_cpumask_map[node_id] is NULL, return cpu_online_mask

[1] https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf

Signed-off-by Yunsheng Lin <[email protected]>

---
 arch/x86/include/asm/topology.h | 6 ++++++
 arch/x86/mm/numa.c              | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

arch/x86/include/asm/topology.h +0 -0

index 4b14d23..f36e9c8 100644
@@ -69,6 +69,12 @@ 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)

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

index e6dad60..5e393d2 100644
@@ -868,7 +868,7 @@ const struct cpumask *cpumask_of_node(int node)
 		dump_stack();
 		return cpu_none_mask;
 	}
-	if (node_to_cpumask_map[node] == NULL) {
+	if (node < 0 || !node_to_cpumask_map[node]) {
 		printk(KERN_WARNING
 			"cpumask_of_node(%d): no node_to_cpumask_map!\n",
 			node);

Signature · 1 line

2.8.1

Quoted 7 lines · Yunsheng Lin

According to Section 6.2.14 from ACPI spec 6.3 [1], the setting of proximity domain is optional, as below:

This optional object is used to describe proximity domain associations within a machine. _PXM evaluates to an integer that identifies a device as belonging to a Proximity Domain defined in the System Resource Affinity Table (SRAT).

That's just words.. what does it actually mean?

Quoted 27 lines

This patch checks node id with the below case before returning node_to_cpumask_map[node]:
1. if node_id >= nr_node_ids, return cpu_none_mask
2. if node_id < 0, return cpu_online_mask
3. if node_to_cpumask_map[node_id] is NULL, return cpu_online_mask

[1] https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf

Signed-off-by Yunsheng Lin <[email protected]>

---
 arch/x86/include/asm/topology.h | 6 ++++++
 arch/x86/mm/numa.c              | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

arch/x86/include/asm/topology.h +4 -0

index 4b14d23..f36e9c8 100644
@@ -69,6 +69,12 @@ 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)
 {
+	if (node >= nr_node_ids)
+		return cpu_none_mask;
+
+	if (node < 0 || !node_to_cpumask_map[node])

arch/x86/include/asm/topology.h extern const struct cpumask *cpumask_of_node(int node); · line 80

Quoted 5 lines

arch/x86/include/asm/topology.h +2 -0

+		return cpu_online_mask;
+
 	return node_to_cpumask_map[node];
 }
 #endif

I _reallly_ hate this. Users are expected to use valid numa ids. Now we're adding all this checking to all users. Why do we want to do that?

Using '(unsigned)node >= nr_nods_ids' is an error.

Quoted 17 lines

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

index e6dad60..5e393d2 100644
@@ -868,7 +868,7 @@ const struct cpumask *cpumask_of_node(int node)
 		dump_stack();
 		return cpu_none_mask;
 	}
-	if (node_to_cpumask_map[node] == NULL) {
+	if (node < 0 || !node_to_cpumask_map[node]) {
 		printk(KERN_WARNING
 			"cpumask_of_node(%d): no node_to_cpumask_map!\n",
 			node);

Signature · 1 line

2.8.1

.

Quoted 10 lines · Peter Zijlstra

Quoted 7 lines · Yunsheng Lin

According to Section 6.2.14 from ACPI spec 6.3 [1], the setting of proximity domain is optional, as below:

This optional object is used to describe proximity domain associations within a machine. _PXM evaluates to an integer that identifies a device as belonging to a Proximity Domain defined in the System Resource Affinity Table (SRAT).

That's just words.. what does it actually mean?

It means the dev_to_node(dev) may return -1 if the bios does not implement the proximity domain feature, user may use that value to call cpumask_of_node and cpumask_of_node does not protect itself from node id being -1, which causes out of bound access.

Quoted 8 lines

Quoted 8 lines

arch/x86/include/asm/topology.h +4 -0

@@ -69,6 +69,12 @@ 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)
 {
+	if (node >= nr_node_ids)
+		return cpu_none_mask;
+
+	if (node < 0 || !node_to_cpumask_map[node])

Quoted 8 lines

arch/x86/include/asm/topology.h extern const struct cpumask *cpumask_of_node(int node); · line 80

Quoted 5 lines

arch/x86/include/asm/topology.h +2 -0

+		return cpu_online_mask;
+
 	return node_to_cpumask_map[node];
 }
 #endif

I _reallly_ hate this. Users are expected to use valid numa ids. Now we're adding all this checking to all users. Why do we want to do that?

As above, the dev_to_node(dev) may return -1.

Quoted 2 lines

Using '(unsigned)node >= nr_nods_ids' is an error.

'node >= nr_node_ids' can be dropped if all user is expected to not call cpumask_of_node with node id greater or equal to nr_nods_ids.

From what I can see, the problem can be fixed in three place:
1. Make user dev_to_node return a valid node id even when proximity domain is not set by bios(or node id set by buggy bios is not valid), which may need info from the numa system to make sure it will return a valid node.

2. User that call cpumask_of_node should ensure the node id is valid before calling cpumask_of_node, and user also need some info to make ensure node id is valid.

3. Make sure cpumask_of_node deal with invalid node id as this patchset.

Which one do you prefer to make sure node id is valid, or do you have any better idea?

Any detail advice and suggestion will be very helpful, thanks.

Quoted 8 lines

Hi, Peter,

I found that this patch has been merged but I haven't received the e-mail for some unknown reasons.
https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/commit/?id=1c6c1ca318585f1096d4d04bc722297c85e9fb8a

Firstly, your comments are correct, so the barrier.h part is perfect.

Secondly, most of the rest is useless, because smp_mb__before_llsc, loongson_llsc_mb and other memory barriers are the same thing on Loongson-3. We don't need to add loongson_llsc_mb if there is already a smp_mb__before_llsc.

Quoted 1 line

Thirdly, maybe the only exception is syscall.c, but mips_atomic_set is not used on Loongson-3. And if in some cases we use it, I think the user-to-kernel context switch has the same effect of a memory barrier.

+. per-cpu like local_t *should only* be written by local cpu, and may be read by remote cpu sometimes

+. if and only if local cpu can write per-cpu, then Loongson3's llsc bug would not be triggerd.

same as this_cpu_cmpxchg_double

If so, then no need to add sync before and after cmpxchg_local

*why* are you replying to some random unrelated thread?

Also, please use a sane MUA and wrap your lines <80 chars.

Quoted 8 lines · On Wed, Sep 04, 2019 at 10:03:31AM +0800, [email protected] wrote:

Quoted 8 lines

Hi, Peter,

I found that this patch has been merged but I haven't received the e-mail for some unknown reasons.
https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/commit/?id=1c6c1ca318585f1096d4d04bc722297c85e9fb8a

Firstly, your comments are correct, so the barrier.h part is perfect.

Secondly, most of the rest is useless, because smp_mb__before_llsc, loongson_llsc_mb and other memory barriers are the same thing on Loongson-3. We don't need to add loongson_llsc_mb if there is already a smp_mb__before_llsc.

There wasn't. Take for example set_bit(), that didn't have smp_mb__before_llsc on.

Also; MIPS should probably convert to asm-generic/bitops/atomic.h.

Quoted 1 line

Quoted 1 line

Thirdly, maybe the only exception is syscall.c, but mips_atomic_set is not used on Loongson-3. And if in some cases we use it, I think the user-to-kernel context switch has the same effect of a memory barrier.

And how is some random person trying to make sense of MIPS to know that?

You all created a badly documented inconsitent trainwreck. You're 'lucky' the MIPS maintainers accepted that mess in the first place.

Anyway, yes there are too many barrers now in some cases, in a previous version I had:

https://lkml.kernel.org/r/[email protected]

But because I dropped changes to local.h that might not be true anymore; it needs careful consideration. Please audit carefully and if you find all smp_mb__before_llsc() usage is now superfluous for this 'funny' chip of yours, then re-submit the above patch.

*why* are you replying to some random unrelated thread?

Also, please use a sane MUA and wrap your lines <80 chars.

Quoted 8 lines

Quoted 8 lines

Hi, Peter,

I found that this patch has been merged but I haven't received the e-mail for some unknown reasons.
https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/commit/?id=1c6c1ca318585f1096d4d04bc722297c85e9fb8a

Firstly, your comments are correct, so the barrier.h part is perfect.

Secondly, most of the rest is useless, because smp_mb__before_llsc, loongson_llsc_mb and other memory barriers are the same thing on Loongson-3. We don't need to add loongson_llsc_mb if there is already a smp_mb__before_llsc.

There wasn't. Take for example set_bit(), that didn't have smp_mb__before_llsc on.

Also; MIPS should probably convert to asm-generic/bitops/atomic.h.

Quoted 1 line

Quoted 1 line

Thirdly, maybe the only exception is syscall.c, but mips_atomic_set is not used on Loongson-3. And if in some cases we use it, I think the user-to-kernel context switch has the same effect of a memory barrier.

And how is some random person trying to make sense of MIPS to know that?

You all created a badly documented inconsitent trainwreck. You're 'lucky' the MIPS maintainers accepted that mess in the first place.

Anyway, yes there are too many barrers now in some cases, in a previous version I had:

   https://lkml.kernel.org/r/[email protected]

But because I dropped changes to local.h that might not be true anymore; it needs careful consideration. Please audit carefully and if you find all smp_mb__before_llsc() usage is now superfluous for this 'funny' chip of yours, then re-submit the above patch.

Quoted 2 lines

+. per-cpu like local_t *should only* be written by local cpu, and may be read by remote cpu sometimes

Quoted 5 lines

+. if and only if local cpu can write per-cpu, then Loongson3's llsc bug would not be triggerd.

same as this_cpu_cmpxchg_double

If so, then no need to add sync before and after cmpxchg_local

Correct, we already dropped the change for other local.h stuff.

Quoted 39 lines · Yunsheng Lin

Quoted 10 lines · Peter Zijlstra

Quoted 7 lines · Yunsheng Lin

According to Section 6.2.14 from ACPI spec 6.3 [1], the setting of proximity domain is optional, as below:

This optional object is used to describe proximity domain associations within a machine. _PXM evaluates to an integer that identifies a device as belonging to a Proximity Domain defined in the System Resource Affinity Table (SRAT).

That's just words.. what does it actually mean?

It means the dev_to_node(dev) may return -1 if the bios does not implement the proximity domain feature, user may use that value to call cpumask_of_node and cpumask_of_node does not protect itself from node id being -1, which causes out of bound access.

Quoted 8 lines

Quoted 8 lines

arch/x86/include/asm/topology.h +4 -0

@@ -69,6 +69,12 @@ 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)
 {
+	if (node >= nr_node_ids)
+		return cpu_none_mask;
+
+	if (node < 0 || !node_to_cpumask_map[node])

Quoted 8 lines

arch/x86/include/asm/topology.h extern const struct cpumask *cpumask_of_node(int node); · line 80

Quoted 5 lines

arch/x86/include/asm/topology.h +2 -0

+		return cpu_online_mask;
+
 	return node_to_cpumask_map[node];
 }
 #endif

I _reallly_ hate this. Users are expected to use valid numa ids. Now we're adding all this checking to all users. Why do we want to do that?

As above, the dev_to_node(dev) may return -1.

Quoted 1 line

Quoted 4 lines

Quoted 1 line

Using '(unsigned)node >= nr_nods_ids' is an error.

'node >= nr_node_ids' can be dropped if all user is expected to not call cpumask_of_node with node id greater or equal to nr_nods_ids.

you copied my typo :-)

Quoted 13 lines

From what I can see, the problem can be fixed in three place:
1. Make user dev_to_node return a valid node id even when proximity domain is not set by bios(or node id set by buggy bios is not valid), which may need info from the numa system to make sure it will return a valid node.

2. User that call cpumask_of_node should ensure the node id is valid before calling cpumask_of_node, and user also need some info to make ensure node id is valid.

3. Make sure cpumask_of_node deal with invalid node id as this patchset.

Quoted 4 lines

Which one do you prefer to make sure node id is valid, or do you have any better idea?

Any detail advice and suggestion will be very helpful, thanks.

1) because even it is not set, the device really does belong to a node. It is impossible a device will have magic uniform access to memory when CPUs cannot.

2) is already true today, cpumask_of_node() requires a valid node_id.

3) is just wrong and increases overhead for everyone.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

.

Quoted 3 lines · Peter Zijlstra

1) because even it is not set, the device really does belong to a node. It is impossible a device will have magic uniform access to memory when CPUs cannot.

So it means dev_to_node() will return either NUMA_NO_NODE or a valid node id?

Quoted 1 line

2) is already true today, cpumask_of_node() requires a valid node_id.

Ok, most of the user does check node_id before calling cpumask_of_node(), but does a little different type of checking:

1) some does " < 0" check;
2) some does "== NUMA_NO_NODE" check;
3) some does ">= MAX_NUMNODES" check;
4) some does "< 0 || >= MAX_NUMNODES || !node_online(node)" check.

Quoted 1 line

3) is just wrong and increases overhead for everyone.

Ok, cpumask_of_node() is also used in some critical path such as scheduling, which may not need those checking, the overhead is unnecessary.

But for non-critical path such as setup or configuration path, it better to have consistent checking, and also simplify the user code that calls cpumask_of_node().

Do you think it is worth the trouble to add a new function such as cpumask_of_node_check(maybe some other name) to do consistent checking?

Or caller just simply check if dev_to_node()'s return value is NUMA_NO_NODE before calling cpumask_of_node()?

Quoted 6 lines · Yunsheng Lin

Quoted 3 lines · Peter Zijlstra

1) because even it is not set, the device really does belong to a node. It is impossible a device will have magic uniform access to memory when CPUs cannot.

Quoted 2 lines

So it means dev_to_node() will return either NUMA_NO_NODE or a valid node id?

NUMA_NO_NODE := -1, which is not a valid node number. It is also, like I said, not a valid device location on a NUMA system.

Just because ACPI/BIOS is shit, doesn't mean the device doesn't have a node association. It just means we don't know and might have to guess.

Quoted 6 lines

Quoted 2 lines

2) is already true today, cpumask_of_node() requires a valid node_id.

Ok, most of the user does check node_id before calling cpumask_of_node(), but does a little different type of checking:

Quoted 4 lines

1) some does " < 0" check;
2) some does "== NUMA_NO_NODE" check;
3) some does ">= MAX_NUMNODES" check;
4) some does "< 0 || >= MAX_NUMNODES || !node_online(node)" check.

The one true way is:

	'(unsigned)node_id >= nr_node_ids'

Quoted 15 lines

Quoted 2 lines

3) is just wrong and increases overhead for everyone.

Ok, cpumask_of_node() is also used in some critical path such as scheduling, which may not need those checking, the overhead is unnecessary.

But for non-critical path such as setup or configuration path, it better to have consistent checking, and also simplify the user code that calls cpumask_of_node().

Do you think it is worth the trouble to add a new function such as cpumask_of_node_check(maybe some other name) to do consistent checking?

Quoted 2 lines

Or caller just simply check if dev_to_node()'s return value is NUMA_NO_NODE before calling cpumask_of_node()?

It is not a matter of convenience. The function is called cpumask_of_node(), when node < 0 || node >= nr_node_ids, it is not a valid node, therefore the function shouldn't return anything except an error.

Also note that the CONFIG_DEBUG_PER_CPU_MAPS version of cpumask_of_node() already does this (although it wants the below fix).

Quoted 10 lines · Peter Zijlstra

Quoted 8 lines · Yunsheng Lin

Quoted 3 lines · Peter Zijlstra

1) because even it is not set, the device really does belong to a node. It is impossible a device will have magic uniform access to memory when CPUs cannot.

So it means dev_to_node() will return either NUMA_NO_NODE or a valid node id?

Quoted 5 lines

NUMA_NO_NODE := -1, which is not a valid node number. It is also, like I said, not a valid device location on a NUMA system.

Just because ACPI/BIOS is shit, doesn't mean the device doesn't have a node association. It just means we don't know and might have to guess.

How do we guess the device's location when ACPI/BIOS does not set it?

It seems dev_to_node() does not do anything about that and leave the job to the caller or whatever function that get called with its return value, such as cpumask_of_node().

Quoted 11 lines · Yunsheng Lin

Quoted 10 lines · Peter Zijlstra

Quoted 8 lines · Yunsheng Lin

Quoted 3 lines · Peter Zijlstra

1) because even it is not set, the device really does belong to a node. It is impossible a device will have magic uniform access to memory when CPUs cannot.

So it means dev_to_node() will return either NUMA_NO_NODE or a valid node id?

Quoted 7 lines

Quoted 5 lines

NUMA_NO_NODE := -1, which is not a valid node number. It is also, like I said, not a valid device location on a NUMA system.

Just because ACPI/BIOS is shit, doesn't mean the device doesn't have a node association. It just means we don't know and might have to guess.

How do we guess the device's location when ACPI/BIOS does not set it?

See device_add(), it looks to the device's parent and on NO_NODE, puts it there.

Lacking any hints, just stick it to node0 and print a FW_BUG or something.

Quoted 3 lines

It seems dev_to_node() does not do anything about that and leave the job to the caller or whatever function that get called with its return value, such as cpumask_of_node().

Well, dev_to_node() doesn't do anything; nor should it. It are the callers of set_dev_node() that should be taking care.

Also note how device_add() sets the device node to the parent device's node on NUMA_NO_NODE. Arguably we should change it to complain when it finds NUMA_NO_NODE and !parent.

---
 drivers/base/core.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

drivers/base/core.c +0 -1

index f0dd8e38fee3..2caf204966a0 100644
@@ -2120,8 +2120,16 @@ int device_add(struct device *dev)
 		dev->kobj.parent = kobj;
 
 	/* use parent numa_node */
-	if (parent && (dev_to_node(dev) == NUMA_NO_NODE))

Quoted 9 lines · Peter Zijlstra

drivers/base/core.c +0 -1

index f0dd8e38fee3..2caf204966a0 100644
@@ -2120,8 +2120,16 @@ int device_add(struct device *dev)
 		dev->kobj.parent = kobj;
 
 	/* use parent numa_node */
-	if (parent && (dev_to_node(dev) == NUMA_NO_NODE))

drivers/base/core.c int device_add(struct device *dev) · line 2131

Quoted 10 lines

drivers/base/core.c +9 -1

-		set_dev_node(dev, dev_to_node(parent));
+	if (dev_to_node(dev) == NUMA_NO_NODE) {
+		if (parent)
+			set_dev_node(dev, dev_to_node(parent));
+#ifdef CONFIG_NUMA
+		else {
+			pr_err("device: '%s': has no assigned NUMA node\n", dev_name(dev));
+			set_dev_node(dev, 0);
+		}
+#endif

BTW., is firmware required to always provide a NUMA node on NUMA systems?

I.e. do we really want this warning on non-NUMA systems that don't assign NUMA nodes?

Also, even on NUMA systems, is firmware required to provide a NUMA node - i.e. is it in principle invalid to offer no NUMA binding?

Quoted 11 lines · Yunsheng Lin

Quoted 10 lines · Peter Zijlstra

Quoted 8 lines · Yunsheng Lin

Quoted 3 lines · Peter Zijlstra

1) because even it is not set, the device really does belong to a node. It is impossible a device will have magic uniform access to memory when CPUs cannot.

So it means dev_to_node() will return either NUMA_NO_NODE or a valid node id?

Quoted 7 lines

Quoted 5 lines

NUMA_NO_NODE := -1, which is not a valid node number. It is also, like I said, not a valid device location on a NUMA system.

Just because ACPI/BIOS is shit, doesn't mean the device doesn't have a node association. It just means we don't know and might have to guess.

How do we guess the device's location when ACPI/BIOS does not set it?

See device_add(), it looks to the device's parent and on NO_NODE, puts it there.

Lacking any hints, just stick it to node0 and print a FW_BUG or something.

Quoted 3 lines

It seems dev_to_node() does not do anything about that and leave the job to the caller or whatever function that get called with its return value, such as cpumask_of_node().

Well, dev_to_node() doesn't do anything; nor should it. It are the callers of set_dev_node() that should be taking care.

Also note how device_add() sets the device node to the parent device's node on NUMA_NO_NODE. Arguably we should change it to complain when it finds NUMA_NO_NODE and !parent.

---
 drivers/base/core.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

drivers/base/core.c +10 -2

index f0dd8e38fee3..2caf204966a0 100644
@@ -2120,8 +2120,16 @@ int device_add(struct device *dev)
 		dev->kobj.parent = kobj;
 
 	/* use parent numa_node */
-	if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
-		set_dev_node(dev, dev_to_node(parent));
+	if (dev_to_node(dev) == NUMA_NO_NODE) {
+		if (parent)
+			set_dev_node(dev, dev_to_node(parent));
+#ifdef CONFIG_NUMA
+		else {
+			pr_err("device: '%s': has no assigned NUMA node\n", dev_name(dev));
+			set_dev_node(dev, 0);
+		}
+#endif
+	}
 
 	/* first, register with generic layer. */
 	/* we require the name to be set before, and pass NULL */

.

Quoted 33 lines · Peter Zijlstra

Quoted 11 lines · Yunsheng Lin

Quoted 10 lines · Peter Zijlstra

Quoted 8 lines · Yunsheng Lin

Quoted 3 lines · Peter Zijlstra

1) because even it is not set, the device really does belong to a node. It is impossible a device will have magic uniform access to memory when CPUs cannot.

So it means dev_to_node() will return either NUMA_NO_NODE or a valid node id?

Quoted 7 lines

Quoted 5 lines

NUMA_NO_NODE := -1, which is not a valid node number. It is also, like I said, not a valid device location on a NUMA system.

Just because ACPI/BIOS is shit, doesn't mean the device doesn't have a node association. It just means we don't know and might have to guess.

How do we guess the device's location when ACPI/BIOS does not set it?

See device_add(), it looks to the device's parent and on NO_NODE, puts it there.

Lacking any hints, just stick it to node0 and print a FW_BUG or something.

Quoted 3 lines

It seems dev_to_node() does not do anything about that and leave the job to the caller or whatever function that get called with its return value, such as cpumask_of_node().

Well, dev_to_node() doesn't do anything; nor should it. It are the callers of set_dev_node() that should be taking care.

Quoted 3 lines

Also note how device_add() sets the device node to the parent device's node on NUMA_NO_NODE. Arguably we should change it to complain when it finds NUMA_NO_NODE and !parent.

Is it possible that the node id set by device_add() become invalid if the node is offlined, then dev_to_node() may return a invalid node id.

From the comment in select_fallback_rq(), it seems that a node can be offlined, not sure if node offline process has taken cared of that?

/*
* If the node that the CPU is on has been offlined, cpu_to_node()
* will return -1. There is no CPU on the node, and we should
* select the CPU on the other node.
*/

With the above assumption that a device is always on a valid node, the node id returned from dev_to_node() can be safely passed to cpumask_of_node() without any checking?

Quoted 38 lines · Yunsheng Lin

Quoted 36 lines · Peter Zijlstra

Quoted 11 lines · Yunsheng Lin

Quoted 10 lines · Peter Zijlstra

Quoted 8 lines · Yunsheng Lin

Quoted 3 lines · Peter Zijlstra

1) because even it is not set, the device really does belong to a node. It is impossible a device will have magic uniform access to memory when CPUs cannot.

So it means dev_to_node() will return either NUMA_NO_NODE or a valid node id?

Quoted 7 lines

Quoted 5 lines

NUMA_NO_NODE := -1, which is not a valid node number. It is also, like I said, not a valid device location on a NUMA system.

Just because ACPI/BIOS is shit, doesn't mean the device doesn't have a node association. It just means we don't know and might have to guess.

How do we guess the device's location when ACPI/BIOS does not set it?

See device_add(), it looks to the device's parent and on NO_NODE, puts it there.

Lacking any hints, just stick it to node0 and print a FW_BUG or something.

Quoted 3 lines

It seems dev_to_node() does not do anything about that and leave the job to the caller or whatever function that get called with its return value, such as cpumask_of_node().

Well, dev_to_node() doesn't do anything; nor should it. It are the callers of set_dev_node() that should be taking care.

Also note how device_add() sets the device node to the parent device's node on NUMA_NO_NODE. Arguably we should change it to complain when it finds NUMA_NO_NODE and !parent.

Quoted 3 lines

Is it possible that the node id set by device_add() become invalid if the node is offlined, then dev_to_node() may return a invalid node id.

In that case I would expect the device to go away too. Once the memory controller goes away, the PCI bus connected to it cannot continue to function.

Quoted 9 lines

From the comment in select_fallback_rq(), it seems that a node can be offlined, not sure if node offline process has taken cared of that?

/*
* If the node that the CPU is on has been offlined, cpu_to_node()
* will return -1. There is no CPU on the node, and we should
* select the CPU on the other node.
*/

Ugh, so I disagree with that notion. cpu_to_node() mapping should be fixed, you simply cannot change it after boot, too much stuff relies on it.

Setting cpu_to_node to -1 on node offline is just wrong. But alas, it seems this is already so.

From: Linuxarm [mailto:[email protected]] On Behalf Of Peter Zijlstra
Sent: Tuesday, September 3, 2019 8:11 AM

Quoted 38 lines · Yunsheng Lin

Quoted 36 lines · Peter Zijlstra

Quoted 11 lines · Yunsheng Lin

Quoted 10 lines · Peter Zijlstra

Quoted 8 lines · Yunsheng Lin

Quoted 3 lines · Peter Zijlstra

1) because even it is not set, the device really does belong to a node. It is impossible a device will have magic uniform access to memory when CPUs cannot.

So it means dev_to_node() will return either NUMA_NO_NODE or a valid node id?

Quoted 7 lines

Quoted 5 lines

NUMA_NO_NODE := -1, which is not a valid node number. It is also, like> I said, not a valid device location on a NUMA system.

Just because ACPI/BIOS is shit, doesn't mean the device doesn't have a node association. It just means we don't know and might have to guess.

How do we guess the device's location when ACPI/BIOS does not set it?

See device_add(), it looks to the device's parent and on NO_NODE, puts it there.

Lacking any hints, just stick it to node0 and print a FW_BUG or something.

Quoted 3 lines

It seems dev_to_node() does not do anything about that and leave the job to the caller or whatever function that get called with its return value, such as cpumask_of_node().

Well, dev_to_node() doesn't do anything; nor should it. It are the callers of set_dev_node() that should be taking care.

Also note how device_add() sets the device node to the parent device's node on NUMA_NO_NODE. Arguably we should change it to complain when it finds NUMA_NO_NODE and !parent.

Quoted 3 lines

Is it possible that the node id set by device_add() become invalid if the node is offlined, then dev_to_node() may return a invalid node id.

In that case I would expect the device to go away too. Once the memory controller goes away, the PCI bus connected to it cannot continue to function.

Quoted 7 lines

From: Linuxarm [mailto:[email protected]] On Behalf Of Peter Zijlstra

Quoted 3 lines · Yunsheng Lin

Is it possible that the node id set by device_add() become invalid if the node is offlined, then dev_to_node() may return a invalid node id.

Quoted 3 lines

In that case I would expect the device to go away too. Once the memory controller goes away, the PCI bus connected to it cannot continue to function.

I am not sure if this is *exactly* true on our system as NUMA nodes are part of the SoCs and devices could still be used even if all the memory and CPUs part of the node are turned off. Although, it is highly unlikely anybody would do that(maybe could be debated for the Power Management case?)

Also note that the CONFIG_DEBUG_PER_CPU_MAPS version of cpumask_of_node() already does this (although it wants the below fix).

---

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

index e6dad600614c..5f49c10201c7 100644

Nitpicking: please also fix the kernel message to say ">=".

According to Section 6.2.14 from ACPI spec 6.3 [1], the setting of proximity domain is optional, as below:

From: Yunsheng Lin <[email protected]>
Date: Sat, 31 Aug 2019 13:58:21 +0800

Quoted 2 lines

According to Section 6.2.14 from ACPI spec 6.3 [1], the setting of proximity domain is optional, as below:

What in the world does the ACPI spec have to do with sparc64 NUMA node ID checking?

.

Did you mean sparc64 system does not has ACPI, the device's node id will not specified by ACPI, so the ACPI is unrelated here?

From: Yunsheng Lin <[email protected]>
Date: Sat, 31 Aug 2019 16:57:04 +0800

Quoted 2 lines

Did you mean sparc64 system does not has ACPI, the device's node id will not specified by ACPI, so the ACPI is unrelated here?

Yes, sparc64 never has and never will have ACPI.

This is also true for several other platforms where you have made this change.

The assumption of your entire patch set is that the semantics of the NUMA node ID are somehow completely defined by ACPI semantics. Which is not true.

.

The NUMA node id in sparc64 system is defined by DT semantics?

According to Section 6.2.14 from ACPI spec 6.3 [1], the setting of proximity domain is optional, as below:

This optional object is used to describe proximity domain associations within a machine. _PXM evaluates to an integer that identifies a device as belonging to a Proximity Domain defined in the System Resource Affinity Table (SRAT).

Since mips ip27 uses hub_data instead of node_to_cpumask_map, this patch checks node id with the below case before returning &hub_data(node)->h_cpus:
1. if node_id >= MAX_COMPACT_NODES, return cpu_none_mask
2. if node_id < 0, return cpu_online_mask
3. if hub_data(node) is NULL, return cpu_online_mask

[1] https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf

Signed-off-by Yunsheng Lin <[email protected]>

---
 arch/mips/include/asm/mach-ip27/topology.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

arch/mips/include/asm/mach-ip27/topology.h +12 -3

index 965f079..914a55a 100644
@@ -15,9 +15,18 @@ struct cpuinfo_ip27 {
 extern struct cpuinfo_ip27 sn_cpu_info[NR_CPUS];
 
 #define cpu_to_node(cpu)	(sn_cpu_info[(cpu)].p_nodeid)
-#define cpumask_of_node(node)	((node) == -1 ?				\
-				 cpu_all_mask :				\
-				 &hub_data(node)->h_cpus)
+
+static inline const struct cpumask *cpumask_of_node(int node)
+{
+	if (node >= MAX_COMPACT_NODES)
+		return cpu_none_mask;
+
+	if (node < 0 || !hub_data(node))
+		return cpu_online_mask;
+
+	return &hub_data(node)->h_cpus;
+}
+
 struct pci_bus;
 extern int pcibus_to_node(struct pci_bus *);
 

Signature · 1 line

2.8.1

Hi Yunsheng,

Quoted 15 lines · Yunsheng Lin

According to Section 6.2.14 from ACPI spec 6.3 [1], the setting of proximity domain is optional, as below:

This optional object is used to describe proximity domain associations within a machine. _PXM evaluates to an integer that identifies a device as belonging to a Proximity Domain defined in the System Resource Affinity Table (SRAT).

Since mips ip27 uses hub_data instead of node_to_cpumask_map, this patch checks node id with the below case before returning &hub_data(node)->h_cpus:
1. if node_id >= MAX_COMPACT_NODES, return cpu_none_mask
2. if node_id < 0, return cpu_online_mask
3. if hub_data(node) is NULL, return cpu_online_mask

Quoted 1 line

[1] https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf

Similar to David's comment on the sparc patch, these systems don't use ACPI so I don't see from your commit message why this change would be relevant.

This same comment applies to patch 9 too.

Thanks,
Paul

Quoted 35 lines

Signed-off-by Yunsheng Lin <[email protected]>

---
 arch/mips/include/asm/mach-ip27/topology.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

arch/mips/include/asm/mach-ip27/topology.h +12 -3

index 965f079..914a55a 100644
@@ -15,9 +15,18 @@ struct cpuinfo_ip27 {
 extern struct cpuinfo_ip27 sn_cpu_info[NR_CPUS];
 
 #define cpu_to_node(cpu)	(sn_cpu_info[(cpu)].p_nodeid)
-#define cpumask_of_node(node)	((node) == -1 ?				\
-				 cpu_all_mask :				\
-				 &hub_data(node)->h_cpus)
+
+static inline const struct cpumask *cpumask_of_node(int node)
+{
+	if (node >= MAX_COMPACT_NODES)
+		return cpu_none_mask;
+
+	if (node < 0 || !hub_data(node))
+		return cpu_online_mask;
+
+	return &hub_data(node)->h_cpus;
+}
+
 struct pci_bus;
 extern int pcibus_to_node(struct pci_bus *);
 

Signature · 1 line

2.8.1

.