Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753438Ab1FPKtK (ORCPT ); Thu, 16 Jun 2011 06:49:10 -0400 Received: from mail-ww0-f44.google.com ([74.125.82.44]:41858 "EHLO mail-ww0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751260Ab1FPKtH (ORCPT ); Thu, 16 Jun 2011 06:49:07 -0400 Message-ID: <4DF9DF9E.4070004@linaro.org> Date: Thu, 16 Jun 2011 12:49:02 +0200 From: Daniel Lezcano User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110424 Thunderbird/3.1.10 MIME-Version: 1.0 To: Vincent Guittot CC: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linaro-dev@lists.linaro.org Subject: Re: [RFC] Add Arm cpu topology definition References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9113 Lines: 300 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 > --- > 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 > + > +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 > > #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 > #include > #include > +#include > #include > #include > #include > @@ -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(); > +} -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/