[PATCH v2 0/7] arm64: numa/topology/smp: update the cpumasks for CPU hotplug

[PATCH v2 0/7] arm64: numa/topology/smp: update the cpumasks for CPU hotplug

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

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

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

[Sudeep Holla]

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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/topology.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index f845a8617812..6ea3ec49d418 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -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

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

[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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 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(-)

diff --git a/arch/arm64/include/asm/numa.h b/arch/arm64/include/asm/numa.h
index 01bc46d5b43a..626ad01e83bf 100644
--- a/arch/arm64/include/asm/numa.h
+++ b/arch/arm64/include/asm/numa.h
@@ -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) { }

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index f3e2e3aec0b0..49a021e30dfb 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -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
diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
index dad128ba98bf..43cc669bc7bc 100644
--- a/arch/arm64/mm/numa.c
+++ b/arch/arm64/mm/numa.c
@@ -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

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

[Ganapatrao Kulkarni]

On 06/18/2018 06:48 PM, Sudeep Holla wrote:
>
> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   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(-)
>
> diff --git a/arch/arm64/include/asm/numa.h b/arch/arm64/include/asm/numa.h
> index 01bc46d5b43a..626ad01e83bf 100644
> --- a/arch/arm64/include/asm/numa.h
> +++ b/arch/arm64/include/asm/numa.h
> @@ -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) { }
>
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index f3e2e3aec0b0..49a021e30dfb 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -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
> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
> index dad128ba98bf..43cc669bc7bc 100644
> --- a/arch/arm64/mm/numa.c
> +++ b/arch/arm64/mm/numa.c
> @@ -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
>

changes looks ok to me.
Reviewed-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>

thanks
Ganapat

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

[Will Deacon]

On Mon, Jun 18, 2018 at 02:18:38PM +0100, Sudeep Holla wrote:
> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  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

> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
> index dad128ba98bf..43cc669bc7bc 100644
> --- a/arch/arm64/mm/numa.c
> +++ b/arch/arm64/mm/numa.c
> @@ -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;

This could be a check against NUMA_NO_NODE, right?

Will

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

[Sudeep Holla]

On 04/07/18 14:52, Will Deacon wrote:
> On Mon, Jun 18, 2018 at 02:18:38PM +0100, Sudeep Holla wrote:
>> 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 <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  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
> 
>> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
>> index dad128ba98bf..43cc669bc7bc 100644
>> --- a/arch/arm64/mm/numa.c
>> +++ b/arch/arm64/mm/numa.c
>> @@ -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;
> 
> This could be a check against NUMA_NO_NODE, right?
> 

Indeed.

-- 
Regards,
Sudeep

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

[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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/include/asm/topology.h |  1 +
 arch/arm64/kernel/topology.c      | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index df48212f767b..fb996f454305 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -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
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 6ea3ec49d418..38b102013708 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -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

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

[Will Deacon]

On Mon, Jun 18, 2018 at 02:18:39PM +0100, Sudeep Holla wrote:
> 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 <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm64/include/asm/topology.h |  1 +
>  arch/arm64/kernel/topology.c      | 15 +++++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> index df48212f767b..fb996f454305 100644
> --- a/arch/arm64/include/asm/topology.h
> +++ b/arch/arm64/include/asm/topology.h
> @@ -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
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 6ea3ec49d418..38b102013708 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -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?

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

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

[Sudeep Holla]

On 04/07/18 14:58, Will Deacon wrote:
> On Mon, Jun 18, 2018 at 02:18:39PM +0100, Sudeep Holla wrote:
>> 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 <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  arch/arm64/include/asm/topology.h |  1 +
>>  arch/arm64/kernel/topology.c      | 15 +++++++++++++++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
>> index df48212f767b..fb996f454305 100644
>> --- a/arch/arm64/include/asm/topology.h
>> +++ b/arch/arm64/include/asm/topology.h
>> @@ -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
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 6ea3ec49d418..38b102013708 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -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?
> 

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.

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

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

-- 
Regards,
Sudeep

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

[Will Deacon]

On Wed, Jul 04, 2018 at 03:11:06PM +0100, Sudeep Holla wrote:
> 
> 
> On 04/07/18 14:58, Will Deacon wrote:
> > On Mon, Jun 18, 2018 at 02:18:39PM +0100, Sudeep Holla wrote:
> >> 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 <catalin.marinas@arm.com>
> >> Cc: Will Deacon <will.deacon@arm.com>
> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >> ---
> >>  arch/arm64/include/asm/topology.h |  1 +
> >>  arch/arm64/kernel/topology.c      | 15 +++++++++++++++
> >>  2 files changed, 16 insertions(+)
> >>
> >> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> >> index df48212f767b..fb996f454305 100644
> >> --- a/arch/arm64/include/asm/topology.h
> >> +++ b/arch/arm64/include/asm/topology.h
> >> @@ -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
> >> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> >> index 6ea3ec49d418..38b102013708 100644
> >> --- a/arch/arm64/kernel/topology.c
> >> +++ b/arch/arm64/kernel/topology.c
> >> @@ -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?
> > 
> 
> 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

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

[Sudeep Holla]

On 04/07/18 15:27, Will Deacon wrote:
> On Wed, Jul 04, 2018 at 03:11:06PM +0100, Sudeep Holla wrote:
>>
>>
>> On 04/07/18 14:58, Will Deacon wrote:
>>> On Mon, Jun 18, 2018 at 02:18:39PM +0100, Sudeep Holla wrote:
>>>> 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 <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>> ---
>>>>  arch/arm64/include/asm/topology.h |  1 +
>>>>  arch/arm64/kernel/topology.c      | 15 +++++++++++++++
>>>>  2 files changed, 16 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
>>>> index df48212f767b..fb996f454305 100644
>>>> --- a/arch/arm64/include/asm/topology.h
>>>> +++ b/arch/arm64/include/asm/topology.h
>>>> @@ -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
>>>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>>>> index 6ea3ec49d418..38b102013708 100644
>>>> --- a/arch/arm64/kernel/topology.c
>>>> +++ b/arch/arm64/kernel/topology.c
>>>> @@ -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?
>>>
>>
>> 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).
> 

Nope, that should be fine.

-- 
Regards,
Sudeep

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

[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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/topology.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 38b102013708..b60d7018e9db 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -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

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

[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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/smp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 49a021e30dfb..63a40ba3cd37 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -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

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

[Sudeep Holla]

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

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/include/asm/topology.h |  2 +-
 arch/arm64/kernel/topology.c      | 14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index fb996f454305..dda4b6dba6b4 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -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];
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index b60d7018e9db..e0db80252cfb 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -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

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

[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 <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/topology.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index e0db80252cfb..01cdc83b1ec7 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -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

[PATCH v2 0/7] arm64: numa/topology/smp: update the cpumasks for CPU hotplug

[Hanjun Guo]

Hi Sudeep,

On 2018/6/18 21:18, Sudeep Holla wrote:
> 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.

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

[PATCH v2 0/7] arm64: numa/topology/smp: update the cpumasks for CPU hotplug

[Sudeep Holla]

On 26/06/18 07:50, Hanjun Guo wrote:
> Hi Sudeep,
> 
> On 2018/6/18 21:18, Sudeep Holla wrote:
>> 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.
> 
> 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...

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.

[..]

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

Expected :)

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

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

-- 
Regards,
Sudeep

[PATCH v2 0/7] arm64: numa/topology/smp: update the cpumasks for CPU hotplug

[Hanjun Guo]

On 2018/6/26 17:23, Sudeep Holla wrote:
[...]
>> 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.
> 
> [..]
> 
>> Offlined 16 cores on NUMA node 0, everything backs to normal:
> 
> Expected :)
> 
>> 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.
> 
>>
>> 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:

Tested-by: Hanjun Guo <hanjun.guo@linaro.org>

Thanks
Hanjun

[PATCH v2 0/7] arm64: numa/topology/smp: update the cpumasks for CPU hotplug

[Sudeep Holla]

Hi Hanjun,

On 27/06/18 04:51, Hanjun Guo wrote:
> On 2018/6/26 17:23, Sudeep Holla wrote:
> [...]
>>> 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.
>>
>> [..]
>>
>>> Offlined 16 cores on NUMA node 0, everything backs to normal:
>>
>> Expected :)
>>
>>> 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.
>>
>>>
>>> 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:
> 

Thanks for taking trouble and confirming this.

> 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:
> 
> Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
> 

Thanks for testing.

-- 
Regards,
Sudeep

[PATCH v2 0/7] arm64: numa/topology/smp: update the cpumasks for CPU hotplug

[Ganapatrao Kulkarni]

Hi Sudeep,

On 06/18/2018 06:48 PM, Sudeep Holla wrote:
>
> 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.

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

Tested-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>

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

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

thanks
Ganapat

[PATCH v2 0/7] arm64: numa/topology/smp: update the cpumasks for CPU hotplug

[Sudeep Holla]

On 27/06/18 06:35, Ganapatrao Kulkarni wrote:
> 
> Hi Sudeep,
> 
> On 06/18/2018 06:48 PM, Sudeep Holla wrote:
>>
>> 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.
> 
> testing looks ok on Cavium's ThunderX2 system.
> please feel free to add,
> 
> Tested-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
> 

Thanks Ganapat.

-- 
Regards,
Sudeep

[PATCH v2 0/7] arm64: numa/topology/smp: update the cpumasks for CPU hotplug

[Will Deacon]

Hi Sudeep,

On Mon, Jun 18, 2018 at 02:18:36PM +0100, Sudeep Holla wrote:
> 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.

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

[PATCH v2 0/7] arm64: numa/topology/smp: update the cpumasks for CPU hotplug

[Sudeep Holla]

On 04/07/18 15:00, Will Deacon wrote:
> Hi Sudeep,
> 
> On Mon, Jun 18, 2018 at 02:18:36PM +0100, Sudeep Holla wrote:
>> 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.
> 
> 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.

-- 
Regards,
Sudeep