Re: [RFC PATCH v2] Add ARM cpu topology definition

Some nits below.

After you fix them, you can add a Reviewed-by: Amit Kucheria amit.kucheria@linaro.org

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 vincent.guittot@linaro.org

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 <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))

These #defines are not used anywhere.

+#define mc_capable() (cpu_topology[0].socket_id != -1) +#define smt_capable() (cpu_topology[0].thread_id != -1)

Neither are these.

+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 <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..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 <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/cputype.h> +#include <asm/topology.h>

+#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.

+#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?

• 	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.

• 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