Skip to contentSkip to conversation

arm64: numa/topology/smp: update the cpumasks for CPU hotplug

PATCH Superseded Sudeep Holla 4 participants 23 messages Jun 18, 2018 – Jul 4, 2018 arm64

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

v2 cover arm64: numa/topology/smp: update the cpumasks for CPU hotplug

Hi,

Commit 37c3ec2d810f ("arm64: topology: divorce MC scheduling domain from core_siblings") was reported to cause some hotplug and system suspend regressions when
it was merged. On investigation, it was found that unlike x86/PPC,
arm64 doesn't update the cpu and numa masks on CPU hotplug. That's somewhat expected from the scheduler.

Since these changes were bit invasive as a solution to the above mentioned regression, as small change was temporarily applied as a fix. This series updates the cpu and numa masks on CPU hotplug and reverts that temporary fix.

It would be good to get this tested(CPU hotplug - few and all CPUs in a socket) on multi-socket/NUMA systems from Cavium and Huawei/Hisilicon.

Regards,
Sudeep

v1->v2:
- Rebased on v4.18-rc1 and hence do revert of the temporary fix that was merged for v4.18
- Removed one of the wrong use of possible_mask

Sudeep Holla (7):
arm64: topology: refactor reset_cpu_topology to add support for removing topology
arm64: numa: separate out updates to percpu nodeid and NUMA node cpumap
arm64: topology: add support to remove cpu topology sibling masks
arm64: topology: restrict updating siblings_masks to online cpus only
arm64: smp: remove cpu and numa topology information when hotplugging out CPU
arm64: topology: rename llc_siblings to align with other struct members
arm64: topology: re-introduce numa mask check for scheduler MC selection

 arch/arm64/include/asm/numa.h     |  4 +++
 arch/arm64/include/asm/topology.h |  3 +-
 arch/arm64/kernel/smp.c           |  5 +++
 arch/arm64/kernel/topology.c      | 69 ++++++++++++++++++++++++++-------------
 arch/arm64/mm/numa.c              | 29 +++++++++++-----
 5 files changed, 79 insertions(+), 31 deletions(-)

--
2.7.4

#1 lore raw

PATCH v2 1/7 arm64: topology: refactor reset_cpu_topology to add support for removing topology

Currently reset_cpu_topology clears all the CPU topology information and resets to default values. However we may need to just clear the information when we hotplug out the CPU. In preparation to add the support the same, let's refactor reset_cpu_topology to clear out the information and reset them only if explicitly requested.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/kernel/topology.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)
arch/arm64/kernel/topology.c +19 -12
index f845a8617812..6ea3ec49d418 100644
@@ -293,26 +293,33 @@ void store_cpu_topology(unsigned int cpuid)
 	update_siblings_masks(cpuid);
 }
 
-static void __init reset_cpu_topology(void)
+static void clear_cpu_topology(int cpu, bool reset)
 {
-	unsigned int cpu;
-
-	for_each_possible_cpu(cpu) {
-		struct cpu_topology *cpu_topo = &cpu_topology[cpu];
+	struct cpu_topology *cpu_topo = &cpu_topology[cpu];
 
+	if (reset) {
 		cpu_topo->thread_id = -1;
 		cpu_topo->core_id = 0;
 		cpu_topo->package_id = -1;
 
 		cpu_topo->llc_id = -1;
-		cpumask_clear(&cpu_topo->llc_siblings);
-		cpumask_set_cpu(cpu, &cpu_topo->llc_siblings);
-
-		cpumask_clear(&cpu_topo->core_sibling);
-		cpumask_set_cpu(cpu, &cpu_topo->core_sibling);
-		cpumask_clear(&cpu_topo->thread_sibling);
-		cpumask_set_cpu(cpu, &cpu_topo->thread_sibling);
 	}
+
+	cpumask_clear(&cpu_topo->llc_siblings);
+	cpumask_set_cpu(cpu, &cpu_topo->llc_siblings);
+
+	cpumask_clear(&cpu_topo->core_sibling);
+	cpumask_set_cpu(cpu, &cpu_topo->core_sibling);
+	cpumask_clear(&cpu_topo->thread_sibling);
+	cpumask_set_cpu(cpu, &cpu_topo->thread_sibling);
+}
+
+static void __init reset_cpu_topology(void)
+{
+	unsigned int cpu;
+
+	for_each_possible_cpu(cpu)
+		clear_cpu_topology(cpu, true);
 }
 
 #ifdef CONFIG_ACPI

--
2.7.4

#2 lore raw

PATCH v2 2/7 arm64: numa: separate out updates to percpu nodeid and NUMA node cpumap

In reply to Sudeep Holla

Currently numa_clear_node removes both cpu information from the NUMA node cpumap as well as the NUMA node id from the cpu. Similarly numa_store_cpu_info updates both percpu nodeid and NUMA cpumap.

However we need to retain the numa node id for the cpu and only remove the cpu information from the numa node cpumap during CPU hotplug out. The same can be extended for hotplugging in the CPU.

This patch separates out numa_{add,remove}_cpu from numa_clear_node and numa_store_cpu_info.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/include/asm/numa.h |  4 ++++
 arch/arm64/kernel/smp.c       |  2 ++
 arch/arm64/mm/numa.c          | 29 +++++++++++++++++++++--------
 3 files changed, 27 insertions(+), 8 deletions(-)
arch/arm64/include/asm/numa.h +4 -0
index 01bc46d5b43a..626ad01e83bf 100644
@@ -35,10 +35,14 @@ void __init numa_set_distance(int from, int to, int distance);
 void __init numa_free_distance(void);
 void __init early_map_cpu_to_node(unsigned int cpu, int nid);
 void numa_store_cpu_info(unsigned int cpu);
+void numa_add_cpu(unsigned int cpu);
+void numa_remove_cpu(unsigned int cpu);
 
 #else	/* CONFIG_NUMA */
 
 static inline void numa_store_cpu_info(unsigned int cpu) { }
+static inline void numa_add_cpu(unsigned int cpu) { }
+static inline void numa_remove_cpu(unsigned int cpu) { }
 static inline void arm64_numa_init(void) { }
 static inline void early_map_cpu_to_node(unsigned int cpu, int nid) { }
 
arch/arm64/kernel/smp.c +2 -0
index f3e2e3aec0b0..49a021e30dfb 100644
@@ -225,6 +225,7 @@ asmlinkage void secondary_start_kernel(void)
 	notify_cpu_starting(cpu);
 
 	store_cpu_topology(cpu);
+	numa_add_cpu(cpu);
 
 	/*
 	 * OK, now it's safe to let the boot CPU continue.  Wait for
@@ -679,6 +680,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	this_cpu = smp_processor_id();
 	store_cpu_topology(this_cpu);
 	numa_store_cpu_info(this_cpu);
+	numa_add_cpu(this_cpu);
 
 	/*
 	 * If UP is mandated by "nosmp" (which implies "maxcpus=0"), don't set
arch/arm64/mm/numa.c +21 -8
index dad128ba98bf..43cc669bc7bc 100644
@@ -70,19 +70,32 @@ EXPORT_SYMBOL(cpumask_of_node);
 
 #endif
 
-static void map_cpu_to_node(unsigned int cpu, int nid)
+static void numa_update_cpu(unsigned int cpu, bool remove)
 {
-	set_cpu_numa_node(cpu, nid);
-	if (nid >= 0)
+	int nid = cpu_to_node(cpu);
+
+	if (nid < 0)
+		return;
+
+	if (remove)
+		cpumask_clear_cpu(cpu, node_to_cpumask_map[nid]);
+	else
 		cpumask_set_cpu(cpu, node_to_cpumask_map[nid]);
 }
 
-void numa_clear_node(unsigned int cpu)
+void numa_add_cpu(unsigned int cpu)
 {
-	int nid = cpu_to_node(cpu);
+	numa_update_cpu(cpu, false);
+}
 
-	if (nid >= 0)
-		cpumask_clear_cpu(cpu, node_to_cpumask_map[nid]);
+void numa_remove_cpu(unsigned int cpu)
+{
+	numa_update_cpu(cpu, true);
+}
+
+void numa_clear_node(unsigned int cpu)
+{
+	numa_remove_cpu(cpu);
 	set_cpu_numa_node(cpu, NUMA_NO_NODE);
 }
 
@@ -116,7 +129,7 @@ static void __init setup_node_to_cpumask_map(void)
  */
 void numa_store_cpu_info(unsigned int cpu)
 {
-	map_cpu_to_node(cpu, cpu_to_node_map[cpu]);
+	set_cpu_numa_node(cpu, cpu_to_node_map[cpu]);
 }
 
 void __init early_map_cpu_to_node(unsigned int cpu, int nid)

--
2.7.4

#3 lore raw

PATCH v2 2/7 arm64: numa: separate out updates to percpu nodeid and NUMA node cpumap

Quoted 114 lines · Sudeep Holla

Currently numa_clear_node removes both cpu information from the NUMA node cpumap as well as the NUMA node id from the cpu. Similarly numa_store_cpu_info updates both percpu nodeid and NUMA cpumap.

However we need to retain the numa node id for the cpu and only remove the cpu information from the numa node cpumap during CPU hotplug out. The same can be extended for hotplugging in the CPU.

This patch separates out numa_{add,remove}_cpu from numa_clear_node and numa_store_cpu_info.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
  arch/arm64/include/asm/numa.h |  4 ++++
  arch/arm64/kernel/smp.c       |  2 ++
  arch/arm64/mm/numa.c          | 29 +++++++++++++++++++++--------
  3 files changed, 27 insertions(+), 8 deletions(-)
arch/arm64/include/asm/numa.h +4 -0
index 01bc46d5b43a..626ad01e83bf 100644
@@ -35,10 +35,14 @@ void __init numa_set_distance(int from, int to, int distance);
  void __init numa_free_distance(void);
  void __init early_map_cpu_to_node(unsigned int cpu, int nid);
  void numa_store_cpu_info(unsigned int cpu);
+void numa_add_cpu(unsigned int cpu);
+void numa_remove_cpu(unsigned int cpu);
 
  #else  /* CONFIG_NUMA */
 
  static inline void numa_store_cpu_info(unsigned int cpu) { }
+static inline void numa_add_cpu(unsigned int cpu) { }
+static inline void numa_remove_cpu(unsigned int cpu) { }
  static inline void arm64_numa_init(void) { }
  static inline void early_map_cpu_to_node(unsigned int cpu, int nid) { }
 
arch/arm64/kernel/smp.c +2 -0
index f3e2e3aec0b0..49a021e30dfb 100644
@@ -225,6 +225,7 @@ asmlinkage void secondary_start_kernel(void)
         notify_cpu_starting(cpu);
 
         store_cpu_topology(cpu);
+       numa_add_cpu(cpu);
 
         /*
          * OK, now it's safe to let the boot CPU continue.  Wait for
@@ -679,6 +680,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
         this_cpu = smp_processor_id();
         store_cpu_topology(this_cpu);
         numa_store_cpu_info(this_cpu);
+       numa_add_cpu(this_cpu);
 
         /*
          * If UP is mandated by "nosmp" (which implies "maxcpus=0"), don't set
arch/arm64/mm/numa.c +21 -9
index dad128ba98bf..43cc669bc7bc 100644
@@ -70,19 +70,32 @@ EXPORT_SYMBOL(cpumask_of_node);
 
  #endif
 
-static void map_cpu_to_node(unsigned int cpu, int nid)
+static void numa_update_cpu(unsigned int cpu, bool remove)
  {
-       set_cpu_numa_node(cpu, nid);
-       if (nid >= 0)
+       int nid = cpu_to_node(cpu);
+
+       if (nid < 0)
+               return;
+
+       if (remove)
+               cpumask_clear_cpu(cpu, node_to_cpumask_map[nid]);
+       else
                 cpumask_set_cpu(cpu, node_to_cpumask_map[nid]);
  }
 
-void numa_clear_node(unsigned int cpu)
+void numa_add_cpu(unsigned int cpu)
  {
-       int nid = cpu_to_node(cpu);
+       numa_update_cpu(cpu, false);
+}
 
-       if (nid >= 0)
-               cpumask_clear_cpu(cpu, node_to_cpumask_map[nid]);
+void numa_remove_cpu(unsigned int cpu)
+{
+       numa_update_cpu(cpu, true);
+}
+
+void numa_clear_node(unsigned int cpu)
+{
+       numa_remove_cpu(cpu);
         set_cpu_numa_node(cpu, NUMA_NO_NODE);
  }
 
@@ -116,7 +129,7 @@ static void __init setup_node_to_cpumask_map(void)
   */
  void numa_store_cpu_info(unsigned int cpu)
  {
-       map_cpu_to_node(cpu, cpu_to_node_map[cpu]);
+       set_cpu_numa_node(cpu, cpu_to_node_map[cpu]);
  }
 
  void __init early_map_cpu_to_node(unsigned int cpu, int nid)
--
Quoted 2 lines

2.7.4

changes looks ok to me.

thanks
Ganapat

#4 lore raw

PATCH v2 2/7 arm64: numa: separate out updates to percpu nodeid and NUMA node cpumap

In reply to Sudeep Holla
Quoted 11 lines · Sudeep Holla

Currently numa_clear_node removes both cpu information from the NUMA node cpumap as well as the NUMA node id from the cpu. Similarly numa_store_cpu_info updates both percpu nodeid and NUMA cpumap.

However we need to retain the numa node id for the cpu and only remove the cpu information from the numa node cpumap during CPU hotplug out. The same can be extended for hotplugging in the CPU.

This patch separates out numa_{add,remove}_cpu from numa_clear_node and numa_store_cpu_info.

Quoted 8 lines
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/include/asm/numa.h |  4 ++++
 arch/arm64/kernel/smp.c       |  2 ++
 arch/arm64/mm/numa.c          | 29 +++++++++++++++++++++--------
 3 files changed, 27 insertions(+), 8 deletions(-)

--->8

Quoted 7 lines
arch/arm64/mm/numa.c +0 -0
index dad128ba98bf..43cc669bc7bc 100644
@@ -70,19 +70,32 @@ EXPORT_SYMBOL(cpumask_of_node);
 
 #endif

arch/arm64/mm/numa.c EXPORT_SYMBOL(cpumask_of_node); · line 78

Quoted 10 lines
arch/arm64/mm/numa.c +5 -3
 
-static void map_cpu_to_node(unsigned int cpu, int nid)
+static void numa_update_cpu(unsigned int cpu, bool remove)
 {
-	set_cpu_numa_node(cpu, nid);
-	if (nid >= 0)
+	int nid = cpu_to_node(cpu);
+
+	if (nid < 0)
+		return;

This could be a check against NUMA_NO_NODE, right?

Will

#5 lore raw

PATCH v2 2/7 arm64: numa: separate out updates to percpu nodeid and NUMA node cpumap

Quoted 31 lines · Will Deacon
Quoted 11 lines · Sudeep Holla

Currently numa_clear_node removes both cpu information from the NUMA node cpumap as well as the NUMA node id from the cpu. Similarly numa_store_cpu_info updates both percpu nodeid and NUMA cpumap.

However we need to retain the numa node id for the cpu and only remove the cpu information from the numa node cpumap during CPU hotplug out. The same can be extended for hotplugging in the CPU.

This patch separates out numa_{add,remove}_cpu from numa_clear_node and numa_store_cpu_info.

Quoted 8 lines
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/include/asm/numa.h |  4 ++++
 arch/arm64/kernel/smp.c       |  2 ++
 arch/arm64/mm/numa.c          | 29 +++++++++++++++++++++--------
 3 files changed, 27 insertions(+), 8 deletions(-)

--->8

Quoted 8 lines
arch/arm64/mm/numa.c +0 -0
index dad128ba98bf..43cc669bc7bc 100644
@@ -70,19 +70,32 @@ EXPORT_SYMBOL(cpumask_of_node);
 
 #endif
 
Quoted 12 lines

arch/arm64/mm/numa.c EXPORT_SYMBOL(cpumask_of_node); · line 78

Quoted 9 lines
arch/arm64/mm/numa.c +5 -3
-static void map_cpu_to_node(unsigned int cpu, int nid)
+static void numa_update_cpu(unsigned int cpu, bool remove)
 {
-	set_cpu_numa_node(cpu, nid);
-	if (nid >= 0)
+	int nid = cpu_to_node(cpu);
+
+	if (nid < 0)
+		return;

This could be a check against NUMA_NO_NODE, right?

Indeed.

Signature · 2 lines
Regards,
Sudeep
#6 lore raw

PATCH v2 3/7 arm64: topology: add support to remove cpu topology sibling masks

In reply to Sudeep Holla

This patch adds support to remove all the CPU topology information using clear_cpu_topology and also resetting the sibling information on other sibling CPUs. This will be used in cpu_disable so that all the topology sibling information is removed on CPU hotplug out.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/include/asm/topology.h |  1 +
 arch/arm64/kernel/topology.c      | 15 +++++++++++++++
 2 files changed, 16 insertions(+)
arch/arm64/include/asm/topology.h +1 -0
index df48212f767b..fb996f454305 100644
@@ -23,6 +23,7 @@ extern struct cpu_topology cpu_topology[NR_CPUS];
 
 void init_cpu_topology(void);
 void store_cpu_topology(unsigned int cpuid);
+void remove_cpu_topology(unsigned int cpuid);
 const struct cpumask *cpu_coregroup_mask(int cpu);
 
 #ifdef CONFIG_NUMA
arch/arm64/kernel/topology.c +15 -0
index 6ea3ec49d418..38b102013708 100644
@@ -322,6 +322,21 @@ static void __init reset_cpu_topology(void)
 		clear_cpu_topology(cpu, true);
 }
 
+#define cpu_llc_shared_mask(cpu)	(&cpu_topology[cpu].llc_siblings)
+void remove_cpu_topology(unsigned int cpu)
+{
+	int sibling;
+
+	for_each_cpu(sibling, topology_core_cpumask(cpu))
+		cpumask_clear_cpu(cpu, topology_core_cpumask(sibling));
+	for_each_cpu(sibling, topology_sibling_cpumask(cpu))
+		cpumask_clear_cpu(cpu, topology_sibling_cpumask(sibling));
+	for_each_cpu(sibling, cpu_llc_shared_mask(cpu))
+		cpumask_clear_cpu(cpu, cpu_llc_shared_mask(sibling));
+
+	clear_cpu_topology(cpu, false);
+}
+
 #ifdef CONFIG_ACPI
 /*
  * Propagate the topology information of the processor_topology_node tree to the

--
2.7.4

#7 lore raw

PATCH v2 3/7 arm64: topology: add support to remove cpu topology sibling masks

Quoted 30 lines · Sudeep Holla

This patch adds support to remove all the CPU topology information using clear_cpu_topology and also resetting the sibling information on other sibling CPUs. This will be used in cpu_disable so that all the topology sibling information is removed on CPU hotplug out.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/include/asm/topology.h |  1 +
 arch/arm64/kernel/topology.c      | 15 +++++++++++++++
 2 files changed, 16 insertions(+)
arch/arm64/include/asm/topology.h +1 -0
index df48212f767b..fb996f454305 100644
@@ -23,6 +23,7 @@ extern struct cpu_topology cpu_topology[NR_CPUS];
 
 void init_cpu_topology(void);
 void store_cpu_topology(unsigned int cpuid);
+void remove_cpu_topology(unsigned int cpuid);
 const struct cpumask *cpu_coregroup_mask(int cpu);
 
 #ifdef CONFIG_NUMA
arch/arm64/kernel/topology.c +0 -0
index 6ea3ec49d418..38b102013708 100644

arch/arm64/kernel/topology.c static void __init reset_cpu_topology(void) · line 325

Quoted 4 lines
arch/arm64/kernel/topology.c +1 -0
@@ -322,6 +322,21 @@ static void __init reset_cpu_topology(void)
 		clear_cpu_topology(cpu, true);
 }
 
+#define cpu_llc_shared_mask(cpu)	(&cpu_topology[cpu].llc_siblings)

Why doesn't this live in topology.h with the other topology masks?

arch/arm64/kernel/topology.c static void __init reset_cpu_topology(void) · line 337

Quoted 12 lines
arch/arm64/kernel/topology.c +12 -0
+void remove_cpu_topology(unsigned int cpu)
+{
+	int sibling;
+
+	for_each_cpu(sibling, topology_core_cpumask(cpu))
+		cpumask_clear_cpu(cpu, topology_core_cpumask(sibling));
+	for_each_cpu(sibling, topology_sibling_cpumask(cpu))
+		cpumask_clear_cpu(cpu, topology_sibling_cpumask(sibling));
+	for_each_cpu(sibling, cpu_llc_shared_mask(cpu))
+		cpumask_clear_cpu(cpu, cpu_llc_shared_mask(sibling));
+
+	clear_cpu_topology(cpu, false);

If there are only two callers of clear_cpu_topology, I'd be inclined to drop the boolean parameter (which is always impossible to read) and just inline the reset code in reset_cpu_topology.

Will

#8 lore raw

PATCH v2 3/7 arm64: topology: add support to remove cpu topology sibling masks

Quoted 34 lines · Will Deacon
Quoted 33 lines · Sudeep Holla

This patch adds support to remove all the CPU topology information using clear_cpu_topology and also resetting the sibling information on other sibling CPUs. This will be used in cpu_disable so that all the topology sibling information is removed on CPU hotplug out.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/include/asm/topology.h |  1 +
 arch/arm64/kernel/topology.c      | 15 +++++++++++++++
 2 files changed, 16 insertions(+)
arch/arm64/include/asm/topology.h +1 -0
index df48212f767b..fb996f454305 100644
@@ -23,6 +23,7 @@ extern struct cpu_topology cpu_topology[NR_CPUS];
 
 void init_cpu_topology(void);
 void store_cpu_topology(unsigned int cpuid);
+void remove_cpu_topology(unsigned int cpuid);
 const struct cpumask *cpu_coregroup_mask(int cpu);
 
 #ifdef CONFIG_NUMA
arch/arm64/kernel/topology.c +0 -0
index 6ea3ec49d418..38b102013708 100644
@@ -322,6 +322,21 @@ static void __init reset_cpu_topology(void)
 		clear_cpu_topology(cpu, true);
 }
 
Quoted 4 lines

arch/arm64/kernel/topology.c static void __init reset_cpu_topology(void) · line 325

Quoted 1 line
arch/arm64/kernel/topology.c +1 -0
+#define cpu_llc_shared_mask(cpu)	(&cpu_topology[cpu].llc_siblings)

Why doesn't this live in topology.h with the other topology masks?

The other macros are aligned with other archs and is used in drivers/base/topology.c which provides sysfs access. This llc mask is not exposed to userspace and hence I thought of containing it along with the sole user. I am fine if you prefer to move, but then may be we need the name to be aligned to topology_llc_shared_mask or something like that.

Quoted 8 lines
Quoted 8 lines
arch/arm64/kernel/topology.c +8 -0
+void remove_cpu_topology(unsigned int cpu)
+{
+	int sibling;
+
+	for_each_cpu(sibling, topology_core_cpumask(cpu))
+		cpumask_clear_cpu(cpu, topology_core_cpumask(sibling));
+	for_each_cpu(sibling, topology_sibling_cpumask(cpu))
+		cpumask_clear_cpu(cpu, topology_sibling_cpumask(sibling));
Quoted 9 lines

arch/arm64/kernel/topology.c static void __init reset_cpu_topology(void) · line 337

Quoted 4 lines
arch/arm64/kernel/topology.c +4 -0
+	for_each_cpu(sibling, cpu_llc_shared_mask(cpu))
+		cpumask_clear_cpu(cpu, cpu_llc_shared_mask(sibling));
+
+	clear_cpu_topology(cpu, false);

If there are only two callers of clear_cpu_topology, I'd be inclined to drop the boolean parameter (which is always impossible to read) and just inline the reset code in reset_cpu_topology.

Sure, now that I look at the code again, that makes it much easier to read. Will do that.

Signature · 2 lines
Regards,
Sudeep
#9 lore raw

PATCH v2 3/7 arm64: topology: add support to remove cpu topology sibling masks

Quoted 42 lines · Sudeep Holla
Quoted 34 lines · Will Deacon
Quoted 33 lines · Sudeep Holla

This patch adds support to remove all the CPU topology information using clear_cpu_topology and also resetting the sibling information on other sibling CPUs. This will be used in cpu_disable so that all the topology sibling information is removed on CPU hotplug out.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/include/asm/topology.h |  1 +
 arch/arm64/kernel/topology.c      | 15 +++++++++++++++
 2 files changed, 16 insertions(+)
arch/arm64/include/asm/topology.h +1 -0
index df48212f767b..fb996f454305 100644
@@ -23,6 +23,7 @@ extern struct cpu_topology cpu_topology[NR_CPUS];
 
 void init_cpu_topology(void);
 void store_cpu_topology(unsigned int cpuid);
+void remove_cpu_topology(unsigned int cpuid);
 const struct cpumask *cpu_coregroup_mask(int cpu);
 
 #ifdef CONFIG_NUMA
arch/arm64/kernel/topology.c +0 -0
index 6ea3ec49d418..38b102013708 100644
@@ -322,6 +322,21 @@ static void __init reset_cpu_topology(void)
 		clear_cpu_topology(cpu, true);
 }
 
Quoted 4 lines

arch/arm64/kernel/topology.c static void __init reset_cpu_topology(void) · line 325

Quoted 1 line
arch/arm64/kernel/topology.c +1 -0
+#define cpu_llc_shared_mask(cpu)	(&cpu_topology[cpu].llc_siblings)

Why doesn't this live in topology.h with the other topology masks?

Quoted 6 lines

The other macros are aligned with other archs and is used in drivers/base/topology.c which provides sysfs access. This llc mask is not exposed to userspace and hence I thought of containing it along with the sole user. I am fine if you prefer to move, but then may be we need the name to be aligned to topology_llc_shared_mask or something like that.

I'm just suggesting to name it topology_llc_cpumask and put it in our asm/topology.h. That doesn't get exposed to userspace afaict (but please shout it I missed something).

Will

#10 lore raw

PATCH v2 3/7 arm64: topology: add support to remove cpu topology sibling masks

Quoted 50 lines · Will Deacon
Quoted 42 lines · Sudeep Holla
Quoted 34 lines · Will Deacon
Quoted 33 lines · Sudeep Holla

This patch adds support to remove all the CPU topology information using clear_cpu_topology and also resetting the sibling information on other sibling CPUs. This will be used in cpu_disable so that all the topology sibling information is removed on CPU hotplug out.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/include/asm/topology.h |  1 +
 arch/arm64/kernel/topology.c      | 15 +++++++++++++++
 2 files changed, 16 insertions(+)
arch/arm64/include/asm/topology.h +1 -0
index df48212f767b..fb996f454305 100644
@@ -23,6 +23,7 @@ extern struct cpu_topology cpu_topology[NR_CPUS];
 
 void init_cpu_topology(void);
 void store_cpu_topology(unsigned int cpuid);
+void remove_cpu_topology(unsigned int cpuid);
 const struct cpumask *cpu_coregroup_mask(int cpu);
 
 #ifdef CONFIG_NUMA
arch/arm64/kernel/topology.c +0 -0
index 6ea3ec49d418..38b102013708 100644
@@ -322,6 +322,21 @@ static void __init reset_cpu_topology(void)
 		clear_cpu_topology(cpu, true);
 }
 
Quoted 4 lines

arch/arm64/kernel/topology.c static void __init reset_cpu_topology(void) · line 325

Quoted 1 line
arch/arm64/kernel/topology.c +1 -0
+#define cpu_llc_shared_mask(cpu)	(&cpu_topology[cpu].llc_siblings)

Why doesn't this live in topology.h with the other topology masks?

Quoted 6 lines

The other macros are aligned with other archs and is used in drivers/base/topology.c which provides sysfs access. This llc mask is not exposed to userspace and hence I thought of containing it along with the sole user. I am fine if you prefer to move, but then may be we need the name to be aligned to topology_llc_shared_mask or something like that.

Quoted 4 lines

I'm just suggesting to name it topology_llc_cpumask and put it in our asm/topology.h. That doesn't get exposed to userspace afaict (but please shout it I missed something).

Nope, that should be fine.

Signature · 2 lines
Regards,
Sudeep
#11 lore raw

PATCH v2 4/7 arm64: topology: restrict updating siblings_masks to online cpus only

In reply to Sudeep Holla

It's incorrect to iterate over all the possible CPUs to update the sibling masks when any CPU is hotplugged in. In case the topology siblings masks of the CPU is removed when is it hotplugged out, we end up updating those masks when one of it's sibling is powered up again. This will provide inconsistent view.

Further, since the CPU calling update_sibling_masks is yet to be set online, there's no need to compare itself with each online CPU when updating the siblings masks.

This patch restricts updation of sibling masks only for CPUs that are already online. It also the drops the unnecessary cpuid check.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/kernel/topology.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
arch/arm64/kernel/topology.c +3 -5
index 38b102013708..b60d7018e9db 100644
@@ -231,7 +231,7 @@ static void update_siblings_masks(unsigned int cpuid)
 	int cpu;
 
 	/* update core and thread sibling masks */
-	for_each_possible_cpu(cpu) {
+	for_each_online_cpu(cpu) {
 		cpu_topo = &cpu_topology[cpu];
 
 		if (cpuid_topo->llc_id == cpu_topo->llc_id) {
@@ -243,15 +243,13 @@ static void update_siblings_masks(unsigned int cpuid)
 			continue;
 
 		cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
-		if (cpu != cpuid)
-			cpumask_set_cpu(cpu, &cpuid_topo->core_sibling);
+		cpumask_set_cpu(cpu, &cpuid_topo->core_sibling);
 
 		if (cpuid_topo->core_id != cpu_topo->core_id)
 			continue;
 
 		cpumask_set_cpu(cpuid, &cpu_topo->thread_sibling);
-		if (cpu != cpuid)
-			cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
+		cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
 	}
 }
 

--
2.7.4

#12 lore raw

PATCH v2 5/7 arm64: smp: remove cpu and numa topology information when hotplugging out CPU

In reply to Sudeep Holla

We already repopulate the information on CPU hotplug-in, so we can safely remove the CPU topology and NUMA cpumap information during CPU hotplug out operation. This will help to provide the correct cpumask for scheduler domains.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/kernel/smp.c | 3 +++
 1 file changed, 3 insertions(+)
arch/arm64/kernel/smp.c +3 -0
index 49a021e30dfb..63a40ba3cd37 100644
@@ -279,6 +279,9 @@ int __cpu_disable(void)
 	if (ret)
 		return ret;
 
+	remove_cpu_topology(cpu);
+	numa_remove_cpu(cpu);
+
 	/*
 	 * Take this CPU offline.  Once we clear this, we can't return,
 	 * and we must not schedule until we're ready to give up the cpu.

--
2.7.4

#13 lore raw

PATCH v2 6/7 arm64: topology: rename llc_siblings to align with other struct members

In reply to Sudeep Holla

Similar to core_sibling and thread_sibling, it's better to align and rename llc_siblings to llc_sibling.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/include/asm/topology.h |  2 +-
 arch/arm64/kernel/topology.c      | 14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)
arch/arm64/include/asm/topology.h +1 -1
index fb996f454305..dda4b6dba6b4 100644
@@ -11,7 +11,7 @@ struct cpu_topology {
 	int llc_id;
 	cpumask_t thread_sibling;
 	cpumask_t core_sibling;
-	cpumask_t llc_siblings;
+	cpumask_t llc_sibling;
 };
 
 extern struct cpu_topology cpu_topology[NR_CPUS];
arch/arm64/kernel/topology.c +7 -7
index b60d7018e9db..e0db80252cfb 100644
@@ -218,8 +218,8 @@ const struct cpumask *cpu_coregroup_mask(int cpu)
 	const cpumask_t *core_mask = &cpu_topology[cpu].core_sibling;
 
 	if (cpu_topology[cpu].llc_id != -1) {
-		if (cpumask_subset(&cpu_topology[cpu].llc_siblings, core_mask))
-			core_mask = &cpu_topology[cpu].llc_siblings;
+		if (cpumask_subset(&cpu_topology[cpu].llc_sibling, core_mask))
+			core_mask = &cpu_topology[cpu].llc_sibling;
 	}
 
 	return core_mask;
@@ -235,8 +235,8 @@ static void update_siblings_masks(unsigned int cpuid)
 		cpu_topo = &cpu_topology[cpu];
 
 		if (cpuid_topo->llc_id == cpu_topo->llc_id) {
-			cpumask_set_cpu(cpu, &cpuid_topo->llc_siblings);
-			cpumask_set_cpu(cpuid, &cpu_topo->llc_siblings);
+			cpumask_set_cpu(cpu, &cpuid_topo->llc_sibling);
+			cpumask_set_cpu(cpuid, &cpu_topo->llc_sibling);
 		}
 
 		if (cpuid_topo->package_id != cpu_topo->package_id)
@@ -303,8 +303,8 @@ static void clear_cpu_topology(int cpu, bool reset)
 		cpu_topo->llc_id = -1;
 	}
 
-	cpumask_clear(&cpu_topo->llc_siblings);
-	cpumask_set_cpu(cpu, &cpu_topo->llc_siblings);
+	cpumask_clear(&cpu_topo->llc_sibling);
+	cpumask_set_cpu(cpu, &cpu_topo->llc_sibling);
 
 	cpumask_clear(&cpu_topo->core_sibling);
 	cpumask_set_cpu(cpu, &cpu_topo->core_sibling);
@@ -320,7 +320,7 @@ static void __init reset_cpu_topology(void)
 		clear_cpu_topology(cpu, true);
 }
 
-#define cpu_llc_shared_mask(cpu)	(&cpu_topology[cpu].llc_siblings)
+#define cpu_llc_shared_mask(cpu)	(&cpu_topology[cpu].llc_sibling)
 void remove_cpu_topology(unsigned int cpu)
 {
 	int sibling;

--
2.7.4

#14 lore raw

PATCH v2 7/7 arm64: topology: re-introduce numa mask check for scheduler MC selection

In reply to Sudeep Holla

Commit 37c3ec2d810f ("arm64: topology: divorce MC scheduling domain from core_siblings") selected the smallest of LLC, socket siblings, and NUMA node siblings to ensure that the sched domain we build for the MC layer isn't larger than the DIE above it or it's shrunk to the socket or NUMA node if LLC exist acrosis NUMA node/chiplets.

Commit acd32e52e4e0 ("arm64: topology: Avoid checking numa mask for scheduler MC selection") reverted the NUMA siblings checks since the CPU topology masks weren't updated on hotplug at that time.

This patch re-introduces numa mask check as the CPU and NUMA topology is now updated in hotplug paths. Effectively, this patch does the partial revert of commit acd32e52e4e0.

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
---
 arch/arm64/kernel/topology.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
arch/arm64/kernel/topology.c +6 -1
index e0db80252cfb..01cdc83b1ec7 100644
@@ -215,8 +215,13 @@ EXPORT_SYMBOL_GPL(cpu_topology);
 
 const struct cpumask *cpu_coregroup_mask(int cpu)
 {
-	const cpumask_t *core_mask = &cpu_topology[cpu].core_sibling;
+	const cpumask_t *core_mask = cpumask_of_node(cpu_to_node(cpu));
 
+	/* Find the smaller of NUMA, core or LLC siblings */
+	if (cpumask_subset(&cpu_topology[cpu].core_sibling, core_mask)) {
+		/* not numa in package, lets use the package siblings */
+		core_mask = &cpu_topology[cpu].core_sibling;
+	}
 	if (cpu_topology[cpu].llc_id != -1) {
 		if (cpumask_subset(&cpu_topology[cpu].llc_sibling, core_mask))
 			core_mask = &cpu_topology[cpu].llc_sibling;

--
2.7.4

#15 lore raw

v2 cover arm64: numa/topology/smp: update the cpumasks for CPU hotplug

In reply to Sudeep Holla

Hi Sudeep,

Quoted 8 lines · Sudeep Holla

Hi,

Commit 37c3ec2d810f ("arm64: topology: divorce MC scheduling domain from core_siblings") was reported to cause some hotplug and system suspend regressions when
it was merged. On investigation, it was found that unlike x86/PPC,
arm64 doesn't update the cpu and numa masks on CPU hotplug. That's somewhat expected from the scheduler.

Quoted 7 lines

Since these changes were bit invasive as a solution to the above mentioned regression, as small change was temporarily applied as a fix. This series updates the cpu and numa masks on CPU hotplug and reverts that temporary fix.

It would be good to get this tested(CPU hotplug - few and all CPUs in a socket) on multi-socket/NUMA systems from Cavium and Huawei/Hisilicon.

Sorry for the late response, tons of other work just distracted me.

I tested this patch set on top of 4.18-rc2, CPU hotplug (online/offline) works, but the "Core(s) per socket:" of lscpu is not right, for example, the system with 4 NUMA nodes and 4 sockets, each socket has 16 CPU cores, from the beginning, everything looks correct:

localhost:~ # lscpu

Architecture:          aarch64
Byte Order:            Little Endian
CPU(s):                64
On-line CPU(s) list:   0-63
Thread(s) per core:    1
Core(s) per socket:    16    // 16 is the right value, and should be unchanged when CPU hotpluging
Socket(s):             4
NUMA node(s):          4
L1d cache:             32K
L1i cache:             48K
L2 cache:              1024K
L3 cache:              16384K
NUMA node0 CPU(s):     0-15
NUMA node1 CPU(s):     16-31
NUMA node2 CPU(s):     32-47
NUMA node3 CPU(s):     48-63

But when I offlined one CPU:
localhost:/sys/devices/system/cpu # echo 0 > cpu22/online localhost:/sys/devices/system/cpu # lscpu

Architecture:          aarch64
Byte Order:            Little Endian
CPU(s):                64
On-line CPU(s) list:   0-21,23-63
Off-line CPU(s) list:  22
Thread(s) per core:    1
Core(s) per socket:    15      //it's 15 now...
Socket(s):             4
NUMA node(s):          4
L1d cache:             32K
L1i cache:             48K
L2 cache:              1024K
L3 cache:              16384K
NUMA node0 CPU(s):     0-15
NUMA node1 CPU(s):     16-21,23-31
NUMA node2 CPU(s):     32-47
NUMA node3 CPU(s):     48-63

Offlined 16 cores on NUMA node 0, everything backs to normal:

Architecture:          aarch64
Byte Order:            Little Endian
CPU(s):                64
On-line CPU(s) list:   16-63
Off-line CPU(s) list:  0-15
Thread(s) per core:    1
Core(s) per socket:    16
Socket(s):             3
NUMA node(s):          4
L1d cache:             32K
L1i cache:             48K
L2 cache:              1024K
L3 cache:              16384K
NUMA node0 CPU(s):
NUMA node1 CPU(s):     16-31
NUMA node2 CPU(s):     32-47
NUMA node3 CPU(s):     48-63

but when onlined CPU15 after,

Architecture:          aarch64
Byte Order:            Little Endian
CPU(s):                64
On-line CPU(s) list:   15-63
Off-line CPU(s) list:  0-14
Thread(s) per core:    1
Core(s) per socket:    12     // it's 12, hmm...
Socket(s):             4
NUMA node(s):          4
L1d cache:             32K
L1i cache:             48K
L2 cache:              1024K
L3 cache:              16384K
NUMA node0 CPU(s):     15
NUMA node1 CPU(s):     16-31
NUMA node2 CPU(s):     32-47
NUMA node3 CPU(s):     48-63

I think maybe the cpumask for the socket is changed in a wrong way when online/offline CPUs, didn't take a deep look into that, hope the test helps.

Thanks
Hanjun

#16 lore raw

v2 cover arm64: numa/topology/smp: update the cpumasks for CPU hotplug

Quoted 47 lines · Hanjun Guo

Hi Sudeep,

Quoted 8 lines · Sudeep Holla

Hi,

Commit 37c3ec2d810f ("arm64: topology: divorce MC scheduling domain from core_siblings") was reported to cause some hotplug and system suspend regressions when
it was merged. On investigation, it was found that unlike x86/PPC,
arm64 doesn't update the cpu and numa masks on CPU hotplug. That's somewhat expected from the scheduler.

Quoted 7 lines

Since these changes were bit invasive as a solution to the above mentioned regression, as small change was temporarily applied as a fix. This series updates the cpu and numa masks on CPU hotplug and reverts that temporary fix.

It would be good to get this tested(CPU hotplug - few and all CPUs in a socket) on multi-socket/NUMA systems from Cavium and Huawei/Hisilicon.

Sorry for the late response, tons of other work just distracted me.

I tested this patch set on top of 4.18-rc2, CPU hotplug (online/offline) works, but the "Core(s) per socket:" of lscpu is not right, for example, the system with 4 NUMA nodes and 4 sockets, each socket has 16 CPU cores, from the beginning, everything looks correct:

localhost:~ # lscpu

Architecture:          aarch64
Byte Order:            Little Endian
CPU(s):                64
On-line CPU(s) list:   0-63
Thread(s) per core:    1
Core(s) per socket:    16    // 16 is the right value, and should be unchanged when CPU hotpluging
Socket(s):             4
NUMA node(s):          4
L1d cache:             32K
L1i cache:             48K
L2 cache:              1024K
L3 cache:              16384K
NUMA node0 CPU(s):     0-15
NUMA node1 CPU(s):     16-31
NUMA node2 CPU(s):     32-47
NUMA node3 CPU(s):     48-63

But when I offlined one CPU:
localhost:/sys/devices/system/cpu # echo 0 > cpu22/online localhost:/sys/devices/system/cpu # lscpu

Quoted 7 lines
Architecture:          aarch64
Byte Order:            Little Endian
CPU(s):                64
On-line CPU(s) list:   0-21,23-63
Off-line CPU(s) list:  22
Thread(s) per core:    1
Core(s) per socket:    15      //it's 15 now...

I know exactly what's the problem. And I think that's exactly the behavior on my x86 box too. Further you might see another issue as most of these applications like lstopo and lscpu parse only CPU0 sysfs, things fall apart if CPU0 is hotplugged out which is not allowed on most of x86 boxes I have tried.

So, for me, that's issue with the application TBH.

[..]

Quoted 1 line

Offlined 16 cores on NUMA node 0, everything backs to normal:

Expected :)

Quoted 8 lines

but when onlined CPU15 after,

Architecture:          aarch64
Byte Order:            Little Endian
CPU(s):                64
On-line CPU(s) list:   15-63
Off-line CPU(s) list:  0-14
Thread(s) per core:    1
Core(s) per socket:    12     // it's 12, hmm...

I had to looks at the lscpu code and I think that explains why you can get 12 here. We have

	add_summary_n(tb, _("Core(s) per socket:"),
		cores_per_socket ?: desc->ncores / desc->nsockets);

Now cores_per_socket because we don't have procfs entry, so ncores=49 and nsockets=4, which means you get 12. TBH lscpu should be used only when all CPUs are online. Lots of inaccurate assumptions in it.

Quoted 5 lines

I think maybe the cpumask for the socket is changed in a wrong way when online/offline CPUs, didn't take a deep look into that, hope the test helps.

I am not convinced if there's any issue unless you see discrepancy in sysfs entries themselves and not though applications interpreting the values for you based on some wrong assumptions.

Signature · 2 lines
Regards,
Sudeep
#17 lore raw

v2 cover arm64: numa/topology/smp: update the cpumasks for CPU hotplug

On 2018/6/26 17:23, Sudeep Holla wrote:
[...]

Quoted 35 lines
Quoted 1 line

Core(s) per socket: 15 //it's 15 now...

I know exactly what's the problem. And I think that's exactly the behavior on my x86 box too. Further you might see another issue as most of these applications like lstopo and lscpu parse only CPU0 sysfs, things fall apart if CPU0 is hotplugged out which is not allowed on most of x86 boxes I have tried.

So, for me, that's issue with the application TBH.

[..]

Quoted 1 line

Offlined 16 cores on NUMA node 0, everything backs to normal:

Expected :)

Quoted 8 lines

but when onlined CPU15 after,

Architecture:          aarch64
Byte Order:            Little Endian
CPU(s):                64
On-line CPU(s) list:   15-63
Off-line CPU(s) list:  0-14
Thread(s) per core:    1
Core(s) per socket:    12     // it's 12, hmm...

I had to looks at the lscpu code and I think that explains why you can get 12 here. We have

	add_summary_n(tb, _("Core(s) per socket:"),
		cores_per_socket ?: desc->ncores / desc->nsockets);

Now cores_per_socket because we don't have procfs entry, so ncores=49 and nsockets=4, which means you get 12. TBH lscpu should be used only when all CPUs are online. Lots of inaccurate assumptions in it.

Quoted 1 line
Quoted 8 lines
Quoted 4 lines

I think maybe the cpumask for the socket is changed in a wrong way when online/offline CPUs, didn't take a deep look into that, hope the test helps.

I am not convinced if there's any issue unless you see discrepancy in sysfs entries themselves and not though applications interpreting the values for you based on some wrong assumptions.

Thanks for the clarify and let me know the detail, I tested 4.17 on a x86 machine which CPU0 can be offlined, also with the same lscpu version, I got the same on x86:

lscpu

Architecture:          x86_64
CPU op-mode(s):        32-bit, 64-bit
Byte Order:            Little Endian
CPU(s):                72
On-line CPU(s) list:   1-9,11-23,25-45,47-65,67-71
Off-line CPU(s) list:  0,10,24,46,66                  // offline CPUs to make no continuous 18 logical CPUs on a socket
Thread(s) per core:    1                              // it's 2 before CPU offlined
Core(s) per socket:    17                             // and not 18 which is the value before CPU offlined
Socket(s):             2
NUMA node(s):          2
Vendor ID:             GenuineIntel
CPU family:            6
Model:                 85
Model name:            Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz
Stepping:              4
CPU MHz:               2999.755
CPU max MHz:           3700.0000
CPU min MHz:           1000.0000
BogoMIPS:              4600.00
Virtualization:        VT-x
L1d cache:             32K
L1i cache:             32K
L2 cache:              1024K
L3 cache:              25344K
NUMA node0 CPU(s):     1-9,11-17,36-45,47-53
NUMA node1 CPU(s):     18-23,25-35,54-65,67-71

So with this patch set:

Thanks
Hanjun

#18 lore raw

v2 cover arm64: numa/topology/smp: update the cpumasks for CPU hotplug

Hi Hanjun,

Quoted 42 lines · Hanjun Guo

On 2018/6/26 17:23, Sudeep Holla wrote:
[...]

Quoted 40 lines
Quoted 1 line

Core(s) per socket: 15 //it's 15 now...

I know exactly what's the problem. And I think that's exactly the behavior on my x86 box too. Further you might see another issue as most of these applications like lstopo and lscpu parse only CPU0 sysfs, things fall apart if CPU0 is hotplugged out which is not allowed on most of x86 boxes I have tried.

So, for me, that's issue with the application TBH.

[..]

Quoted 1 line

Offlined 16 cores on NUMA node 0, everything backs to normal:

Expected :)

Quoted 8 lines

but when onlined CPU15 after,

Architecture:          aarch64
Byte Order:            Little Endian
CPU(s):                64
On-line CPU(s) list:   15-63
Off-line CPU(s) list:  0-14
Thread(s) per core:    1
Core(s) per socket:    12     // it's 12, hmm...

I had to looks at the lscpu code and I think that explains why you can get 12 here. We have

	add_summary_n(tb, _("Core(s) per socket:"),
		cores_per_socket ?: desc->ncores / desc->nsockets);

Now cores_per_socket because we don't have procfs entry, so ncores=49 and nsockets=4, which means you get 12. TBH lscpu should be used only when all CPUs are online. Lots of inaccurate assumptions in it.

Quoted 5 lines

I think maybe the cpumask for the socket is changed in a wrong way when online/offline CPUs, didn't take a deep look into that, hope the test helps.

Quoted 8 lines
Quoted 3 lines

I am not convinced if there's any issue unless you see discrepancy in sysfs entries themselves and not though applications interpreting the values for you based on some wrong assumptions.

Thanks for the clarify and let me know the detail, I tested 4.17 on a x86 machine which CPU0 can be offlined, also with the same lscpu version, I got the same on x86:

Thanks for taking trouble and confirming this.

Quoted 28 lines

lscpu

Architecture:          x86_64
CPU op-mode(s):        32-bit, 64-bit
Byte Order:            Little Endian
CPU(s):                72
On-line CPU(s) list:   1-9,11-23,25-45,47-65,67-71
Off-line CPU(s) list:  0,10,24,46,66                  // offline CPUs to make no continuous 18 logical CPUs on a socket
Thread(s) per core:    1                              // it's 2 before CPU offlined
Core(s) per socket:    17                             // and not 18 which is the value before CPU offlined
Socket(s):             2
NUMA node(s):          2
Vendor ID:             GenuineIntel
CPU family:            6
Model:                 85
Model name:            Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz
Stepping:              4
CPU MHz:               2999.755
CPU max MHz:           3700.0000
CPU min MHz:           1000.0000
BogoMIPS:              4600.00
Virtualization:        VT-x
L1d cache:             32K
L1i cache:             32K
L2 cache:              1024K
L3 cache:              25344K
NUMA node0 CPU(s):     1-9,11-17,36-45,47-53
NUMA node1 CPU(s):     18-23,25-35,54-65,67-71
Quoted 4 lines

So with this patch set:

Thanks for testing.

Signature · 2 lines
Regards,
Sudeep
#19 lore raw

v2 cover arm64: numa/topology/smp: update the cpumasks for CPU hotplug

In reply to Sudeep Holla

Hi Sudeep,

Quoted 9 lines · Sudeep Holla

Hi,

Commit 37c3ec2d810f ("arm64: topology: divorce MC scheduling domain from core_siblings") was reported to cause some hotplug and system suspend regressions when
it was merged. On investigation, it was found that unlike x86/PPC,
arm64 doesn't update the cpu and numa masks on CPU hotplug. That's somewhat expected from the scheduler.

Quoted 7 lines

Since these changes were bit invasive as a solution to the above mentioned regression, as small change was temporarily applied as a fix. This series updates the cpu and numa masks on CPU hotplug and reverts that temporary fix.

It would be good to get this tested(CPU hotplug - few and all CPUs in a socket) on multi-socket/NUMA systems from Cavium and Huawei/Hisilicon.

testing looks ok on Cavium's ThunderX2 system. please feel free to add,

root at B1SABER_2>sudeep>> lscpu

Architecture:          aarch64
Byte Order:            Little Endian
CPU(s):                224
On-line CPU(s) list:   0-223
Thread(s) per core:    4
Core(s) per socket:    28
Socket(s):             2
NUMA node(s):          2
L1d cache:             32K
L1i cache:             32K
L2 cache:              256K
L3 cache:              32768K
NUMA node0 CPU(s):     0-111
NUMA node1 CPU(s):     112-223
root at B1SABER_2>sudeep>> sh hp.sh 0 111 0
root at B1SABER_2>sudeep>> lscpu
Architecture:          aarch64
Byte Order:            Little Endian
On-line CPU(s) list:   112-223
Off-line CPU(s) list:  0-111
Thread(s) per core:    4
Core(s) per socket:    28
Socket(s):             1
NUMA node(s):          2
L1d cache:             32K
L1i cache:             32K
L2 cache:              256K
L3 cache:              32768K

NUMA node0 CPU(s):
NUMA node1 CPU(s): 112-223
root at B1SABER_2>sudeep>> sh hp.sh 0 111 1 root at B1SABER_2>sudeep>> lscpu

Architecture:          aarch64
Byte Order:            Little Endian
CPU(s):                224
On-line CPU(s) list:   0-223
Thread(s) per core:    4
Core(s) per socket:    28
Socket(s):             2
NUMA node(s):          2
L1d cache:             32K
L1i cache:             32K
L2 cache:              256K
L3 cache:              32768K
NUMA node0 CPU(s):     0-111
NUMA node1 CPU(s):     112-223

root at B1SABER_2>sudeep>>

Quoted 25 lines

Regards,
Sudeep

v1->v2:
- Rebased on v4.18-rc1 and hence do revert of the temporary fix that was merged for v4.18
- Removed one of the wrong use of possible_mask

Sudeep Holla (7):
arm64: topology: refactor reset_cpu_topology to add support for removing topology
arm64: numa: separate out updates to percpu nodeid and NUMA node cpumap
arm64: topology: add support to remove cpu topology sibling masks
arm64: topology: restrict updating siblings_masks to online cpus only
arm64: smp: remove cpu and numa topology information when hotplugging out CPU
arm64: topology: rename llc_siblings to align with other struct members
arm64: topology: re-introduce numa mask check for scheduler MC selection

  arch/arm64/include/asm/numa.h     |  4 +++
  arch/arm64/include/asm/topology.h |  3 +-
  arch/arm64/kernel/smp.c           |  5 +++
  arch/arm64/kernel/topology.c      | 69 ++++++++++++++++++++++++++-------------
  arch/arm64/mm/numa.c              | 29 +++++++++++-----
  5 files changed, 79 insertions(+), 31 deletions(-)
Quoted 3 lines

--
2.7.4

thanks
Ganapat

#20 lore raw

v2 cover arm64: numa/topology/smp: update the cpumasks for CPU hotplug

Quoted 19 lines · Ganapatrao Kulkarni

Hi Sudeep,

Quoted 15 lines · Sudeep Holla

Hi,

Commit 37c3ec2d810f ("arm64: topology: divorce MC scheduling domain from core_siblings")
was reported to cause some hotplug and system suspend regressions when it was merged. On investigation, it was found that unlike x86/PPC, arm64 doesn't update the cpu and numa masks on CPU hotplug. That's somewhat expected from the scheduler.

Since these changes were bit invasive as a solution to the above mentioned regression, as small change was temporarily applied as a fix. This series updates the cpu and numa masks on CPU hotplug and reverts that temporary fix.

Quoted 8 lines
Quoted 2 lines

It would be good to get this tested(CPU hotplug - few and all CPUs in a socket) on multi-socket/NUMA systems from Cavium and Huawei/Hisilicon.

testing looks ok on Cavium's ThunderX2 system.
please feel free to add,

Thanks Ganapat.

Signature · 2 lines
Regards,
Sudeep
#21 lore raw

v2 cover arm64: numa/topology/smp: update the cpumasks for CPU hotplug

In reply to Sudeep Holla

Hi Sudeep,

Quoted 5 lines · Sudeep Holla

was reported to cause some hotplug and system suspend regressions when it was merged. On investigation, it was found that unlike x86/PPC, arm64 doesn't update the cpu and numa masks on CPU hotplug. That's somewhat expected from the scheduler.

Quoted 7 lines

Since these changes were bit invasive as a solution to the above mentioned regression, as small change was temporarily applied as a fix. This series updates the cpu and numa masks on CPU hotplug and reverts that temporary fix.

It would be good to get this tested(CPU hotplug - few and all CPUs in a socket) on multi-socket/NUMA systems from Cavium and Huawei/Hisilicon.

This is looking pretty good to me. I've made some minor comments on a couple of the patches, so please address those and repost with the Tested-bys that you received, then I can queue this for 4.19.

Thanks,

Will

#22 lore raw

v2 cover arm64: numa/topology/smp: update the cpumasks for CPU hotplug

Quoted 13 lines · Will Deacon

Hi Sudeep,

Quoted 10 lines · Sudeep Holla

was reported to cause some hotplug and system suspend regressions when it was merged. On investigation, it was found that unlike x86/PPC, arm64 doesn't update the cpu and numa masks on CPU hotplug. That's somewhat expected from the scheduler.

Since these changes were bit invasive as a solution to the above mentioned regression, as small change was temporarily applied as a fix. This series updates the cpu and numa masks on CPU hotplug and reverts that temporary fix.

Quoted 7 lines
Quoted 2 lines

It would be good to get this tested(CPU hotplug - few and all CPUs in a socket) on multi-socket/NUMA systems from Cavium and Huawei/Hisilicon.

This is looking pretty good to me. I've made some minor comments on a couple of the patches, so please address those and repost with the Tested-bys that you received, then I can queue this for 4.19.

Sure, will do so. Thanks Will.

Signature · 2 lines
Regards,
Sudeep