Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/9] check the node id consistently across different arches
@ 2019-08-31  5:58 Yunsheng Lin
  2019-08-31  5:58 ` [PATCH v2 1/9] arm64: numa: check the node id consistently for arm64 Yunsheng Lin
                   ` (8 more replies)
  0 siblings, 9 replies; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31  5:58 UTC (permalink / raw)
  To: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc
  Cc: akpm, rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	peterz, len.brown, axboe, dledford, jeffrey.t.kirsher,
	linux-alpha, nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390,
	linux-sh, sparclinux, tbogendoerfer, linux-mips, linuxarm

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).

When enabling KASAN and bios has not implemented the proximity
domain of the hns3 device, there is a global-out-of-bounds error
below:

[   42.970381] ==================================================================
[   42.977595] BUG: KASAN: global-out-of-bounds in __bitmap_weight+0x48/0xb0
[   42.984370] Read of size 8 at addr ffff20008cdf8790 by task kworker/0:1/13
[   42.991230]
[   42.992712] CPU: 0 PID: 13 Comm: kworker/0:1 Tainted: G           O      5.2.0-rc4-g8bde06a-dirty #3
[   43.001830] Hardware name: Huawei TaiShan 2280 V2/BC82AMDA, BIOS TA BIOS 2280-A CS V2.B050.01 08/08/2019
[   43.011298] Workqueue: events work_for_cpu_fn
[   43.015643] Call trace:
[   43.018078]  dump_backtrace+0x0/0x1e8
[   43.021727]  show_stack+0x14/0x20
[   43.025031]  dump_stack+0xc4/0xfc
[   43.028335]  print_address_description+0x178/0x270
[   43.033113]  __kasan_report+0x164/0x1b8
[   43.036936]  kasan_report+0xc/0x18
[   43.040325]  __asan_load8+0x84/0xa8
[   43.043801]  __bitmap_weight+0x48/0xb0
[   43.047552]  hclge_init_ae_dev+0x988/0x1e78 [hclge]
[   43.052418]  hnae3_register_ae_dev+0xcc/0x278 [hnae3]
[   43.057467]  hns3_probe+0xe0/0x120 [hns3]
[   43.061464]  local_pci_probe+0x74/0xf0
[   43.065200]  work_for_cpu_fn+0x2c/0x48
[   43.068937]  process_one_work+0x3c0/0x878
[   43.072934]  worker_thread+0x400/0x670
[   43.076670]  kthread+0x1b0/0x1b8
[   43.079885]  ret_from_fork+0x10/0x18
[   43.083446]
[   43.084925] The buggy address belongs to the variable:
[   43.090052]  numa_distance+0x30/0x40
[   43.093613]
[   43.095091] Memory state around the buggy address:
[   43.099870]  ffff20008cdf8680: fa fa fa fa 04 fa fa fa fa fa fa fa 00 00 fa fa
[   43.107078]  ffff20008cdf8700: fa fa fa fa 04 fa fa fa fa fa fa fa 00 fa fa fa
[   43.114286] >ffff20008cdf8780: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
[   43.121494]                          ^
[   43.125230]  ffff20008cdf8800: 01 fa fa fa fa fa fa fa 04 fa fa fa fa fa fa fa
[   43.132439]  ffff20008cdf8880: fa fa fa fa fa fa fa fa 00 00 fa fa fa fa fa fa
[   43.139646] ==================================================================

As suggested [2] by Michal Hocko:
"if the specification really allows to provide NUMA_NO_NODE (-1) then
the code really has to be prepared for that. And ideally all arches
should deal with that."

This patchset checks the node id with the below case consistently
across different arches in order to be compliant with spec and
backward compatible as much as possible:
1. if node_id < 0, return cpu_online_mask
2. if node_id >= nr_node_ids, return cpu_none_mask
3. if node_to_cpumask_map[node_id] is NULL, return cpu_online_mask

Note:
1. Only arm64 has been compile tested and tested on real board.
2. x86 has been compile tested with defconfig.
3. Other arch has not been compile tested or tested on real board.

Changelog:
V2: Change commit log as suggested by Michal Hocko, and make the change to
    other arches as well.

[1] https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
[2] https://patchwork.kernel.org/patch/11122823/

Yunsheng Lin (9):
  arm64: numa: check the node id consistently for arm64
  x86: numa: check the node id consistently for x86
  alpha: numa: check the node id consistently for alpha
  powerpc: numa: check the node id consistently for powerpc
  s390: numa: check the node id consistently for s390
  sh: numa: check the node id consistently for sh
  sparc64: numa: check the node id consistently for sparc64
  mips: numa: check the node id consistently for mips ip27
  mips: numa: check the node id consistently for mips loongson64

 arch/alpha/include/asm/topology.h                |  7 +++++--
 arch/arm64/include/asm/numa.h                    |  6 ++++++
 arch/arm64/mm/numa.c                             |  2 +-
 arch/mips/include/asm/mach-ip27/topology.h       | 15 ++++++++++++---
 arch/mips/include/asm/mach-loongson64/topology.h | 12 +++++++++++-
 arch/powerpc/include/asm/topology.h              | 13 ++++++++++---
 arch/s390/include/asm/topology.h                 |  6 ++++++
 arch/sh/include/asm/topology.h                   | 14 +++++++++++++-
 arch/sparc/include/asm/topology_64.h             | 16 +++++++++++++---
 arch/x86/include/asm/topology.h                  |  6 ++++++
 arch/x86/mm/numa.c                               |  2 +-
 11 files changed, 84 insertions(+), 15 deletions(-)

-- 
2.8.1


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 1/9] arm64: numa: check the node id consistently for arm64
  2019-08-31  5:58 [PATCH v2 0/9] check the node id consistently across different arches Yunsheng Lin
@ 2019-08-31  5:58 ` Yunsheng Lin
  2019-08-31  5:58 ` [PATCH v2 2/9] x86: numa: check the node id consistently for x86 Yunsheng Lin
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31  5:58 UTC (permalink / raw)
  To: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc
  Cc: akpm, rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	peterz, len.brown, axboe, dledford, jeffrey.t.kirsher,
	linux-alpha, nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390,
	linux-sh, sparclinux, tbogendoerfer, linux-mips, linuxarm

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).

When enabling KASAN and bios has not implemented the proximity
domain of the hns3 device, there is a global-out-of-bounds error
below:

[   42.970381] ==================================================================
[   42.977595] BUG: KASAN: global-out-of-bounds in __bitmap_weight+0x48/0xb0
[   42.984370] Read of size 8 at addr ffff20008cdf8790 by task kworker/0:1/13
[   42.991230]
[   42.992712] CPU: 0 PID: 13 Comm: kworker/0:1 Tainted: G           O      5.2.0-rc4-g8bde06a-dirty #3
[   43.001830] Hardware name: Huawei TaiShan 2280 V2/BC82AMDA, BIOS TA BIOS 2280-A CS V2.B050.01 08/08/2019
[   43.011298] Workqueue: events work_for_cpu_fn
[   43.015643] Call trace:
[   43.018078]  dump_backtrace+0x0/0x1e8
[   43.021727]  show_stack+0x14/0x20
[   43.025031]  dump_stack+0xc4/0xfc
[   43.028335]  print_address_description+0x178/0x270
[   43.033113]  __kasan_report+0x164/0x1b8
[   43.036936]  kasan_report+0xc/0x18
[   43.040325]  __asan_load8+0x84/0xa8
[   43.043801]  __bitmap_weight+0x48/0xb0
[   43.047552]  hclge_init_ae_dev+0x988/0x1e78 [hclge]
[   43.052418]  hnae3_register_ae_dev+0xcc/0x278 [hnae3]
[   43.057467]  hns3_probe+0xe0/0x120 [hns3]
[   43.061464]  local_pci_probe+0x74/0xf0
[   43.065200]  work_for_cpu_fn+0x2c/0x48
[   43.068937]  process_one_work+0x3c0/0x878
[   43.072934]  worker_thread+0x400/0x670
[   43.076670]  kthread+0x1b0/0x1b8
[   43.079885]  ret_from_fork+0x10/0x18
[   43.083446]
[   43.084925] The buggy address belongs to the variable:
[   43.090052]  numa_distance+0x30/0x40
[   43.093613]
[   43.095091] Memory state around the buggy address:
[   43.099870]  ffff20008cdf8680: fa fa fa fa 04 fa fa fa fa fa fa fa 00 00 fa fa
[   43.107078]  ffff20008cdf8700: fa fa fa fa 04 fa fa fa fa fa fa fa 00 fa fa fa
[   43.114286] >ffff20008cdf8780: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
[   43.121494]                          ^
[   43.125230]  ffff20008cdf8800: 01 fa fa fa fa fa fa fa 04 fa fa fa fa fa fa fa
[   43.132439]  ffff20008cdf8880: fa fa fa fa fa fa fa fa 00 00 fa fa fa fa fa fa
[   43.139646] ==================================================================

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

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 arch/arm64/include/asm/numa.h | 6 ++++++
 arch/arm64/mm/numa.c          | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/numa.h b/arch/arm64/include/asm/numa.h
index 626ad01..65a0ef6 100644
--- a/arch/arm64/include/asm/numa.h
+++ b/arch/arm64/include/asm/numa.h
@@ -25,6 +25,12 @@ 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])
+		return cpu_online_mask;
+
 	return node_to_cpumask_map[node];
 }
 #endif
diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
index 4f241cc..7eca267 100644
--- a/arch/arm64/mm/numa.c
+++ b/arch/arm64/mm/numa.c
@@ -49,7 +49,7 @@ const struct cpumask *cpumask_of_node(int node)
 	if (WARN_ON(node >= nr_node_ids))
 		return cpu_none_mask;
 
-	if (WARN_ON(node_to_cpumask_map[node] == NULL))
+	if (WARN_ON(node < 0 || !node_to_cpumask_map[node]))
 		return cpu_online_mask;
 
 	return node_to_cpumask_map[node];
-- 
2.8.1


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-08-31  5:58 [PATCH v2 0/9] check the node id consistently across different arches Yunsheng Lin
  2019-08-31  5:58 ` [PATCH v2 1/9] arm64: numa: check the node id consistently for arm64 Yunsheng Lin
@ 2019-08-31  5:58 ` Yunsheng Lin
  2019-08-31  8:55   ` Peter Zijlstra
  2019-08-31  5:58 ` [PATCH v2 3/9] alpha: numa: check the node id consistently for alpha Yunsheng Lin
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31  5:58 UTC (permalink / raw)
  To: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc
  Cc: akpm, rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	peterz, len.brown, axboe, dledford, jeffrey.t.kirsher,
	linux-alpha, nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390,
	linux-sh, sparclinux, tbogendoerfer, linux-mips, linuxarm

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 <linyunsheng@huawei.com>
---
 arch/x86/include/asm/topology.h | 6 ++++++
 arch/x86/mm/numa.c              | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
index 4b14d23..f36e9c8 100644
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -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])
+		return cpu_online_mask;
+
 	return node_to_cpumask_map[node];
 }
 #endif
diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index e6dad60..5e393d2 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -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);
-- 
2.8.1


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 3/9] alpha: numa: check the node id consistently for alpha
  2019-08-31  5:58 [PATCH v2 0/9] check the node id consistently across different arches Yunsheng Lin
  2019-08-31  5:58 ` [PATCH v2 1/9] arm64: numa: check the node id consistently for arm64 Yunsheng Lin
  2019-08-31  5:58 ` [PATCH v2 2/9] x86: numa: check the node id consistently for x86 Yunsheng Lin
@ 2019-08-31  5:58 ` Yunsheng Lin
  2019-08-31  5:58 ` [PATCH v2 4/9] powerpc: numa: check the node id consistently for powerpc Yunsheng Lin
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31  5:58 UTC (permalink / raw)
  To: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc
  Cc: akpm, rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	peterz, len.brown, axboe, dledford, jeffrey.t.kirsher,
	linux-alpha, nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390,
	linux-sh, sparclinux, tbogendoerfer, linux-mips, linuxarm

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 <linyunsheng@huawei.com>
---
note node_to_cpumask_map[node] is already a pointer, so the
cpumask_clear should be called with node_to_cpumask_map[node]
instead of &node_to_cpumask_map[node]? And cpumask_of_node()
function need to be inlined when defined in a header file?
If the above are problems, maybe another patch to fix or clean
it up.
---
 arch/alpha/include/asm/topology.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/alpha/include/asm/topology.h b/arch/alpha/include/asm/topology.h
index 5a77a40..9e0b1cd1 100644
--- a/arch/alpha/include/asm/topology.h
+++ b/arch/alpha/include/asm/topology.h
@@ -30,8 +30,11 @@ static const struct cpumask *cpumask_of_node(int node)
 {
 	int cpu;
 
-	if (node == NUMA_NO_NODE)
-		return cpu_all_mask;
+	if (node >= nr_node_ids)
+		return cpu_none_mask;
+
+	if (node < 0 || !node_to_cpumask_map[node])
+		return cpu_online_mask;
 
 	cpumask_clear(&node_to_cpumask_map[node]);
 
-- 
2.8.1


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 4/9] powerpc: numa: check the node id consistently for powerpc
  2019-08-31  5:58 [PATCH v2 0/9] check the node id consistently across different arches Yunsheng Lin
                   ` (2 preceding siblings ...)
  2019-08-31  5:58 ` [PATCH v2 3/9] alpha: numa: check the node id consistently for alpha Yunsheng Lin
@ 2019-08-31  5:58 ` Yunsheng Lin
  2019-08-31  5:58 ` [PATCH v2 5/9] s390: numa: check the node id consistently for s390 Yunsheng Lin
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31  5:58 UTC (permalink / raw)
  To: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc
  Cc: akpm, rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	peterz, len.brown, axboe, dledford, jeffrey.t.kirsher,
	linux-alpha, nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390,
	linux-sh, sparclinux, tbogendoerfer, linux-mips, linuxarm

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 <linyunsheng@huawei.com>
---
 arch/powerpc/include/asm/topology.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
index 2f7e1ea..217dc9b 100644
--- a/arch/powerpc/include/asm/topology.h
+++ b/arch/powerpc/include/asm/topology.h
@@ -17,9 +17,16 @@ struct device_node;
 
 #include <asm/mmzone.h>
 
-#define cpumask_of_node(node) ((node) == -1 ?				\
-			       cpu_all_mask :				\
-			       node_to_cpumask_map[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])
+		return cpu_online_mask;
+
+	return node_to_cpumask_map[node];
+}
 
 struct pci_bus;
 #ifdef CONFIG_PCI
-- 
2.8.1


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 5/9] s390: numa: check the node id consistently for s390
  2019-08-31  5:58 [PATCH v2 0/9] check the node id consistently across different arches Yunsheng Lin
                   ` (3 preceding siblings ...)
  2019-08-31  5:58 ` [PATCH v2 4/9] powerpc: numa: check the node id consistently for powerpc Yunsheng Lin
@ 2019-08-31  5:58 ` Yunsheng Lin
  2019-09-02  4:05   ` kbuild test robot
  2019-08-31  5:58 ` [PATCH v2 6/9] sh: numa: check the node id consistently for sh Yunsheng Lin
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31  5:58 UTC (permalink / raw)
  To: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc
  Cc: akpm, rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	peterz, len.brown, axboe, dledford, jeffrey.t.kirsher,
	linux-alpha, nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390,
	linux-sh, sparclinux, tbogendoerfer, linux-mips, linuxarm

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 <linyunsheng@huawei.com>
---
Note node_to_cpumask_map[node] is already a pointer, so
returning &node_to_cpumask_map[node] does not seem to
be correct, if this is problem, maybe clean it up in another
patch.
---
 arch/s390/include/asm/topology.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/s390/include/asm/topology.h b/arch/s390/include/asm/topology.h
index cca406f..75340ca 100644
--- a/arch/s390/include/asm/topology.h
+++ b/arch/s390/include/asm/topology.h
@@ -78,6 +78,12 @@ static inline int cpu_to_node(int cpu)
 #define cpumask_of_node cpumask_of_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])
+		return cpu_online_mask;
+
 	return &node_to_cpumask_map[node];
 }
 
-- 
2.8.1


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 6/9] sh: numa: check the node id consistently for sh
  2019-08-31  5:58 [PATCH v2 0/9] check the node id consistently across different arches Yunsheng Lin
                   ` (4 preceding siblings ...)
  2019-08-31  5:58 ` [PATCH v2 5/9] s390: numa: check the node id consistently for s390 Yunsheng Lin
@ 2019-08-31  5:58 ` Yunsheng Lin
  2019-08-31  5:58 ` [PATCH v2 7/9] sparc64: numa: check the node id consistently for sparc64 Yunsheng Lin
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31  5:58 UTC (permalink / raw)
  To: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc
  Cc: akpm, rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	peterz, len.brown, axboe, dledford, jeffrey.t.kirsher,
	linux-alpha, nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390,
	linux-sh, sparclinux, tbogendoerfer, linux-mips, linuxarm

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).

It seems sh does not have real numa support or uncompleted
numa support, this patch still checks node id with the below
case to ensure future support is consistent:
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 <linyunsheng@huawei.com>
---
 arch/sh/include/asm/topology.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/sh/include/asm/topology.h b/arch/sh/include/asm/topology.h
index 1db470e..e71e0a0 100644
--- a/arch/sh/include/asm/topology.h
+++ b/arch/sh/include/asm/topology.h
@@ -6,7 +6,19 @@
 
 #define cpu_to_node(cpu)	((void)(cpu),0)
 
-#define cpumask_of_node(node)	((void)node, cpu_online_mask)
+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])
+		return cpu_online_mask;
+
+	/* Should return actual mask based on node_to_cpumask_map
+	 * if sh arch supports real numa node.
+	 */
+	return cpu_online_mask;
+}
 
 #define pcibus_to_node(bus)	((void)(bus), -1)
 #define cpumask_of_pcibus(bus)	(pcibus_to_node(bus) == -1 ? \
-- 
2.8.1


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 7/9] sparc64: numa: check the node id consistently for sparc64
  2019-08-31  5:58 [PATCH v2 0/9] check the node id consistently across different arches Yunsheng Lin
                   ` (5 preceding siblings ...)
  2019-08-31  5:58 ` [PATCH v2 6/9] sh: numa: check the node id consistently for sh Yunsheng Lin
@ 2019-08-31  5:58 ` Yunsheng Lin
  2019-08-31  6:53   ` David Miller
  2019-08-31  5:58 ` [PATCH v2 8/9] mips: numa: check the node id consistently for mips ip27 Yunsheng Lin
  2019-08-31  5:58 ` [PATCH v2 9/9] mips: numa: check the node id consistently for mips loongson64 Yunsheng Lin
  8 siblings, 1 reply; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31  5:58 UTC (permalink / raw)
  To: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc
  Cc: akpm, rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	peterz, len.brown, axboe, dledford, jeffrey.t.kirsher,
	linux-alpha, nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390,
	linux-sh, sparclinux, tbogendoerfer, linux-mips, linuxarm

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
&numa_cpumask_lookup_table[node]:
1. if node_id >= nr_node_ids, return cpu_none_mask
2. if node_id < 0, return cpu_online_mask
3. Since numa_cpumask_lookup_table is not a pointer, a comment
   is added to indicate that

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

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 arch/sparc/include/asm/topology_64.h | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/sparc/include/asm/topology_64.h b/arch/sparc/include/asm/topology_64.h
index 34c628a..66a7917 100644
--- a/arch/sparc/include/asm/topology_64.h
+++ b/arch/sparc/include/asm/topology_64.h
@@ -11,9 +11,19 @@ static inline int cpu_to_node(int cpu)
 	return numa_cpu_lookup_table[cpu];
 }
 
-#define cpumask_of_node(node) ((node) == -1 ?				\
-			       cpu_all_mask :				\
-			       &numa_cpumask_lookup_table[node])
+static inline const struct cpumask *cpumask_of_node(int node)
+{
+	if (node >= MAX_NUMNODES)
+		return cpu_none_mask;
+
+	/* numa_cpumask_lookup_table[node] is not a pointer, so
+	 * no need to check for NULL here.
+	 */
+	if (node < 0)
+		return cpu_online_mask;
+
+	return &numa_cpumask_lookup_table[node];
+}
 
 struct pci_bus;
 #ifdef CONFIG_PCI
-- 
2.8.1


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 8/9] mips: numa: check the node id consistently for mips ip27
  2019-08-31  5:58 [PATCH v2 0/9] check the node id consistently across different arches Yunsheng Lin
                   ` (6 preceding siblings ...)
  2019-08-31  5:58 ` [PATCH v2 7/9] sparc64: numa: check the node id consistently for sparc64 Yunsheng Lin
@ 2019-08-31  5:58 ` Yunsheng Lin
  2019-08-31  5:58 ` [PATCH v2 9/9] mips: numa: check the node id consistently for mips loongson64 Yunsheng Lin
  8 siblings, 0 replies; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31  5:58 UTC (permalink / raw)
  To: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc
  Cc: akpm, rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	peterz, len.brown, axboe, dledford, jeffrey.t.kirsher,
	linux-alpha, nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390,
	linux-sh, sparclinux, tbogendoerfer, linux-mips, linuxarm

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 <linyunsheng@huawei.com>
---
 arch/mips/include/asm/mach-ip27/topology.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/mips/include/asm/mach-ip27/topology.h b/arch/mips/include/asm/mach-ip27/topology.h
index 965f079..914a55a 100644
--- a/arch/mips/include/asm/mach-ip27/topology.h
+++ b/arch/mips/include/asm/mach-ip27/topology.h
@@ -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 *);
 
-- 
2.8.1


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 9/9] mips: numa: check the node id consistently for mips loongson64
  2019-08-31  5:58 [PATCH v2 0/9] check the node id consistently across different arches Yunsheng Lin
                   ` (7 preceding siblings ...)
  2019-08-31  5:58 ` [PATCH v2 8/9] mips: numa: check the node id consistently for mips ip27 Yunsheng Lin
@ 2019-08-31  5:58 ` Yunsheng Lin
  8 siblings, 0 replies; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31  5:58 UTC (permalink / raw)
  To: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc
  Cc: akpm, rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	peterz, len.brown, axboe, dledford, jeffrey.t.kirsher,
	linux-alpha, nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390,
	linux-sh, sparclinux, tbogendoerfer, linux-mips, linuxarm

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 loongson64 uses __node_data instead of
node_to_cpumask_map, this patch checks node id with the below
case before returning &__node_data[node]->cpumask:
1. if node_id >= MAX_NUMNODES, 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 <linyunsheng@huawei.com>
---
 arch/mips/include/asm/mach-loongson64/topology.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/mach-loongson64/topology.h b/arch/mips/include/asm/mach-loongson64/topology.h
index 7ff819a..b19b983 100644
--- a/arch/mips/include/asm/mach-loongson64/topology.h
+++ b/arch/mips/include/asm/mach-loongson64/topology.h
@@ -5,7 +5,17 @@
 #ifdef CONFIG_NUMA
 
 #define cpu_to_node(cpu)	(cpu_logical_map(cpu) >> 2)
-#define cpumask_of_node(node)	(&__node_data[(node)]->cpumask)
+
+static inline const struct cpumask *cpumask_of_node(int node)
+{
+	if (node >= MAX_NUMNODES)
+		return cpu_none_mask;
+
+	if (node < 0 || !__node_data[node])
+		return cpu_online_mask;
+
+	return &__node_data[node]->cpumask;
+}
 
 struct pci_bus;
 extern int pcibus_to_node(struct pci_bus *);
-- 
2.8.1


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 7/9] sparc64: numa: check the node id consistently for sparc64
  2019-08-31  5:58 ` [PATCH v2 7/9] sparc64: numa: check the node id consistently for sparc64 Yunsheng Lin
@ 2019-08-31  6:53   ` David Miller
  2019-08-31  8:57     ` Yunsheng Lin
  0 siblings, 1 reply; 34+ messages in thread
From: David Miller @ 2019-08-31  6:53 UTC (permalink / raw)
  To: linyunsheng
  Cc: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	ralf, paul.burton, jhogan, jiaxun.yang, chenhc, akpm, rppt,
	anshuman.khandual, tglx, cai, robin.murphy, linux-arm-kernel,
	linux-kernel, hpa, x86, dave.hansen, luto, peterz, len.brown,
	axboe, dledford, jeffrey.t.kirsher, linux-alpha, nfont,
	naveen.n.rao, mwb, linuxppc-dev, linux-s390, linux-sh,
	sparclinux, tbogendoerfer, linux-mips, linuxarm

From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Sat, 31 Aug 2019 13:58:21 +0800

> 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?

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-08-31  5:58 ` [PATCH v2 2/9] x86: numa: check the node id consistently for x86 Yunsheng Lin
@ 2019-08-31  8:55   ` Peter Zijlstra
  2019-08-31 10:09     ` Yunsheng Lin
  0 siblings, 1 reply; 34+ messages in thread
From: Peter Zijlstra @ 2019-08-31  8:55 UTC (permalink / raw)
  To: Yunsheng Lin
  Cc: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc, akpm,
	rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	len.brown, axboe, dledford, jeffrey.t.kirsher, linux-alpha,
	nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390, linux-sh,
	sparclinux, tbogendoerfer, linux-mips, linuxarm

On Sat, Aug 31, 2019 at 01:58:16PM +0800, Yunsheng Lin wrote:
> 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?

> 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 <linyunsheng@huawei.com>
> ---
>  arch/x86/include/asm/topology.h | 6 ++++++
>  arch/x86/mm/numa.c              | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
> index 4b14d23..f36e9c8 100644
> --- a/arch/x86/include/asm/topology.h
> +++ b/arch/x86/include/asm/topology.h
> @@ -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])
> +		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.

> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index e6dad60..5e393d2 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -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);
> -- 
> 2.8.1
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 7/9] sparc64: numa: check the node id consistently for sparc64
  2019-08-31  6:53   ` David Miller
@ 2019-08-31  8:57     ` Yunsheng Lin
  2019-08-31 20:02       ` David Miller
  0 siblings, 1 reply; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31  8:57 UTC (permalink / raw)
  To: David Miller
  Cc: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	ralf, paul.burton, jhogan, jiaxun.yang, chenhc, akpm, rppt,
	anshuman.khandual, tglx, cai, robin.murphy, linux-arm-kernel,
	linux-kernel, hpa, x86, dave.hansen, luto, peterz, len.brown,
	axboe, dledford, jeffrey.t.kirsher, linux-alpha, nfont,
	naveen.n.rao, mwb, linuxppc-dev, linux-s390, linux-sh,
	sparclinux, tbogendoerfer, linux-mips, linuxarm

On 2019/8/31 14:53, David Miller wrote:
> From: Yunsheng Lin <linyunsheng@huawei.com>
> Date: Sat, 31 Aug 2019 13:58:21 +0800
> 
>> 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?

I am not sure I understand your question fully here.

Here is my issue when the bios does not implement the proximity domain
of a device because the feature is optional according to the ACPI spec,
the dev_to_node(dev) return -1, which causes out of bound access when
using the value to get the device's cpu mask by calling cpumask_of_node.

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?

Or did you mean the commit log is not clear enough to justify the change?

Or did you mean this problem should be fixed in somewhere else?

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

> 
> .
> 


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-08-31  8:55   ` Peter Zijlstra
@ 2019-08-31 10:09     ` Yunsheng Lin
  2019-08-31 16:12       ` Peter Zijlstra
  0 siblings, 1 reply; 34+ messages in thread
From: Yunsheng Lin @ 2019-08-31 10:09 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc, akpm,
	rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	len.brown, axboe, dledford, jeffrey.t.kirsher, linux-alpha,
	nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390, linux-sh,
	sparclinux, tbogendoerfer, linux-mips, linuxarm



On 2019/8/31 16:55, Peter Zijlstra wrote:
> On Sat, Aug 31, 2019 at 01:58:16PM +0800, Yunsheng Lin wrote:
>> 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.

> 
>> 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 <linyunsheng@huawei.com>
>> ---
>>  arch/x86/include/asm/topology.h | 6 ++++++
>>  arch/x86/mm/numa.c              | 2 +-
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/topology.h b/arch/x86/include/asm/topology.h
>> index 4b14d23..f36e9c8 100644
>> --- a/arch/x86/include/asm/topology.h
>> +++ b/arch/x86/include/asm/topology.h
>> @@ -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])
>> +		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.

> 
> 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.

> 
>> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
>> index e6dad60..5e393d2 100644
>> --- a/arch/x86/mm/numa.c
>> +++ b/arch/x86/mm/numa.c
>> @@ -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);
>> -- 
>> 2.8.1
>>
> 
> .
> 


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-08-31 10:09     ` Yunsheng Lin
@ 2019-08-31 16:12       ` Peter Zijlstra
  2019-09-01  4:45         ` Something about loongson_llsc_mb 陈华才
  2019-09-02  5:46         ` [PATCH v2 2/9] x86: numa: check the node id consistently for x86 Yunsheng Lin
  0 siblings, 2 replies; 34+ messages in thread
From: Peter Zijlstra @ 2019-08-31 16:12 UTC (permalink / raw)
  To: Yunsheng Lin
  Cc: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc, akpm,
	rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	len.brown, axboe, dledford, jeffrey.t.kirsher, linux-alpha,
	nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390, linux-sh,
	sparclinux, tbogendoerfer, linux-mips, linuxarm

On Sat, Aug 31, 2019 at 06:09:39PM +0800, Yunsheng Lin wrote:
> 
> 
> On 2019/8/31 16:55, Peter Zijlstra wrote:
> > On Sat, Aug 31, 2019 at 01:58:16PM +0800, Yunsheng Lin wrote:
> >> 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.

> >> @@ -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])
> >> +		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.
> 
> > 
> > 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 :-)

> 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.

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.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 7/9] sparc64: numa: check the node id consistently for sparc64
  2019-08-31  8:57     ` Yunsheng Lin
@ 2019-08-31 20:02       ` David Miller
  2019-09-02  6:08         ` Yunsheng Lin
  0 siblings, 1 reply; 34+ messages in thread
From: David Miller @ 2019-08-31 20:02 UTC (permalink / raw)
  To: linyunsheng
  Cc: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	ralf, paul.burton, jhogan, jiaxun.yang, chenhc, akpm, rppt,
	anshuman.khandual, tglx, cai, robin.murphy, linux-arm-kernel,
	linux-kernel, hpa, x86, dave.hansen, luto, peterz, len.brown,
	axboe, dledford, jeffrey.t.kirsher, linux-alpha, nfont,
	naveen.n.rao, mwb, linuxppc-dev, linux-s390, linux-sh,
	sparclinux, tbogendoerfer, linux-mips, linuxarm

From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Sat, 31 Aug 2019 16:57:04 +0800

> 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.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Something about loongson_llsc_mb
  2019-08-31 16:12       ` Peter Zijlstra
@ 2019-09-01  4:45         ` 陈华才
       [not found]           ` <2019090410032559707512@loongson.cn>
  2019-09-02  5:46         ` [PATCH v2 2/9] x86: numa: check the node id consistently for x86 Yunsheng Lin
  1 sibling, 1 reply; 34+ messages in thread
From: 陈华才 @ 2019-09-01  4:45 UTC (permalink / raw)
  To: Peter Zijlstra, huangpei
  Cc: ralf, paul.burton, jhogan, jiaxun.yang, linux-mips

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.

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.

Lastly, I think the real exception is test_and_set_bit_lock, it need a loongson_llsc_mb.

Huacai

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 5/9] s390: numa: check the node id consistently for s390
  2019-08-31  5:58 ` [PATCH v2 5/9] s390: numa: check the node id consistently for s390 Yunsheng Lin
@ 2019-09-02  4:05   ` kbuild test robot
  0 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2019-09-02  4:05 UTC (permalink / raw)
  To: Yunsheng Lin
  Cc: kbuild-all, catalin.marinas, will, mingo, bp, rth, ink, mattst88,
	benh, paulus, mpe, heiko.carstens, gor, borntraeger, ysato,
	dalias, davem, ralf, paul.burton, jhogan, jiaxun.yang, chenhc,
	akpm, rppt, anshuman.khandual, tglx, cai, robin.murphy,
	linux-arm-kernel, linux-kernel, hpa, x86, dave.hansen, luto,
	peterz, len.brown, axboe, dledford, jeffrey.t.kirsher,
	linux-alpha, nfont, naveen.n.rao, mwb, linuxppc-dev, linux-s390,
	linux-sh, sparclinux, tbogendoerfer, linux-mips, linuxarm

[-- Attachment #1: Type: text/plain, Size: 2283 bytes --]

Hi Yunsheng,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc6 next-20190830]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Yunsheng-Lin/check-the-node-id-consistently-across-different-arches/20190901-143722
config: s390-allmodconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=s390 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/topology.h:35:0,
                    from include/linux/gfp.h:9,
                    from include/linux/mm.h:10,
                    from include/linux/kvm_host.h:14,
                    from arch/s390/kernel/asm-offsets.c:11:
   arch/s390/include/asm/topology.h: In function 'cpumask_of_node':
>> arch/s390/include/asm/topology.h:84:18: error: wrong type argument to unary exclamation mark
     if (node < 0 || !node_to_cpumask_map[node])
                     ^
   make[2]: *** [arch/s390/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [sub-make] Error 2
   43 real  4 user  3 sys  16.91% cpu 	make prepare

vim +84 arch/s390/include/asm/topology.h

    76	
    77	/* Returns a pointer to the cpumask of CPUs on node 'node'. */
    78	#define cpumask_of_node cpumask_of_node
    79	static inline const struct cpumask *cpumask_of_node(int node)
    80	{
    81		if (node >= nr_node_ids)
    82			return cpu_none_mask;
    83	
  > 84		if (node < 0 || !node_to_cpumask_map[node])
    85			return cpu_online_mask;
    86	
    87		return &node_to_cpumask_map[node];
    88	}
    89	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55830 bytes --]

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-08-31 16:12       ` Peter Zijlstra
  2019-09-01  4:45         ` Something about loongson_llsc_mb 陈华才
@ 2019-09-02  5:46         ` Yunsheng Lin
  2019-09-02  7:25           ` Peter Zijlstra
  1 sibling, 1 reply; 34+ messages in thread
From: Yunsheng Lin @ 2019-09-02  5:46 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: dalias, linux-sh, catalin.marinas, dave.hansen, heiko.carstens,
	linuxarm, jiaxun.yang, linux-mips, mwb, paulus, hpa, sparclinux,
	chenhc, will, cai, linux-s390, ysato, mpe, x86, rppt,
	borntraeger, dledford, mingo, jeffrey.t.kirsher, benh, jhogan,
	nfont, mattst88, len.brown, gor, anshuman.khandual, bp, luto,
	tglx, naveen.n.rao, linux-arm-kernel, rth, axboe, linuxppc-dev,
	linux-kernel, ralf, tbogendoerfer, paul.burton, linux-alpha, ink,
	akpm, robin.murphy, davem

On 2019/9/1 0:12, Peter Zijlstra wrote:
> On Sat, Aug 31, 2019 at 06:09:39PM +0800, Yunsheng Lin wrote:
>>
>>
>> On 2019/8/31 16:55, Peter Zijlstra wrote:
>>> On Sat, Aug 31, 2019 at 01:58:16PM +0800, Yunsheng Lin wrote:
>>>> 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.
> 
>>>> @@ -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])
>>>> +		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.
>>
>>>
>>> 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 :-)

I did note the typo, corrected the first one, but missed the second one :)

> 
>> 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.
> 
> 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?

> 
> 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.


> 
> 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()?


> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> .
> 


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 7/9] sparc64: numa: check the node id consistently for sparc64
  2019-08-31 20:02       ` David Miller
@ 2019-09-02  6:08         ` Yunsheng Lin
  2019-09-02 15:17           ` David Miller
  0 siblings, 1 reply; 34+ messages in thread
From: Yunsheng Lin @ 2019-09-02  6:08 UTC (permalink / raw)
  To: David Miller
  Cc: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	ralf, paul.burton, jhogan, jiaxun.yang, chenhc, akpm, rppt,
	anshuman.khandual, tglx, cai, robin.murphy, linux-arm-kernel,
	linux-kernel, hpa, x86, dave.hansen, luto, peterz, len.brown,
	axboe, dledford, jeffrey.t.kirsher, linux-alpha, nfont,
	naveen.n.rao, mwb, linuxppc-dev, linux-s390, linux-sh,
	sparclinux, tbogendoerfer, linux-mips, linuxarm

On 2019/9/1 4:02, David Miller wrote:
> From: Yunsheng Lin <linyunsheng@huawei.com>
> Date: Sat, 31 Aug 2019 16:57:04 +0800
> 
>> 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.

Thanks for pointing out.

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

> 
> .
> 


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-09-02  5:46         ` [PATCH v2 2/9] x86: numa: check the node id consistently for x86 Yunsheng Lin
@ 2019-09-02  7:25           ` Peter Zijlstra
  2019-09-02 12:25             ` Yunsheng Lin
  2019-09-02 18:17             ` Ingo Molnar
  0 siblings, 2 replies; 34+ messages in thread
From: Peter Zijlstra @ 2019-09-02  7:25 UTC (permalink / raw)
  To: Yunsheng Lin
  Cc: dalias, linux-sh, catalin.marinas, dave.hansen, heiko.carstens,
	linuxarm, jiaxun.yang, linux-mips, mwb, paulus, hpa, sparclinux,
	chenhc, will, cai, linux-s390, ysato, mpe, x86, rppt,
	borntraeger, dledford, mingo, jeffrey.t.kirsher, benh, jhogan,
	nfont, mattst88, len.brown, gor, anshuman.khandual, bp, luto,
	tglx, naveen.n.rao, linux-arm-kernel, rth, axboe, linuxppc-dev,
	linux-kernel, ralf, tbogendoerfer, paul.burton, linux-alpha, ink,
	akpm, robin.murphy, davem

On Mon, Sep 02, 2019 at 01:46:51PM +0800, Yunsheng Lin wrote:
> On 2019/9/1 0:12, Peter Zijlstra wrote:

> > 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?

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.

> > 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.

The one true way is:

	'(unsigned)node_id >= nr_node_ids'

> > 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()?

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).

---
diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index e6dad600614c..5f49c10201c7 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -861,7 +861,7 @@ void numa_remove_cpu(int cpu)
  */
 const struct cpumask *cpumask_of_node(int node)
 {
-	if (node >= nr_node_ids) {
+	if ((unsigned)node >= nr_node_ids) {
 		printk(KERN_WARNING
 			"cpumask_of_node(%d): node > nr_node_ids(%u)\n",
 			node, nr_node_ids);

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-09-02  7:25           ` Peter Zijlstra
@ 2019-09-02 12:25             ` Yunsheng Lin
  2019-09-02 12:56               ` Peter Zijlstra
  2019-09-02 18:17             ` Ingo Molnar
  1 sibling, 1 reply; 34+ messages in thread
From: Yunsheng Lin @ 2019-09-02 12:25 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: dalias, linux-sh, catalin.marinas, dave.hansen, heiko.carstens,
	linuxarm, jiaxun.yang, linux-kernel, mwb, paulus, hpa,
	sparclinux, chenhc, will, linux-s390, ysato, mpe, x86, rppt,
	borntraeger, dledford, mingo, jeffrey.t.kirsher, benh, jhogan,
	nfont, mattst88, len.brown, gor, anshuman.khandual, ink, cai,
	luto, tglx, naveen.n.rao, linux-arm-kernel, rth, axboe,
	robin.murphy, linux-mips, ralf, tbogendoerfer, paul.burton,
	linux-alpha, bp, akpm, linuxppc-dev, davem

On 2019/9/2 15:25, Peter Zijlstra wrote:
> On Mon, Sep 02, 2019 at 01:46:51PM +0800, Yunsheng Lin wrote:
>> On 2019/9/1 0:12, Peter Zijlstra wrote:
> 
>>> 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?
> 
> 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().

> 
>>> 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.
> 
> The one true way is:
> 
> 	'(unsigned)node_id >= nr_node_ids'

I missed the magic of the "unsigned" in your previous reply.

> 
>>> 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()?
> 
> 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.
what do you mean by error? What I can think is three type of errors:
1) return NULL, this way it seems cpumask_of_node() also leave the
   job to the function that calls it.
2) cpu_none_mask, I am not sure what this means, maybe it means there
   is no cpu on the same node with the device?
3) give a warning, stack dump, or even a BUG_ON?

I would prefer the second one, and implement the third one when the
CONFIG_DEBUG_PER_CPU_MAPS is selected.

Any suggestion?

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

Thanks for the note and example.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-09-02 12:25             ` Yunsheng Lin
@ 2019-09-02 12:56               ` Peter Zijlstra
  2019-09-02 18:22                 ` Ingo Molnar
  2019-09-03  6:19                 ` Yunsheng Lin
  0 siblings, 2 replies; 34+ messages in thread
From: Peter Zijlstra @ 2019-09-02 12:56 UTC (permalink / raw)
  To: Yunsheng Lin
  Cc: dalias, linux-sh, catalin.marinas, dave.hansen, heiko.carstens,
	linuxarm, jiaxun.yang, linux-kernel, mwb, paulus, hpa,
	sparclinux, chenhc, will, linux-s390, ysato, mpe, x86, rppt,
	borntraeger, dledford, mingo, jeffrey.t.kirsher, benh, jhogan,
	nfont, mattst88, len.brown, gor, anshuman.khandual, ink, cai,
	luto, tglx, naveen.n.rao, linux-arm-kernel, rth, axboe,
	robin.murphy, linux-mips, ralf, tbogendoerfer, paul.burton,
	linux-alpha, bp, akpm, linuxppc-dev, davem, Greg Kroah-Hartman,
	Rafael J. Wysocki

On Mon, Sep 02, 2019 at 08:25:24PM +0800, Yunsheng Lin wrote:
> On 2019/9/2 15:25, Peter Zijlstra wrote:
> > On Mon, Sep 02, 2019 at 01:46:51PM +0800, Yunsheng Lin wrote:
> >> On 2019/9/1 0:12, Peter Zijlstra wrote:
> > 
> >>> 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?
> > 
> > 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.

> 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(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index f0dd8e38fee3..2caf204966a0 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -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 */

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 7/9] sparc64: numa: check the node id consistently for sparc64
  2019-09-02  6:08         ` Yunsheng Lin
@ 2019-09-02 15:17           ` David Miller
  0 siblings, 0 replies; 34+ messages in thread
From: David Miller @ 2019-09-02 15:17 UTC (permalink / raw)
  To: linyunsheng
  Cc: catalin.marinas, will, mingo, bp, rth, ink, mattst88, benh,
	paulus, mpe, heiko.carstens, gor, borntraeger, ysato, dalias,
	ralf, paul.burton, jhogan, jiaxun.yang, chenhc, akpm, rppt,
	anshuman.khandual, tglx, cai, robin.murphy, linux-arm-kernel,
	linux-kernel, hpa, x86, dave.hansen, luto, peterz, len.brown,
	axboe, dledford, jeffrey.t.kirsher, linux-alpha, nfont,
	naveen.n.rao, mwb, linuxppc-dev, linux-s390, linux-sh,
	sparclinux, tbogendoerfer, linux-mips, linuxarm

From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Mon, 2 Sep 2019 14:08:31 +0800

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

Sometimes, and in other cases other methods are used to determine
the NUMA node id.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-09-02  7:25           ` Peter Zijlstra
  2019-09-02 12:25             ` Yunsheng Lin
@ 2019-09-02 18:17             ` Ingo Molnar
  2019-09-03  7:53               ` [PATCH] x86/mm: Fix cpumask_of_node() error condition Peter Zijlstra
  1 sibling, 1 reply; 34+ messages in thread
From: Ingo Molnar @ 2019-09-02 18:17 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Yunsheng Lin, dalias, linux-sh, catalin.marinas, dave.hansen,
	heiko.carstens, linuxarm, jiaxun.yang, linux-mips, mwb, paulus,
	hpa, sparclinux, chenhc, will, cai, linux-s390, ysato, mpe, x86,
	rppt, borntraeger, dledford, mingo, jeffrey.t.kirsher, benh,
	jhogan, nfont, mattst88, len.brown, gor, anshuman.khandual, bp,
	luto, tglx, naveen.n.rao, linux-arm-kernel, rth, axboe,
	linuxppc-dev, linux-kernel, ralf, tbogendoerfer, paul.burton,
	linux-alpha, ink, akpm, robin.murphy, davem


* Peter Zijlstra <peterz@infradead.org> wrote:

> Also note that the CONFIG_DEBUG_PER_CPU_MAPS version of
> cpumask_of_node() already does this (although it wants the below fix).
> 
> ---
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index e6dad600614c..5f49c10201c7 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -861,7 +861,7 @@ void numa_remove_cpu(int cpu)
>   */
>  const struct cpumask *cpumask_of_node(int node)
>  {
> -	if (node >= nr_node_ids) {
> +	if ((unsigned)node >= nr_node_ids) {
>  		printk(KERN_WARNING
>  			"cpumask_of_node(%d): node > nr_node_ids(%u)\n",
>  			node, nr_node_ids);

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

With that:

Acked-by: Ingo Molnar <mingo@kernel.org>

Note that:

- arch/arm64/mm/numa.c copied the same sign bug (or unrobustness if we 
  don't want to call it a bug).

- Kudos to the mm/memcontrol.c and kernel/bpf/syscall.c teams for not 
  copying that bug. Booo for none of them fixing the buggy pattern 
  elsewhere in the kernel ;-)

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-09-02 12:56               ` Peter Zijlstra
@ 2019-09-02 18:22                 ` Ingo Molnar
  2019-09-02 19:14                   ` Peter Zijlstra
  2019-09-03  6:19                 ` Yunsheng Lin
  1 sibling, 1 reply; 34+ messages in thread
From: Ingo Molnar @ 2019-09-02 18:22 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Yunsheng Lin, dalias, linux-sh, catalin.marinas, dave.hansen,
	heiko.carstens, linuxarm, jiaxun.yang, linux-kernel, mwb, paulus,
	hpa, sparclinux, chenhc, will, linux-s390, ysato, mpe, x86, rppt,
	borntraeger, dledford, mingo, jeffrey.t.kirsher, benh, jhogan,
	nfont, mattst88, len.brown, gor, anshuman.khandual, ink, cai,
	luto, tglx, naveen.n.rao, linux-arm-kernel, rth, axboe,
	robin.murphy, linux-mips, ralf, tbogendoerfer, paul.burton,
	linux-alpha, bp, akpm, linuxppc-dev, davem, Greg Kroah-Hartman,
	Rafael J. Wysocki


* Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Sep 02, 2019 at 08:25:24PM +0800, Yunsheng Lin wrote:
> > On 2019/9/2 15:25, Peter Zijlstra wrote:
> > > On Mon, Sep 02, 2019 at 01:46:51PM +0800, Yunsheng Lin wrote:
> > >> On 2019/9/1 0:12, Peter Zijlstra wrote:
> > > 
> > >>> 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?
> > > 
> > > 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.
> 
> > 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(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index f0dd8e38fee3..2caf204966a0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -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

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?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-09-02 18:22                 ` Ingo Molnar
@ 2019-09-02 19:14                   ` Peter Zijlstra
  0 siblings, 0 replies; 34+ messages in thread
From: Peter Zijlstra @ 2019-09-02 19:14 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Yunsheng Lin, dalias, linux-sh, catalin.marinas, dave.hansen,
	heiko.carstens, linuxarm, jiaxun.yang, linux-kernel, mwb, paulus,
	hpa, sparclinux, chenhc, will, linux-s390, ysato, mpe, x86, rppt,
	borntraeger, dledford, mingo, jeffrey.t.kirsher, benh, jhogan,
	nfont, mattst88, len.brown, gor, anshuman.khandual, ink, cai,
	luto, tglx, naveen.n.rao, linux-arm-kernel, rth, axboe,
	robin.murphy, linux-mips, ralf, tbogendoerfer, paul.burton,
	linux-alpha, bp, akpm, linuxppc-dev, davem, Greg Kroah-Hartman,
	Rafael J. Wysocki

On Mon, Sep 02, 2019 at 08:22:52PM +0200, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:

> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index f0dd8e38fee3..2caf204966a0 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -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
> 
> 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?

Good point; we might have to exclude nr_node_ids==1 systems from
warning.

> 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?

I think so; a device needs to be _somewhere_, right? Typically though;
devices are on a PCI bus, and the PCI bridge itself will have a NUMA
binding and then the above parent rule will make everything just work.

But I don't see how you can be outside of the NUMA topology.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-09-02 12:56               ` Peter Zijlstra
  2019-09-02 18:22                 ` Ingo Molnar
@ 2019-09-03  6:19                 ` Yunsheng Lin
  2019-09-03  7:11                   ` Peter Zijlstra
  1 sibling, 1 reply; 34+ messages in thread
From: Yunsheng Lin @ 2019-09-03  6:19 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: dalias, linux-sh, catalin.marinas, dave.hansen, heiko.carstens,
	linuxarm, jiaxun.yang, linux-kernel, mwb, paulus, hpa,
	sparclinux, chenhc, will, linux-s390, ysato, mpe, x86, rppt,
	borntraeger, dledford, mingo, jeffrey.t.kirsher, benh, jhogan,
	nfont, mattst88, len.brown, gor, anshuman.khandual, ink, cai,
	luto, tglx, naveen.n.rao, linux-arm-kernel, rth, axboe,
	robin.murphy, linux-mips, ralf, tbogendoerfer, paul.burton,
	linux-alpha, bp, akpm, linuxppc-dev, davem, Greg Kroah-Hartman,
	Rafael J. Wysocki

On 2019/9/2 20:56, Peter Zijlstra wrote:
> On Mon, Sep 02, 2019 at 08:25:24PM +0800, Yunsheng Lin wrote:
>> On 2019/9/2 15:25, Peter Zijlstra wrote:
>>> On Mon, Sep 02, 2019 at 01:46:51PM +0800, Yunsheng Lin wrote:
>>>> On 2019/9/1 0:12, Peter Zijlstra wrote:
>>>
>>>>> 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?
>>>
>>> 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.
> 
>> 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.

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?

> 
> ---
>  drivers/base/core.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index f0dd8e38fee3..2caf204966a0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -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 */
> 
> .
> 


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-09-03  6:19                 ` Yunsheng Lin
@ 2019-09-03  7:11                   ` Peter Zijlstra
  2019-09-03  8:31                     ` Yunsheng Lin
  0 siblings, 1 reply; 34+ messages in thread
From: Peter Zijlstra @ 2019-09-03  7:11 UTC (permalink / raw)
  To: Yunsheng Lin
  Cc: dalias, linux-sh, catalin.marinas, dave.hansen, heiko.carstens,
	linuxarm, jiaxun.yang, linux-kernel, mwb, paulus, hpa,
	sparclinux, chenhc, will, linux-s390, ysato, mpe, x86, rppt,
	borntraeger, dledford, mingo, jeffrey.t.kirsher, benh, jhogan,
	nfont, mattst88, len.brown, gor, anshuman.khandual, ink, cai,
	luto, tglx, naveen.n.rao, linux-arm-kernel, rth, axboe,
	robin.murphy, linux-mips, ralf, tbogendoerfer, paul.burton,
	linux-alpha, bp, akpm, linuxppc-dev, davem, Greg Kroah-Hartman,
	Rafael J. Wysocki

On Tue, Sep 03, 2019 at 02:19:04PM +0800, Yunsheng Lin wrote:
> On 2019/9/2 20:56, Peter Zijlstra wrote:
> > On Mon, Sep 02, 2019 at 08:25:24PM +0800, Yunsheng Lin wrote:
> >> On 2019/9/2 15:25, Peter Zijlstra wrote:
> >>> On Mon, Sep 02, 2019 at 01:46:51PM +0800, Yunsheng Lin wrote:
> >>>> On 2019/9/1 0:12, Peter Zijlstra wrote:
> >>>
> >>>>> 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?
> >>>
> >>> 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.
> > 
> >> 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.
> 
> 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.

> 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.

> 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?



^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH] x86/mm: Fix cpumask_of_node() error condition
  2019-09-02 18:17             ` Ingo Molnar
@ 2019-09-03  7:53               ` Peter Zijlstra
  0 siblings, 0 replies; 34+ messages in thread
From: Peter Zijlstra @ 2019-09-03  7:53 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Yunsheng Lin, dalias, linux-sh, catalin.marinas, dave.hansen,
	heiko.carstens, linuxarm, jiaxun.yang, linux-mips, mwb, paulus,
	hpa, sparclinux, chenhc, will, cai, linux-s390, ysato, mpe, x86,
	rppt, borntraeger, dledford, mingo, jeffrey.t.kirsher, benh,
	jhogan, nfont, mattst88, len.brown, gor, anshuman.khandual, bp,
	luto, tglx, naveen.n.rao, linux-arm-kernel, rth, axboe,
	linuxppc-dev, linux-kernel, ralf, tbogendoerfer, paul.burton,
	linux-alpha, ink, akpm, robin.murphy, davem

On Mon, Sep 02, 2019 at 08:17:31PM +0200, Ingo Molnar wrote:

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

Full patch below.

---
Subject: x86/mm: Fix cpumask_of_node() error condition

When CONFIG_DEBUG_PER_CPU_MAPS we validate that the @node argument of
cpumask_of_node() is a valid node_id. It however forgets to check for
negative numbers. Fix this by explicitly casting to unsigned.

  (unsigned)node >= nr_node_ids

verifies: 0 <= node < nr_node_ids

Also ammend the error message to match the condition.

Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/mm/numa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index e6dad600614c..4123100e0eaf 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -861,9 +861,9 @@ void numa_remove_cpu(int cpu)
  */
 const struct cpumask *cpumask_of_node(int node)
 {
-	if (node >= nr_node_ids) {
+	if ((unsigned)node >= nr_node_ids) {
 		printk(KERN_WARNING
-			"cpumask_of_node(%d): node > nr_node_ids(%u)\n",
+			"cpumask_of_node(%d): (unsigned)node >= nr_node_ids(%u)\n",
 			node, nr_node_ids);
 		dump_stack();
 		return cpu_none_mask;

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/9] x86: numa: check the node id consistently for x86
  2019-09-03  7:11                   ` Peter Zijlstra
@ 2019-09-03  8:31                     ` Yunsheng Lin
  0 siblings, 0 replies; 34+ messages in thread
From: Yunsheng Lin @ 2019-09-03  8:31 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: dalias, linux-sh, catalin.marinas, dave.hansen, heiko.carstens,
	linuxarm, jiaxun.yang, linux-kernel, mwb, paulus, hpa,
	sparclinux, chenhc, will, linux-s390, ysato, mpe, x86, rppt,
	borntraeger, dledford, mingo, jeffrey.t.kirsher, benh, jhogan,
	nfont, mattst88, len.brown, gor, anshuman.khandual, ink, cai,
	luto, tglx, naveen.n.rao, linux-arm-kernel, rth, axboe,
	robin.murphy, linux-mips, ralf, tbogendoerfer, paul.burton,
	linux-alpha, bp, akpm, linuxppc-dev, davem, Greg Kroah-Hartman,
	Rafael J. Wysocki

On 2019/9/3 15:11, Peter Zijlstra wrote:
> On Tue, Sep 03, 2019 at 02:19:04PM +0800, Yunsheng Lin wrote:
>> On 2019/9/2 20:56, Peter Zijlstra wrote:
>>> On Mon, Sep 02, 2019 at 08:25:24PM +0800, Yunsheng Lin wrote:
>>>> On 2019/9/2 15:25, Peter Zijlstra wrote:
>>>>> On Mon, Sep 02, 2019 at 01:46:51PM +0800, Yunsheng Lin wrote:
>>>>>> On 2019/9/1 0:12, Peter Zijlstra wrote:
>>>>>
>>>>>>> 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?
>>>>>
>>>>> 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.
>>>
>>>> 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.
>>
>> 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.

Ok. To summarize the discussion in order to for me to understand it
correctly:

1) Make sure device_add() set to default node0 to a device if
   ACPI/BIOS does not set the node id and it has not no parent device.

2) Use '(unsigned)node_id >= nr_node_ids' to fix the
   CONFIG_DEBUG_PER_CPU_MAPS version of cpumask_of_node() for x86
   and arm64, x86 just has a fix from you now, a patch for arm64 is
   also needed.

3) Maybe fix some other the sign bug for node id checking through the
   kernel using the '(unsigned)node_id >= nr_node_ids'.

Please see if I understand it correctly or miss something.
Maybe I can begin by sending a patch about item one to see if everyone
is ok with the idea?


> 
>> 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.



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Something about loongson_llsc_mb
       [not found]           ` <2019090410032559707512@loongson.cn>
@ 2019-09-04  9:21             ` Peter Zijlstra
  2019-09-04 10:04               ` Peter Zijlstra
  2019-09-04 12:57               ` Huang Pei
  0 siblings, 2 replies; 34+ messages in thread
From: Peter Zijlstra @ 2019-09-04  9:21 UTC (permalink / raw)
  To: huangpei
  Cc: 陈华才,
	ralf, Paul Burton, jhogan, jiaxun.yang, linux-mips


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

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

On Wed, Sep 04, 2019 at 10:03:31AM +0800, huangpei@loongson.cn wrote:
> >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.

> >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/20190424124421.693353463@infradead.org

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.

> +. 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

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


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Something about loongson_llsc_mb
  2019-09-04  9:21             ` Peter Zijlstra
@ 2019-09-04 10:04               ` Peter Zijlstra
  2019-09-04 12:57               ` Huang Pei
  1 sibling, 0 replies; 34+ messages in thread
From: Peter Zijlstra @ 2019-09-04 10:04 UTC (permalink / raw)
  To: huangpei
  Cc: 陈华才,
	ralf, Paul Burton, jhogan, jiaxun.yang, linux-mips

On Wed, Sep 04, 2019 at 11:21:54AM +0200, Peter Zijlstra wrote:
> 
> *why* are you replying to some random unrelated thread?
> 
> Also, please use a sane MUA and wrap your lines <80 chars.
> 
> On Wed, Sep 04, 2019 at 10:03:31AM +0800, huangpei@loongson.cn wrote:
> > >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.
> 
> > >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/20190424124421.693353463@infradead.org
> 
> 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.

I think we're good, smp_mb__{before,after}_atomic() already doesn't work
with local_t.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Something about loongson_llsc_mb
  2019-09-04  9:21             ` Peter Zijlstra
  2019-09-04 10:04               ` Peter Zijlstra
@ 2019-09-04 12:57               ` Huang Pei
  1 sibling, 0 replies; 34+ messages in thread
From: Huang Pei @ 2019-09-04 12:57 UTC (permalink / raw)
  To: ambrosehua; +Cc: Paul Burton, jhogan, jiaxun.yang, linux-mips

On 09/04/2019 05:21 PM, Peter Zijlstra wrote:
> 
> *why* are you replying to some random unrelated thread?

Chen ask me if whether your patch has more sync than needed, but I'm not sure 
whether sync before and after *cmpxchg_local* is.
> 
> Also, please use a sane MUA and wrap your lines <80 chars.

Sorry, I finally got thunderbird in plain text mode with < 80 chars. It wont 
happen again.

> 
> On Wed, Sep 04, 2019 at 10:03:31AM +0800, huangpei@loongson.cn wrote:
>>> 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.
> 
>>> 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/20190424124421.693353463@infradead.org
> 
> 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.
> 
>> +. 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
> 
> Correct, we already dropped the change for other local.h stuff.
> 

What about cmpxchg_local? Your patch add sync before and after ll/sc in 
__cmpxchg, so *cmpxchg_local* has sync around it. But cmpxchg_local operate on 
per-cpu, which *shall not* trigger loongson's LLSC bug, since only *this* cpu 
write, other cpus read.


^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, back to index

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-31  5:58 [PATCH v2 0/9] check the node id consistently across different arches Yunsheng Lin
2019-08-31  5:58 ` [PATCH v2 1/9] arm64: numa: check the node id consistently for arm64 Yunsheng Lin
2019-08-31  5:58 ` [PATCH v2 2/9] x86: numa: check the node id consistently for x86 Yunsheng Lin
2019-08-31  8:55   ` Peter Zijlstra
2019-08-31 10:09     ` Yunsheng Lin
2019-08-31 16:12       ` Peter Zijlstra
2019-09-01  4:45         ` Something about loongson_llsc_mb 陈华才
     [not found]           ` <2019090410032559707512@loongson.cn>
2019-09-04  9:21             ` Peter Zijlstra
2019-09-04 10:04               ` Peter Zijlstra
2019-09-04 12:57               ` Huang Pei
2019-09-02  5:46         ` [PATCH v2 2/9] x86: numa: check the node id consistently for x86 Yunsheng Lin
2019-09-02  7:25           ` Peter Zijlstra
2019-09-02 12:25             ` Yunsheng Lin
2019-09-02 12:56               ` Peter Zijlstra
2019-09-02 18:22                 ` Ingo Molnar
2019-09-02 19:14                   ` Peter Zijlstra
2019-09-03  6:19                 ` Yunsheng Lin
2019-09-03  7:11                   ` Peter Zijlstra
2019-09-03  8:31                     ` Yunsheng Lin
2019-09-02 18:17             ` Ingo Molnar
2019-09-03  7:53               ` [PATCH] x86/mm: Fix cpumask_of_node() error condition Peter Zijlstra
2019-08-31  5:58 ` [PATCH v2 3/9] alpha: numa: check the node id consistently for alpha Yunsheng Lin
2019-08-31  5:58 ` [PATCH v2 4/9] powerpc: numa: check the node id consistently for powerpc Yunsheng Lin
2019-08-31  5:58 ` [PATCH v2 5/9] s390: numa: check the node id consistently for s390 Yunsheng Lin
2019-09-02  4:05   ` kbuild test robot
2019-08-31  5:58 ` [PATCH v2 6/9] sh: numa: check the node id consistently for sh Yunsheng Lin
2019-08-31  5:58 ` [PATCH v2 7/9] sparc64: numa: check the node id consistently for sparc64 Yunsheng Lin
2019-08-31  6:53   ` David Miller
2019-08-31  8:57     ` Yunsheng Lin
2019-08-31 20:02       ` David Miller
2019-09-02  6:08         ` Yunsheng Lin
2019-09-02 15:17           ` David Miller
2019-08-31  5:58 ` [PATCH v2 8/9] mips: numa: check the node id consistently for mips ip27 Yunsheng Lin
2019-08-31  5:58 ` [PATCH v2 9/9] mips: numa: check the node id consistently for mips loongson64 Yunsheng Lin

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org linux-mips@archiver.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/ public-inbox