lkml.org 
[lkml]   [2011]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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
    >


    \
     
     \ /
      Last update: 2011-06-22 12:17    [W:0.043 / U:123.884 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site