| Message ID | 1387212565-12668-1-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Mark, On Mon, Dec 16, 2013 at 04:49:22PM +0000, Mark Brown wrote: > +void store_cpu_topology(unsigned int cpuid); I still think this function is not needed at all and therefore should not be exported. I understand this patch is there to provide the infrastructure, it is not clear to me why store_cpu_topology() should be part of it, even if we had to parse affinity levels in the MPIDR bitfields. [...] > +static void __cpuinit smp_store_cpu_info(unsigned int cpuid) > +{ > + store_cpu_topology(cpuid); As I mentioned this need not be done here. Parse the topology from DT, (cpu_logical_map or cpu-map) in one go and get rid of store_cpu_topology(). > /* > * This is the secondary CPU boot entry. We're using this CPUs > * idle thread stack, but a set of temporary page tables. > @@ -150,6 +155,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 > @@ -388,6 +395,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..b078afa6958d > --- /dev/null > +++ b/arch/arm64/kernel/topology.c > @@ -0,0 +1,95 @@ > +/* > + * arch/arm64/kernel/topology.c > + * > + * Copyright (C) 2011,2013 Linaro Limited. > + * Written by: Vincent Guittot You should probably update the header, just a nitpick though. > + * 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> I think that some of these includes are not needed, they were needed in previous versions, but not in this patch as it stands. Lorenzo
On Tue, Dec 17, 2013 at 05:06:25PM +0000, Lorenzo Pieralisi wrote: > On Mon, Dec 16, 2013 at 04:49:22PM +0000, Mark Brown wrote: > > +void store_cpu_topology(unsigned int cpuid); > I still think this function is not needed at all and therefore should > not be exported. > > +static void __cpuinit smp_store_cpu_info(unsigned int cpuid) > > +{ > > + store_cpu_topology(cpuid); > As I mentioned this need not be done here. Parse the topology from DT, > (cpu_logical_map or cpu-map) in one go and get rid of store_cpu_topology(). That's what we're actually doing for parsing, the parsing all happens at once. I do think it's useful to to have the function there partly just as a hook but more for the warning it generates if we manage to boot a core that we didn't hook up with the topology information. This seems like it'll be helpful for debugging issues - we'll actively warn if there are CPUs that have been omitted from the definition in the firmware. The other things the function does could be done elsewhere I think, it didn't seem worth moving them though. > > + * arch/arm64/kernel/topology.c > > + * > > + * Copyright (C) 2011,2013 Linaro Limited. > > + * Written by: Vincent Guittot > > You should probably update the header, just a nitpick though. Well, most of this still is Vincent's code with the serial numbers filed off but yeah. > > +#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> > I think that some of these includes are not needed, they were needed in > previous versions, but not in this patch as it stands. Some can probably go. I don't think some of those were even needed on arm.
On Tue, Dec 17, 2013 at 07:55:19PM +0000, Mark Brown wrote: > On Tue, Dec 17, 2013 at 05:06:25PM +0000, Lorenzo Pieralisi wrote: > > On Mon, Dec 16, 2013 at 04:49:22PM +0000, Mark Brown wrote: > > > > +void store_cpu_topology(unsigned int cpuid); > > > I still think this function is not needed at all and therefore should > > not be exported. > > > > +static void __cpuinit smp_store_cpu_info(unsigned int cpuid) > > > +{ > > > + store_cpu_topology(cpuid); > > > As I mentioned this need not be done here. Parse the topology from DT, > > (cpu_logical_map or cpu-map) in one go and get rid of store_cpu_topology(). > > That's what we're actually doing for parsing, the parsing all happens at > once. > > I do think it's useful to to have the function there partly just as a > hook but more for the warning it generates if we manage to boot a core > that we didn't hook up with the topology information. This seems like > it'll be helpful for debugging issues - we'll actively warn if there are > CPUs that have been omitted from the definition in the firmware. The > other things the function does could be done elsewhere I think, it > didn't seem worth moving them though. I think you have a point, the question is whether we need that function to run the check you mention or not, we can run the check for all possible cpus after completing the topology parsing. Actually, we might be building topology for cores that do not come online as well, I do not think this is a problem, but I have to cast a proper look into that before jumping to conclusions, thanks for pointing that out. Lorenzo
On Wed, Dec 18, 2013 at 05:39:42PM +0000, Lorenzo Pieralisi wrote: > On Tue, Dec 17, 2013 at 07:55:19PM +0000, Mark Brown wrote: > > I do think it's useful to to have the function there partly just as a > > hook but more for the warning it generates if we manage to boot a core > > that we didn't hook up with the topology information. This seems like > I think you have a point, the question is whether we need that function > to run the check you mention or not, we can run the check for all possible > cpus after completing the topology parsing. Actually, we might be Yeah, it should be possible to do it elsewhere but then since the SMP code is already enumerating all the cores it's going to boot it seemed sensible to keep piggybacking on that and not duplicate the logic. > building topology for cores that do not come online as well, I do not think > this is a problem, but I have to cast a proper look into that before > jumping to conclusions, thanks for pointing that out. I'd expect that could reasonably happen if cores are administratively disabled for some reason - that's one of the reasons for the status marking in DT.
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 6d4dd22ee4b7..00fcd490b3be 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -154,6 +154,30 @@ 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 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 NR_CPUS int "Maximum number of CPUs (2-32)" range 2 32 diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h new file mode 100644 index 000000000000..58b8b84adcd2 --- /dev/null +++ b/arch/arm64/include/asm/topology.h @@ -0,0 +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) + +#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); + +#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/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/smp.c b/arch/arm64/kernel/smp.c index a0c2ca602cf8..0271fbde5363 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -113,6 +113,11 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle) return ret; } +static void __cpuinit smp_store_cpu_info(unsigned int cpuid) +{ + 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 +155,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 @@ -388,6 +395,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..b078afa6958d --- /dev/null +++ b/arch/arm64/kernel/topology.c @@ -0,0 +1,95 @@ +/* + * 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(); +} + +void store_cpu_topology(unsigned int cpuid) +{ + struct cputopo_arm *cpuid_topo = &cpu_topology[cpuid]; + + /* DT should have been parsed by the time we get here */ + if (cpuid_topo->core_id == -1) + pr_info("CPU%u: No topology information configured\n", cpuid); + else + update_siblings_masks(cpuid); +} + +/* + * 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(); +}