Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757598Ab1FVMU5 (ORCPT ); Wed, 22 Jun 2011 08:20:57 -0400 Received: from mail-iy0-f174.google.com ([209.85.210.174]:60692 "EHLO mail-iy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753924Ab1FVMUz convert rfc822-to-8bit (ORCPT ); Wed, 22 Jun 2011 08:20:55 -0400 MIME-Version: 1.0 In-Reply-To: <20110622101423.GB2402@matterhorn1> References: <1308568795-1985-1-git-send-email-vincent.guittot@linaro.org> <20110622101423.GB2402@matterhorn1> Date: Wed, 22 Jun 2011 14:20:54 +0200 Message-ID: Subject: Re: [RFC PATCH v2] Add ARM cpu topology definition From: Vincent Guittot To: Vincent Guittot , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linaro-dev@lists.linaro.org, patches@linaro.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13293 Lines: 380 On 22 June 2011 12:14, Amit Kucheria wrote: > Some nits below. > > After you fix them, you can add a > Reviewed-by: Amit Kucheria > > On 11 Jun 20, 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.We can define the > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ^^ space between full-stop > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? and next word >> cpu topology of ARM platform, which is used by sched_mc and sched_smt. > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^^^^^^^^ change to 'that is then' >> The default state of sched_mc and sched_smt config is disable. > > Change to "sched_mc and sched_smt is disabled by default" > >> When enable, the behavior of the scheduler can be modified with > ? ? ? ? ? ? ^ enabled >> sched_mc_power_savings and sched_smt_power_savings sysfs interfaces. >> >> Changes since v1 : >> * Update the commit message >> * Add read_cpuid_mpidr in arch/arm/include/asm/cputype.h >> * Modify header of arch/arm/kernel/topology.c >> * Modify tests and manipulation of MPIDR's bitfields >> * Modify the place and dependancy of the config >> * Modify Noop functions >> >> Signed-off-by: Vincent Guittot >> --- >> ?arch/arm/Kconfig ? ? ? ? ? ? ? ?| ? 25 +++++++ >> ?arch/arm/include/asm/cputype.h ?| ? ?6 ++ >> ?arch/arm/include/asm/topology.h | ? 33 +++++++++ >> ?arch/arm/kernel/Makefile ? ? ? ?| ? ?1 + >> ?arch/arm/kernel/smp.c ? ? ? ? ? | ? ?6 ++ >> ?arch/arm/kernel/topology.c ? ? ?| ?145 +++++++++++++++++++++++++++++++++++++++ >> ?6 files changed, 216 insertions(+), 0 deletions(-) >> ?create mode 100644 arch/arm/kernel/topology.c >> >> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig >> index 9adc278..c7b4ff9 100644 >> --- a/arch/arm/Kconfig >> +++ b/arch/arm/Kconfig >> @@ -1344,6 +1344,31 @@ config SMP_ON_UP >> >> ? ? ? ? If you don't know what to do here, say Y. >> >> +config ARM_CPU_TOPOLOGY >> + ? ? bool "Support cpu topology definition" >> + ? ? depends on SMP && CPU_V7 >> + ? ? default y >> + ? ? help >> + ? ? ? Support ARM cpu topology definition. The MPIDR register defines >> + ? ? ? affinity between processors which is used to set the cpu > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^^^^^^^^^^^^^^^^^ change to 'which is then > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?used to describe the' >> + ? ? ? topology of an ARM System. >> + >> +config SCHED_MC >> + ? ? bool "Multi-core scheduler support" >> + ? ? depends on ARM_CPU_TOPOLOGY >> + ? ? 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 ARM_CPU_TOPOLOGY >> + ? ? 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 HAVE_ARM_SCU >> ? ? ? bool >> ? ? ? depends on SMP >> diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h >> index cd4458f..cb47d28 100644 >> --- a/arch/arm/include/asm/cputype.h >> +++ b/arch/arm/include/asm/cputype.h >> @@ -8,6 +8,7 @@ >> ?#define CPUID_CACHETYPE ? ? ?1 >> ?#define CPUID_TCM ? ?2 >> ?#define CPUID_TLBTYPE ? ? ? ?3 >> +#define CPUID_MPIDR ?5 >> >> ?#define CPUID_EXT_PFR0 ? ? ? "c1, 0" >> ?#define CPUID_EXT_PFR1 ? ? ? "c1, 1" >> @@ -70,6 +71,11 @@ static inline unsigned int __attribute_const__ read_cpuid_tcmstatus(void) >> ? ? ? return read_cpuid(CPUID_TCM); >> ?} >> >> +static inline unsigned int __attribute_const__ read_cpuid_mpidr(void) >> +{ >> + ? ? return read_cpuid(CPUID_MPIDR); >> +} >> + >> ?/* >> ? * Intel's XScale3 core supports some v6 features (supersections, L2) >> ? * but advertises itself as v5 as it does not support the v6 ISA. ?For >> diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h >> index accbd7c..63a7454 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; >> + ? ? 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)) > > These #defines are not used anywhere. They are used in topology.c driver and sched.c > >> +#define mc_capable() (cpu_topology[0].socket_id != -1) >> +#define smt_capable() ? ? ? ?(cpu_topology[0].thread_id != -1) > > Neither are these. They are used in sched.c > >> +void init_cpu_topology(void); >> +void store_cpu_topology(unsigned int cpuid); >> +const struct cpumask *cpu_coregroup_mask(unsigned int cpu); >> + >> +#else >> + >> +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); >> + >> ?} >> >> ?/* >> @@ -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..dc9e4be >> --- /dev/null >> +++ b/arch/arm/kernel/topology.c >> @@ -0,0 +1,145 @@ >> +/* >> + * arch/arm/kernel/topology.c >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> + >> +#define MPIDR_SMP_BITMASK (0x3 << 30) >> +#define MPIDR_SMP_VALUE (0x2 << 30) >> + >> +#define MPIDR_MT_BITMASK (0x1 << 24) >> + >> +#define MPIDR_LEVEL0_MASK 0x3 > > Perhaps add a comment reflecting the discussion on IRC. For Cortex-A9, only > 2 bits are supported for LEVEL0 and 4 bits for LEVEL1. In the future, each of > the LEVELS can use all 8 bits. > ok >> +#define MPIDR_LEVEL0_SHIFT 0 >> + >> +#define MPIDR_LEVEL1_MASK 0xF >> +#define MPIDR_LEVEL1_SHIFT 8 >> + >> +#define MPIDR_LEVEL2_MASK 0xFF >> +#define MPIDR_LEVEL2_SHIFT 16 >> + >> +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 = read_cpuid_mpidr(); >> + >> + ? ? /* create cpu topology mapping */ >> + ? ? if ((mpidr & MPIDR_SMP_BITMASK) == MPIDR_SMP_VALUE) { >> + ? ? ? ? ? ? /* >> + ? ? ? ? ? ? ?* This is a multiprocessor system >> + ? ? ? ? ? ? ?* multiprocessor format & multiprocessor mode field are set >> + ? ? ? ? ? ? ?*/ >> + >> + ? ? ? ? ? ? if (mpidr & MPIDR_MT_BITMASK) { >> + ? ? ? ? ? ? ? ? ? ? /* core performance interdependency */ >> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = ((mpidr >> MPIDR_LEVEL0_SHIFT) >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? & MPIDR_LEVEL0_MASK); >> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = ?((mpidr >> MPIDR_LEVEL1_SHIFT) >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? & MPIDR_LEVEL1_MASK); >> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr >> MPIDR_LEVEL2_SHIFT) >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? & MPIDR_LEVEL2_MASK); >> + ? ? ? ? ? ? } else { >> + ? ? ? ? ? ? ? ? ? ? /* normal core interdependency */ > ? ? ? ? ? ? ? ? ? ? ? ? ? ^^^^^^ Normal or No? ARM ARM says "largely independent". I can use this description > >> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->thread_id = -1; >> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->core_id = ((mpidr >> MPIDR_LEVEL0_SHIFT) >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? & MPIDR_LEVEL0_MASK); >> + ? ? ? ? ? ? ? ? ? ? cpuid_topo->socket_id = ((mpidr >> MPIDR_LEVEL1_SHIFT) >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? & MPIDR_LEVEL1_MASK); >> + ? ? ? ? ? ? } >> + ? ? } else { >> + ? ? ? ? ? ? /* >> + ? ? ? ? ? ? ?* This is an uniprocessor system >> + ? ? ? ? ? ? ?* we are in multiprocessor format but uniprocessor system >> + ? ? ? ? ? ? ?* or in the old uniprocessor format >> + ? ? ? ? ? ? ?*/ > > These are 3 different conditions, I think. Separate with commas. we are either in the "multiprocessor register format but an uniprocessor" or in the "old uniprocessor register 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 >> > -- 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/