* [RFC] Add Arm cpu topology definition
2011-06-16 8:49 [RFC] Add Arm cpu topology definition Vincent Guittot
@ 2011-06-16 8:55 ` Samuel Thibault
2011-06-16 9:44 ` Vincent Guittot
2011-06-16 10:49 ` Daniel Lezcano
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Samuel Thibault @ 2011-06-16 8:55 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
Vincent Guittot, le Thu 16 Jun 2011 10:49:13 +0200, a ?crit :
> The affinity between Arm processors is defined in the MPIDR register.
> We can identify which processors are in the same cluster,
> and which ones have performance interdependency. The cpu topology
> of an Arm platform can be set thanks to this register and this topology
> is then used by sched_mc and sched_smt.
Cool! Could you check that the hwloc tool gets also gets this
information from userland through /sys, and/or send me the output of the
hwloc-gather-topology tool from hwloc so we can add an testcase for
this?
Samuel
^ permalink raw reply [flat|nested] 20+ messages in thread* [RFC] Add Arm cpu topology definition
2011-06-16 8:55 ` Samuel Thibault
@ 2011-06-16 9:44 ` Vincent Guittot
2011-06-16 9:47 ` Samuel Thibault
0 siblings, 1 reply; 20+ messages in thread
From: Vincent Guittot @ 2011-06-16 9:44 UTC (permalink / raw)
To: linux-arm-kernel
On 16 June 2011 10:55, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> Hello,
>
> Vincent Guittot, le Thu 16 Jun 2011 10:49:13 +0200, a ?crit :
>> The affinity between Arm processors is defined in the MPIDR register.
>> We can identify which processors are in the same cluster,
>> and which ones have performance interdependency. The cpu topology
>> ?of an Arm platform can be set thanks to this register and this topology
>> is then used by sched_mc and sched_smt.
>
> Cool! ?Could you check that the hwloc tool gets also gets this
> information from userland through /sys, and/or send me the output of the
> hwloc-gather-topology tool from hwloc so we can add an testcase for
> this?
>
The output of hwloc-gather-topology is :
Machine (phys=0 local=280840KB total=280840KB)
Socket #0 (phys=3)
Core #0 (phys=0)
PU #0 (phys=0)
Core #1 (phys=1)
PU #1 (phys=1)
depth 0: 1 Machine (type #1)
depth 1: 1 Socket (type #3)
depth 2: 2 Cores (type #5)
depth 3: 2 PUs (type #6)
Topology not from this system
let me know if it's what you want
> Samuel
>
^ permalink raw reply [flat|nested] 20+ messages in thread* [RFC] Add Arm cpu topology definition
2011-06-16 9:44 ` Vincent Guittot
@ 2011-06-16 9:47 ` Samuel Thibault
2011-06-16 9:56 ` Vincent Guittot
0 siblings, 1 reply; 20+ messages in thread
From: Samuel Thibault @ 2011-06-16 9:47 UTC (permalink / raw)
To: linux-arm-kernel
Vincent Guittot, le Thu 16 Jun 2011 11:44:30 +0200, a ?crit :
> The output of hwloc-gather-topology is :
>
> Machine (phys=0 local=280840KB total=280840KB)
> Socket #0 (phys=3)
> Core #0 (phys=0)
> PU #0 (phys=0)
> Core #1 (phys=1)
> PU #1 (phys=1)
> depth 0: 1 Machine (type #1)
> depth 1: 1 Socket (type #3)
> depth 2: 2 Cores (type #5)
> depth 3: 2 PUs (type #6)
> Topology not from this system
>
> let me know if it's what you want
This looks good for a bicore socket, yes. Could you send me the tarball
it created, so we can include it in the hwloc testsuite?
Do you have a example with smt cores?
Samuel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC] Add Arm cpu topology definition
2011-06-16 9:47 ` Samuel Thibault
@ 2011-06-16 9:56 ` Vincent Guittot
0 siblings, 0 replies; 20+ messages in thread
From: Vincent Guittot @ 2011-06-16 9:56 UTC (permalink / raw)
To: linux-arm-kernel
On 16 June 2011 11:47, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> Vincent Guittot, le Thu 16 Jun 2011 11:44:30 +0200, a ?crit :
>> The output of hwloc-gather-topology is :
>>
>> Machine (phys=0 local=280840KB total=280840KB)
>> ? Socket #0 (phys=3)
>> ? ? Core #0 (phys=0)
>> ? ? ? PU #0 (phys=0)
>> ? ? Core #1 (phys=1)
>> ? ? ? PU #1 (phys=1)
>> depth 0: ? ? ? ?1 Machine (type #1)
>> ?depth 1: ? ? ? 1 Socket (type #3)
>> ? depth 2: ? ? ?2 Cores (type #5)
>> ? ?depth 3: ? ? 2 PUs (type #6)
>> Topology not from this system
>>
>> let me know if it's what you want
>
> This looks good for a bicore socket, yes. ?Could you send me the tarball
> it created, so we can include it in the hwloc testsuite?
>
yes, I will send it to you
> Do you have a example with smt cores?
no, I haven't
>
> Samuel
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC] Add Arm cpu topology definition
2011-06-16 8:49 [RFC] Add Arm cpu topology definition Vincent Guittot
2011-06-16 8:55 ` Samuel Thibault
@ 2011-06-16 10:49 ` Daniel Lezcano
2011-06-16 12:05 ` Vincent Guittot
2011-06-16 11:48 ` Amit Kucheria
` (4 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Daniel Lezcano @ 2011-06-16 10:49 UTC (permalink / raw)
To: linux-arm-kernel
On 06/16/2011 10:49 AM, Vincent Guittot wrote:
> The affinity between Arm processors is defined in the MPIDR register.
> We can identify which processors are in the same cluster,
> and which ones have performance interdependency. The cpu topology
> of an Arm platform can be set thanks to this register and this topology
> is then used by sched_mc and sched_smt.
>
> Signed-off-by: Vincent Guittot<vincent.guittot@linaro.org>
> ---
> arch/arm/Kconfig | 26 ++++++++
> arch/arm/include/asm/topology.h | 33 ++++++++++
> arch/arm/kernel/Makefile | 1 +
> arch/arm/kernel/smp.c | 6 ++
> arch/arm/kernel/topology.c | 133 +++++++++++++++++++++++++++++++++++++++
> 5 files changed, 199 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/kernel/topology.c
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 9adc278..bacf9af 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -219,6 +219,24 @@ source "kernel/Kconfig.freezer"
>
> menu "System Type"
>
> +config SCHED_MC
> + bool "Multi-core scheduler support"
> + depends on SMP&& ARM_CPU_TOPOLOGY
ARM_CPU_TOPOLOGY depends on SMP, so the check can be reduced to
depends on ARM_CPU_TOPOLOGY
> + default n
> + help
> + Multi-core scheduler support improves the CPU scheduler's decision
> + making when dealing with multi-core CPU chips at a cost of slightly
> + increased overhead in some places. If unsure say N here.
> +
> +config SCHED_SMT
> + bool "SMT scheduler support"
> + depends on SMP&& ARM_CPU_TOPOLOGY
depends on SMT && ARM_CPU_TOPOLOGY ?
> + default n
> + help
> + Improves the CPU scheduler's decision making when dealing with
> + MultiThreading at a cost of slightly increased overhead in some
> + places. If unsure say N here.
> +
> config MMU
> bool "MMU-based Paged Memory Management Support"
> default y
> @@ -1062,6 +1080,14 @@ if !MMU
> source "arch/arm/Kconfig-nommu"
> endif
>
> +config ARM_CPU_TOPOLOGY
> + bool "Support cpu topology definition"
> + depends on SMP&& CPU_V7
> + help
> + Support Arm cpu topology definition. The MPIDR register defines
> + affinity between processors which is used to set the cpu
> + topology of an Arm System.
> +
> config ARM_ERRATA_411920
> bool "ARM errata: Invalidation of the Instruction Cache operation can fail"
> depends on CPU_V6 || CPU_V6K
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index accbd7c..cb90d0a 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -1,6 +1,39 @@
> #ifndef _ASM_ARM_TOPOLOGY_H
> #define _ASM_ARM_TOPOLOGY_H
>
> +#ifdef CONFIG_ARM_CPU_TOPOLOGY
> +
> +#include<linux/cpumask.h>
> +
> +struct cputopo_arm {
> + int thread_id;
> + int core_id;
> + int socket_id;
I am not sure how that deals with the rest of the functions prototype
but wouldn't u16 be more adequate ?
> + cpumask_t thread_sibling;
> + cpumask_t core_sibling;
> +};
> +
> +extern struct cputopo_arm cpu_topology[NR_CPUS];
> +
> +#define topology_physical_package_id(cpu) (cpu_topology[cpu].socket_id)
> +#define topology_core_id(cpu) (cpu_topology[cpu].core_id)
> +#define topology_core_cpumask(cpu) (&(cpu_topology[cpu].core_sibling))
> +#define topology_thread_cpumask(cpu) (&(cpu_topology[cpu].thread_sibling))
> +
> +#define mc_capable() (cpu_topology[0].socket_id != -1)
> +#define smt_capable() (cpu_topology[0].thread_id != -1)
> +
> +void init_cpu_topology(void);
> +void store_cpu_topology(unsigned int cpuid);
> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu);
> +
> +#else
> +
> +#define init_cpu_topology() {};
> +#define store_cpu_topology(cpuid) {};
AFAIK the convention is to declare static inline noop functions.
static inline void init_cpu_topology(void) { };
static inline void store_cpu_topology(unsigned int cpuid) { };
> +
> +#endif
> +
> #include<asm-generic/topology.h>
>
> #endif /* _ASM_ARM_TOPOLOGY_H */
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index a5b31af..816a481 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_IWMMXT) += iwmmxt.o
> obj-$(CONFIG_CPU_HAS_PMU) += pmu.o
> obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o
> AFLAGS_iwmmxt.o := -Wa,-mcpu=iwmmxt
> +obj-$(CONFIG_ARM_CPU_TOPOLOGY) += topology.o
>
> ifneq ($(CONFIG_ARCH_EBSA110),y)
> obj-y += io.o
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 344e52b..3e8dc3b 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -31,6 +31,7 @@
> #include<asm/cacheflush.h>
> #include<asm/cpu.h>
> #include<asm/cputype.h>
> +#include<asm/topology.h>
> #include<asm/mmu_context.h>
> #include<asm/pgtable.h>
> #include<asm/pgalloc.h>
> @@ -268,6 +269,9 @@ static void __cpuinit smp_store_cpu_info(unsigned int cpuid)
> struct cpuinfo_arm *cpu_info =&per_cpu(cpu_data, cpuid);
>
> cpu_info->loops_per_jiffy = loops_per_jiffy;
> +
> + store_cpu_topology(cpuid);
> +
> }
If the store_cpu_topology function is called once, can it be changed to
a __cpuinit function, declared as a subsys_initcall and removed from here ?
> /*
> @@ -354,6 +358,8 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> {
> unsigned int ncores = num_possible_cpus();
>
> + init_cpu_topology();
Why do you need to call the init function here ?
On the other architecture I see:
static int __init topology_init(void)
{
...
}
subsys_initcall(topology_init);
Isn't possible to use the same way ? (with the benefit to save two
declarations in the header).
[ ... ]
> +
> +struct cputopo_arm cpu_topology[NR_CPUS];
IMO, you can define it static here no ?
> +
> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu)
> +{
> + return&(cpu_topology[cpu].core_sibling);
> +}
> +
> +/*
> + * store_cpu_topology is called at boot when only one cpu is running
> + * and with the mutex cpu_hotplug.lock locked, when several cpus have booted,
> + * which prevents simultaneous write access to cpu_topology array
> + */
> +void store_cpu_topology(unsigned int cpuid)
> +{
> + struct cputopo_arm *cpuid_topo =&(cpu_topology[cpuid]);
> + unsigned int mpidr;
> + unsigned int cpu;
> +
> + /* If the cpu topology has been already set, just return */
> + if (cpuid_topo->core_id != -1)
> + return;
If the code calls store_cpu_topology but with no effect because it was
already called before, that means it shouldn't be called at all, no ?
IMHO, this test should be removed or at least add a WARN_ONCE.
> +
> + mpidr = hard_smp_mpidr();
> +
> + /* create cpu topology mapping */
> + if (mpidr& (0x3<< 30)) {
> + /*
> + * This is a multiprocessor system
> + * multiprocessor format& multiprocessor mode field are set
> + */
> +
> + if (mpidr& (0x1<< 24)) {
> + /* core performance interdependency */
> + cpuid_topo->thread_id = (mpidr& 0x3);
> + cpuid_topo->core_id = ((mpidr>> 8)& 0xF);
> + cpuid_topo->socket_id = ((mpidr>> 16)& 0xFF);
> + } else {
> + /* normal core interdependency */
> + cpuid_topo->thread_id = -1;
> + cpuid_topo->core_id = (mpidr& 0x3);
> + cpuid_topo->socket_id = ((mpidr>> 8)& 0xF);
> + }
> + } else {
> + /*
> + * This is an uniprocessor system
> + * we are in multiprocessor format but uniprocessor system
> + * or in the old uniprocessor format
> + */
> +
> + cpuid_topo->thread_id = -1;
> + cpuid_topo->core_id = 0;
> + cpuid_topo->socket_id = -1;
> + }
> +
> + /* update core and thread sibling masks */
> + for_each_possible_cpu(cpu) {
> + struct cputopo_arm *cpu_topo =&(cpu_topology[cpu]);
> +
> + if (cpuid_topo->socket_id == cpu_topo->socket_id) {
> + cpumask_set_cpu(cpuid,&cpu_topo->core_sibling);
> + if (cpu != cpuid)
> + cpumask_set_cpu(cpu,
> + &cpuid_topo->core_sibling);
> +
> + if (cpuid_topo->core_id == cpu_topo->core_id) {
> + cpumask_set_cpu(cpuid,
> + &cpu_topo->thread_sibling);
> + if (cpu != cpuid)
> + cpumask_set_cpu(cpu,
> + &cpuid_topo->thread_sibling);
> + }
> + }
> + }
> + smp_wmb();
> +
> + printk(KERN_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].socket_id, mpidr);
> +
> +}
> +
> +/*
> + * init_cpu_topology is called at boot when only one cpu is running
> + * which prevent simultaneous write access to cpu_topology array
> + */
> +void init_cpu_topology(void)
> +{
> + unsigned int cpu;
> +
> + /* init core mask */
> + for_each_possible_cpu(cpu) {
> + struct cputopo_arm *cpu_topo =&(cpu_topology[cpu]);
> +
> + cpu_topo->thread_id = -1;
> + cpu_topo->core_id = -1;
nit : extra space
> + cpu_topo->socket_id = -1;
> + cpumask_clear(&cpu_topo->core_sibling);
> + cpumask_clear(&cpu_topo->thread_sibling);
> + }
> + smp_wmb();
> +}
^ permalink raw reply [flat|nested] 20+ messages in thread* [RFC] Add Arm cpu topology definition
2011-06-16 10:49 ` Daniel Lezcano
@ 2011-06-16 12:05 ` Vincent Guittot
0 siblings, 0 replies; 20+ messages in thread
From: Vincent Guittot @ 2011-06-16 12:05 UTC (permalink / raw)
To: linux-arm-kernel
On 16 June 2011 12:49, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> On 06/16/2011 10:49 AM, Vincent Guittot wrote:
>>
>> The affinity between Arm processors is defined in the MPIDR register.
>> We can identify which processors are in the same cluster,
>> and which ones have performance interdependency. The cpu topology
>> ?of an Arm platform can be set thanks to this register and this topology
>> is then used by sched_mc and sched_smt.
>>
>> Signed-off-by: Vincent Guittot<vincent.guittot@linaro.org>
>> ---
>> ?arch/arm/Kconfig ? ? ? ? ? ? ? ?| ? 26 ++++++++
>> ?arch/arm/include/asm/topology.h | ? 33 ++++++++++
>> ?arch/arm/kernel/Makefile ? ? ? ?| ? ?1 +
>> ?arch/arm/kernel/smp.c ? ? ? ? ? | ? ?6 ++
>> ?arch/arm/kernel/topology.c ? ? ?| ?133
>> +++++++++++++++++++++++++++++++++++++++
>> ?5 files changed, 199 insertions(+), 0 deletions(-)
>> ?create mode 100644 arch/arm/kernel/topology.c
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 9adc278..bacf9af 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -219,6 +219,24 @@ source "kernel/Kconfig.freezer"
>>
>> ?menu "System Type"
>>
>> +config SCHED_MC
>> + ? ? ? bool "Multi-core scheduler support"
>> + ? ? ? depends on SMP&& ?ARM_CPU_TOPOLOGY
>
> ARM_CPU_TOPOLOGY depends on SMP, so the check can be reduced to
>
> depends on ARM_CPU_TOPOLOGY
you're right
>>
>> + ? ? ? default n
>> + ? ? ? help
>> + ? ? ? ? Multi-core scheduler support improves the CPU scheduler's
>> decision
>> + ? ? ? ? making when dealing with multi-core CPU chips at a cost of
>> slightly
>> + ? ? ? ? increased overhead in some places. If unsure say N here.
>> +
>> +config SCHED_SMT
>> + ? ? ? bool "SMT scheduler support"
>> + ? ? ? depends on SMP&& ?ARM_CPU_TOPOLOGY
>
> depends on SMT && ARM_CPU_TOPOLOGY ?
SMP is the right one but it can be reduced to : depends on ARM_CPU_TOPOLOGY
like SCHED_MC,
>
>> + ? ? ? default n
>> + ? ? ? help
>> + ? ? ? ? Improves the CPU scheduler's decision making when dealing with
>> + ? ? ? ? MultiThreading at a cost of slightly increased overhead in some
>> + ? ? ? ? places. If unsure say N here.
>> +
>> ?config MMU
>> ? ? ? ?bool "MMU-based Paged Memory Management Support"
>> ? ? ? ?default y
>> @@ -1062,6 +1080,14 @@ if !MMU
>> ?source "arch/arm/Kconfig-nommu"
>> ?endif
>>
>> +config ARM_CPU_TOPOLOGY
>> + ? ? ? bool "Support cpu topology definition"
>> + ? ? ? depends on SMP&& ?CPU_V7
>> + ? ? ? help
>> + ? ? ? ? Support Arm cpu topology definition. The MPIDR register defines
>> + ? ? ? ? affinity between processors which is used to set the cpu
>> + ? ? ? ? topology of an Arm System.
>> +
>> ?config ARM_ERRATA_411920
>> ? ? ? ?bool "ARM errata: Invalidation of the Instruction Cache operation
>> can fail"
>> ? ? ? ?depends on CPU_V6 || CPU_V6K
>> diff --git a/arch/arm/include/asm/topology.h
>> b/arch/arm/include/asm/topology.h
>> index accbd7c..cb90d0a 100644
>> --- a/arch/arm/include/asm/topology.h
>> +++ b/arch/arm/include/asm/topology.h
>> @@ -1,6 +1,39 @@
>> ?#ifndef _ASM_ARM_TOPOLOGY_H
>> ?#define _ASM_ARM_TOPOLOGY_H
>>
>> +#ifdef CONFIG_ARM_CPU_TOPOLOGY
>> +
>> +#include<linux/cpumask.h>
>> +
>> +struct cputopo_arm {
>> + ? ? ? int thread_id;
>> + ? ? ? int core_id;
>> + ? ? ? int socket_id;
>
> I am not sure how that deals with the rest of the functions prototype but
> wouldn't u16 be more adequate ?
>
I have used int to be aligned on register size and minimize register
manipulation
>> + ? ? ? cpumask_t thread_sibling;
>> + ? ? ? cpumask_t core_sibling;
>> +};
>> +
>> +extern struct cputopo_arm cpu_topology[NR_CPUS];
>> +
>> +#define topology_physical_package_id(cpu)
>> ?(cpu_topology[cpu].socket_id)
>> +#define topology_core_id(cpu) ? ? ? ? ?(cpu_topology[cpu].core_id)
>> +#define topology_core_cpumask(cpu)
>> (&(cpu_topology[cpu].core_sibling))
>> +#define topology_thread_cpumask(cpu)
>> (&(cpu_topology[cpu].thread_sibling))
>> +
>> +#define mc_capable() ? (cpu_topology[0].socket_id != -1)
>> +#define smt_capable() ?(cpu_topology[0].thread_id != -1)
>> +
>> +void init_cpu_topology(void);
>> +void store_cpu_topology(unsigned int cpuid);
>> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu);
>> +
>> +#else
>> +
>> +#define init_cpu_topology() {};
>> +#define store_cpu_topology(cpuid) {};
>
> AFAIK the convention is to declare static inline noop functions.
>
> static inline void init_cpu_topology(void) { };
> static inline void store_cpu_topology(unsigned int cpuid) { };
>
ok
>> +
>> +#endif
>> +
>> ?#include<asm-generic/topology.h>
>>
>> ?#endif /* _ASM_ARM_TOPOLOGY_H */
>> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
>> index a5b31af..816a481 100644
>> --- a/arch/arm/kernel/Makefile
>> +++ b/arch/arm/kernel/Makefile
>> @@ -61,6 +61,7 @@ obj-$(CONFIG_IWMMXT) ? ? ? ? ?+= iwmmxt.o
>> ?obj-$(CONFIG_CPU_HAS_PMU) ? ? += pmu.o
>> ?obj-$(CONFIG_HW_PERF_EVENTS) ?+= perf_event.o
>> ?AFLAGS_iwmmxt.o ? ? ? ? ? ? ? ? ? ? ? := -Wa,-mcpu=iwmmxt
>> +obj-$(CONFIG_ARM_CPU_TOPOLOGY) ?+= topology.o
>>
>> ?ifneq ($(CONFIG_ARCH_EBSA110),y)
>> ? ?obj-y ? ? ? ? ? ? ? += io.o
>> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
>> index 344e52b..3e8dc3b 100644
>> --- a/arch/arm/kernel/smp.c
>> +++ b/arch/arm/kernel/smp.c
>> @@ -31,6 +31,7 @@
>> ?#include<asm/cacheflush.h>
>> ?#include<asm/cpu.h>
>> ?#include<asm/cputype.h>
>> +#include<asm/topology.h>
>> ?#include<asm/mmu_context.h>
>> ?#include<asm/pgtable.h>
>> ?#include<asm/pgalloc.h>
>> @@ -268,6 +269,9 @@ static void __cpuinit smp_store_cpu_info(unsigned int
>> cpuid)
>> ? ? ? ?struct cpuinfo_arm *cpu_info =&per_cpu(cpu_data, cpuid);
>>
>> ? ? ? ?cpu_info->loops_per_jiffy = loops_per_jiffy;
>> +
>> + ? ? ? store_cpu_topology(cpuid);
>> +
>> ?}
>
> If the store_cpu_topology function is called once, can it be changed to a
> __cpuinit function, declared as a subsys_initcall and removed from here ?
>
it must be called once on each cpu before the call of sched_init_smp
>> ?/*
>> @@ -354,6 +358,8 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>> ?{
>> ? ? ? ?unsigned int ncores = num_possible_cpus();
>>
>> + ? ? ? init_cpu_topology();
>
> Why do you need to call the init function here ?
>
this function must be called before the 1st call to smp_store_cpu_info
> On the other architecture I see:
>
> static int __init topology_init(void)
> {
> ? ?...
> }
>
> subsys_initcall(topology_init);
>
> Isn't possible to use the same way ? (with the benefit to save two
> declarations in the header).
>
>
> [ ... ]
>
>> +
>> +struct cputopo_arm cpu_topology[NR_CPUS];
>
> IMO, you can define it static here no ?
>
This array is used by "#define topology_xxx" and "#define xx_capable"
which are used by the scheduler and the topology driver
>> +
>> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu)
>> +{
>> + ? ? ? return&(cpu_topology[cpu].core_sibling);
>> +}
>> +
>> +/*
>> + * store_cpu_topology is called at boot when only one cpu is running
>> + * and with the mutex cpu_hotplug.lock locked, when several cpus have
>> booted,
>> + * which prevents simultaneous write access to cpu_topology array
>> + */
>> +void store_cpu_topology(unsigned int cpuid)
>> +{
>> + ? ? ? struct cputopo_arm *cpuid_topo =&(cpu_topology[cpuid]);
>> + ? ? ? unsigned int mpidr;
>> + ? ? ? unsigned int cpu;
>> +
>> + ? ? ? /* If the cpu topology has been already set, just return */
>> + ? ? ? if (cpuid_topo->core_id != -1)
>> + ? ? ? ? ? ? ? return;
>
> If the code calls store_cpu_topology but with no effect because it was
> already called before, that means it shouldn't be called at all, no ?
> IMHO, this test should be removed or at least add a WARN_ONCE.
>
We will call smp_store_cpu_info each time the cpu will be plugged. But
once set, we don't need to update topology information
>> +
>> + ? ? ? mpidr = hard_smp_mpidr();
>> +
>> + ? ? ? /* create cpu topology mapping */
>> + ? ? ? if (mpidr& ?(0x3<< ?30)) {
>> + ? ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ? ?* This is a multiprocessor system
>> + ? ? ? ? ? ? ? ?* multiprocessor format& ?multiprocessor mode field are
>> set
>> + ? ? ? ? ? ? ? ?*/
>> +
>> + ? ? ? ? ? ? ? if (mpidr& ?(0x1<< ?24)) {
>> + ? ? ? ? ? ? ? ? ? ? ? /* core performance interdependency */
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = (mpidr& ?0x3);
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = ?((mpidr>> ?8)& ?0xF);
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr>> ?16)& ?0xFF);
>> + ? ? ? ? ? ? ? } else {
>> + ? ? ? ? ? ? ? ? ? ? ? /* normal core interdependency */
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = -1;
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = (mpidr& ?0x3);
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr>> ?8)& ?0xF);
>> + ? ? ? ? ? ? ? }
>> + ? ? ? } else {
>> + ? ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ? ?* This is an uniprocessor system
>> + ? ? ? ? ? ? ? ?* we are in multiprocessor format but uniprocessor system
>> + ? ? ? ? ? ? ? ?* or in the old uniprocessor format
>> + ? ? ? ? ? ? ? ?*/
>> +
>> + ? ? ? ? ? ? ? cpuid_topo->thread_id = -1;
>> + ? ? ? ? ? ? ? cpuid_topo->core_id = 0;
>> + ? ? ? ? ? ? ? cpuid_topo->socket_id = -1;
>> + ? ? ? }
>> +
>> + ? ? ? /* update core and thread sibling masks */
>> + ? ? ? for_each_possible_cpu(cpu) {
>> + ? ? ? ? ? ? ? struct cputopo_arm *cpu_topo =&(cpu_topology[cpu]);
>> +
>> + ? ? ? ? ? ? ? if (cpuid_topo->socket_id == cpu_topo->socket_id) {
>> + ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpuid,&cpu_topo->core_sibling);
>> + ? ? ? ? ? ? ? ? ? ? ? if (cpu != cpuid)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpu,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cpuid_topo->core_sibling);
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? if (cpuid_topo->core_id == cpu_topo->core_id) {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpuid,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cpu_topo->thread_sibling);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (cpu != cpuid)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpu,
>> +
>> &cpuid_topo->thread_sibling);
>> + ? ? ? ? ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? }
>> + ? ? ? }
>> + ? ? ? smp_wmb();
>> +
>> + ? ? ? printk(KERN_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].socket_id, mpidr);
>> +
>> +}
>> +
>> +/*
>> + * init_cpu_topology is called at boot when only one cpu is running
>> + * which prevent simultaneous write access to cpu_topology array
>> + */
>> +void init_cpu_topology(void)
>> +{
>> + ? ? ? unsigned int cpu;
>> +
>> + ? ? ? /* init core mask */
>> + ? ? ? for_each_possible_cpu(cpu) {
>> + ? ? ? ? ? ? ? struct cputopo_arm *cpu_topo =&(cpu_topology[cpu]);
>> +
>> + ? ? ? ? ? ? ? cpu_topo->thread_id = -1;
>> + ? ? ? ? ? ? ? cpu_topo->core_id = ?-1;
>
> nit : extra space
ok
>>
>> + ? ? ? ? ? ? ? cpu_topo->socket_id = -1;
>> + ? ? ? ? ? ? ? cpumask_clear(&cpu_topo->core_sibling);
>> + ? ? ? ? ? ? ? cpumask_clear(&cpu_topo->thread_sibling);
>> + ? ? ? }
>> + ? ? ? smp_wmb();
>> +}
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC] Add Arm cpu topology definition
2011-06-16 8:49 [RFC] Add Arm cpu topology definition Vincent Guittot
2011-06-16 8:55 ` Samuel Thibault
2011-06-16 10:49 ` Daniel Lezcano
@ 2011-06-16 11:48 ` Amit Kucheria
2011-06-16 12:30 ` Vincent Guittot
2011-06-16 11:55 ` Amit Kachhap
` (3 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Amit Kucheria @ 2011-06-16 11:48 UTC (permalink / raw)
To: linux-arm-kernel
On 11 Jun 16, Vincent Guittot wrote:
> The affinity between Arm processors is defined in the MPIDR register.
> We can identify which processors are in the same cluster,
> and which ones have performance interdependency. The cpu topology
> of an Arm platform can be set thanks to this register and this topology
> is then used by sched_mc and sched_smt.
Probably worth adding a note that sched_mc/sched_smt are disabled by default
and need to be enabled by writing to their respective sysfs files.
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> arch/arm/Kconfig | 26 ++++++++
> arch/arm/include/asm/topology.h | 33 ++++++++++
> arch/arm/kernel/Makefile | 1 +
> arch/arm/kernel/smp.c | 6 ++
> arch/arm/kernel/topology.c | 133 +++++++++++++++++++++++++++++++++++++++
> 5 files changed, 199 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/kernel/topology.c
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 9adc278..bacf9af 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -219,6 +219,24 @@ source "kernel/Kconfig.freezer"
>
> menu "System Type"
>
> +config SCHED_MC
> + bool "Multi-core scheduler support"
> + depends on SMP && ARM_CPU_TOPOLOGY
> + default n
> + help
> + Multi-core scheduler support improves the CPU scheduler's decision
> + making when dealing with multi-core CPU chips at a cost of slightly
> + increased overhead in some places. If unsure say N here.
> +
> +config SCHED_SMT
> + bool "SMT scheduler support"
> + depends on SMP && ARM_CPU_TOPOLOGY
> + default n
> + help
> + Improves the CPU scheduler's decision making when dealing with
> + MultiThreading at a cost of slightly increased overhead in some
> + places. If unsure say N here.
> +
> config MMU
> bool "MMU-based Paged Memory Management Support"
> default y
> @@ -1062,6 +1080,14 @@ if !MMU
> source "arch/arm/Kconfig-nommu"
> endif
>
> +config ARM_CPU_TOPOLOGY
> + bool "Support cpu topology definition"
> + depends on SMP && CPU_V7
> + help
> + Support Arm cpu topology definition. The MPIDR register defines
> + affinity between processors which is used to set the cpu
> + topology of an Arm System.
> +
> config ARM_ERRATA_411920
> bool "ARM errata: Invalidation of the Instruction Cache operation can fail"
> depends on CPU_V6 || CPU_V6K
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index accbd7c..cb90d0a 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -1,6 +1,39 @@
> #ifndef _ASM_ARM_TOPOLOGY_H
> #define _ASM_ARM_TOPOLOGY_H
>
> +#ifdef CONFIG_ARM_CPU_TOPOLOGY
> +
> +#include <linux/cpumask.h>
> +
> +struct cputopo_arm {
> + int thread_id;
> + int core_id;
> + int socket_id;
> + cpumask_t thread_sibling;
> + cpumask_t core_sibling;
> +};
> +
> +extern struct cputopo_arm cpu_topology[NR_CPUS];
> +
> +#define topology_physical_package_id(cpu) (cpu_topology[cpu].socket_id)
> +#define topology_core_id(cpu) (cpu_topology[cpu].core_id)
> +#define topology_core_cpumask(cpu) (&(cpu_topology[cpu].core_sibling))
> +#define topology_thread_cpumask(cpu) (&(cpu_topology[cpu].thread_sibling))
> +
> +#define mc_capable() (cpu_topology[0].socket_id != -1)
> +#define smt_capable() (cpu_topology[0].thread_id != -1)
> +
> +void init_cpu_topology(void);
> +void store_cpu_topology(unsigned int cpuid);
> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu);
> +
> +#else
> +
> +#define init_cpu_topology() {};
> +#define store_cpu_topology(cpuid) {};
> +
> +#endif
> +
> #include <asm-generic/topology.h>
>
> #endif /* _ASM_ARM_TOPOLOGY_H */
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index a5b31af..816a481 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_IWMMXT) += iwmmxt.o
> obj-$(CONFIG_CPU_HAS_PMU) += pmu.o
> obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o
> AFLAGS_iwmmxt.o := -Wa,-mcpu=iwmmxt
> +obj-$(CONFIG_ARM_CPU_TOPOLOGY) += topology.o
>
> ifneq ($(CONFIG_ARCH_EBSA110),y)
> obj-y += io.o
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 344e52b..3e8dc3b 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -31,6 +31,7 @@
> #include <asm/cacheflush.h>
> #include <asm/cpu.h>
> #include <asm/cputype.h>
> +#include <asm/topology.h>
> #include <asm/mmu_context.h>
> #include <asm/pgtable.h>
> #include <asm/pgalloc.h>
> @@ -268,6 +269,9 @@ static void __cpuinit smp_store_cpu_info(unsigned int cpuid)
> struct cpuinfo_arm *cpu_info = &per_cpu(cpu_data, cpuid);
>
> cpu_info->loops_per_jiffy = loops_per_jiffy;
> +
> + store_cpu_topology(cpuid);
> +
> }
>
> /*
> @@ -354,6 +358,8 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> {
> unsigned int ncores = num_possible_cpus();
>
> + init_cpu_topology();
> +
> smp_store_cpu_info(smp_processor_id());
>
> /*
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> new file mode 100644
> index 0000000..d61723c
> --- /dev/null
> +++ b/arch/arm/kernel/topology.c
> @@ -0,0 +1,133 @@
> +/*
> + * arch/arm/kernel/topology.c
> + *
> + * Copyright (C) 2011 vincent.guittot at linaro.org
This should be
Copyright (C) 2011 Linaro Limited.
Written by: Vincent Guittot
> + *
> + * based on arch/sh/kernel/topology.c
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. See the file "COPYING" in the main directory of this archive
> + * for more details.
> + */
> +
> +#include <linux/cpu.h>
> +#include <linux/cpumask.h>
> +#include <linux/init.h>
> +#include <linux/percpu.h>
> +#include <linux/node.h>
> +#include <linux/nodemask.h>
> +#include <linux/sched.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/topology.h>
> +
> +#define hard_smp_mpidr() \
> + ({ \
> + unsigned int cpunum; \
> + __asm__("mrc p15, 0, %0, c0, c0, 5" \
> + : "=r" (cpunum)); \
> + cpunum; \
> + })
> +
> +struct cputopo_arm cpu_topology[NR_CPUS];
> +
> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu)
> +{
> + return &(cpu_topology[cpu].core_sibling);
> +}
> +
> +/*
> + * store_cpu_topology is called at boot when only one cpu is running
> + * and with the mutex cpu_hotplug.lock locked, when several cpus have booted,
> + * which prevents simultaneous write access to cpu_topology array
> + */
> +void store_cpu_topology(unsigned int cpuid)
> +{
> + struct cputopo_arm *cpuid_topo = &(cpu_topology[cpuid]);
> + unsigned int mpidr;
> + unsigned int cpu;
> +
> + /* If the cpu topology has been already set, just return */
> + if (cpuid_topo->core_id != -1)
> + return;
> +
> + mpidr = hard_smp_mpidr();
> +
> + /* create cpu topology mapping */
> + if (mpidr & (0x3 << 30)) {
Use #defines for the MPIDR bit-fields
#define MPIDR_U_BITMASK (0x3 << 30)
BTW, Bit 31 seems to always be 1, do you really want that in the mask?
> + /*
> + * This is a multiprocessor system
> + * multiprocessor format & multiprocessor mode field are set
> + */
> +
> + if (mpidr & (0x1 << 24)) {
According to
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388f/CIHCHADB.html
bit 24 is SBZ. Where is this redefined for MT?
> + /* core performance interdependency */
> + cpuid_topo->thread_id = (mpidr & 0x3);
> + cpuid_topo->core_id = ((mpidr >> 8) & 0xF);
> + cpuid_topo->socket_id = ((mpidr >> 16) & 0xFF);
Define BITMASKs for these bits too.
> + } else {
> + /* normal core interdependency */
> + cpuid_topo->thread_id = -1;
> + cpuid_topo->core_id = (mpidr & 0x3);
> + cpuid_topo->socket_id = ((mpidr >> 8) & 0xF);
> + }
> + } else {
> + /*
> + * This is an uniprocessor system
> + * we are in multiprocessor format but uniprocessor system
> + * or in the old uniprocessor format
> + */
> +
> + cpuid_topo->thread_id = -1;
> + cpuid_topo->core_id = 0;
> + cpuid_topo->socket_id = -1;
> + }
> +
> + /* update core and thread sibling masks */
> + for_each_possible_cpu(cpu) {
> + struct cputopo_arm *cpu_topo = &(cpu_topology[cpu]);
> +
> + if (cpuid_topo->socket_id == cpu_topo->socket_id) {
> + cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
> + if (cpu != cpuid)
> + cpumask_set_cpu(cpu,
> + &cpuid_topo->core_sibling);
> +
> + if (cpuid_topo->core_id == cpu_topo->core_id) {
> + cpumask_set_cpu(cpuid,
> + &cpu_topo->thread_sibling);
> + if (cpu != cpuid)
> + cpumask_set_cpu(cpu,
> + &cpuid_topo->thread_sibling);
> + }
> + }
> + }
> + smp_wmb();
> +
> + printk(KERN_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].socket_id, mpidr);
> +
> +}
> +
> +/*
> + * init_cpu_topology is called at boot when only one cpu is running
> + * which prevent simultaneous write access to cpu_topology array
> + */
> +void init_cpu_topology(void)
> +{
> + unsigned int cpu;
> +
> + /* init core mask */
> + for_each_possible_cpu(cpu) {
> + struct cputopo_arm *cpu_topo = &(cpu_topology[cpu]);
> +
> + cpu_topo->thread_id = -1;
> + cpu_topo->core_id = -1;
> + cpu_topo->socket_id = -1;
> + cpumask_clear(&cpu_topo->core_sibling);
> + cpumask_clear(&cpu_topo->thread_sibling);
> + }
> + smp_wmb();
> +}
> --
> 1.7.4.1
>
^ permalink raw reply [flat|nested] 20+ messages in thread* [RFC] Add Arm cpu topology definition
2011-06-16 11:48 ` Amit Kucheria
@ 2011-06-16 12:30 ` Vincent Guittot
0 siblings, 0 replies; 20+ messages in thread
From: Vincent Guittot @ 2011-06-16 12:30 UTC (permalink / raw)
To: linux-arm-kernel
On 16 June 2011 13:48, Amit Kucheria <amit.kucheria@linaro.org> wrote:
> On 11 Jun 16, Vincent Guittot wrote:
>> The affinity between Arm processors is defined in the MPIDR register.
>> We can identify which processors are in the same cluster,
>> and which ones have performance interdependency. The cpu topology
>> ?of an Arm platform can be set thanks to this register and this topology
>> is then used by sched_mc and sched_smt.
>
> Probably worth adding a note that sched_mc/sched_smt are disabled by default
> and need to be enabled by writing to their respective sysfs files.
>
ok
>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>> ---
>> ?arch/arm/Kconfig ? ? ? ? ? ? ? ?| ? 26 ++++++++
>> ?arch/arm/include/asm/topology.h | ? 33 ++++++++++
>> ?arch/arm/kernel/Makefile ? ? ? ?| ? ?1 +
>> ?arch/arm/kernel/smp.c ? ? ? ? ? | ? ?6 ++
>> ?arch/arm/kernel/topology.c ? ? ?| ?133 +++++++++++++++++++++++++++++++++++++++
>> ?5 files changed, 199 insertions(+), 0 deletions(-)
>> ?create mode 100644 arch/arm/kernel/topology.c
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 9adc278..bacf9af 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -219,6 +219,24 @@ source "kernel/Kconfig.freezer"
>>
>> ?menu "System Type"
>>
>> +config SCHED_MC
>> + ? ? bool "Multi-core scheduler support"
>> + ? ? depends on SMP && ARM_CPU_TOPOLOGY
>> + ? ? default n
>> + ? ? help
>> + ? ? ? Multi-core scheduler support improves the CPU scheduler's decision
>> + ? ? ? making when dealing with multi-core CPU chips at a cost of slightly
>> + ? ? ? increased overhead in some places. If unsure say N here.
>> +
>> +config SCHED_SMT
>> + ? ? bool "SMT scheduler support"
>> + ? ? depends on SMP && ARM_CPU_TOPOLOGY
>> + ? ? default n
>> + ? ? help
>> + ? ? ? Improves the CPU scheduler's decision making when dealing with
>> + ? ? ? MultiThreading at a cost of slightly increased overhead in some
>> + ? ? ? places. If unsure say N here.
>> +
>> ?config MMU
>> ? ? ? bool "MMU-based Paged Memory Management Support"
>> ? ? ? default y
>> @@ -1062,6 +1080,14 @@ if !MMU
>> ?source "arch/arm/Kconfig-nommu"
>> ?endif
>>
>> +config ARM_CPU_TOPOLOGY
>> + ? ? bool "Support cpu topology definition"
>> + ? ? depends on SMP && CPU_V7
>> + ? ? help
>> + ? ? ? Support Arm cpu topology definition. The MPIDR register defines
>> + ? ? ? affinity between processors which is used to set the cpu
>> + ? ? ? topology of an Arm System.
>> +
>> ?config ARM_ERRATA_411920
>> ? ? ? bool "ARM errata: Invalidation of the Instruction Cache operation can fail"
>> ? ? ? depends on CPU_V6 || CPU_V6K
>> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
>> index accbd7c..cb90d0a 100644
>> --- a/arch/arm/include/asm/topology.h
>> +++ b/arch/arm/include/asm/topology.h
>> @@ -1,6 +1,39 @@
>> ?#ifndef _ASM_ARM_TOPOLOGY_H
>> ?#define _ASM_ARM_TOPOLOGY_H
>>
>> +#ifdef CONFIG_ARM_CPU_TOPOLOGY
>> +
>> +#include <linux/cpumask.h>
>> +
>> +struct cputopo_arm {
>> + ? ? int thread_id;
>> + ? ? int core_id;
>> + ? ? int socket_id;
>> + ? ? cpumask_t thread_sibling;
>> + ? ? cpumask_t core_sibling;
>> +};
>> +
>> +extern struct cputopo_arm cpu_topology[NR_CPUS];
>> +
>> +#define topology_physical_package_id(cpu) ? ?(cpu_topology[cpu].socket_id)
>> +#define topology_core_id(cpu) ? ? ? ? ? ? ? ?(cpu_topology[cpu].core_id)
>> +#define topology_core_cpumask(cpu) ? (&(cpu_topology[cpu].core_sibling))
>> +#define topology_thread_cpumask(cpu) (&(cpu_topology[cpu].thread_sibling))
>> +
>> +#define mc_capable() (cpu_topology[0].socket_id != -1)
>> +#define smt_capable() ? ? ? ?(cpu_topology[0].thread_id != -1)
>> +
>> +void init_cpu_topology(void);
>> +void store_cpu_topology(unsigned int cpuid);
>> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu);
>> +
>> +#else
>> +
>> +#define init_cpu_topology() {};
>> +#define store_cpu_topology(cpuid) {};
>> +
>> +#endif
>> +
>> ?#include <asm-generic/topology.h>
>>
>> ?#endif /* _ASM_ARM_TOPOLOGY_H */
>> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
>> index a5b31af..816a481 100644
>> --- a/arch/arm/kernel/Makefile
>> +++ b/arch/arm/kernel/Makefile
>> @@ -61,6 +61,7 @@ obj-$(CONFIG_IWMMXT) ? ? ? ? ? ? ? ?+= iwmmxt.o
>> ?obj-$(CONFIG_CPU_HAS_PMU) ? ?+= pmu.o
>> ?obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o
>> ?AFLAGS_iwmmxt.o ? ? ? ? ? ? ? ? ? ? ?:= -Wa,-mcpu=iwmmxt
>> +obj-$(CONFIG_ARM_CPU_TOPOLOGY) ?+= topology.o
>>
>> ?ifneq ($(CONFIG_ARCH_EBSA110),y)
>> ? ?obj-y ? ? ? ? ? ? ?+= io.o
>> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
>> index 344e52b..3e8dc3b 100644
>> --- a/arch/arm/kernel/smp.c
>> +++ b/arch/arm/kernel/smp.c
>> @@ -31,6 +31,7 @@
>> ?#include <asm/cacheflush.h>
>> ?#include <asm/cpu.h>
>> ?#include <asm/cputype.h>
>> +#include <asm/topology.h>
>> ?#include <asm/mmu_context.h>
>> ?#include <asm/pgtable.h>
>> ?#include <asm/pgalloc.h>
>> @@ -268,6 +269,9 @@ static void __cpuinit smp_store_cpu_info(unsigned int cpuid)
>> ? ? ? struct cpuinfo_arm *cpu_info = &per_cpu(cpu_data, cpuid);
>>
>> ? ? ? cpu_info->loops_per_jiffy = loops_per_jiffy;
>> +
>> + ? ? store_cpu_topology(cpuid);
>> +
>> ?}
>>
>> ?/*
>> @@ -354,6 +358,8 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>> ?{
>> ? ? ? unsigned int ncores = num_possible_cpus();
>>
>> + ? ? init_cpu_topology();
>> +
>> ? ? ? smp_store_cpu_info(smp_processor_id());
>>
>> ? ? ? /*
>> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
>> new file mode 100644
>> index 0000000..d61723c
>> --- /dev/null
>> +++ b/arch/arm/kernel/topology.c
>> @@ -0,0 +1,133 @@
>> +/*
>> + * arch/arm/kernel/topology.c
>> + *
>> + * Copyright (C) 2011 ?vincent.guittot at linaro.org
>
> This should be
>
> Copyright (C) 2011 Linaro Limited.
>
> Written by: Vincent Guittot
>
ok
>> + *
>> + * based on arch/sh/kernel/topology.c
>> + *
>> + * This file is subject to the terms and conditions of the GNU General Public
>> + * License. ?See the file "COPYING" in the main directory of this archive
>> + * for more details.
>> + */
>> +
>> +#include <linux/cpu.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/init.h>
>> +#include <linux/percpu.h>
>> +#include <linux/node.h>
>> +#include <linux/nodemask.h>
>> +#include <linux/sched.h>
>> +
>> +#include <asm/cacheflush.h>
>> +#include <asm/topology.h>
>> +
>> +#define hard_smp_mpidr() \
>> + ? ? ({ \
>> + ? ? ? ? ? ? unsigned int cpunum; \
>> + ? ? ? ? ? ? __asm__("mrc p15, 0, %0, c0, c0, 5" ? ? \
>> + ? ? ? ? ? ? ? ? ? ? : "=r" (cpunum)); \
>> + ? ? ? ? ? ? cpunum; \
>> + ? ? })
>> +
>> +struct cputopo_arm cpu_topology[NR_CPUS];
>> +
>> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu)
>> +{
>> + ? ? return &(cpu_topology[cpu].core_sibling);
>> +}
>> +
>> +/*
>> + * store_cpu_topology is called at boot when only one cpu is running
>> + * and with the mutex cpu_hotplug.lock locked, when several cpus have booted,
>> + * which prevents simultaneous write access to cpu_topology array
>> + */
>> +void store_cpu_topology(unsigned int cpuid)
>> +{
>> + ? ? struct cputopo_arm *cpuid_topo = &(cpu_topology[cpuid]);
>> + ? ? unsigned int mpidr;
>> + ? ? unsigned int cpu;
>> +
>> + ? ? /* If the cpu topology has been already set, just return */
>> + ? ? if (cpuid_topo->core_id != -1)
>> + ? ? ? ? ? ? return;
>> +
>> + ? ? mpidr = hard_smp_mpidr();
>> +
>> + ? ? /* create cpu topology mapping */
>> + ? ? if (mpidr & (0x3 << 30)) {
>
> Use #defines for the MPIDR bit-fields
>
> #define MPIDR_U_BITMASK (0x3 << 30)
>
> BTW, Bit 31 seems to always be 1, do you really want that in the mask?
>
According to
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0406b/index.html
we need to test both bit 31 and bit 24
>> + ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ?* This is a multiprocessor system
>> + ? ? ? ? ? ? ?* multiprocessor format & multiprocessor mode field are set
>> + ? ? ? ? ? ? ?*/
>> +
>> + ? ? ? ? ? ? if (mpidr & (0x1 << 24)) {
>
> According to
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388f/CIHCHADB.html
>
> bit 24 is SBZ. Where is this redefined for MT?
>
>> + ? ? ? ? ? ? ? ? ? ? /* core performance interdependency */
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = (mpidr & 0x3);
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = ?((mpidr >> 8) & 0xF);
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr >> 16) & 0xFF);
>
> Define BITMASKs for these bits too.
>
ok
>> + ? ? ? ? ? ? } else {
>> + ? ? ? ? ? ? ? ? ? ? /* normal core interdependency */
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = -1;
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = (mpidr & 0x3);
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr >> 8) & 0xF);
>> + ? ? ? ? ? ? }
>> + ? ? } else {
>> + ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ?* This is an uniprocessor system
>> + ? ? ? ? ? ? ?* we are in multiprocessor format but uniprocessor system
>> + ? ? ? ? ? ? ?* or in the old uniprocessor format
>> + ? ? ? ? ? ? ?*/
>> +
>> + ? ? ? ? ? ? cpuid_topo->thread_id = -1;
>> + ? ? ? ? ? ? cpuid_topo->core_id = 0;
>> + ? ? ? ? ? ? cpuid_topo->socket_id = -1;
>> + ? ? }
>> +
>> + ? ? /* update core and thread sibling masks */
>> + ? ? for_each_possible_cpu(cpu) {
>> + ? ? ? ? ? ? struct cputopo_arm *cpu_topo = &(cpu_topology[cpu]);
>> +
>> + ? ? ? ? ? ? if (cpuid_topo->socket_id == cpu_topo->socket_id) {
>> + ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
>> + ? ? ? ? ? ? ? ? ? ? if (cpu != cpuid)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpu,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cpuid_topo->core_sibling);
>> +
>> + ? ? ? ? ? ? ? ? ? ? if (cpuid_topo->core_id == cpu_topo->core_id) {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpuid,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cpu_topo->thread_sibling);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (cpu != cpuid)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpu,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cpuid_topo->thread_sibling);
>> + ? ? ? ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? }
>> + ? ? }
>> + ? ? smp_wmb();
>> +
>> + ? ? printk(KERN_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].socket_id, mpidr);
>> +
>> +}
>> +
>> +/*
>> + * init_cpu_topology is called at boot when only one cpu is running
>> + * which prevent simultaneous write access to cpu_topology array
>> + */
>> +void init_cpu_topology(void)
>> +{
>> + ? ? unsigned int cpu;
>> +
>> + ? ? /* init core mask */
>> + ? ? for_each_possible_cpu(cpu) {
>> + ? ? ? ? ? ? struct cputopo_arm *cpu_topo = &(cpu_topology[cpu]);
>> +
>> + ? ? ? ? ? ? cpu_topo->thread_id = -1;
>> + ? ? ? ? ? ? cpu_topo->core_id = ?-1;
>> + ? ? ? ? ? ? cpu_topo->socket_id = -1;
>> + ? ? ? ? ? ? cpumask_clear(&cpu_topo->core_sibling);
>> + ? ? ? ? ? ? cpumask_clear(&cpu_topo->thread_sibling);
>> + ? ? }
>> + ? ? smp_wmb();
>> +}
>> --
>> 1.7.4.1
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC] Add Arm cpu topology definition
2011-06-16 8:49 [RFC] Add Arm cpu topology definition Vincent Guittot
` (2 preceding siblings ...)
2011-06-16 11:48 ` Amit Kucheria
@ 2011-06-16 11:55 ` Amit Kachhap
2011-06-16 12:10 ` Vincent Guittot
2011-06-16 13:24 ` Christian Robottom Reis
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Amit Kachhap @ 2011-06-16 11:55 UTC (permalink / raw)
To: linux-arm-kernel
I have some doubts about the bit fields of the MPIDR register.
Comments added below.
On 16 June 2011 14:19, Vincent Guittot <vincent.guittot@linaro.org> wrote:
> The affinity between Arm processors is defined in the MPIDR register.
> We can identify which processors are in the same cluster,
> and which ones have performance interdependency. The cpu topology
> ?of an Arm platform can be set thanks to this register and this topology
> is then used by sched_mc and sched_smt.
>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> ?arch/arm/Kconfig ? ? ? ? ? ? ? ?| ? 26 ++++++++
> ?arch/arm/include/asm/topology.h | ? 33 ++++++++++
> ?arch/arm/kernel/Makefile ? ? ? ?| ? ?1 +
> ?arch/arm/kernel/smp.c ? ? ? ? ? | ? ?6 ++
> ?arch/arm/kernel/topology.c ? ? ?| ?133 +++++++++++++++++++++++++++++++++++++++
> ?5 files changed, 199 insertions(+), 0 deletions(-)
> ?create mode 100644 arch/arm/kernel/topology.c
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 9adc278..bacf9af 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -219,6 +219,24 @@ source "kernel/Kconfig.freezer"
>
> ?menu "System Type"
>
> +config SCHED_MC
> + ? ? ? bool "Multi-core scheduler support"
> + ? ? ? depends on SMP && ARM_CPU_TOPOLOGY
> + ? ? ? default n
> + ? ? ? help
> + ? ? ? ? Multi-core scheduler support improves the CPU scheduler's decision
> + ? ? ? ? making when dealing with multi-core CPU chips at a cost of slightly
> + ? ? ? ? increased overhead in some places. If unsure say N here.
> +
> +config SCHED_SMT
> + ? ? ? bool "SMT scheduler support"
> + ? ? ? depends on SMP && ARM_CPU_TOPOLOGY
> + ? ? ? default n
> + ? ? ? help
> + ? ? ? ? Improves the CPU scheduler's decision making when dealing with
> + ? ? ? ? MultiThreading at a cost of slightly increased overhead in some
> + ? ? ? ? places. If unsure say N here.
> +
> ?config MMU
> ? ? ? ?bool "MMU-based Paged Memory Management Support"
> ? ? ? ?default y
> @@ -1062,6 +1080,14 @@ if !MMU
> ?source "arch/arm/Kconfig-nommu"
> ?endif
>
> +config ARM_CPU_TOPOLOGY
> + ? ? ? bool "Support cpu topology definition"
> + ? ? ? depends on SMP && CPU_V7
> + ? ? ? help
> + ? ? ? ? Support Arm cpu topology definition. The MPIDR register defines
> + ? ? ? ? affinity between processors which is used to set the cpu
> + ? ? ? ? topology of an Arm System.
> +
> ?config ARM_ERRATA_411920
> ? ? ? ?bool "ARM errata: Invalidation of the Instruction Cache operation can fail"
> ? ? ? ?depends on CPU_V6 || CPU_V6K
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index accbd7c..cb90d0a 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -1,6 +1,39 @@
> ?#ifndef _ASM_ARM_TOPOLOGY_H
> ?#define _ASM_ARM_TOPOLOGY_H
>
> +#ifdef CONFIG_ARM_CPU_TOPOLOGY
> +
> +#include <linux/cpumask.h>
> +
> +struct cputopo_arm {
> + ? ? ? int thread_id;
> + ? ? ? int core_id;
> + ? ? ? int socket_id;
> + ? ? ? cpumask_t thread_sibling;
> + ? ? ? cpumask_t core_sibling;
> +};
> +
> +extern struct cputopo_arm cpu_topology[NR_CPUS];
> +
> +#define topology_physical_package_id(cpu) ? ? ?(cpu_topology[cpu].socket_id)
> +#define topology_core_id(cpu) ? ? ? ? ?(cpu_topology[cpu].core_id)
> +#define topology_core_cpumask(cpu) ? ? (&(cpu_topology[cpu].core_sibling))
> +#define topology_thread_cpumask(cpu) ? (&(cpu_topology[cpu].thread_sibling))
> +
> +#define mc_capable() ? (cpu_topology[0].socket_id != -1)
> +#define smt_capable() ?(cpu_topology[0].thread_id != -1)
> +
> +void init_cpu_topology(void);
> +void store_cpu_topology(unsigned int cpuid);
> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu);
> +
> +#else
> +
> +#define init_cpu_topology() {};
> +#define store_cpu_topology(cpuid) {};
> +
> +#endif
> +
> ?#include <asm-generic/topology.h>
>
> ?#endif /* _ASM_ARM_TOPOLOGY_H */
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index a5b31af..816a481 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_IWMMXT) ? ? ? ? ?+= iwmmxt.o
> ?obj-$(CONFIG_CPU_HAS_PMU) ? ? ?+= pmu.o
> ?obj-$(CONFIG_HW_PERF_EVENTS) ? += perf_event.o
> ?AFLAGS_iwmmxt.o ? ? ? ? ? ? ? ? ? ? ? ?:= -Wa,-mcpu=iwmmxt
> +obj-$(CONFIG_ARM_CPU_TOPOLOGY) ?+= topology.o
>
> ?ifneq ($(CONFIG_ARCH_EBSA110),y)
> ? obj-y ? ? ? ? ? ? ? ?+= io.o
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 344e52b..3e8dc3b 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -31,6 +31,7 @@
> ?#include <asm/cacheflush.h>
> ?#include <asm/cpu.h>
> ?#include <asm/cputype.h>
> +#include <asm/topology.h>
> ?#include <asm/mmu_context.h>
> ?#include <asm/pgtable.h>
> ?#include <asm/pgalloc.h>
> @@ -268,6 +269,9 @@ static void __cpuinit smp_store_cpu_info(unsigned int cpuid)
> ? ? ? ?struct cpuinfo_arm *cpu_info = &per_cpu(cpu_data, cpuid);
>
> ? ? ? ?cpu_info->loops_per_jiffy = loops_per_jiffy;
> +
> + ? ? ? store_cpu_topology(cpuid);
> +
> ?}
>
> ?/*
> @@ -354,6 +358,8 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> ?{
> ? ? ? ?unsigned int ncores = num_possible_cpus();
>
> + ? ? ? init_cpu_topology();
> +
> ? ? ? ?smp_store_cpu_info(smp_processor_id());
>
> ? ? ? ?/*
> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
> new file mode 100644
> index 0000000..d61723c
> --- /dev/null
> +++ b/arch/arm/kernel/topology.c
> @@ -0,0 +1,133 @@
> +/*
> + * arch/arm/kernel/topology.c
> + *
> + * Copyright (C) 2011 ?vincent.guittot at linaro.org
> + *
> + * based on arch/sh/kernel/topology.c
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. ?See the file "COPYING" in the main directory of this archive
> + * for more details.
> + */
> +
> +#include <linux/cpu.h>
> +#include <linux/cpumask.h>
> +#include <linux/init.h>
> +#include <linux/percpu.h>
> +#include <linux/node.h>
> +#include <linux/nodemask.h>
> +#include <linux/sched.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/topology.h>
> +
> +#define hard_smp_mpidr() \
> + ? ? ? ({ \
> + ? ? ? ? ? ? ? unsigned int cpunum; \
> + ? ? ? ? ? ? ? __asm__("mrc p15, 0, %0, c0, c0, 5" ? ? \
> + ? ? ? ? ? ? ? ? ? ? ? : "=r" (cpunum)); \
> + ? ? ? ? ? ? ? cpunum; \
> + ? ? ? })
> +
> +struct cputopo_arm cpu_topology[NR_CPUS];
> +
> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu)
> +{
> + ? ? ? return &(cpu_topology[cpu].core_sibling);
> +}
> +
> +/*
> + * store_cpu_topology is called at boot when only one cpu is running
> + * and with the mutex cpu_hotplug.lock locked, when several cpus have booted,
> + * which prevents simultaneous write access to cpu_topology array
> + */
> +void store_cpu_topology(unsigned int cpuid)
> +{
> + ? ? ? struct cputopo_arm *cpuid_topo = &(cpu_topology[cpuid]);
> + ? ? ? unsigned int mpidr;
> + ? ? ? unsigned int cpu;
> +
> + ? ? ? /* If the cpu topology has been already set, just return */
> + ? ? ? if (cpuid_topo->core_id != -1)
> + ? ? ? ? ? ? ? return;
> +
> + ? ? ? mpidr = hard_smp_mpidr();
> +
> + ? ? ? /* create cpu topology mapping */
> + ? ? ? if (mpidr & (0x3 << 30)) {
I checked the A9 reference manual. It says bit 30 if 0 denotes that processor is
part of multiprocessor system.
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* This is a multiprocessor system
> + ? ? ? ? ? ? ? ?* multiprocessor format & multiprocessor mode field are set
> + ? ? ? ? ? ? ? ?*/
> +
> + ? ? ? ? ? ? ? if (mpidr & (0x1 << 24)) {
> + ? ? ? ? ? ? ? ? ? ? ? /* core performance interdependency */
> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = (mpidr & 0x3);
> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = ?((mpidr >> 8) & 0xF);
> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr >> 16) & 0xFF);
> + ? ? ? ? ? ? ? } else {
> + ? ? ? ? ? ? ? ? ? ? ? /* normal core interdependency */
> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = -1;
> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = (mpidr & 0x3);
> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr >> 8) & 0xF);
> + ? ? ? ? ? ? ? }
> + ? ? ? } else {
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* This is an uniprocessor system
> + ? ? ? ? ? ? ? ?* we are in multiprocessor format but uniprocessor system
> + ? ? ? ? ? ? ? ?* or in the old uniprocessor format
> + ? ? ? ? ? ? ? ?*/
> +
> + ? ? ? ? ? ? ? cpuid_topo->thread_id = -1;
> + ? ? ? ? ? ? ? cpuid_topo->core_id = 0;
> + ? ? ? ? ? ? ? cpuid_topo->socket_id = -1;
> + ? ? ? }
> +
> + ? ? ? /* update core and thread sibling masks */
> + ? ? ? for_each_possible_cpu(cpu) {
> + ? ? ? ? ? ? ? struct cputopo_arm *cpu_topo = &(cpu_topology[cpu]);
> +
> + ? ? ? ? ? ? ? if (cpuid_topo->socket_id == cpu_topo->socket_id) {
> + ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
> + ? ? ? ? ? ? ? ? ? ? ? if (cpu != cpuid)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpu,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cpuid_topo->core_sibling);
> +
> + ? ? ? ? ? ? ? ? ? ? ? if (cpuid_topo->core_id == cpu_topo->core_id) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpuid,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cpu_topo->thread_sibling);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (cpu != cpuid)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpu,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cpuid_topo->thread_sibling);
> + ? ? ? ? ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? }
> + ? ? ? }
> + ? ? ? smp_wmb();
> +
> + ? ? ? printk(KERN_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].socket_id, mpidr);
> +
> +}
> +
> +/*
> + * init_cpu_topology is called at boot when only one cpu is running
> + * which prevent simultaneous write access to cpu_topology array
> + */
> +void init_cpu_topology(void)
> +{
> + ? ? ? unsigned int cpu;
> +
> + ? ? ? /* init core mask */
> + ? ? ? for_each_possible_cpu(cpu) {
> + ? ? ? ? ? ? ? struct cputopo_arm *cpu_topo = &(cpu_topology[cpu]);
> +
> + ? ? ? ? ? ? ? cpu_topo->thread_id = -1;
> + ? ? ? ? ? ? ? cpu_topo->core_id = ?-1;
> + ? ? ? ? ? ? ? cpu_topo->socket_id = -1;
> + ? ? ? ? ? ? ? cpumask_clear(&cpu_topo->core_sibling);
> + ? ? ? ? ? ? ? cpumask_clear(&cpu_topo->thread_sibling);
> + ? ? ? }
> + ? ? ? smp_wmb();
> +}
> --
> 1.7.4.1
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>
^ permalink raw reply [flat|nested] 20+ messages in thread* [RFC] Add Arm cpu topology definition
2011-06-16 11:55 ` Amit Kachhap
@ 2011-06-16 12:10 ` Vincent Guittot
0 siblings, 0 replies; 20+ messages in thread
From: Vincent Guittot @ 2011-06-16 12:10 UTC (permalink / raw)
To: linux-arm-kernel
On 16 June 2011 13:55, Amit Kachhap <amit.kachhap@linaro.org> wrote:
> I have some doubts about the bit fields of the MPIDR register.
> Comments added below.
> On 16 June 2011 14:19, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>> The affinity between Arm processors is defined in the MPIDR register.
>> We can identify which processors are in the same cluster,
>> and which ones have performance interdependency. The cpu topology
>> ?of an Arm platform can be set thanks to this register and this topology
>> is then used by sched_mc and sched_smt.
>>
>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>> ---
>> ?arch/arm/Kconfig ? ? ? ? ? ? ? ?| ? 26 ++++++++
>> ?arch/arm/include/asm/topology.h | ? 33 ++++++++++
>> ?arch/arm/kernel/Makefile ? ? ? ?| ? ?1 +
>> ?arch/arm/kernel/smp.c ? ? ? ? ? | ? ?6 ++
>> ?arch/arm/kernel/topology.c ? ? ?| ?133 +++++++++++++++++++++++++++++++++++++++
>> ?5 files changed, 199 insertions(+), 0 deletions(-)
>> ?create mode 100644 arch/arm/kernel/topology.c
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 9adc278..bacf9af 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -219,6 +219,24 @@ source "kernel/Kconfig.freezer"
>>
>> ?menu "System Type"
>>
>> +config SCHED_MC
>> + ? ? ? bool "Multi-core scheduler support"
>> + ? ? ? depends on SMP && ARM_CPU_TOPOLOGY
>> + ? ? ? default n
>> + ? ? ? help
>> + ? ? ? ? Multi-core scheduler support improves the CPU scheduler's decision
>> + ? ? ? ? making when dealing with multi-core CPU chips at a cost of slightly
>> + ? ? ? ? increased overhead in some places. If unsure say N here.
>> +
>> +config SCHED_SMT
>> + ? ? ? bool "SMT scheduler support"
>> + ? ? ? depends on SMP && ARM_CPU_TOPOLOGY
>> + ? ? ? default n
>> + ? ? ? help
>> + ? ? ? ? Improves the CPU scheduler's decision making when dealing with
>> + ? ? ? ? MultiThreading at a cost of slightly increased overhead in some
>> + ? ? ? ? places. If unsure say N here.
>> +
>> ?config MMU
>> ? ? ? ?bool "MMU-based Paged Memory Management Support"
>> ? ? ? ?default y
>> @@ -1062,6 +1080,14 @@ if !MMU
>> ?source "arch/arm/Kconfig-nommu"
>> ?endif
>>
>> +config ARM_CPU_TOPOLOGY
>> + ? ? ? bool "Support cpu topology definition"
>> + ? ? ? depends on SMP && CPU_V7
>> + ? ? ? help
>> + ? ? ? ? Support Arm cpu topology definition. The MPIDR register defines
>> + ? ? ? ? affinity between processors which is used to set the cpu
>> + ? ? ? ? topology of an Arm System.
>> +
>> ?config ARM_ERRATA_411920
>> ? ? ? ?bool "ARM errata: Invalidation of the Instruction Cache operation can fail"
>> ? ? ? ?depends on CPU_V6 || CPU_V6K
>> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
>> index accbd7c..cb90d0a 100644
>> --- a/arch/arm/include/asm/topology.h
>> +++ b/arch/arm/include/asm/topology.h
>> @@ -1,6 +1,39 @@
>> ?#ifndef _ASM_ARM_TOPOLOGY_H
>> ?#define _ASM_ARM_TOPOLOGY_H
>>
>> +#ifdef CONFIG_ARM_CPU_TOPOLOGY
>> +
>> +#include <linux/cpumask.h>
>> +
>> +struct cputopo_arm {
>> + ? ? ? int thread_id;
>> + ? ? ? int core_id;
>> + ? ? ? int socket_id;
>> + ? ? ? cpumask_t thread_sibling;
>> + ? ? ? cpumask_t core_sibling;
>> +};
>> +
>> +extern struct cputopo_arm cpu_topology[NR_CPUS];
>> +
>> +#define topology_physical_package_id(cpu) ? ? ?(cpu_topology[cpu].socket_id)
>> +#define topology_core_id(cpu) ? ? ? ? ?(cpu_topology[cpu].core_id)
>> +#define topology_core_cpumask(cpu) ? ? (&(cpu_topology[cpu].core_sibling))
>> +#define topology_thread_cpumask(cpu) ? (&(cpu_topology[cpu].thread_sibling))
>> +
>> +#define mc_capable() ? (cpu_topology[0].socket_id != -1)
>> +#define smt_capable() ?(cpu_topology[0].thread_id != -1)
>> +
>> +void init_cpu_topology(void);
>> +void store_cpu_topology(unsigned int cpuid);
>> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu);
>> +
>> +#else
>> +
>> +#define init_cpu_topology() {};
>> +#define store_cpu_topology(cpuid) {};
>> +
>> +#endif
>> +
>> ?#include <asm-generic/topology.h>
>>
>> ?#endif /* _ASM_ARM_TOPOLOGY_H */
>> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
>> index a5b31af..816a481 100644
>> --- a/arch/arm/kernel/Makefile
>> +++ b/arch/arm/kernel/Makefile
>> @@ -61,6 +61,7 @@ obj-$(CONFIG_IWMMXT) ? ? ? ? ?+= iwmmxt.o
>> ?obj-$(CONFIG_CPU_HAS_PMU) ? ? ?+= pmu.o
>> ?obj-$(CONFIG_HW_PERF_EVENTS) ? += perf_event.o
>> ?AFLAGS_iwmmxt.o ? ? ? ? ? ? ? ? ? ? ? ?:= -Wa,-mcpu=iwmmxt
>> +obj-$(CONFIG_ARM_CPU_TOPOLOGY) ?+= topology.o
>>
>> ?ifneq ($(CONFIG_ARCH_EBSA110),y)
>> ? obj-y ? ? ? ? ? ? ? ?+= io.o
>> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
>> index 344e52b..3e8dc3b 100644
>> --- a/arch/arm/kernel/smp.c
>> +++ b/arch/arm/kernel/smp.c
>> @@ -31,6 +31,7 @@
>> ?#include <asm/cacheflush.h>
>> ?#include <asm/cpu.h>
>> ?#include <asm/cputype.h>
>> +#include <asm/topology.h>
>> ?#include <asm/mmu_context.h>
>> ?#include <asm/pgtable.h>
>> ?#include <asm/pgalloc.h>
>> @@ -268,6 +269,9 @@ static void __cpuinit smp_store_cpu_info(unsigned int cpuid)
>> ? ? ? ?struct cpuinfo_arm *cpu_info = &per_cpu(cpu_data, cpuid);
>>
>> ? ? ? ?cpu_info->loops_per_jiffy = loops_per_jiffy;
>> +
>> + ? ? ? store_cpu_topology(cpuid);
>> +
>> ?}
>>
>> ?/*
>> @@ -354,6 +358,8 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>> ?{
>> ? ? ? ?unsigned int ncores = num_possible_cpus();
>>
>> + ? ? ? init_cpu_topology();
>> +
>> ? ? ? ?smp_store_cpu_info(smp_processor_id());
>>
>> ? ? ? ?/*
>> diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
>> new file mode 100644
>> index 0000000..d61723c
>> --- /dev/null
>> +++ b/arch/arm/kernel/topology.c
>> @@ -0,0 +1,133 @@
>> +/*
>> + * arch/arm/kernel/topology.c
>> + *
>> + * Copyright (C) 2011 ?vincent.guittot at linaro.org
>> + *
>> + * based on arch/sh/kernel/topology.c
>> + *
>> + * This file is subject to the terms and conditions of the GNU General Public
>> + * License. ?See the file "COPYING" in the main directory of this archive
>> + * for more details.
>> + */
>> +
>> +#include <linux/cpu.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/init.h>
>> +#include <linux/percpu.h>
>> +#include <linux/node.h>
>> +#include <linux/nodemask.h>
>> +#include <linux/sched.h>
>> +
>> +#include <asm/cacheflush.h>
>> +#include <asm/topology.h>
>> +
>> +#define hard_smp_mpidr() \
>> + ? ? ? ({ \
>> + ? ? ? ? ? ? ? unsigned int cpunum; \
>> + ? ? ? ? ? ? ? __asm__("mrc p15, 0, %0, c0, c0, 5" ? ? \
>> + ? ? ? ? ? ? ? ? ? ? ? : "=r" (cpunum)); \
>> + ? ? ? ? ? ? ? cpunum; \
>> + ? ? ? })
>> +
>> +struct cputopo_arm cpu_topology[NR_CPUS];
>> +
>> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu)
>> +{
>> + ? ? ? return &(cpu_topology[cpu].core_sibling);
>> +}
>> +
>> +/*
>> + * store_cpu_topology is called at boot when only one cpu is running
>> + * and with the mutex cpu_hotplug.lock locked, when several cpus have booted,
>> + * which prevents simultaneous write access to cpu_topology array
>> + */
>> +void store_cpu_topology(unsigned int cpuid)
>> +{
>> + ? ? ? struct cputopo_arm *cpuid_topo = &(cpu_topology[cpuid]);
>> + ? ? ? unsigned int mpidr;
>> + ? ? ? unsigned int cpu;
>> +
>> + ? ? ? /* If the cpu topology has been already set, just return */
>> + ? ? ? if (cpuid_topo->core_id != -1)
>> + ? ? ? ? ? ? ? return;
>> +
>> + ? ? ? mpidr = hard_smp_mpidr();
>> +
>> + ? ? ? /* create cpu topology mapping */
>> + ? ? ? if (mpidr & (0x3 << 30)) {
>
> I checked the A9 reference manual. It says bit 30 if 0 denotes that processor is
> part of multiprocessor system.
>
You're right, i'm going to update my test
>
>> + ? ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ? ?* This is a multiprocessor system
>> + ? ? ? ? ? ? ? ?* multiprocessor format & multiprocessor mode field are set
>> + ? ? ? ? ? ? ? ?*/
>> +
>> + ? ? ? ? ? ? ? if (mpidr & (0x1 << 24)) {
>> + ? ? ? ? ? ? ? ? ? ? ? /* core performance interdependency */
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = (mpidr & 0x3);
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = ?((mpidr >> 8) & 0xF);
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr >> 16) & 0xFF);
>> + ? ? ? ? ? ? ? } else {
>> + ? ? ? ? ? ? ? ? ? ? ? /* normal core interdependency */
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = -1;
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = (mpidr & 0x3);
>> + ? ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr >> 8) & 0xF);
>> + ? ? ? ? ? ? ? }
>> + ? ? ? } else {
>> + ? ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ? ?* This is an uniprocessor system
>> + ? ? ? ? ? ? ? ?* we are in multiprocessor format but uniprocessor system
>> + ? ? ? ? ? ? ? ?* or in the old uniprocessor format
>> + ? ? ? ? ? ? ? ?*/
>> +
>> + ? ? ? ? ? ? ? cpuid_topo->thread_id = -1;
>> + ? ? ? ? ? ? ? cpuid_topo->core_id = 0;
>> + ? ? ? ? ? ? ? cpuid_topo->socket_id = -1;
>> + ? ? ? }
>> +
>> + ? ? ? /* update core and thread sibling masks */
>> + ? ? ? for_each_possible_cpu(cpu) {
>> + ? ? ? ? ? ? ? struct cputopo_arm *cpu_topo = &(cpu_topology[cpu]);
>> +
>> + ? ? ? ? ? ? ? if (cpuid_topo->socket_id == cpu_topo->socket_id) {
>> + ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
>> + ? ? ? ? ? ? ? ? ? ? ? if (cpu != cpuid)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpu,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cpuid_topo->core_sibling);
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? if (cpuid_topo->core_id == cpu_topo->core_id) {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpuid,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cpu_topo->thread_sibling);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (cpu != cpuid)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cpumask_set_cpu(cpu,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cpuid_topo->thread_sibling);
>> + ? ? ? ? ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? }
>> + ? ? ? }
>> + ? ? ? smp_wmb();
>> +
>> + ? ? ? printk(KERN_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].socket_id, mpidr);
>> +
>> +}
>> +
>> +/*
>> + * init_cpu_topology is called at boot when only one cpu is running
>> + * which prevent simultaneous write access to cpu_topology array
>> + */
>> +void init_cpu_topology(void)
>> +{
>> + ? ? ? unsigned int cpu;
>> +
>> + ? ? ? /* init core mask */
>> + ? ? ? for_each_possible_cpu(cpu) {
>> + ? ? ? ? ? ? ? struct cputopo_arm *cpu_topo = &(cpu_topology[cpu]);
>> +
>> + ? ? ? ? ? ? ? cpu_topo->thread_id = -1;
>> + ? ? ? ? ? ? ? cpu_topo->core_id = ?-1;
>> + ? ? ? ? ? ? ? cpu_topo->socket_id = -1;
>> + ? ? ? ? ? ? ? cpumask_clear(&cpu_topo->core_sibling);
>> + ? ? ? ? ? ? ? cpumask_clear(&cpu_topo->thread_sibling);
>> + ? ? ? }
>> + ? ? ? smp_wmb();
>> +}
>> --
>> 1.7.4.1
>>
>> _______________________________________________
>> linaro-dev mailing list
>> linaro-dev at lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/linaro-dev
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC] Add Arm cpu topology definition
2011-06-16 8:49 [RFC] Add Arm cpu topology definition Vincent Guittot
` (3 preceding siblings ...)
2011-06-16 11:55 ` Amit Kachhap
@ 2011-06-16 13:24 ` Christian Robottom Reis
2011-06-16 13:48 ` Vincent Guittot
2011-06-16 19:40 ` Stephen Boyd
2011-06-16 21:13 ` Russell King - ARM Linux
6 siblings, 1 reply; 20+ messages in thread
From: Christian Robottom Reis @ 2011-06-16 13:24 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jun 16, 2011 at 10:49:13AM +0200, Vincent Guittot wrote:
> @@ -219,6 +219,24 @@ source "kernel/Kconfig.freezer"
>
> menu "System Type"
>
> +config SCHED_MC
> + bool "Multi-core scheduler support"
> + depends on SMP && ARM_CPU_TOPOLOGY
> + default n
> + help
> + Multi-core scheduler support improves the CPU scheduler's decision
> + making when dealing with multi-core CPU chips at a cost of slightly
> + increased overhead in some places. If unsure say N here.
> +
> +config SCHED_SMT
> + bool "SMT scheduler support"
> + depends on SMP && ARM_CPU_TOPOLOGY
> + default n
> + help
> + Improves the CPU scheduler's decision making when dealing with
> + MultiThreading at a cost of slightly increased overhead in some
> + places. If unsure say N here.
> +
Quick question: the patch doesn't actually seem to use the SCHED_MC and
SCHED_SMT config bits; is that actually correct?
--
Christian Robottom Reis | [+55] 16 9112 6430 | http://launchpad.net/~kiko
Linaro Engineering VP | [ +1] 612 216 4935 | http://async.com.br/~kiko
^ permalink raw reply [flat|nested] 20+ messages in thread* [RFC] Add Arm cpu topology definition
2011-06-16 13:24 ` Christian Robottom Reis
@ 2011-06-16 13:48 ` Vincent Guittot
0 siblings, 0 replies; 20+ messages in thread
From: Vincent Guittot @ 2011-06-16 13:48 UTC (permalink / raw)
To: linux-arm-kernel
On 16 June 2011 15:24, Christian Robottom Reis <kiko@linaro.org> wrote:
> On Thu, Jun 16, 2011 at 10:49:13AM +0200, Vincent Guittot wrote:
>> @@ -219,6 +219,24 @@ source "kernel/Kconfig.freezer"
>>
>> ?menu "System Type"
>>
>> +config SCHED_MC
>> + ? ? bool "Multi-core scheduler support"
>> + ? ? depends on SMP && ARM_CPU_TOPOLOGY
>> + ? ? default n
>> + ? ? help
>> + ? ? ? Multi-core scheduler support improves the CPU scheduler's decision
>> + ? ? ? making when dealing with multi-core CPU chips at a cost of slightly
>> + ? ? ? increased overhead in some places. If unsure say N here.
>> +
>> +config SCHED_SMT
>> + ? ? bool "SMT scheduler support"
>> + ? ? depends on SMP && ARM_CPU_TOPOLOGY
>> + ? ? default n
>> + ? ? help
>> + ? ? ? Improves the CPU scheduler's decision making when dealing with
>> + ? ? ? MultiThreading at a cost of slightly increased overhead in some
>> + ? ? ? places. If unsure say N here.
>> +
>
> Quick question: the patch doesn't actually seem to use the SCHED_MC and
> SCHED_SMT config bits; is that actually correct?
The default config of the patch is to disable sched_mc and sched_smt.
The 1st goal is to let each platform enable and test the feature.
According to feedback, we could then change the default state of the
configuration
> --
> Christian Robottom Reis ? | [+55] 16 9112 6430 | http://launchpad.net/~kiko
> Linaro Engineering VP ? ? | [ +1] 612 216 4935 | http://async.com.br/~kiko
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC] Add Arm cpu topology definition
2011-06-16 8:49 [RFC] Add Arm cpu topology definition Vincent Guittot
` (4 preceding siblings ...)
2011-06-16 13:24 ` Christian Robottom Reis
@ 2011-06-16 19:40 ` Stephen Boyd
2011-06-17 6:54 ` Vincent Guittot
2011-06-16 21:13 ` Russell King - ARM Linux
6 siblings, 1 reply; 20+ messages in thread
From: Stephen Boyd @ 2011-06-16 19:40 UTC (permalink / raw)
To: linux-arm-kernel
On 06/16/2011 01:49 AM, Vincent Guittot wrote:
> +config SCHED_MC
> + bool "Multi-core scheduler support"
> + depends on SMP && ARM_CPU_TOPOLOGY
> + default n
> + help
> + Multi-core scheduler support improves the CPU scheduler's decision
> + making when dealing with multi-core CPU chips at a cost of slightly
> + increased overhead in some places. If unsure say N here.
> +
> +config SCHED_SMT
> + bool "SMT scheduler support"
> + depends on SMP && ARM_CPU_TOPOLOGY
> + default n
> + help
> + Improves the CPU scheduler's decision making when dealing with
> + MultiThreading at a cost of slightly increased overhead in some
> + places. If unsure say N here.
> +
The default is already n so you can drop those two lines.
> + * This is a multiprocessor system
> + * multiprocessor format & multiprocessor mode field are set
> + */
> +
> + if (mpidr & (0x1 << 24)) {
> + /* core performance interdependency */
> + cpuid_topo->thread_id = (mpidr & 0x3);
> + cpuid_topo->core_id = ((mpidr >> 8) & 0xF);
> + cpuid_topo->socket_id = ((mpidr >> 16) & 0xFF);
> + } else {
> + /* normal core interdependency */
> + cpuid_topo->thread_id = -1;
> + cpuid_topo->core_id = (mpidr & 0x3);
> + cpuid_topo->socket_id = ((mpidr >> 8) & 0xF);
> + }
> +
The ARM ARM says these fields are IMPLEMENTATION DEFINED meaning that
different vendors may attribute different meaning to these fields if
they wish. Does that mean this should be a platform_*() function?
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 20+ messages in thread* [RFC] Add Arm cpu topology definition
2011-06-16 19:40 ` Stephen Boyd
@ 2011-06-17 6:54 ` Vincent Guittot
2011-06-21 20:36 ` Stephen Boyd
0 siblings, 1 reply; 20+ messages in thread
From: Vincent Guittot @ 2011-06-17 6:54 UTC (permalink / raw)
To: linux-arm-kernel
On 16 June 2011 21:40, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/16/2011 01:49 AM, Vincent Guittot wrote:
>> +config SCHED_MC
>> + ? ? bool "Multi-core scheduler support"
>> + ? ? depends on SMP && ARM_CPU_TOPOLOGY
>> + ? ? default n
>> + ? ? help
>> + ? ? ? Multi-core scheduler support improves the CPU scheduler's decision
>> + ? ? ? making when dealing with multi-core CPU chips at a cost of slightly
>> + ? ? ? increased overhead in some places. If unsure say N here.
>> +
>> +config SCHED_SMT
>> + ? ? bool "SMT scheduler support"
>> + ? ? depends on SMP && ARM_CPU_TOPOLOGY
>> + ? ? default n
>> + ? ? help
>> + ? ? ? Improves the CPU scheduler's decision making when dealing with
>> + ? ? ? MultiThreading at a cost of slightly increased overhead in some
>> + ? ? ? places. If unsure say N here.
>> +
>
> The default is already n so you can drop those two lines.
>
ok
>> + ? ? ? ? ? ? ?* This is a multiprocessor system
>> + ? ? ? ? ? ? ?* multiprocessor format & multiprocessor mode field are set
>> + ? ? ? ? ? ? ?*/
>> +
>> + ? ? ? ? ? ? if (mpidr & (0x1 << 24)) {
>> + ? ? ? ? ? ? ? ? ? ? /* core performance interdependency */
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = (mpidr & 0x3);
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = ?((mpidr >> 8) & 0xF);
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr >> 16) & 0xFF);
>> + ? ? ? ? ? ? } else {
>> + ? ? ? ? ? ? ? ? ? ? /* normal core interdependency */
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = -1;
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = (mpidr & 0x3);
>> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr >> 8) & 0xF);
>> + ? ? ? ? ? ? }
>> +
>
> The ARM ARM says these fields are IMPLEMENTATION DEFINED meaning that
> different vendors may attribute different meaning to these fields if
> they wish. Does that mean this should be a platform_*() function?
>
The ARM ARM also provides a recommended use of the fields of this
register and the TRM of each Cortex adds some details. On the cortex
A9, each platform can only set the value of the Cluster ID with the
CLUSTERID pins. I have tried to consolidate the value of MPIDR across
several platforms and they all match with the description.
Have you got an example of a MPIDR register which doesn't match with
the implementation ?
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* [RFC] Add Arm cpu topology definition
2011-06-17 6:54 ` Vincent Guittot
@ 2011-06-21 20:36 ` Stephen Boyd
2011-06-22 9:17 ` Catalin Marinas
0 siblings, 1 reply; 20+ messages in thread
From: Stephen Boyd @ 2011-06-21 20:36 UTC (permalink / raw)
To: linux-arm-kernel
On 06/16/2011 11:54 PM, Vincent Guittot wrote:
> On 16 June 2011 21:40, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> The ARM ARM says these fields are IMPLEMENTATION DEFINED meaning that
>> different vendors may attribute different meaning to these fields if
>> they wish. Does that mean this should be a platform_*() function?
>>
> The ARM ARM also provides a recommended use of the fields of this
> register and the TRM of each Cortex adds some details. On the cortex
> A9, each platform can only set the value of the Cluster ID with the
> CLUSTERID pins. I have tried to consolidate the value of MPIDR across
> several platforms and they all match with the description.
>
> Have you got an example of a MPIDR register which doesn't match with
> the implementation ?
Not that I know of. I'm more concerned with how the ARM ARM has two
recommended usages for these fields depending on virtualization or not.
I suppose we can handle that issue when it arises (or does your
implementation already handle that?)
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC] Add Arm cpu topology definition
2011-06-21 20:36 ` Stephen Boyd
@ 2011-06-22 9:17 ` Catalin Marinas
2011-06-22 10:19 ` Amit Kucheria
0 siblings, 1 reply; 20+ messages in thread
From: Catalin Marinas @ 2011-06-22 9:17 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 21, 2011 at 01:36:15PM -0700, Stephen Boyd wrote:
> On 06/16/2011 11:54 PM, Vincent Guittot wrote:
> > On 16 June 2011 21:40, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >> The ARM ARM says these fields are IMPLEMENTATION DEFINED meaning that
> >> different vendors may attribute different meaning to these fields if
> >> they wish. Does that mean this should be a platform_*() function?
> >>
> > The ARM ARM also provides a recommended use of the fields of this
> > register and the TRM of each Cortex adds some details. On the cortex
> > A9, each platform can only set the value of the Cluster ID with the
> > CLUSTERID pins. I have tried to consolidate the value of MPIDR across
> > several platforms and they all match with the description.
> >
> > Have you got an example of a MPIDR register which doesn't match with
> > the implementation ?
>
> Not that I know of. I'm more concerned with how the ARM ARM has two
> recommended usages for these fields depending on virtualization or not.
> I suppose we can handle that issue when it arises (or does your
> implementation already handle that?)
According to the ARM ARM:
MPIDR provides a mechanism with up to three levels of affinity
information, but the meaning of those levels of affinity is
entirely IMPLEMENTATION DEFINED.
So we can't really tell the meaning of the affinity bits. There are two
recommended ways indeed (with or without virtualisation) which are not
that different with regards to the topology (just introducing another
level for virtual CPUs).
But I think a more general solution would be for the CPU topology to be
provided via the FDT.
--
Catalin
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC] Add Arm cpu topology definition
2011-06-22 9:17 ` Catalin Marinas
@ 2011-06-22 10:19 ` Amit Kucheria
0 siblings, 0 replies; 20+ messages in thread
From: Amit Kucheria @ 2011-06-22 10:19 UTC (permalink / raw)
To: linux-arm-kernel
On 11 Jun 22, Catalin Marinas wrote:
> On Tue, Jun 21, 2011 at 01:36:15PM -0700, Stephen Boyd wrote:
> > On 06/16/2011 11:54 PM, Vincent Guittot wrote:
> > > On 16 June 2011 21:40, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > >> The ARM ARM says these fields are IMPLEMENTATION DEFINED meaning that
> > >> different vendors may attribute different meaning to these fields if
> > >> they wish. Does that mean this should be a platform_*() function?
> > >>
> > > The ARM ARM also provides a recommended use of the fields of this
> > > register and the TRM of each Cortex adds some details. On the cortex
> > > A9, each platform can only set the value of the Cluster ID with the
> > > CLUSTERID pins. I have tried to consolidate the value of MPIDR across
> > > several platforms and they all match with the description.
> > >
> > > Have you got an example of a MPIDR register which doesn't match with
> > > the implementation ?
> >
> > Not that I know of. I'm more concerned with how the ARM ARM has two
> > recommended usages for these fields depending on virtualization or not.
> > I suppose we can handle that issue when it arises (or does your
> > implementation already handle that?)
>
> According to the ARM ARM:
>
> MPIDR provides a mechanism with up to three levels of affinity
> information, but the meaning of those levels of affinity is
> entirely IMPLEMENTATION DEFINED.
>
> So we can't really tell the meaning of the affinity bits. There are two
> recommended ways indeed (with or without virtualisation) which are not
> that different with regards to the topology (just introducing another
> level for virtual CPUs).
>
> But I think a more general solution would be for the CPU topology to be
> provided via the FDT.
Agreed. That will be the next step.
We decided on doing it this way to allow non-DT-enabled platforms to be able
to use the feature and to allow DT-enabled platforms to settle down in the
mean time.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC] Add Arm cpu topology definition
2011-06-16 8:49 [RFC] Add Arm cpu topology definition Vincent Guittot
` (5 preceding siblings ...)
2011-06-16 19:40 ` Stephen Boyd
@ 2011-06-16 21:13 ` Russell King - ARM Linux
2011-06-17 7:43 ` Vincent Guittot
6 siblings, 1 reply; 20+ messages in thread
From: Russell King - ARM Linux @ 2011-06-16 21:13 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jun 16, 2011 at 10:49:13AM +0200, Vincent Guittot wrote:
> @@ -219,6 +219,24 @@ source "kernel/Kconfig.freezer"
>
> menu "System Type"
>
> +config SCHED_MC
> + bool "Multi-core scheduler support"
> + depends on SMP && ARM_CPU_TOPOLOGY
> + default n
> + help
> + Multi-core scheduler support improves the CPU scheduler's decision
> + making when dealing with multi-core CPU chips at a cost of slightly
> + increased overhead in some places. If unsure say N here.
> +
> +config SCHED_SMT
> + bool "SMT scheduler support"
> + depends on SMP && ARM_CPU_TOPOLOGY
> + default n
> + help
> + Improves the CPU scheduler's decision making when dealing with
> + MultiThreading at a cost of slightly increased overhead in some
> + places. If unsure say N here.
Why place these in the "system type" menu? Wouldn't it be better to
place them along side ARM_CPU_TOPOLOGY, and place that along side
the SMP option too?
> +
> config MMU
> bool "MMU-based Paged Memory Management Support"
> default y
> @@ -1062,6 +1080,14 @@ if !MMU
> source "arch/arm/Kconfig-nommu"
> endif
>
> +config ARM_CPU_TOPOLOGY
> + bool "Support cpu topology definition"
> + depends on SMP && CPU_V7
> + help
> + Support Arm cpu topology definition. The MPIDR register defines
> + affinity between processors which is used to set the cpu
> + topology of an Arm System.
Is there a reason you'd want to disable this?
Please also note that it's "ARM" not "Arm".
> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
> index accbd7c..cb90d0a 100644
> --- a/arch/arm/include/asm/topology.h
> +++ b/arch/arm/include/asm/topology.h
> @@ -1,6 +1,39 @@
> #ifndef _ASM_ARM_TOPOLOGY_H
> #define _ASM_ARM_TOPOLOGY_H
>
> +#ifdef CONFIG_ARM_CPU_TOPOLOGY
> +
> +#include <linux/cpumask.h>
> +
> +struct cputopo_arm {
> + int thread_id;
> + int core_id;
> + int socket_id;
> + cpumask_t thread_sibling;
> + cpumask_t core_sibling;
> +};
> +
> +extern struct cputopo_arm cpu_topology[NR_CPUS];
> +
> +#define topology_physical_package_id(cpu) (cpu_topology[cpu].socket_id)
> +#define topology_core_id(cpu) (cpu_topology[cpu].core_id)
> +#define topology_core_cpumask(cpu) (&(cpu_topology[cpu].core_sibling))
> +#define topology_thread_cpumask(cpu) (&(cpu_topology[cpu].thread_sibling))
The thing which naggs me about this is that its a static array, and we
know from x86 that static arrays of per-cpu data have various issues
(cache line bouncing, as well as limitations when we end up with large
numbers of CPUs.)
Lastly, x86 seems to do this:
#define arch_provides_topology_pointers yes
and the only effect I can find of that define is in drivers/base/topology.c:
#ifdef arch_provides_topology_pointers
...
unsigned int cpu = dev->id; \
return show_cpumap(0, topology_##name(cpu), buf); \
...
#else
...
return show_cpumap(0, topology_##name(dev->id), buf); \
...
#endif
dev->id is type 'u32' which devolves to 'unsigned int' on all supported
arches. So it looks to me like this arch_provides_topology_pointers
define is completely pointless and we just have useless obfuscation for
the hell of it.
That's not a criticism of your patch, it's pointing out a difference
between x86 and our implementation.
> +#include <linux/cpu.h>
> +#include <linux/cpumask.h>
> +#include <linux/init.h>
> +#include <linux/percpu.h>
> +#include <linux/node.h>
> +#include <linux/nodemask.h>
> +#include <linux/sched.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/topology.h>
> +
> +#define hard_smp_mpidr() \
> + ({ \
> + unsigned int cpunum; \
> + __asm__("mrc p15, 0, %0, c0, c0, 5" \
> + : "=r" (cpunum)); \
> + cpunum; \
> + })
Please add a definition for CPUID_MPIDR to arch/arm/include/asm/cputype.h
and a read_cpuid_mpidr() function, and use that instead.
^ permalink raw reply [flat|nested] 20+ messages in thread* [RFC] Add Arm cpu topology definition
2011-06-16 21:13 ` Russell King - ARM Linux
@ 2011-06-17 7:43 ` Vincent Guittot
0 siblings, 0 replies; 20+ messages in thread
From: Vincent Guittot @ 2011-06-17 7:43 UTC (permalink / raw)
To: linux-arm-kernel
On 16 June 2011 23:13, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Thu, Jun 16, 2011 at 10:49:13AM +0200, Vincent Guittot wrote:
>> @@ -219,6 +219,24 @@ source "kernel/Kconfig.freezer"
>>
>> ?menu "System Type"
>>
>> +config SCHED_MC
>> + ? ? bool "Multi-core scheduler support"
>> + ? ? depends on SMP && ARM_CPU_TOPOLOGY
>> + ? ? default n
>> + ? ? help
>> + ? ? ? Multi-core scheduler support improves the CPU scheduler's decision
>> + ? ? ? making when dealing with multi-core CPU chips at a cost of slightly
>> + ? ? ? increased overhead in some places. If unsure say N here.
>> +
>> +config SCHED_SMT
>> + ? ? bool "SMT scheduler support"
>> + ? ? depends on SMP && ARM_CPU_TOPOLOGY
>> + ? ? default n
>> + ? ? help
>> + ? ? ? Improves the CPU scheduler's decision making when dealing with
>> + ? ? ? MultiThreading at a cost of slightly increased overhead in some
>> + ? ? ? places. If unsure say N here.
>
> Why place these in the "system type" menu? ?Wouldn't it be better to
> place them along side ARM_CPU_TOPOLOGY, and place that along side
> the SMP option too?
>
yes, it's a better place
>> +
>> ?config MMU
>> ? ? ? bool "MMU-based Paged Memory Management Support"
>> ? ? ? default y
>> @@ -1062,6 +1080,14 @@ if !MMU
>> ?source "arch/arm/Kconfig-nommu"
>> ?endif
>>
>> +config ARM_CPU_TOPOLOGY
>> + ? ? bool "Support cpu topology definition"
>> + ? ? depends on SMP && CPU_V7
>> + ? ? help
>> + ? ? ? Support Arm cpu topology definition. The MPIDR register defines
>> + ? ? ? affinity between processors which is used to set the cpu
>> + ? ? ? topology of an Arm System.
>
> Is there a reason you'd want to disable this?
>
In fact, I only want to disable sched_mc and sched_smt. I'm going to
add default y for ARM_CPU_TOPOLOGY
> Please also note that it's "ARM" not "Arm".
OK
>
>> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h
>> index accbd7c..cb90d0a 100644
>> --- a/arch/arm/include/asm/topology.h
>> +++ b/arch/arm/include/asm/topology.h
>> @@ -1,6 +1,39 @@
>> ?#ifndef _ASM_ARM_TOPOLOGY_H
>> ?#define _ASM_ARM_TOPOLOGY_H
>>
>> +#ifdef CONFIG_ARM_CPU_TOPOLOGY
>> +
>> +#include <linux/cpumask.h>
>> +
>> +struct cputopo_arm {
>> + ? ? int thread_id;
>> + ? ? int core_id;
>> + ? ? int socket_id;
>> + ? ? cpumask_t thread_sibling;
>> + ? ? cpumask_t core_sibling;
>> +};
>> +
>> +extern struct cputopo_arm cpu_topology[NR_CPUS];
>> +
>> +#define topology_physical_package_id(cpu) ? ?(cpu_topology[cpu].socket_id)
>> +#define topology_core_id(cpu) ? ? ? ? ? ? ? ?(cpu_topology[cpu].core_id)
>> +#define topology_core_cpumask(cpu) ? (&(cpu_topology[cpu].core_sibling))
>> +#define topology_thread_cpumask(cpu) (&(cpu_topology[cpu].thread_sibling))
>
> The thing which naggs me about this is that its a static array, and we
> know from x86 that static arrays of per-cpu data have various issues
> (cache line bouncing, as well as limitations when we end up with large
> numbers of CPUs.)
>
The array is updated sequentially by each processor during boot. Then,
it should be used by one cpu when building the sched_domain and when
calling the topology sysfs entry.
We should not have so many cases where several cpu are accessing
simultaneously the cells of the array.
> Lastly, x86 seems to do this:
>
> #define arch_provides_topology_pointers ? ? ? ? yes
>
> and the only effect I can find of that define is in drivers/base/topology.c:
>
> #ifdef arch_provides_topology_pointers
> ...
> ? ? ? ?unsigned int cpu = dev->id; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> ? ? ? ?return show_cpumap(0, topology_##name(cpu), buf); ? ? ? ? ? ? ? \
> ...
> #else
> ...
> ? ? ? ?return show_cpumap(0, topology_##name(dev->id), buf); ? ? ? ? ? \
> ...
> #endif
>
> dev->id is type 'u32' which devolves to 'unsigned int' on all supported
> arches. ?So it looks to me like this arch_provides_topology_pointers
> define is completely pointless and we just have useless obfuscation for
> the hell of it.
>
> That's not a criticism of your patch, it's pointing out a difference
> between x86 and our implementation.
>
I haven't set arch_provides_topology_pointers because I can't find a
difference between setting it or not
>> +#include <linux/cpu.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/init.h>
>> +#include <linux/percpu.h>
>> +#include <linux/node.h>
>> +#include <linux/nodemask.h>
>> +#include <linux/sched.h>
>> +
>> +#include <asm/cacheflush.h>
>> +#include <asm/topology.h>
>> +
>> +#define hard_smp_mpidr() \
>> + ? ? ({ \
>> + ? ? ? ? ? ? unsigned int cpunum; \
>> + ? ? ? ? ? ? __asm__("mrc p15, 0, %0, c0, c0, 5" ? ? \
>> + ? ? ? ? ? ? ? ? ? ? : "=r" (cpunum)); \
>> + ? ? ? ? ? ? cpunum; \
>> + ? ? })
>
> Please add a definition for CPUID_MPIDR to arch/arm/include/asm/cputype.h
> and a read_cpuid_mpidr() function, and use that instead.
>
ok
I can directly use read_cpuid with a definition for CPUID_MPIDR
^ permalink raw reply [flat|nested] 20+ messages in thread