| Message ID | BANLkTinRGRUxKB=C_2Y2F93czWScEUo+Jw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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 >
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(); > +}
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@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 >
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@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@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-dev >
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(); >> +} > >
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@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@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/linaro-dev >> >
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@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 >> >
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?
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 >
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?
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.
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. > >
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
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?)
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.
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.
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@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)) { + /* + * 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(); +}
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