| Message ID | 1389287106-29979-2-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 09, 2014 at 05:05:03PM +0000, Mark Brown wrote: > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index dd4327f09ba4..8feef9cb65df 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -163,6 +163,30 @@ config SMP > > If you don't know what to do here, say N. > > +config ARM_CPU_TOPOLOGY Nitpicks: please use just CPU_TOPOLOGY. > 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 We tend to stick to __ASM_TOPOLOGY_H like guards under arch/arm64. > + > +#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; > +}; And you could rename this structure to just cpu_topology. The patches seem fine otherwise, apart from the last one which I won't merge until we get some real numbers. I would like to get an ack or reviewed-by from Lorenzo (and possibly Mark Rutland) but it is pretty late for the 3.14 merging window so it will have to be queued for 3.15. Thanks for putting the effort into preparing these patches.
On Fri, Jan 10, 2014 at 05:45:59PM +0000, Catalin Marinas wrote: > The patches seem fine otherwise, apart from the last one which I > won't merge until we get some real numbers. As I previously said I am really concerned about diverging from what arm32 has done here and the numbers don't seem any less real than the ones we're using there (they were generated in an identical fashion). Given the hardware availability for arm64 in general is limited and likely to be even more so for big.LITTLE systems it seems like asking for problems down the line to do something different. Honestly I'm not sure there will ever be more real numbers that aren't benchmarked at runtime.
On Fri, Jan 10, 2014 at 06:27:05PM +0000, Mark Brown wrote: > On Fri, Jan 10, 2014 at 05:45:59PM +0000, Catalin Marinas wrote: > > The patches seem fine otherwise, apart from the last one which I > > won't merge until we get some real numbers. > > As I previously said I am really concerned about diverging from what > arm32 has done here There is no diverging, these are new processors with possibly different values for these parameters. > and the numbers don't seem any less real than the > ones we're using there (they were generated in an identical fashion). Were the numbers in this patch generated in any way or simply copied from arch/arm? > Given the hardware availability for arm64 in general is limited and > likely to be even more so for big.LITTLE systems it seems like asking > for problems down the line to do something different. I'm not saying we do something different, only that we can add the numbers once we get hold of some real hardware. > Honestly I'm not sure there will ever be more real numbers that aren't > benchmarked at runtime. This could be a later addition.
On Mon, Jan 13, 2014 at 03:50:04PM +0000, Catalin Marinas wrote: > On Fri, Jan 10, 2014 at 06:27:05PM +0000, Mark Brown wrote: > > On Fri, Jan 10, 2014 at 05:45:59PM +0000, Catalin Marinas wrote: > > > The patches seem fine otherwise, apart from the last one which I > > > won't merge until we get some real numbers. > > As I previously said I am really concerned about diverging from what > > arm32 has done here > There is no diverging, these are new processors with possibly different > values for these parameters. I would argue that not providing the numbers at all is a divergence, we can be reasonably confident that the A53 and A57 do differ. We're giving the scheduler a ballpark figure here, it's not an exact science. > > and the numbers don't seem any less real than the > > ones we're using there (they were generated in an identical fashion). > Were the numbers in this patch generated in any way or simply copied > from arch/arm? Both IIRC - I did initially cut'n'paste but then went to try to find numbers for the A53 and A57, came up with the idea of using the DMIPS numbers to do that independently and then realised that the numbers I was getting when I tried to do the maths were the same. I only found out Vincent had done the same thing in one of the earlier reviews for the patches. > > Given the hardware availability for arm64 in general is limited and > > likely to be even more so for big.LITTLE systems it seems like asking > > for problems down the line to do something different. > I'm not saying we do something different, only that we can add the > numbers once we get hold of some real hardware. I'm saying we can tune them later on if we need to but that the finger in the air saying there is expected to be a performance difference is likely to be helpful. The scheduler shouldn't be *that* fragile and I expect you'll find that even with silicon it will be possible to have endless debates about precisely which benchmarks to use. In reality good enough is probably fine, even with real hardware I'm not sure we know exactly what to measure. Hopefully people will be able to take released kernels and get them doing useful stuff right off the bat and unless the information turns out to be way off the mark I don't think people are going to be that worried. The more stress that gets put on picking the numbers the harder it will be. I think Nico was right in an earlier version of this discussion when he said that we're likely to end up with the scheduler doing runtime tuning. Long term if these remain I'd expect they'll be providing a hint to make sure it starts off in roughly the right place and in the meantime the scheduler does need some help here.
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index dd4327f09ba4..8feef9cb65df 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -163,6 +163,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 2d4554b13410..2647f711171e 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -20,6 +20,7 @@ arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT)+= hw_breakpoint.o arm64-obj-$(CONFIG_EARLY_PRINTK) += early_printk.o arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND) += sleep.o suspend.o arm64-obj-$(CONFIG_JUMP_LABEL) += jump_label.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 1b7617ab499b..40e20efc13e6 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -114,6 +114,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. @@ -152,6 +157,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 @@ -391,6 +398,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..20eef01a4707 --- /dev/null +++ b/arch/arm64/kernel/topology.c @@ -0,0 +1,91 @@ +/* + * arch/arm64/kernel/topology.c + * + * Copyright (C) 2011,2013 Linaro Limited. + * + * Based on the arm32 version written by Vincent Guittot in turn 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/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(); +}