* [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
@ 2019-11-20 10:42 ` Dietmar Eggemann
0 siblings, 0 replies; 16+ messages in thread
From: Dietmar Eggemann @ 2019-11-20 10:42 UTC (permalink / raw)
To: Atish Patra, Russell King
Cc: linux-arm-kernel, linux-kernel, Morten Rasmussen, Lukasz Luba,
Sudeep Holla
Commit ca74b316df96 ("arm: Use common cpu_topology structure and
functions.") changed cpu_coregroup_mask() from the ARM32 specific
implementation in arch/arm/include/asm/topology.h to the one shared
with ARM64 and RISCV in drivers/base/arch_topology.c.
Currently on Arm32 (TC2 w/ CONFIG_SCHED_MC) the task scheduler setup
code (w/ CONFIG_SCHED_DEBUG) shows this during CPU hotplug:
ERROR: groups don't span domain->span
It happens to CPUs of the cluster of the CPU which gets hot-plugged
out on scheduler domain MC.
Turns out that the shared cpu_coregroup_mask() requires that the
hot-plugged CPU is removed from the core_sibling mask via
remove_cpu_topology(). Otherwise the 'is core_sibling subset of
cpumask_of_node()' doesn't work. In this case the task scheduler has to
deal with cpumask_of_node instead of core_sibling which is wrong on
scheduler domain MC.
e.g. CPU3 hot-plugged out on TC2 [cluster0: 0,3-4 cluster1: 1-2]:
cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,3-4
^
should be:
cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,4
Add remove_cpu_topology() to __cpu_disable() to remove the CPU from the
topology masks in case of a CPU hotplug out operation.
At the same time tweak store_cpu_topology() slightly so it will call
update_siblings_masks() in case of CPU hotplug in operation via
secondary_start_kernel()->smp_store_cpu_info().
This aligns the Arm32 implementation with the Arm64 one.
Fixes: ca74b316df96 ("arm: Use common cpu_topology structure and functions")
Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---
arch/arm/kernel/smp.c | 2 ++
arch/arm/kernel/topology.c | 15 +++++++--------
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 4b0bab2607e4..139c0d98fa29 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -240,6 +240,8 @@ int __cpu_disable(void)
if (ret)
return ret;
+ remove_cpu_topology(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.
diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index 5b9faba03afb..b37b0a340991 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -196,9 +196,8 @@ void store_cpu_topology(unsigned int cpuid)
struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
unsigned int mpidr;
- /* If the cpu topology has been already set, just return */
- if (cpuid_topo->core_id != -1)
- return;
+ if (cpuid_topo->package_id != -1)
+ goto topology_populated;
mpidr = read_cpuid_mpidr();
@@ -231,14 +230,14 @@ void store_cpu_topology(unsigned int cpuid)
cpuid_topo->package_id = -1;
}
- update_siblings_masks(cpuid);
-
update_cpu_capacity(cpuid);
pr_info("CPU%u: thread %d, cpu %d, socket %d, mpidr %x\n",
- cpuid, cpu_topology[cpuid].thread_id,
- cpu_topology[cpuid].core_id,
- cpu_topology[cpuid].package_id, mpidr);
+ cpuid, cpuid_topo->thread_id, cpuid_topo->core_id,
+ cpuid_topo->package_id, mpidr);
+
+topology_populated:
+ update_siblings_masks(cpuid);
}
static inline int cpu_corepower_flags(void)
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
2019-11-20 10:42 ` Dietmar Eggemann
@ 2019-11-20 11:09 ` Sudeep Holla
-1 siblings, 0 replies; 16+ messages in thread
From: Sudeep Holla @ 2019-11-20 11:09 UTC (permalink / raw)
To: Dietmar Eggemann
Cc: Atish Patra, Russell King, Morten Rasmussen, Lukasz Luba,
linux-kernel, linux-arm-kernel, Sudeep Holla
Hi Dietmar, Lukasz,
Thanks for digging this bug and fixing it.
On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
> Commit ca74b316df96 ("arm: Use common cpu_topology structure and
> functions.") changed cpu_coregroup_mask() from the ARM32 specific
> implementation in arch/arm/include/asm/topology.h to the one shared
> with ARM64 and RISCV in drivers/base/arch_topology.c.
>
> Currently on Arm32 (TC2 w/ CONFIG_SCHED_MC) the task scheduler setup
> code (w/ CONFIG_SCHED_DEBUG) shows this during CPU hotplug:
>
> ERROR: groups don't span domain->span
>
> It happens to CPUs of the cluster of the CPU which gets hot-plugged
> out on scheduler domain MC.
>
> Turns out that the shared cpu_coregroup_mask() requires that the
> hot-plugged CPU is removed from the core_sibling mask via
> remove_cpu_topology(). Otherwise the 'is core_sibling subset of
> cpumask_of_node()' doesn't work. In this case the task scheduler has to
> deal with cpumask_of_node instead of core_sibling which is wrong on
> scheduler domain MC.
>
> e.g. CPU3 hot-plugged out on TC2 [cluster0: 0,3-4 cluster1: 1-2]:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,3-4
> ^
> should be:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,4
>
> Add remove_cpu_topology() to __cpu_disable() to remove the CPU from the
> topology masks in case of a CPU hotplug out operation.
>
> At the same time tweak store_cpu_topology() slightly so it will call
> update_siblings_masks() in case of CPU hotplug in operation via
> secondary_start_kernel()->smp_store_cpu_info().
>
> This aligns the Arm32 implementation with the Arm64 one.
>
> Fixes: ca74b316df96 ("arm: Use common cpu_topology structure and functions")
Apart from the minor nit below,
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
> arch/arm/kernel/smp.c | 2 ++
> arch/arm/kernel/topology.c | 15 +++++++--------
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 4b0bab2607e4..139c0d98fa29 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -240,6 +240,8 @@ int __cpu_disable(void)
> if (ret)
> return ret;
>
> + remove_cpu_topology(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.
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index 5b9faba03afb..b37b0a340991 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -196,9 +196,8 @@ void store_cpu_topology(unsigned int cpuid)
> struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> unsigned int mpidr;
>
> - /* If the cpu topology has been already set, just return */
> - if (cpuid_topo->core_id != -1)
> - return;
> + if (cpuid_topo->package_id != -1)
> + goto topology_populated;
>
> mpidr = read_cpuid_mpidr();
>
> @@ -231,14 +230,14 @@ void store_cpu_topology(unsigned int cpuid)
> cpuid_topo->package_id = -1;
> }
>
> - update_siblings_masks(cpuid);
> -
> update_cpu_capacity(cpuid);
>
> pr_info("CPU%u: thread %d, cpu %d, socket %d, mpidr %x\n",
> - cpuid, cpu_topology[cpuid].thread_id,
> - cpu_topology[cpuid].core_id,
> - cpu_topology[cpuid].package_id, mpidr);
> + cpuid, cpuid_topo->thread_id, cpuid_topo->core_id,
> + cpuid_topo->package_id, mpidr);
> +
[nit] The above is a clearly cosmetic cleanup and shouldn't be part of
this fix as they will be backported automatically. So I prefer to drop
this or make it separate patch if required.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
@ 2019-11-20 11:09 ` Sudeep Holla
0 siblings, 0 replies; 16+ messages in thread
From: Sudeep Holla @ 2019-11-20 11:09 UTC (permalink / raw)
To: Dietmar Eggemann
Cc: linux-kernel, Russell King, Atish Patra, Lukasz Luba,
Sudeep Holla, Morten Rasmussen, linux-arm-kernel
Hi Dietmar, Lukasz,
Thanks for digging this bug and fixing it.
On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
> Commit ca74b316df96 ("arm: Use common cpu_topology structure and
> functions.") changed cpu_coregroup_mask() from the ARM32 specific
> implementation in arch/arm/include/asm/topology.h to the one shared
> with ARM64 and RISCV in drivers/base/arch_topology.c.
>
> Currently on Arm32 (TC2 w/ CONFIG_SCHED_MC) the task scheduler setup
> code (w/ CONFIG_SCHED_DEBUG) shows this during CPU hotplug:
>
> ERROR: groups don't span domain->span
>
> It happens to CPUs of the cluster of the CPU which gets hot-plugged
> out on scheduler domain MC.
>
> Turns out that the shared cpu_coregroup_mask() requires that the
> hot-plugged CPU is removed from the core_sibling mask via
> remove_cpu_topology(). Otherwise the 'is core_sibling subset of
> cpumask_of_node()' doesn't work. In this case the task scheduler has to
> deal with cpumask_of_node instead of core_sibling which is wrong on
> scheduler domain MC.
>
> e.g. CPU3 hot-plugged out on TC2 [cluster0: 0,3-4 cluster1: 1-2]:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,3-4
> ^
> should be:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,4
>
> Add remove_cpu_topology() to __cpu_disable() to remove the CPU from the
> topology masks in case of a CPU hotplug out operation.
>
> At the same time tweak store_cpu_topology() slightly so it will call
> update_siblings_masks() in case of CPU hotplug in operation via
> secondary_start_kernel()->smp_store_cpu_info().
>
> This aligns the Arm32 implementation with the Arm64 one.
>
> Fixes: ca74b316df96 ("arm: Use common cpu_topology structure and functions")
Apart from the minor nit below,
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
> arch/arm/kernel/smp.c | 2 ++
> arch/arm/kernel/topology.c | 15 +++++++--------
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 4b0bab2607e4..139c0d98fa29 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -240,6 +240,8 @@ int __cpu_disable(void)
> if (ret)
> return ret;
>
> + remove_cpu_topology(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.
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index 5b9faba03afb..b37b0a340991 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -196,9 +196,8 @@ void store_cpu_topology(unsigned int cpuid)
> struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> unsigned int mpidr;
>
> - /* If the cpu topology has been already set, just return */
> - if (cpuid_topo->core_id != -1)
> - return;
> + if (cpuid_topo->package_id != -1)
> + goto topology_populated;
>
> mpidr = read_cpuid_mpidr();
>
> @@ -231,14 +230,14 @@ void store_cpu_topology(unsigned int cpuid)
> cpuid_topo->package_id = -1;
> }
>
> - update_siblings_masks(cpuid);
> -
> update_cpu_capacity(cpuid);
>
> pr_info("CPU%u: thread %d, cpu %d, socket %d, mpidr %x\n",
> - cpuid, cpu_topology[cpuid].thread_id,
> - cpu_topology[cpuid].core_id,
> - cpu_topology[cpuid].package_id, mpidr);
> + cpuid, cpuid_topo->thread_id, cpuid_topo->core_id,
> + cpuid_topo->package_id, mpidr);
> +
[nit] The above is a clearly cosmetic cleanup and shouldn't be part of
this fix as they will be backported automatically. So I prefer to drop
this or make it separate patch if required.
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
2019-11-20 11:09 ` Sudeep Holla
@ 2019-11-20 12:30 ` Dietmar Eggemann
-1 siblings, 0 replies; 16+ messages in thread
From: Dietmar Eggemann @ 2019-11-20 12:30 UTC (permalink / raw)
To: Sudeep Holla
Cc: Atish Patra, Russell King, Morten Rasmussen, Lukasz Luba,
linux-kernel, linux-arm-kernel
On 20.11.19 11:09, Sudeep Holla wrote:
> Hi Dietmar, Lukasz,
>
> Thanks for digging this bug and fixing it.
>
> On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
[...]
>> @@ -231,14 +230,14 @@ void store_cpu_topology(unsigned int cpuid)
>> cpuid_topo->package_id = -1;
>> }
>>
>> - update_siblings_masks(cpuid);
>> -
>> update_cpu_capacity(cpuid);
>>
>> pr_info("CPU%u: thread %d, cpu %d, socket %d, mpidr %x\n",
>> - cpuid, cpu_topology[cpuid].thread_id,
>> - cpu_topology[cpuid].core_id,
>> - cpu_topology[cpuid].package_id, mpidr);
>> + cpuid, cpuid_topo->thread_id, cpuid_topo->core_id,
>> + cpuid_topo->package_id, mpidr);
>> +
>
> [nit] The above is a clearly cosmetic cleanup and shouldn't be part of
> this fix as they will be backported automatically. So I prefer to drop
> this or make it separate patch if required.
I can get rid of this cleanup in v2. Thanks for the review!
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
@ 2019-11-20 12:30 ` Dietmar Eggemann
0 siblings, 0 replies; 16+ messages in thread
From: Dietmar Eggemann @ 2019-11-20 12:30 UTC (permalink / raw)
To: Sudeep Holla
Cc: linux-kernel, Russell King, Atish Patra, Lukasz Luba,
Morten Rasmussen, linux-arm-kernel
On 20.11.19 11:09, Sudeep Holla wrote:
> Hi Dietmar, Lukasz,
>
> Thanks for digging this bug and fixing it.
>
> On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
[...]
>> @@ -231,14 +230,14 @@ void store_cpu_topology(unsigned int cpuid)
>> cpuid_topo->package_id = -1;
>> }
>>
>> - update_siblings_masks(cpuid);
>> -
>> update_cpu_capacity(cpuid);
>>
>> pr_info("CPU%u: thread %d, cpu %d, socket %d, mpidr %x\n",
>> - cpuid, cpu_topology[cpuid].thread_id,
>> - cpu_topology[cpuid].core_id,
>> - cpu_topology[cpuid].package_id, mpidr);
>> + cpuid, cpuid_topo->thread_id, cpuid_topo->core_id,
>> + cpuid_topo->package_id, mpidr);
>> +
>
> [nit] The above is a clearly cosmetic cleanup and shouldn't be part of
> this fix as they will be backported automatically. So I prefer to drop
> this or make it separate patch if required.
I can get rid of this cleanup in v2. Thanks for the review!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
2019-11-20 10:42 ` Dietmar Eggemann
@ 2019-11-20 11:16 ` Lukasz Luba
-1 siblings, 0 replies; 16+ messages in thread
From: Lukasz Luba @ 2019-11-20 11:16 UTC (permalink / raw)
To: Dietmar Eggemann, Atish Patra, Russell King
Cc: Sudeep Holla, Morten Rasmussen, linux-kernel, linux-arm-kernel
Hi Dietmar,
On 11/20/19 10:42 AM, Dietmar Eggemann wrote:
> Commit ca74b316df96 ("arm: Use common cpu_topology structure and
> functions.") changed cpu_coregroup_mask() from the ARM32 specific
> implementation in arch/arm/include/asm/topology.h to the one shared
> with ARM64 and RISCV in drivers/base/arch_topology.c.
>
> Currently on Arm32 (TC2 w/ CONFIG_SCHED_MC) the task scheduler setup
> code (w/ CONFIG_SCHED_DEBUG) shows this during CPU hotplug:
>
> ERROR: groups don't span domain->span
>
> It happens to CPUs of the cluster of the CPU which gets hot-plugged
> out on scheduler domain MC.
>
> Turns out that the shared cpu_coregroup_mask() requires that the
> hot-plugged CPU is removed from the core_sibling mask via
> remove_cpu_topology(). Otherwise the 'is core_sibling subset of
> cpumask_of_node()' doesn't work. In this case the task scheduler has to
> deal with cpumask_of_node instead of core_sibling which is wrong on
> scheduler domain MC.
>
> e.g. CPU3 hot-plugged out on TC2 [cluster0: 0,3-4 cluster1: 1-2]:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,3-4
> ^
> should be:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,4
>
> Add remove_cpu_topology() to __cpu_disable() to remove the CPU from the
> topology masks in case of a CPU hotplug out operation.
>
> At the same time tweak store_cpu_topology() slightly so it will call
> update_siblings_masks() in case of CPU hotplug in operation via
> secondary_start_kernel()->smp_store_cpu_info().
>
> This aligns the Arm32 implementation with the Arm64 one.
Looks good to me.
Tested-and-Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
>
> Fixes: ca74b316df96 ("arm: Use common cpu_topology structure and functions")
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
> arch/arm/kernel/smp.c | 2 ++
> arch/arm/kernel/topology.c | 15 +++++++--------
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 4b0bab2607e4..139c0d98fa29 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -240,6 +240,8 @@ int __cpu_disable(void)
> if (ret)
> return ret;
>
> + remove_cpu_topology(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.
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index 5b9faba03afb..b37b0a340991 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -196,9 +196,8 @@ void store_cpu_topology(unsigned int cpuid)
> struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> unsigned int mpidr;
>
> - /* If the cpu topology has been already set, just return */
> - if (cpuid_topo->core_id != -1)
> - return;
> + if (cpuid_topo->package_id != -1)
> + goto topology_populated;
>
> mpidr = read_cpuid_mpidr();
>
> @@ -231,14 +230,14 @@ void store_cpu_topology(unsigned int cpuid)
> cpuid_topo->package_id = -1;
> }
>
> - update_siblings_masks(cpuid);
> -
> update_cpu_capacity(cpuid);
>
> pr_info("CPU%u: thread %d, cpu %d, socket %d, mpidr %x\n",
> - cpuid, cpu_topology[cpuid].thread_id,
> - cpu_topology[cpuid].core_id,
> - cpu_topology[cpuid].package_id, mpidr);
> + cpuid, cpuid_topo->thread_id, cpuid_topo->core_id,
> + cpuid_topo->package_id, mpidr);
> +
> +topology_populated:
> + update_siblings_masks(cpuid);
> }
>
> static inline int cpu_corepower_flags(void)
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
@ 2019-11-20 11:16 ` Lukasz Luba
0 siblings, 0 replies; 16+ messages in thread
From: Lukasz Luba @ 2019-11-20 11:16 UTC (permalink / raw)
To: Dietmar Eggemann, Atish Patra, Russell King
Cc: linux-kernel, Morten Rasmussen, linux-arm-kernel, Sudeep Holla
Hi Dietmar,
On 11/20/19 10:42 AM, Dietmar Eggemann wrote:
> Commit ca74b316df96 ("arm: Use common cpu_topology structure and
> functions.") changed cpu_coregroup_mask() from the ARM32 specific
> implementation in arch/arm/include/asm/topology.h to the one shared
> with ARM64 and RISCV in drivers/base/arch_topology.c.
>
> Currently on Arm32 (TC2 w/ CONFIG_SCHED_MC) the task scheduler setup
> code (w/ CONFIG_SCHED_DEBUG) shows this during CPU hotplug:
>
> ERROR: groups don't span domain->span
>
> It happens to CPUs of the cluster of the CPU which gets hot-plugged
> out on scheduler domain MC.
>
> Turns out that the shared cpu_coregroup_mask() requires that the
> hot-plugged CPU is removed from the core_sibling mask via
> remove_cpu_topology(). Otherwise the 'is core_sibling subset of
> cpumask_of_node()' doesn't work. In this case the task scheduler has to
> deal with cpumask_of_node instead of core_sibling which is wrong on
> scheduler domain MC.
>
> e.g. CPU3 hot-plugged out on TC2 [cluster0: 0,3-4 cluster1: 1-2]:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,3-4
> ^
> should be:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,4
>
> Add remove_cpu_topology() to __cpu_disable() to remove the CPU from the
> topology masks in case of a CPU hotplug out operation.
>
> At the same time tweak store_cpu_topology() slightly so it will call
> update_siblings_masks() in case of CPU hotplug in operation via
> secondary_start_kernel()->smp_store_cpu_info().
>
> This aligns the Arm32 implementation with the Arm64 one.
Looks good to me.
Tested-and-Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
>
> Fixes: ca74b316df96 ("arm: Use common cpu_topology structure and functions")
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> ---
> arch/arm/kernel/smp.c | 2 ++
> arch/arm/kernel/topology.c | 15 +++++++--------
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 4b0bab2607e4..139c0d98fa29 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -240,6 +240,8 @@ int __cpu_disable(void)
> if (ret)
> return ret;
>
> + remove_cpu_topology(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.
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index 5b9faba03afb..b37b0a340991 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -196,9 +196,8 @@ void store_cpu_topology(unsigned int cpuid)
> struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> unsigned int mpidr;
>
> - /* If the cpu topology has been already set, just return */
> - if (cpuid_topo->core_id != -1)
> - return;
> + if (cpuid_topo->package_id != -1)
> + goto topology_populated;
>
> mpidr = read_cpuid_mpidr();
>
> @@ -231,14 +230,14 @@ void store_cpu_topology(unsigned int cpuid)
> cpuid_topo->package_id = -1;
> }
>
> - update_siblings_masks(cpuid);
> -
> update_cpu_capacity(cpuid);
>
> pr_info("CPU%u: thread %d, cpu %d, socket %d, mpidr %x\n",
> - cpuid, cpu_topology[cpuid].thread_id,
> - cpu_topology[cpuid].core_id,
> - cpu_topology[cpuid].package_id, mpidr);
> + cpuid, cpuid_topo->thread_id, cpuid_topo->core_id,
> + cpuid_topo->package_id, mpidr);
> +
> +topology_populated:
> + update_siblings_masks(cpuid);
> }
>
> static inline int cpu_corepower_flags(void)
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
2019-11-20 10:42 ` Dietmar Eggemann
@ 2019-11-24 21:47 ` Ondřej Jirman
-1 siblings, 0 replies; 16+ messages in thread
From: Ondřej Jirman @ 2019-11-24 21:47 UTC (permalink / raw)
To: Dietmar Eggemann
Cc: Atish Patra, Russell King, linux-arm-kernel, linux-kernel,
Morten Rasmussen, Lukasz Luba, Sudeep Holla
Hello,
On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
> Commit ca74b316df96 ("arm: Use common cpu_topology structure and
> functions.") changed cpu_coregroup_mask() from the ARM32 specific
> implementation in arch/arm/include/asm/topology.h to the one shared
> with ARM64 and RISCV in drivers/base/arch_topology.c.
>
> Currently on Arm32 (TC2 w/ CONFIG_SCHED_MC) the task scheduler setup
> code (w/ CONFIG_SCHED_DEBUG) shows this during CPU hotplug:
>
> ERROR: groups don't span domain->span
>
> It happens to CPUs of the cluster of the CPU which gets hot-plugged
> out on scheduler domain MC.
>
> Turns out that the shared cpu_coregroup_mask() requires that the
> hot-plugged CPU is removed from the core_sibling mask via
> remove_cpu_topology(). Otherwise the 'is core_sibling subset of
> cpumask_of_node()' doesn't work. In this case the task scheduler has to
> deal with cpumask_of_node instead of core_sibling which is wrong on
> scheduler domain MC.
>
> e.g. CPU3 hot-plugged out on TC2 [cluster0: 0,3-4 cluster1: 1-2]:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,3-4
> ^
> should be:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,4
>
> Add remove_cpu_topology() to __cpu_disable() to remove the CPU from the
> topology masks in case of a CPU hotplug out operation.
>
> At the same time tweak store_cpu_topology() slightly so it will call
> update_siblings_masks() in case of CPU hotplug in operation via
> secondary_start_kernel()->smp_store_cpu_info().
>
> This aligns the Arm32 implementation with the Arm64 one.
>
> Fixes: ca74b316df96 ("arm: Use common cpu_topology structure and functions")
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
This fixes CPU hotplug and correspondent suspend to ram/resume failures (that
disables and re-enables non-boot CPUs) on A83T SoC, that I've seen since
5.4-rc1.
Tested-by: Ondrej Jirman <megous@megous.com>
thanks,
o.
> ---
> arch/arm/kernel/smp.c | 2 ++
> arch/arm/kernel/topology.c | 15 +++++++--------
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 4b0bab2607e4..139c0d98fa29 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -240,6 +240,8 @@ int __cpu_disable(void)
> if (ret)
> return ret;
>
> + remove_cpu_topology(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.
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index 5b9faba03afb..b37b0a340991 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -196,9 +196,8 @@ void store_cpu_topology(unsigned int cpuid)
> struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> unsigned int mpidr;
>
> - /* If the cpu topology has been already set, just return */
> - if (cpuid_topo->core_id != -1)
> - return;
> + if (cpuid_topo->package_id != -1)
> + goto topology_populated;
>
> mpidr = read_cpuid_mpidr();
>
> @@ -231,14 +230,14 @@ void store_cpu_topology(unsigned int cpuid)
> cpuid_topo->package_id = -1;
> }
>
> - update_siblings_masks(cpuid);
> -
> update_cpu_capacity(cpuid);
>
> pr_info("CPU%u: thread %d, cpu %d, socket %d, mpidr %x\n",
> - cpuid, cpu_topology[cpuid].thread_id,
> - cpu_topology[cpuid].core_id,
> - cpu_topology[cpuid].package_id, mpidr);
> + cpuid, cpuid_topo->thread_id, cpuid_topo->core_id,
> + cpuid_topo->package_id, mpidr);
> +
> +topology_populated:
> + update_siblings_masks(cpuid);
> }
>
> static inline int cpu_corepower_flags(void)
> --
> 2.17.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
@ 2019-11-24 21:47 ` Ondřej Jirman
0 siblings, 0 replies; 16+ messages in thread
From: Ondřej Jirman @ 2019-11-24 21:47 UTC (permalink / raw)
To: Dietmar Eggemann
Cc: Russell King, linux-kernel, Atish Patra, linux-arm-kernel,
Sudeep Holla, Morten Rasmussen, Lukasz Luba
Hello,
On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
> Commit ca74b316df96 ("arm: Use common cpu_topology structure and
> functions.") changed cpu_coregroup_mask() from the ARM32 specific
> implementation in arch/arm/include/asm/topology.h to the one shared
> with ARM64 and RISCV in drivers/base/arch_topology.c.
>
> Currently on Arm32 (TC2 w/ CONFIG_SCHED_MC) the task scheduler setup
> code (w/ CONFIG_SCHED_DEBUG) shows this during CPU hotplug:
>
> ERROR: groups don't span domain->span
>
> It happens to CPUs of the cluster of the CPU which gets hot-plugged
> out on scheduler domain MC.
>
> Turns out that the shared cpu_coregroup_mask() requires that the
> hot-plugged CPU is removed from the core_sibling mask via
> remove_cpu_topology(). Otherwise the 'is core_sibling subset of
> cpumask_of_node()' doesn't work. In this case the task scheduler has to
> deal with cpumask_of_node instead of core_sibling which is wrong on
> scheduler domain MC.
>
> e.g. CPU3 hot-plugged out on TC2 [cluster0: 0,3-4 cluster1: 1-2]:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,3-4
> ^
> should be:
>
> cpu_coregroup_mask(): CPU3 cpumask_of_node=0-2,4 core_sibling=0,4
>
> Add remove_cpu_topology() to __cpu_disable() to remove the CPU from the
> topology masks in case of a CPU hotplug out operation.
>
> At the same time tweak store_cpu_topology() slightly so it will call
> update_siblings_masks() in case of CPU hotplug in operation via
> secondary_start_kernel()->smp_store_cpu_info().
>
> This aligns the Arm32 implementation with the Arm64 one.
>
> Fixes: ca74b316df96 ("arm: Use common cpu_topology structure and functions")
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
This fixes CPU hotplug and correspondent suspend to ram/resume failures (that
disables and re-enables non-boot CPUs) on A83T SoC, that I've seen since
5.4-rc1.
Tested-by: Ondrej Jirman <megous@megous.com>
thanks,
o.
> ---
> arch/arm/kernel/smp.c | 2 ++
> arch/arm/kernel/topology.c | 15 +++++++--------
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 4b0bab2607e4..139c0d98fa29 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -240,6 +240,8 @@ int __cpu_disable(void)
> if (ret)
> return ret;
>
> + remove_cpu_topology(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.
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> index 5b9faba03afb..b37b0a340991 100644
> --- a/arch/arm/kernel/topology.c
> +++ b/arch/arm/kernel/topology.c
> @@ -196,9 +196,8 @@ void store_cpu_topology(unsigned int cpuid)
> struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> unsigned int mpidr;
>
> - /* If the cpu topology has been already set, just return */
> - if (cpuid_topo->core_id != -1)
> - return;
> + if (cpuid_topo->package_id != -1)
> + goto topology_populated;
>
> mpidr = read_cpuid_mpidr();
>
> @@ -231,14 +230,14 @@ void store_cpu_topology(unsigned int cpuid)
> cpuid_topo->package_id = -1;
> }
>
> - update_siblings_masks(cpuid);
> -
> update_cpu_capacity(cpuid);
>
> pr_info("CPU%u: thread %d, cpu %d, socket %d, mpidr %x\n",
> - cpuid, cpu_topology[cpuid].thread_id,
> - cpu_topology[cpuid].core_id,
> - cpu_topology[cpuid].package_id, mpidr);
> + cpuid, cpuid_topo->thread_id, cpuid_topo->core_id,
> + cpuid_topo->package_id, mpidr);
> +
> +topology_populated:
> + update_siblings_masks(cpuid);
> }
>
> static inline int cpu_corepower_flags(void)
> --
> 2.17.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
2019-11-24 21:47 ` Ondřej Jirman
@ 2019-11-26 10:42 ` Dietmar Eggemann
-1 siblings, 0 replies; 16+ messages in thread
From: Dietmar Eggemann @ 2019-11-26 10:42 UTC (permalink / raw)
To: Atish Patra, Russell King, linux-arm-kernel, linux-kernel,
Morten Rasmussen, Lukasz Luba, Sudeep Holla
On 24/11/2019 22:47, Ondřej Jirman wrote:
> Hello,
>
> On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
[...]
>> Fixes: ca74b316df96 ("arm: Use common cpu_topology structure and functions")
>> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
>
> This fixes CPU hotplug and correspondent suspend to ram/resume failures (that
> disables and re-enables non-boot CPUs) on A83T SoC, that I've seen since
> 5.4-rc1.
>
> Tested-by: Ondrej Jirman <megous@megous.com>
Thanks for testing! Which Cpufreq driver is your system using?
[...]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
@ 2019-11-26 10:42 ` Dietmar Eggemann
0 siblings, 0 replies; 16+ messages in thread
From: Dietmar Eggemann @ 2019-11-26 10:42 UTC (permalink / raw)
To: Atish Patra, Russell King, linux-arm-kernel, linux-kernel,
Morten Rasmussen, Lukasz Luba, Sudeep Holla
On 24/11/2019 22:47, Ondřej Jirman wrote:
> Hello,
>
> On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
[...]
>> Fixes: ca74b316df96 ("arm: Use common cpu_topology structure and functions")
>> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
>
> This fixes CPU hotplug and correspondent suspend to ram/resume failures (that
> disables and re-enables non-boot CPUs) on A83T SoC, that I've seen since
> 5.4-rc1.
>
> Tested-by: Ondrej Jirman <megous@megous.com>
Thanks for testing! Which Cpufreq driver is your system using?
[...]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
2019-11-26 10:42 ` Dietmar Eggemann
@ 2019-11-26 21:45 ` Ondřej Jirman
-1 siblings, 0 replies; 16+ messages in thread
From: Ondřej Jirman @ 2019-11-26 21:45 UTC (permalink / raw)
To: Dietmar Eggemann
Cc: Atish Patra, Russell King, linux-arm-kernel, linux-kernel,
Morten Rasmussen, Lukasz Luba, Sudeep Holla
On Tue, Nov 26, 2019 at 11:42:02AM +0100, Dietmar Eggemann wrote:
> On 24/11/2019 22:47, Ondřej Jirman wrote:
> > Hello,
> >
> > On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
>
> [...]
>
> >> Fixes: ca74b316df96 ("arm: Use common cpu_topology structure and functions")
> >> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> >
> > This fixes CPU hotplug and correspondent suspend to ram/resume failures (that
> > disables and re-enables non-boot CPUs) on A83T SoC, that I've seen since
> > 5.4-rc1.
> >
> > Tested-by: Ondrej Jirman <megous@megous.com>
>
> Thanks for testing! Which Cpufreq driver is your system using?
Hello,
it's using cpufreq-dt.
regards,
o.
> [...]
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
@ 2019-11-26 21:45 ` Ondřej Jirman
0 siblings, 0 replies; 16+ messages in thread
From: Ondřej Jirman @ 2019-11-26 21:45 UTC (permalink / raw)
To: Dietmar Eggemann
Cc: Russell King, linux-kernel, Atish Patra, linux-arm-kernel,
Sudeep Holla, Morten Rasmussen, Lukasz Luba
On Tue, Nov 26, 2019 at 11:42:02AM +0100, Dietmar Eggemann wrote:
> On 24/11/2019 22:47, Ondřej Jirman wrote:
> > Hello,
> >
> > On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
>
> [...]
>
> >> Fixes: ca74b316df96 ("arm: Use common cpu_topology structure and functions")
> >> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> >
> > This fixes CPU hotplug and correspondent suspend to ram/resume failures (that
> > disables and re-enables non-boot CPUs) on A83T SoC, that I've seen since
> > 5.4-rc1.
> >
> > Tested-by: Ondrej Jirman <megous@megous.com>
>
> Thanks for testing! Which Cpufreq driver is your system using?
Hello,
it's using cpufreq-dt.
regards,
o.
> [...]
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
2019-11-26 21:45 ` Ondřej Jirman
@ 2019-11-27 14:31 ` Dietmar Eggemann
-1 siblings, 0 replies; 16+ messages in thread
From: Dietmar Eggemann @ 2019-11-27 14:31 UTC (permalink / raw)
To: Atish Patra, Russell King, linux-arm-kernel, linux-kernel,
Morten Rasmussen, Lukasz Luba, Sudeep Holla
On 26/11/2019 22:45, Ondřej Jirman wrote:
> On Tue, Nov 26, 2019 at 11:42:02AM +0100, Dietmar Eggemann wrote:
>> On 24/11/2019 22:47, Ondřej Jirman wrote:
>>> Hello,
>>>
>>> On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
[...]
>> Thanks for testing! Which Cpufreq driver is your system using?
>
> Hello,
>
> it's using cpufreq-dt.
Thanks for the info! It's dt-based so this one is safe in regards to
retrieving the correct topology core mask during CPU hp stress (i.e.
from multiple CPUs at the same time).
[...]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] arm: Fix topology setup in case of CPU hotplug for CONFIG_SCHED_MC
@ 2019-11-27 14:31 ` Dietmar Eggemann
0 siblings, 0 replies; 16+ messages in thread
From: Dietmar Eggemann @ 2019-11-27 14:31 UTC (permalink / raw)
To: Atish Patra, Russell King, linux-arm-kernel, linux-kernel,
Morten Rasmussen, Lukasz Luba, Sudeep Holla
On 26/11/2019 22:45, Ondřej Jirman wrote:
> On Tue, Nov 26, 2019 at 11:42:02AM +0100, Dietmar Eggemann wrote:
>> On 24/11/2019 22:47, Ondřej Jirman wrote:
>>> Hello,
>>>
>>> On Wed, Nov 20, 2019 at 10:42:12AM +0000, Dietmar Eggemann wrote:
[...]
>> Thanks for testing! Which Cpufreq driver is your system using?
>
> Hello,
>
> it's using cpufreq-dt.
Thanks for the info! It's dt-based so this one is safe in regards to
retrieving the correct topology core mask during CPU hp stress (i.e.
from multiple CPUs at the same time).
[...]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread