| Message ID | 1386767606-6391-4-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 11, 2013 at 01:13:24PM +0000, Mark Brown wrote: > From: Mark Brown <broonie@linaro.org> > > Add basic CPU topology support to arm64, based on the existing pre-v8 > code and some work done by Mark Hambleton. This patch does not > implement the ARM CPU topology bindings, it implements equivalent > support to the existing the equivalent pre-v8 capability using the > mandatory MPIDR information in the CPU binding in device tree and > assuming that a simple SMP or multi-cluster topology is in use. > > The primary goal is to separate the architecture hookup for providing > topology information from the DT parsing in order to ease review and > avoid blocking the architecture code (which will be built on by other > work) with the DT code review by providing something something simple > and basic. Having this support should also make the kernel cope better > with incomplete DTs. > > Further patches will provide support for overriding this using the > topology bindings, providing richer support for a wider range of systems. > > Signed-off-by: Mark Brown <broonie@linaro.org> > --- > arch/arm64/Kconfig | 8 +++ > arch/arm64/include/asm/cpu.h | 1 - > arch/arm64/include/asm/cputype.h | 9 +++ > arch/arm64/include/asm/smp_plat.h | 1 + > arch/arm64/include/asm/topology.h | 42 +++++++++++ > arch/arm64/kernel/Makefile | 1 + > arch/arm64/kernel/setup.c | 9 +-- > arch/arm64/kernel/smp.c | 19 ++++- > arch/arm64/kernel/topology.c | 143 ++++++++++++++++++++++++++++++++++++++ > 9 files changed, 227 insertions(+), 6 deletions(-) > create mode 100644 arch/arm64/include/asm/topology.h > create mode 100644 arch/arm64/kernel/topology.c > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 88c8b6c1341a..7b4dab852937 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -154,6 +154,14 @@ config SMP > > If you don't know what to do here, say N. > > +config ARM_CPU_TOPOLOGY > + bool "Support CPU topology definition" > + depends on SMP > + default y > + help > + Support CPU topology definition, based on configuration > + provided by the firmware. > + > config NR_CPUS > int "Maximum number of CPUs (2-32)" > range 2 32 > diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h > index d67ff011d361..8a26b690110c 100644 > --- a/arch/arm64/include/asm/cpu.h > +++ b/arch/arm64/include/asm/cpu.h > @@ -10,7 +10,6 @@ > > #include <linux/percpu.h> > #include <linux/cpu.h> > -#include <linux/topology.h> > > struct cpuinfo_arm { > struct cpu cpu; > diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h > index 5fe138e0b828..bd504739cbfd 100644 > --- a/arch/arm64/include/asm/cputype.h > +++ b/arch/arm64/include/asm/cputype.h > @@ -29,6 +29,15 @@ > #define INVALID_HWID ULONG_MAX > > #define MPIDR_HWID_BITMASK 0xff00ffffff > +#define MPIDR_SMP_BITMASK (0x3 << 30) > +#define MPIDR_SMP_VALUE (0x2 << 30) > +#define MPIDR_MT_BITMASK (0x1 << 24) > +#define MPIDR_LEVEL_BITS 8 > +#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) > + > +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ > + ((mpidr >> (MPIDR_LEVEL_BITS * level)) & MPIDR_LEVEL_MASK) This macros does not cover all affinity levels, I have a patch that implements this macro in my arm64 cpu_{suspend}/{resume} series. http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/213031.html > + > > #define read_cpuid(reg) ({ \ > u64 __val; \ > diff --git a/arch/arm64/include/asm/smp_plat.h b/arch/arm64/include/asm/smp_plat.h > index ed43a0d2b1b2..4ad4ecc93bcf 100644 > --- a/arch/arm64/include/asm/smp_plat.h > +++ b/arch/arm64/include/asm/smp_plat.h > @@ -19,6 +19,7 @@ > #ifndef __ASM_SMP_PLAT_H > #define __ASM_SMP_PLAT_H > > +#include <linux/cpumask.h> > #include <asm/types.h> > > /* > diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h > new file mode 100644 > index 000000000000..611edefaeaf1 > --- /dev/null > +++ b/arch/arm64/include/asm/topology.h > @@ -0,0 +1,42 @@ > +#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); Do not think function above should be exported. Topology can be built in one go from DT, code needing this function was there because the MPIDR in arm32 was "probed", hence all CPUs (primaries and secondaries) had to call it IIRC. > +const struct cpumask *cpu_coregroup_mask(int cpu); > +int cluster_to_logical_mask(unsigned int socket_id, cpumask_t *cluster_mask); I think the function signature above needs changing when we move to DT topology bindings, put it differently the look up won't be based on a socket id anymore, I need some time to think about it. [...] > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index a5aeefab03c3..f29c7ffad84a 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -35,7 +35,6 @@ > #include <linux/clockchips.h> > #include <linux/completion.h> > #include <linux/of.h> > - > #include <asm/atomic.h> > #include <asm/cacheflush.h> > #include <asm/cputype.h> > @@ -48,6 +47,7 @@ > #include <asm/sections.h> > #include <asm/tlbflush.h> > #include <asm/ptrace.h> > +#include <asm/cpu.h> > > /* > * as from 2.5, kernels no longer have an init_tasks structure > @@ -113,6 +113,16 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) > return ret; > } > > +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; > + cpu_info->cpuid = read_cpuid_id(); > + > + store_cpu_topology(cpuid); All bits of info are in the DT, topology can be built by primary CPU, no need to call store_cpu_topology() on each CPU, that was only needed because on arm32 the topology code relies on each CPU reading its own MPIDR. > + > /* > * This is the secondary CPU boot entry. We're using this CPUs > * idle thread stack, but a set of temporary page tables. > @@ -150,6 +160,8 @@ asmlinkage void secondary_start_kernel(void) > */ > notify_cpu_starting(cpu); > > + smp_store_cpu_info(cpu); > + > /* > * OK, now it's safe to let the boot CPU continue. Wait for > * the CPU migration code to notice that the CPU is online > @@ -387,6 +399,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus) > int err; > unsigned int cpu, ncores = num_possible_cpus(); > > + init_cpu_topology(); > + > + smp_store_cpu_info(smp_processor_id()); > + > + > /* > * are we trying to boot more cores than exist? > */ > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c > new file mode 100644 > index 000000000000..e0b40f48b448 > --- /dev/null > +++ b/arch/arm64/kernel/topology.c > @@ -0,0 +1,143 @@ > +/* > + * arch/arm64/kernel/topology.c > + * > + * Copyright (C) 2011,2013 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/export.h> > +#include <linux/init.h> > +#include <linux/percpu.h> > +#include <linux/node.h> > +#include <linux/nodemask.h> > +#include <linux/sched.h> > +#include <linux/slab.h> > + > +#include <asm/cputype.h> > +#include <asm/smp_plat.h> > +#include <asm/topology.h> > + > +/* > + * cpu topology table > + */ > +struct cputopo_arm cpu_topology[NR_CPUS]; > +EXPORT_SYMBOL_GPL(cpu_topology); > + > +const struct cpumask *cpu_coregroup_mask(int cpu) > +{ > + return &cpu_topology[cpu].core_sibling; > +} > + > +static void update_siblings_masks(unsigned int cpuid) > +{ > + struct cputopo_arm *cpu_topo, *cpuid_topo = &cpu_topology[cpuid]; > + int cpu; > + > + /* update core and thread sibling masks */ > + for_each_possible_cpu(cpu) { > + cpu_topo = &cpu_topology[cpu]; > + > + if (cpuid_topo->socket_id != cpu_topo->socket_id) > + continue; > + > + 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) > + continue; > + > + cpumask_set_cpu(cpuid, &cpu_topo->thread_sibling); > + if (cpu != cpuid) > + cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling); > + } > + smp_wmb(); > +} > + > +/* > + * 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]; > + u64 mpidr; > + > + /* If the cpu topology has been already set, just return */ > + if (cpuid_topo->core_id != -1) > + return; > + > + mpidr = cpu_logical_map(cpuid); > + > + /* > + * Create cpu topology mapping, assume the cores are largely > + * independent since the DT bindings do not include the flags > + * for MT. > + */ > + cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0); > + cpuid_topo->socket_id = MPIDR_AFFINITY_LEVEL(mpidr, 1); This is what we are trying to prevent. The way levels are mapped to core and cluster id is just a recommendation. Better to follow DT bindings, they are stricter and provide us with all the required bits of information. > + > + update_siblings_masks(cpuid); > + > + pr_info("CPU%u: cpu %d, socket %d mapped using MPIDR %llx\n", > + cpuid, cpu_topology[cpuid].core_id, > + cpu_topology[cpuid].socket_id, mpidr); > +} > + > + > +/* > + * cluster_to_logical_mask - return cpu logical mask of CPUs in a cluster > + * @socket_id: cluster HW identifier > + * @cluster_mask: the cpumask location to be initialized, modified by the > + * function only if return value == 0 > + * > + * Return: > + * > + * 0 on success > + * -EINVAL if cluster_mask is NULL or there is no record matching socket_id > + */ > +int cluster_to_logical_mask(unsigned int socket_id, cpumask_t *cluster_mask) > +{ > + int cpu; > + > + if (!cluster_mask) > + return -EINVAL; > + > + for_each_online_cpu(cpu) > + if (socket_id == topology_physical_package_id(cpu)) { > + cpumask_copy(cluster_mask, topology_core_cpumask(cpu)); > + return 0; > + } > + > + return -EINVAL; > +} As mentioned, I think this function will have to change. Masks can be built using phandles to topology nodes. I know this is how cluster masks are currently built in arm32 kernels, but this does not mean that's the correct approach, given the laxity of the MPIDR specification. > +/* > + * init_cpu_topology is called at boot when only one cpu is running > + * which prevent simultaneous write access to cpu_topology array > + */ > +void __init init_cpu_topology(void) > +{ > + unsigned int cpu; > + > + /* init core mask and power*/ > + 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); > + } This is probably the place where the topology should be parsed and built in one go, from DT, I did that and then needed to rewrite the code since topology bindings changed before getting merged. Thanks, Lorenzo
On Mon, Dec 16, 2013 at 10:57:34AM +0000, Lorenzo Pieralisi wrote: > > +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ > > + ((mpidr >> (MPIDR_LEVEL_BITS * level)) & MPIDR_LEVEL_MASK) > This macros does not cover all affinity levels, I have a patch that > implements this macro in my arm64 cpu_{suspend}/{resume} series. > > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/213031.html That hunk has been dropped from the latest postings of the series anyway, please see the more current threads (and I'm going to do another spin today too). It'd be good to get things like your helpers integrated so we don't have duplication of effort or people running around trying to keep track of out of tree patches.
On Mon, Dec 16, 2013 at 10:57:34AM +0000, Lorenzo Pieralisi wrote: > On Wed, Dec 11, 2013 at 01:13:24PM +0000, Mark Brown wrote: Replying again since I didn't notice half the content here - please delete unneeded context from your mails, it makes it much easier to find the content you've added. > > +const struct cpumask *cpu_coregroup_mask(int cpu); > > +int cluster_to_logical_mask(unsigned int socket_id, cpumask_t *cluster_mask); > I think the function signature above needs changing when we move to DT > topology bindings, put it differently the look up won't be based on a > socket id anymore, I need some time to think about it. Well, we need to consider the possibility of ACPI or whatever as well. > > + cpu_info->loops_per_jiffy = loops_per_jiffy; > > + cpu_info->cpuid = read_cpuid_id(); > > + store_cpu_topology(cpuid); > All bits of info are in the DT, topology can be built by primary CPU, no need > to call store_cpu_topology() on each CPU, that was only needed because on > arm32 the topology code relies on each CPU reading its own MPIDR. This is also gone in the current versions of the series. > > + /* > > + * Create cpu topology mapping, assume the cores are largely > > + * independent since the DT bindings do not include the flags > > + * for MT. > > + */ > > + cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0); > > + cpuid_topo->socket_id = MPIDR_AFFINITY_LEVEL(mpidr, 1); > This is what we are trying to prevent. The way levels are mapped to > core and cluster id is just a recommendation. Better to follow DT bindings, > they are stricter and provide us with all the required bits of information. Again, this is gone from the current version. Like I said to Catalin it does feel like this is making more work for systems that have done the right thing with their MPIDRs which doesn't seem ideal (and note that all the DTs that you guys are publishing for your models lack any topology information at present). > > + for_each_online_cpu(cpu) > > + if (socket_id == topology_physical_package_id(cpu)) { > > + cpumask_copy(cluster_mask, topology_core_cpumask(cpu)); > > + return 0; > > + } > > + > > + return -EINVAL; > > +} > As mentioned, I think this function will have to change. Masks can be > built using phandles to topology nodes. I know this is how cluster masks > are currently built in arm32 kernels, but this does not mean that's the > correct approach, given the laxity of the MPIDR specification. So, this can actually be removed completely since there aren't any references to this function any more that said the socket_id assignment is still there... This isn't being done using MPDIR, it's being done based on using the lowest level of cluster however we found it. What we're doing is that while parsing the topology information source we use it to pick a physical package identifier and then later on we use that identifier as a handle. The socket ID isn't really taken from a field in the MPDIR, it's the result of doing the mapping you mention. I definitely don't think we should be using phandles directly unless we want to have a separate implementation for ACPI (or whatever else might come up) which would mean less encapsulation of the topology code. Having the parsing code assign socket IDs doesn't seem like a particularly bad way of doing things, we need IDs for the generic topology API functions to use anyway. > > + 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); > > + } > This is probably the place where the topology should be parsed and built > in one go, from DT, I did that and then needed to rewrite the code since > topology bindings changed before getting merged. Right, and that's where it happens for the DT parsing code.
On 12/11/2013 09:13 PM, Mark Brown wrote: > diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h > new file mode 100644 > index 000000000000..611edefaeaf1 > --- /dev/null > +++ b/arch/arm64/include/asm/topology.h > @@ -0,0 +1,42 @@ > +#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; > +}; Forgive me if I am stupid. :) why we don't need a cluster_id? and does one cpu socket include few clusters?
On Mon, Dec 16, 2013 at 12:29:48PM +0000, Mark Brown wrote: > On Mon, Dec 16, 2013 at 10:57:34AM +0000, Lorenzo Pieralisi wrote: > > On Wed, Dec 11, 2013 at 01:13:24PM +0000, Mark Brown wrote: > > Replying again since I didn't notice half the content here - please > delete unneeded context from your mails, it makes it much easier to > find the content you've added. > > > > +const struct cpumask *cpu_coregroup_mask(int cpu); > > > +int cluster_to_logical_mask(unsigned int socket_id, cpumask_t *cluster_mask); > > > I think the function signature above needs changing when we move to DT > > topology bindings, put it differently the look up won't be based on a > > socket id anymore, I need some time to think about it. > > Well, we need to consider the possibility of ACPI or whatever as well. That's a fair point, I will have a look at v2. [...] > > > + /* > > > + * Create cpu topology mapping, assume the cores are largely > > > + * independent since the DT bindings do not include the flags > > > + * for MT. > > > + */ > > > + cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0); > > > + cpuid_topo->socket_id = MPIDR_AFFINITY_LEVEL(mpidr, 1); > > > This is what we are trying to prevent. The way levels are mapped to > > core and cluster id is just a recommendation. Better to follow DT bindings, > > they are stricter and provide us with all the required bits of information. > > Again, this is gone from the current version. Like I said to Catalin it > does feel like this is making more work for systems that have done the > right thing with their MPIDRs which doesn't seem ideal (and note that > all the DTs that you guys are publishing for your models lack any > topology information at present). This is an age-old question and the problem has always been that the "right thing" is recommended, not enforced. I do not want to turn this into bikeshedding, as long as cpu-map node takes priority if present, fine by me. > > > + for_each_online_cpu(cpu) > > > + if (socket_id == topology_physical_package_id(cpu)) { > > > + cpumask_copy(cluster_mask, topology_core_cpumask(cpu)); > > > + return 0; > > > + } > > > + > > > + return -EINVAL; > > > +} > > > As mentioned, I think this function will have to change. Masks can be > > built using phandles to topology nodes. I know this is how cluster masks > > are currently built in arm32 kernels, but this does not mean that's the > > correct approach, given the laxity of the MPIDR specification. > > So, this can actually be removed completely since there aren't any > references to this function any more that said the socket_id assignment > is still there... > > This isn't being done using MPDIR, it's being done based on using the > lowest level of cluster however we found it. What we're doing is that > while parsing the topology information source we use it to pick a > physical package identifier and then later on we use that identifier as > a handle. The socket ID isn't really taken from a field in the MPDIR, > it's the result of doing the mapping you mention. > > I definitely don't think we should be using phandles directly unless we > want to have a separate implementation for ACPI (or whatever else might > come up) which would mean less encapsulation of the topology code. > Having the parsing code assign socket IDs doesn't seem like a > particularly bad way of doing things, we need IDs for the generic > topology API functions to use anyway. It makes sense, again I will have a look at v2 and comment on that. Thanks, Lorenzo
On Mon, Dec 16, 2013 at 02:46:38PM +0000, Lorenzo Pieralisi wrote: > On Mon, Dec 16, 2013 at 12:29:48PM +0000, Mark Brown wrote: > > Well, we need to consider the possibility of ACPI or whatever as well. > That's a fair point, I will have a look at v2. Probably best to wait until the v4 or whatever that I'm going to post shortly (need to do a few more checks locally before I post). I'll CC you. [MPIDR parsing] > > Again, this is gone from the current version. Like I said to Catalin it > > does feel like this is making more work for systems that have done the > > right thing with their MPIDRs which doesn't seem ideal (and note that > > all the DTs that you guys are publishing for your models lack any > > topology information at present). > This is an age-old question and the problem has always been that the > "right thing" is recommended, not enforced. I do not want to turn this into > bikeshedding, as long as cpu-map node takes priority if present, fine by me. I already dropped that code, though I could resurrect it (perhaps as a separate patch). The way the code was written was as you describe as a last resort - MPIDR would only be considered if the explict topology binding was not present, it was done as a last step before reporting if no other topology information was discovered. Actually now I think about it if we're not going to parse the MPIDR we should probably update the bindings to say that the topology binding is mandatory for any v8 system with more than one core.
On Mon, Dec 16, 2013 at 10:45:18PM +0800, Alex Shi wrote: > On 12/11/2013 09:13 PM, Mark Brown wrote: > > +struct cputopo_arm { > > + int thread_id; > > + int core_id; > > + int socket_id; > Forgive me if I am stupid. :) > why we don't need a cluster_id? and does one cpu socket include few > clusters? The ARMv7 code calls a cluster a socket I think because it's trying to maintain similarity with other architectures and the ARMv8 code follows ARMv7 in this regard. At the minute we're using this ID for whatever the lowest level of grouping is above a core and presenting the scheduler with a flat topology there - the topology binding and MPIDR both allow multiple layers of cluster so you could definitely have multiple clusters in a supercluster. Without practical examples of such systems or more architecture documentation than I've been able to find it's not clear how to represent them to the scheduler, it will depend on how closely associated the clusters are and what the scheduler's features are, perhaps we should describe such a system as NUMA but it's not clear to me that this would produce the desired results. I wonder if we may end up figuring this out from other data such as descriptions of caches and interconnects rather than purely from the topology information.
On 12/16/2013 11:22 PM, Mark Brown wrote: > On Mon, Dec 16, 2013 at 10:45:18PM +0800, Alex Shi wrote: >> On 12/11/2013 09:13 PM, Mark Brown wrote: > >>> +struct cputopo_arm { >>> + int thread_id; >>> + int core_id; >>> + int socket_id; > >> Forgive me if I am stupid. :) > >> why we don't need a cluster_id? and does one cpu socket include few >> clusters? > > The ARMv7 code calls a cluster a socket I think because it's trying to > maintain similarity with other architectures and the ARMv8 code follows > ARMv7 in this regard. At the minute we're using this ID for whatever > the lowest level of grouping is above a core and presenting the > scheduler with a flat topology there - the topology binding and MPIDR > both allow multiple layers of cluster so you could definitely have > multiple clusters in a supercluster. > > Without practical examples of such systems or more architecture > documentation than I've been able to find it's not clear how to > represent them to the scheduler, it will depend on how closely > associated the clusters are and what the scheduler's features are, > perhaps we should describe such a system as NUMA but it's not clear to > me that this would produce the desired results. I wonder if we may end > up figuring this out from other data such as descriptions of caches and > interconnects rather than purely from the topology information. For topology meaning, it may be better to have cluster concept there. And don't know if there will has a real ARM NUMA system for 64bit server. If so, socket_id is good in a NUMA system.
On Mon, Dec 16, 2013 at 03:12:32PM +0000, Mark Brown wrote: > On Mon, Dec 16, 2013 at 02:46:38PM +0000, Lorenzo Pieralisi wrote: > > On Mon, Dec 16, 2013 at 12:29:48PM +0000, Mark Brown wrote: > [MPIDR parsing] > > > Again, this is gone from the current version. Like I said to Catalin it > > > does feel like this is making more work for systems that have done the > > > right thing with their MPIDRs which doesn't seem ideal (and note that > > > all the DTs that you guys are publishing for your models lack any > > > topology information at present). > > > This is an age-old question and the problem has always been that the > > "right thing" is recommended, not enforced. I do not want to turn this into > > bikeshedding, as long as cpu-map node takes priority if present, fine by me. > > I already dropped that code, though I could resurrect it (perhaps as a > separate patch). The way the code was written was as you describe as a > last resort - MPIDR would only be considered if the explict topology > binding was not present, it was done as a last step before reporting if > no other topology information was discovered. > > Actually now I think about it if we're not going to parse the MPIDR we > should probably update the bindings to say that the topology binding is > mandatory for any v8 system with more than one core. Do we need such information if only a flat topology is needed?
On Tue, Dec 17, 2013 at 03:19:39PM +0800, Alex Shi wrote: > For topology meaning, it may be better to have cluster concept there. > And don't know if there will has a real ARM NUMA system for 64bit > server. If so, socket_id is good in a NUMA system. The scheduler does have a separate NUMA node ID that we're not currently using which I'd have expected to be a better fit for actual NUMA stuff; looking at the options we've got it's not clear to me that with what the scheduler has at the minute sockets aren't a good mapping for what the hardware has. There doesn't seem to be anything between threaded cores and physical packages.
On Tue, Dec 17, 2013 at 11:47:34AM +0000, Catalin Marinas wrote: > On Mon, Dec 16, 2013 at 03:12:32PM +0000, Mark Brown wrote: > > Actually now I think about it if we're not going to parse the MPIDR we > > should probably update the bindings to say that the topology binding is > > mandatory for any v8 system with more than one core. > Do we need such information if only a flat topology is needed? Probably not but it's simpler to specify that rather than saying it's mandatory only if you need it and it might be useful to know if everything is a flat collection of threads or a flat collection of cores, it seems better to get the information in there if we can in case the DT is fixed. Single core systems are the only ones where it's definitely never going to be helpful.
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 88c8b6c1341a..7b4dab852937 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -154,6 +154,14 @@ config SMP If you don't know what to do here, say N. +config ARM_CPU_TOPOLOGY + bool "Support CPU topology definition" + depends on SMP + default y + help + Support CPU topology definition, based on configuration + provided by the firmware. + config NR_CPUS int "Maximum number of CPUs (2-32)" range 2 32 diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h index d67ff011d361..8a26b690110c 100644 --- a/arch/arm64/include/asm/cpu.h +++ b/arch/arm64/include/asm/cpu.h @@ -10,7 +10,6 @@ #include <linux/percpu.h> #include <linux/cpu.h> -#include <linux/topology.h> struct cpuinfo_arm { struct cpu cpu; diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h index 5fe138e0b828..bd504739cbfd 100644 --- a/arch/arm64/include/asm/cputype.h +++ b/arch/arm64/include/asm/cputype.h @@ -29,6 +29,15 @@ #define INVALID_HWID ULONG_MAX #define MPIDR_HWID_BITMASK 0xff00ffffff +#define MPIDR_SMP_BITMASK (0x3 << 30) +#define MPIDR_SMP_VALUE (0x2 << 30) +#define MPIDR_MT_BITMASK (0x1 << 24) +#define MPIDR_LEVEL_BITS 8 +#define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1) + +#define MPIDR_AFFINITY_LEVEL(mpidr, level) \ + ((mpidr >> (MPIDR_LEVEL_BITS * level)) & MPIDR_LEVEL_MASK) + #define read_cpuid(reg) ({ \ u64 __val; \ diff --git a/arch/arm64/include/asm/smp_plat.h b/arch/arm64/include/asm/smp_plat.h index ed43a0d2b1b2..4ad4ecc93bcf 100644 --- a/arch/arm64/include/asm/smp_plat.h +++ b/arch/arm64/include/asm/smp_plat.h @@ -19,6 +19,7 @@ #ifndef __ASM_SMP_PLAT_H #define __ASM_SMP_PLAT_H +#include <linux/cpumask.h> #include <asm/types.h> /* diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h new file mode 100644 index 000000000000..611edefaeaf1 --- /dev/null +++ b/arch/arm64/include/asm/topology.h @@ -0,0 +1,42 @@ +#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(int cpu); +int cluster_to_logical_mask(unsigned int socket_id, cpumask_t *cluster_mask); + +#else + +static inline void init_cpu_topology(void) { } +static inline void store_cpu_topology(unsigned int cpuid) { } +static inline int cluster_to_logical_mask(unsigned int socket_id, + cpumask_t *cluster_mask) { return -EINVAL; } + +#endif + +#include <asm-generic/topology.h> + +#endif /* _ASM_ARM_TOPOLOGY_H */ diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index 5ba2fd43a75b..2d145e38ad49 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -18,6 +18,7 @@ arm64-obj-$(CONFIG_SMP) += smp.o smp_spin_table.o arm64-obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT)+= hw_breakpoint.o arm64-obj-$(CONFIG_EARLY_PRINTK) += early_printk.o +arm64-obj-$(CONFIG_ARM_CPU_TOPOLOGY) += topology.o obj-y += $(arm64-obj-y) vdso/ obj-m += $(arm64-obj-m) diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 0bc5e4cbc017..dbe4a9ba90cb 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -54,6 +54,7 @@ #include <asm/traps.h> #include <asm/memblock.h> #include <asm/psci.h> +#include <asm/cpu.h> unsigned int processor_id; EXPORT_SYMBOL(processor_id); @@ -250,16 +251,16 @@ static int __init arm64_device_init(void) } arch_initcall(arm64_device_init); -static DEFINE_PER_CPU(struct cpu, cpu_data); +DEFINE_PER_CPU(struct cpuinfo_arm, cpu_data); static int __init topology_init(void) { int i; for_each_possible_cpu(i) { - struct cpu *cpu = &per_cpu(cpu_data, i); - cpu->hotpluggable = 1; - register_cpu(cpu, i); + struct cpuinfo_arm *cpu = &per_cpu(cpu_data, i); + cpu->cpu.hotpluggable = 1; + register_cpu(&cpu->cpu, i); } return 0; diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index a5aeefab03c3..f29c7ffad84a 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -35,7 +35,6 @@ #include <linux/clockchips.h> #include <linux/completion.h> #include <linux/of.h> - #include <asm/atomic.h> #include <asm/cacheflush.h> #include <asm/cputype.h> @@ -48,6 +47,7 @@ #include <asm/sections.h> #include <asm/tlbflush.h> #include <asm/ptrace.h> +#include <asm/cpu.h> /* * as from 2.5, kernels no longer have an init_tasks structure @@ -113,6 +113,16 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) return ret; } +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; + cpu_info->cpuid = read_cpuid_id(); + + store_cpu_topology(cpuid); +} + /* * This is the secondary CPU boot entry. We're using this CPUs * idle thread stack, but a set of temporary page tables. @@ -150,6 +160,8 @@ asmlinkage void secondary_start_kernel(void) */ notify_cpu_starting(cpu); + smp_store_cpu_info(cpu); + /* * OK, now it's safe to let the boot CPU continue. Wait for * the CPU migration code to notice that the CPU is online @@ -387,6 +399,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus) int err; unsigned int cpu, ncores = num_possible_cpus(); + init_cpu_topology(); + + smp_store_cpu_info(smp_processor_id()); + + /* * are we trying to boot more cores than exist? */ diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c new file mode 100644 index 000000000000..e0b40f48b448 --- /dev/null +++ b/arch/arm64/kernel/topology.c @@ -0,0 +1,143 @@ +/* + * arch/arm64/kernel/topology.c + * + * Copyright (C) 2011,2013 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/export.h> +#include <linux/init.h> +#include <linux/percpu.h> +#include <linux/node.h> +#include <linux/nodemask.h> +#include <linux/sched.h> +#include <linux/slab.h> + +#include <asm/cputype.h> +#include <asm/smp_plat.h> +#include <asm/topology.h> + +/* + * cpu topology table + */ +struct cputopo_arm cpu_topology[NR_CPUS]; +EXPORT_SYMBOL_GPL(cpu_topology); + +const struct cpumask *cpu_coregroup_mask(int cpu) +{ + return &cpu_topology[cpu].core_sibling; +} + +static void update_siblings_masks(unsigned int cpuid) +{ + struct cputopo_arm *cpu_topo, *cpuid_topo = &cpu_topology[cpuid]; + int cpu; + + /* update core and thread sibling masks */ + for_each_possible_cpu(cpu) { + cpu_topo = &cpu_topology[cpu]; + + if (cpuid_topo->socket_id != cpu_topo->socket_id) + continue; + + 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) + continue; + + cpumask_set_cpu(cpuid, &cpu_topo->thread_sibling); + if (cpu != cpuid) + cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling); + } + smp_wmb(); +} + +/* + * 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]; + u64 mpidr; + + /* If the cpu topology has been already set, just return */ + if (cpuid_topo->core_id != -1) + return; + + mpidr = cpu_logical_map(cpuid); + + /* + * Create cpu topology mapping, assume the cores are largely + * independent since the DT bindings do not include the flags + * for MT. + */ + cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0); + cpuid_topo->socket_id = MPIDR_AFFINITY_LEVEL(mpidr, 1); + + update_siblings_masks(cpuid); + + pr_info("CPU%u: cpu %d, socket %d mapped using MPIDR %llx\n", + cpuid, cpu_topology[cpuid].core_id, + cpu_topology[cpuid].socket_id, mpidr); +} + + +/* + * cluster_to_logical_mask - return cpu logical mask of CPUs in a cluster + * @socket_id: cluster HW identifier + * @cluster_mask: the cpumask location to be initialized, modified by the + * function only if return value == 0 + * + * Return: + * + * 0 on success + * -EINVAL if cluster_mask is NULL or there is no record matching socket_id + */ +int cluster_to_logical_mask(unsigned int socket_id, cpumask_t *cluster_mask) +{ + int cpu; + + if (!cluster_mask) + return -EINVAL; + + for_each_online_cpu(cpu) + if (socket_id == topology_physical_package_id(cpu)) { + cpumask_copy(cluster_mask, topology_core_cpumask(cpu)); + return 0; + } + + return -EINVAL; +} + +/* + * init_cpu_topology is called at boot when only one cpu is running + * which prevent simultaneous write access to cpu_topology array + */ +void __init init_cpu_topology(void) +{ + unsigned int cpu; + + /* init core mask and power*/ + 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(); +}