[PATCH v10 0/4] arm64 topology

[PATCH v10 0/4] arm64 topology

[Mark Brown]

Another round of minor tweaks

 - Remove some problematic and controversial prints
 - Pass cluster_id around as a pointer
 - Move slab.h further back in the series

   arm64: topology: Implement basic CPU topology support
   arm64: topology: Add support for topology DT bindings
   arm64: topology: Tell the scheduler about the relative
   arm64: topology: Provide relative power numbers for cores

[PATCH v10 1/4] arm64: topology: Implement basic CPU topology support

[Mark Brown]

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 any topology discovery support since that should be based on
information from firmware, it merely implements the scaffolding for
integration of topology support in the architecture.

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

A following patch will implement support for parsing the DT topology
bindings for ARM, similar patches will be needed for ACPI.

Signed-off-by: Mark Brown <broonie@linaro.org>
---
 arch/arm64/Kconfig                | 24 +++++++++++
 arch/arm64/include/asm/topology.h | 39 +++++++++++++++++
 arch/arm64/kernel/Makefile        |  1 +
 arch/arm64/kernel/smp.c           | 12 ++++++
 arch/arm64/kernel/topology.c      | 91 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 167 insertions(+)
 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 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();
+}
-- 
1.8.5.2

[PATCH v10 2/4] arm64: topology: Add support for topology DT bindings

[Mark Brown]

From: Mark Brown <broonie@linaro.org>

Add support for parsing the explicit topology bindings to discover the
topology of the system.

Since it is not currently clear how to map multi-level clusters for the
scheduler all leaf clusters are presented to the scheduler at the same
level. This should be enough to provide good support for current systems.

Signed-off-by: Mark Brown <broonie@linaro.org>
---
 arch/arm64/kernel/topology.c | 145 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 20eef01a4707..b153bfe4cab9 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -17,10 +17,152 @@
 #include <linux/percpu.h>
 #include <linux/node.h>
 #include <linux/nodemask.h>
+#include <linux/of.h>
 #include <linux/sched.h>
+#include <linux/slab.h>
 
 #include <asm/topology.h>
 
+#ifdef CONFIG_OF
+static int __init get_cpu_for_node(struct device_node *node)
+{
+	struct device_node *cpu_node;
+	int cpu;
+
+	cpu_node = of_parse_phandle(node, "cpu", 0);
+	if (!cpu_node)
+		return -1;
+
+	for_each_possible_cpu(cpu) {
+		if (of_get_cpu_node(cpu, NULL) == cpu_node)
+			return cpu;
+	}
+
+	pr_crit("Unable to find CPU node for %s\n", cpu_node->full_name);
+	return -1;
+}
+
+static void __init parse_core(struct device_node *core, int *cluster_id,
+			      int core_id)
+{
+	char name[10];
+	bool leaf = true;
+	int i, cpu;
+	struct device_node *t;
+
+	i = 0;
+	do {
+		snprintf(name, sizeof(name), "thread%d", i);
+		t = of_get_child_by_name(core, name);
+		if (t) {
+			leaf = false;
+			cpu = get_cpu_for_node(t);
+			if (cpu >= 0) {
+				cpu_topology[cpu].socket_id = *cluster_id;
+				cpu_topology[cpu].core_id = core_id;
+				cpu_topology[cpu].thread_id = i;
+			} else {
+				pr_err("%s: Can't get CPU for thread\n",
+				       t->full_name);
+			}
+		}
+		i++;
+	} while (t);
+
+	cpu = get_cpu_for_node(core);
+	if (cpu >= 0) {
+		if (!leaf) {
+			pr_err("%s: Core has both threads and CPU\n",
+			       core->full_name);
+			return;
+		}
+
+		cpu_topology[cpu].socket_id = *cluster_id;
+		cpu_topology[cpu].core_id = core_id;
+	} else if (leaf) {
+		pr_err("%s: Can't get CPU for leaf core\n", core->full_name);
+	}
+}
+
+static void __init parse_cluster(struct device_node *cluster,
+				 int depth, int *cluster_id)
+{
+	char name[10];
+	bool leaf = true;
+	bool has_cores = false;
+	struct device_node *c;
+	int core_id = 0;
+	int i;
+
+	/*
+	 * First check for child clusters; we currently ignore any
+	 * information about the nesting of clusters and present the
+	 * scheduler with a flat list of them.
+	 */
+	i = 0;
+	do {
+		snprintf(name, sizeof(name), "cluster%d", i);
+		c = of_get_child_by_name(cluster, name);
+		if (c) {
+			parse_cluster(c, depth + 1, cluster_id);
+			leaf = false;
+		}
+		i++;
+	} while (c);
+
+	/* Now check for cores */
+	i = 0;
+	do {
+		snprintf(name, sizeof(name), "core%d", i);
+		c = of_get_child_by_name(cluster, name);
+		if (c) {
+			has_cores = true;
+
+			if (depth == 0)
+				pr_err("%s: cpu-map children should be clusters\n",
+				       c->full_name);
+
+			if (leaf)
+				parse_core(c, cluster_id, core_id++);
+			else
+				pr_err("%s: Non-leaf cluster with core %s\n",
+				       cluster->full_name, name);
+		}
+		i++;
+	} while (c);
+
+	if (leaf && !has_cores)
+		pr_warn("%s: empty cluster\n", cluster->full_name);
+
+	if (leaf)
+		*cluster_id = *cluster_id + 1;
+}
+
+static void __init parse_dt_topology(void)
+{
+	struct device_node *cn;
+	int cluster_id = 0;
+
+	cn = of_find_node_by_path("/cpus");
+	if (!cn) {
+		pr_err("No CPU information found in DT\n");
+		return;
+	}
+
+	/*
+	 * If topology is provided as a cpu-map it is essentially a
+	 * root cluster.
+	 */
+	cn = of_find_node_by_name(cn, "cpu-map");
+	if (!cn)
+		return;
+	parse_cluster(cn, 0, &cluster_id);
+}
+
+#else
+static inline void parse_dt_topology(void) {}
+#endif
+
 /*
  * cpu topology table
  */
@@ -87,5 +229,8 @@ void __init init_cpu_topology(void)
 		cpumask_clear(&cpu_topo->core_sibling);
 		cpumask_clear(&cpu_topo->thread_sibling);
 	}
+
+	parse_dt_topology();
+
 	smp_wmb();
 }
-- 
1.8.5.2

[PATCH v10 3/4] arm64: topology: Tell the scheduler about the relative power of cores

[Mark Brown]

From: Mark Brown <broonie@linaro.org>

In heterogeneous systems like big.LITTLE systems the scheduler will be
able to make better use of the available cores if we provide power numbers
to it indicating their relative performance. Do this by parsing the CPU
nodes in the DT.

This code currently has no effect as no information on the relative
performance of the cores is provided.

Signed-off-by: Mark Brown <broonie@linaro.org>
---
 arch/arm64/kernel/topology.c | 141 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 141 insertions(+)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index b153bfe4cab9..1f086fc7665d 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -23,6 +23,29 @@
 
 #include <asm/topology.h>
 
+/*
+ * cpu power table
+ * This per cpu data structure describes the relative capacity of each core.
+ * On a heteregenous system, cores don't have the same computation capacity
+ * and we reflect that difference in the cpu_power field so the scheduler can
+ * take this difference into account during load balance. A per cpu structure
+ * is preferred because each CPU updates its own cpu_power field during the
+ * load balance except for idle cores. One idle core is selected to run the
+ * rebalance_domains for all idle cores and the cpu_power can be updated
+ * during this sequence.
+ */
+static DEFINE_PER_CPU(unsigned long, cpu_scale);
+
+unsigned long arch_scale_freq_power(struct sched_domain *sd, int cpu)
+{
+	return per_cpu(cpu_scale, cpu);
+}
+
+static void set_power_scale(unsigned int cpu, unsigned long power)
+{
+	per_cpu(cpu_scale, cpu) = power;
+}
+
 #ifdef CONFIG_OF
 static int __init get_cpu_for_node(struct device_node *node)
 {
@@ -138,10 +161,50 @@ static void __init parse_cluster(struct device_node *cluster,
 		*cluster_id = *cluster_id + 1;
 }
 
+struct cpu_efficiency {
+	const char *compatible;
+	unsigned long efficiency;
+};
+
+/*
+ * Table of relative efficiency of each processors
+ * The efficiency value must fit in 20bit and the final
+ * cpu_scale value must be in the range
+ *   0 < cpu_scale < 3*SCHED_POWER_SCALE/2
+ * in order to return@most 1 when DIV_ROUND_CLOSEST
+ * is used to compute the capacity of a CPU.
+ * Processors that are not defined in the table,
+ * use the default SCHED_POWER_SCALE value for cpu_scale.
+ */
+static const struct cpu_efficiency table_efficiency[] = {
+	{ NULL, },
+};
+
+static unsigned long *__cpu_capacity;
+#define cpu_capacity(cpu)	__cpu_capacity[cpu]
+
+static unsigned long middle_capacity = 1;
+
+/*
+ * Iterate all CPUs' descriptor in DT and compute the efficiency
+ * (as per table_efficiency). Also calculate a middle efficiency
+ * as close as possible to  (max{eff_i} - min{eff_i}) / 2
+ * This is later used to scale the cpu_power field such that an
+ * 'average' CPU is of middle power. Also see the comments near
+ * table_efficiency[] and update_cpu_power().
+ */
 static void __init parse_dt_topology(void)
 {
+	const struct cpu_efficiency *cpu_eff;
 	struct device_node *cn;
+	unsigned long min_capacity = ULONG_MAX;
+	unsigned long max_capacity = 0;
+	unsigned long capacity = 0;
 	int cluster_id = 0;
+	int cpu;
+
+	__cpu_capacity = kcalloc(nr_cpu_ids, sizeof(*__cpu_capacity),
+				 GFP_NOWAIT);
 
 	cn = of_find_node_by_path("/cpus");
 	if (!cn) {
@@ -157,10 +220,84 @@ static void __init parse_dt_topology(void)
 	if (!cn)
 		return;
 	parse_cluster(cn, 0, &cluster_id);
+
+	for_each_possible_cpu(cpu) {
+		const u32 *rate;
+		int len;
+
+		/* Too early to use cpu->of_node */
+		cn = of_get_cpu_node(cpu, NULL);
+		if (!cn) {
+			pr_err("Missing device node for CPU %d\n", cpu);
+			continue;
+		}
+
+		for (cpu_eff = table_efficiency; cpu_eff->compatible; cpu_eff++)
+			if (of_device_is_compatible(cn, cpu_eff->compatible))
+				break;
+
+		if (cpu_eff->compatible == NULL) {
+			pr_warn("%s: Unknown CPU type\n", cn->full_name);
+			continue;
+		}
+
+		rate = of_get_property(cn, "clock-frequency", &len);
+		if (!rate || len != 4) {
+			pr_err("%s: Missing clock-frequency property\n",
+				cn->full_name);
+			continue;
+		}
+
+		capacity = ((be32_to_cpup(rate)) >> 20) * cpu_eff->efficiency;
+
+		/* Save min capacity of the system */
+		if (capacity < min_capacity)
+			min_capacity = capacity;
+
+		/* Save max capacity of the system */
+		if (capacity > max_capacity)
+			max_capacity = capacity;
+
+		cpu_capacity(cpu) = capacity;
+	}
+
+	/* If min and max capacities are equal we bypass the update of the
+	 * cpu_scale because all CPUs have the same capacity. Otherwise, we
+	 * compute a middle_capacity factor that will ensure that the capacity
+	 * of an 'average' CPU of the system will be as close as possible to
+	 * SCHED_POWER_SCALE, which is the default value, but with the
+	 * constraint explained near table_efficiency[].
+	 */
+	if (min_capacity == max_capacity)
+		return;
+	else if (4 * max_capacity < (3 * (max_capacity + min_capacity)))
+		middle_capacity = (min_capacity + max_capacity)
+				>> (SCHED_POWER_SHIFT+1);
+	else
+		middle_capacity = ((max_capacity / 3)
+				>> (SCHED_POWER_SHIFT-1)) + 1;
+
+}
+
+/*
+ * Look for a customed capacity of a CPU in the cpu_topo_data table during the
+ * boot. The update of all CPUs is in O(n^2) for heteregeneous system but the
+ * function returns directly for SMP system.
+ */
+static void update_cpu_power(unsigned int cpu)
+{
+	if (!cpu_capacity(cpu))
+		return;
+
+	set_power_scale(cpu, cpu_capacity(cpu) / middle_capacity);
+
+	pr_info("CPU%u: update cpu_power %lu\n",
+		cpu, arch_scale_freq_power(NULL, cpu));
 }
 
 #else
 static inline void parse_dt_topology(void) {}
+static inline void update_cpu_power(unsigned int cpuid) {}
 #endif
 
 /*
@@ -209,6 +346,8 @@ void store_cpu_topology(unsigned int cpuid)
 		pr_info("CPU%u: No topology information configured\n", cpuid);
 	else
 		update_siblings_masks(cpuid);
+
+	update_cpu_power(cpuid);
 }
 
 /*
@@ -228,6 +367,8 @@ void __init init_cpu_topology(void)
 		cpu_topo->socket_id = -1;
 		cpumask_clear(&cpu_topo->core_sibling);
 		cpumask_clear(&cpu_topo->thread_sibling);
+
+		set_power_scale(cpu, SCHED_POWER_SCALE);
 	}
 
 	parse_dt_topology();
-- 
1.8.5.2

[PATCH v10 4/4] arm64: topology: Provide relative power numbers for cores

[Mark Brown]

From: Mark Brown <broonie@linaro.org>

Provide performance numbers to the scheduler to help it fill the cores in
the system on big.LITTLE systems. With the current scheduler this may
perform poorly for applications that try to do OpenMP style work over all
cores but should help for more common workloads.

The power numbers are the same as for ARMv7 since it seems that the
expected differential between the big and little cores is very similar on
both ARMv7 and ARMv8. These numbers are just an initial and basic
approximation for use with the current scheduler, it is likely that both
experience with silicon and ongoing work on improving the scheduler will
lead to further tuning. In both ARMv7 and ARMv8 cases the numbers were
based on the published DMIPS numbers.

Signed-off-by: Mark Brown <broonie@linaro.org>
---
 arch/arm64/kernel/topology.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 1f086fc7665d..022ad0905a2a 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -177,6 +177,8 @@ struct cpu_efficiency {
  * use the default SCHED_POWER_SCALE value for cpu_scale.
  */
 static const struct cpu_efficiency table_efficiency[] = {
+	{ "arm,cortex-a57", 3891 },
+	{ "arm,cortex-a53", 2048 },
 	{ NULL, },
 };
 
-- 
1.8.5.2

[PATCH v10 2/4] arm64: topology: Add support for topology DT bindings

[Lorenzo Pieralisi]

On Thu, Jan 09, 2014 at 05:05:04PM +0000, Mark Brown wrote:
> From: Mark Brown <broonie@linaro.org>
> +#include <linux/slab.h>

Move it to patch 3 please where kcalloc is actually used.

[...]

> +static void __init parse_dt_topology(void)
> +{
> +	struct device_node *cn;
> +	int cluster_id = 0;
> +
> +	cn = of_find_node_by_path("/cpus");
> +	if (!cn) {
> +		pr_err("No CPU information found in DT\n");
> +		return;
> +	}
> +
> +	/*
> +	 * If topology is provided as a cpu-map it is essentially a
> +	 * root cluster.
> +	 */
> +	cn = of_find_node_by_name(cn, "cpu-map");
> +	if (!cn)
> +		return;
> +	parse_cluster(cn, 0, &cluster_id);

You do not need to pass a pointer, sorry I wanted to comment but I did
not manage to do it in time. Define cluster_id it as static __initdata in
parse_cluster and pass it by value to parse_core, that's it no need for
a pointer unless I am missing something. This way cluster_id is defined
where it is used and still does what you expect from it. I just did not
like the fact that was defined outside function scope to pass state
across functions, it is just a matter of taste really.

Apart from that I will re-review the entire series and add proper tags
accordingly.

Lorenzo

[PATCH v10 2/4] arm64: topology: Add support for topology DT bindings

[Mark Brown]

On Thu, Jan 09, 2014 at 05:44:58PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Jan 09, 2014 at 05:05:04PM +0000, Mark Brown wrote:
> > From: Mark Brown <broonie@linaro.org>
> > +#include <linux/slab.h>

> Move it to patch 3 please where kcalloc is actually used.

Gah, forgot that git format-patch doesn't overwrite patches - it is
actually moved in the code.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140109/5828b7fb/attachment.sig>

[PATCH v10 1/4] arm64: topology: Implement basic CPU topology support

[Catalin Marinas]

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.

-- 
Catalin

[PATCH v10 1/4] arm64: topology: Implement basic CPU topology support

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140110/9baebcba/attachment.sig>

[PATCH v10 1/4] arm64: topology: Implement basic CPU topology support

[Catalin Marinas]

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.

-- 
Catalin

[PATCH v10 1/4] arm64: topology: Implement basic CPU topology support

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140113/65092db9/attachment.sig>

[PATCH v10 4/4] arm64: topology: Provide relative power numbers for cores

[Paul Walmsley]

Hi

On 01/09/2014 09:05 AM, Mark Brown wrote:
> The power numbers are the same as for ARMv7 since it seems that the
> expected differential between the big and little cores is very similar on
> both ARMv7 and ARMv8. These numbers are just an initial and basic
> approximation for use with the current scheduler, it is likely that both
> experience with silicon and ongoing work on improving the scheduler will
> lead to further tuning. In both ARMv7 and ARMv8 cases the numbers were
> based on the published DMIPS numbers.

Just to chime in on this.

As I think someone else proposed, ideally both arch/arm32 and arch/arm64 
should be determining these metrics dynamically, and allowing the 
metrics to be overridden from DT data.  These metrics can vary 
significantly based on variables other than just the core type.

For example, for some ARMv7 CPUs, we know that different revision levels 
of the same core type can result in significant performance 
differences.  Also CPU bug workarounds applied early by the bootloader, 
or different integrator hardware core configurations, can impact these 
metrics.


- Paul

[PATCH v10 4/4] arm64: topology: Provide relative power numbers for cores

[Mark Brown]

On Mon, Jan 13, 2014 at 05:16:41PM -0800, Paul Walmsley wrote:

> As I think someone else proposed, ideally both arch/arm32 and
> arch/arm64 should be determining these metrics dynamically, and
> allowing the metrics to be overridden from DT data.  These metrics
> can vary significantly based on variables other than just the core
> type.

> For example, for some ARMv7 CPUs, we know that different revision
> levels of the same core type can result in significant performance
> differences.  Also CPU bug workarounds applied early by the
> bootloader, or different integrator hardware core configurations,
> can impact these metrics.

So, determining any numbers dynamically is obviously the ideal (and
shouldn't really be an architecture thing - there is nothing obviously
architecture specific about that) and we ought to take things like core
revision differences into account.

However doing this by putting specific tunable numbers into the DT isn't
good practice since the implementation that interprets those numbers is
subject to change.  The numbers are basically just benchmark numbers
with all the specificness and so on that this entails, saying "this is
core with rating X" isn't robust for the long term since it's not really
objective information and is heavily tied to the the specific software
doing the parsing (and most likely also some particular application
stack that was being tuned).

The best practice is therefore for any relevant information that needs
to be factored in and can't be read from the silicon at runtime to be
put in the DT in a way that describes something objective like the core
revisions.  This lets the system take account of these factors in a way
that makes sense within the implementation rather than having to try to
interpret numbers which may not apply well to the current implementation
and may hide information which would help the implementation tune itself.

The other option here is runtime tunables via sysfs or whatever which
avoids baking numbers into the DT in the same way, though that is partly
just a punt of the same problem to userspace.  That's probably also more
helpful to people doing the tuning since it can be tweaked dynamically.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140114/aab2d266/attachment.sig>

[PATCH v10 4/4] arm64: topology: Provide relative power numbers for cores

[Mark Brown]

On Tue, Jan 14, 2014 at 01:43:10PM +0000, Mark Brown wrote:

> The other option here is runtime tunables via sysfs or whatever which
> avoids baking numbers into the DT in the same way, though that is partly
> just a punt of the same problem to userspace.  That's probably also more
> helpful to people doing the tuning since it can be tweaked dynamically.

One other thing on this which occurred to me later on: if we do want to
make these values directly tunable we probably want to do that from the
scheduler rather than individual architectures.  These are scheduler
interfaces and the issues which make tuning desirable are essentially
architecture neutral ones.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140114/ce351e7a/attachment.sig>