[PATCH 0/4] arm64: topology: CPU topology support

[PATCH 0/4] arm64: topology: CPU topology support

[Mark Brown]

This revision of the series places any otherwise unmapped CPUs into a
cluster in order to avoid confusing the scheduler if no topology
information is provided.  This means that we should never bring up a CPU
with no topology information so while the warning is still present and
has been upgraded it should never triger unless there is a genuine bug
(as opposed to missing information).

Mark Brown (4):
      arm64: topology: Implement basic CPU topology support
      arm64: topology: Add support for topology DT bindings
      arm64: topology: Tell the scheduler about the relative power of cores
      arm64: topology: Provide relative power numbers for cores

 arch/arm64/Kconfig                |  24 +++
 arch/arm64/include/asm/topology.h |  39 ++++
 arch/arm64/kernel/Makefile        |   1 +
 arch/arm64/kernel/smp.c           |  11 ++
 arch/arm64/kernel/topology.c      | 388 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 463 insertions(+)

[PATCH 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.

No locking of the topology data is done since it is only modified during
CPU bringup with external serialisation from the SMP code.

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 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           |  11 ++++
 arch/arm64/kernel/topology.c      | 102 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 177 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 27bbcfc..fea7b47 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -164,6 +164,30 @@ config SMP
 
 	  If you don't know what to do here, say N.
 
+config 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 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 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 0000000..c8a47e8
--- /dev/null
+++ b/arch/arm64/include/asm/topology.h
@@ -0,0 +1,39 @@
+#ifndef __ASM_TOPOLOGY_H
+#define __ASM_TOPOLOGY_H
+
+#ifdef CONFIG_CPU_TOPOLOGY
+
+#include <linux/cpumask.h>
+
+struct cpu_topology {
+	int thread_id;
+	int core_id;
+	int cluster_id;
+	cpumask_t thread_sibling;
+	cpumask_t core_sibling;
+};
+
+extern struct cpu_topology cpu_topology[NR_CPUS];
+
+#define topology_physical_package_id(cpu)	(cpu_topology[cpu].cluster_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].cluster_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 2d4554b..252b621 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_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 7cfb92a..9660750 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 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
@@ -390,6 +397,10 @@ 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 0000000..c1e44d5
--- /dev/null
+++ b/arch/arm64/kernel/topology.c
@@ -0,0 +1,102 @@
+/*
+ * 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 cpu_topology 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 cpu_topology *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->cluster_id != cpu_topo->cluster_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);
+	}
+}
+
+void store_cpu_topology(unsigned int cpuid)
+{
+	struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
+
+	/* Something should have picked a topology by the time we get here */
+	if (cpuid_topo->core_id == -1)
+		pr_warn("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 cpu_topology *cpu_topo = &cpu_topology[cpu];
+
+		cpu_topo->thread_id = -1;
+		cpu_topo->core_id =  -1;
+		cpu_topo->cluster_id = -1;
+		cpumask_clear(&cpu_topo->core_sibling);
+		cpumask_clear(&cpu_topo->thread_sibling);
+	}
+
+	/*
+	 * Assign all remaining CPUs to a cluster so the scheduler
+	 * doesn't get confused.
+	 */
+	for_each_possible_cpu(cpu) {
+		struct cpu_topology *cpu_topo = &cpu_topology[cpu];
+
+		if (cpu_topo->cluster_id == -1) {
+			cpu_topo->cluster_id = INT_MAX;
+			cpu_topo->core_id = cpu;
+		}
+	}
+}
-- 
1.9.0.rc3

[PATCH 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 | 142 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index c1e44d5..cda9a48 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -17,10 +17,150 @@
 #include <linux/percpu.h>
 #include <linux/node.h>
 #include <linux/nodemask.h>
+#include <linux/of.h>
 #include <linux/sched.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 = 0;
+	int cpu;
+	struct device_node *t;
+
+	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].cluster_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].cluster_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)
+{
+	char name[10];
+	bool leaf = true;
+	bool has_cores = false;
+	struct device_node *c;
+	static int __initdata cluster_id;
+	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);
+			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++;
+}
+
+static void __init parse_dt_topology(void)
+{
+	struct device_node *cn;
+
+	cn = of_find_node_by_path("/cpus");
+	if (!cn) {
+		pr_err("No CPU information found in DT\n");
+		return;
+	}
+
+	/*
+	 * When topology is provided cpu-map is essentially a root
+	 * cluster with restricted subnodes.
+	 */
+	cn = of_find_node_by_name(cn, "cpu-map");
+	if (!cn)
+		return;
+	parse_cluster(cn, 0);
+}
+
+#else
+static inline void parse_dt_topology(void) {}
+#endif
+
 /*
  * cpu topology table
  */
@@ -87,6 +227,8 @@ void __init init_cpu_topology(void)
 		cpumask_clear(&cpu_topo->thread_sibling);
 	}
 
+	parse_dt_topology();
+
 	/*
 	 * Assign all remaining CPUs to a cluster so the scheduler
 	 * doesn't get confused.
-- 
1.9.0.rc3

[PATCH 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 | 142 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index cda9a48..b330e79 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -19,9 +19,33 @@
 #include <linux/nodemask.h>
 #include <linux/of.h>
 #include <linux/sched.h>
+#include <linux/slab.h>
 
 #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)
 {
@@ -137,9 +161,49 @@ static void __init parse_cluster(struct device_node *cluster, int depth)
 		cluster_id++;
 }
 
+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 cpu;
+
+	__cpu_capacity = kcalloc(nr_cpu_ids, sizeof(*__cpu_capacity),
+				 GFP_NOWAIT);
 
 	cn = of_find_node_by_path("/cpus");
 	if (!cn) {
@@ -155,10 +219,84 @@ static void __init parse_dt_topology(void)
 	if (!cn)
 		return;
 	parse_cluster(cn, 0);
+
+	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
 
 /*
@@ -206,6 +344,8 @@ void store_cpu_topology(unsigned int cpuid)
 		pr_warn("CPU%u: No topology information configured\n", cpuid);
 	else
 		update_siblings_masks(cpuid);
+
+	update_cpu_power(cpuid);
 }
 
 /*
@@ -225,6 +365,8 @@ void __init init_cpu_topology(void)
 		cpu_topo->cluster_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.9.0.rc3

[PATCH 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 current 32 bit ARM
implementation provides a similar estimate so this helps ensure that
work to improve big.LITTLE systems on ARMv7 systems performs similarly
on ARMv8 systems.

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.  In both ARMv7 and ARMv8 cases the numbers were
based on the published DMIPS numbers.

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
or will tune automatically at runtime and so make the specific choice of
numbers here less critical.

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 b330e79..e85560a 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.9.0.rc3

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

[Lorenzo Pieralisi]

Hi Mark,

On Tue, Feb 25, 2014 at 04:25:42AM +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 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.
> 
> No locking of the topology data is done since it is only modified during
> CPU bringup with external serialisation from the SMP code.
> 
> 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 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>

I have been clobbering the code with some random dts configs, running on
top of the patch attached (to make code compliant with default if DT
info is missing for some CPUs). Can you give it a go please and run your
tests against it (I have just put it together) ? It should simplify things,
thoughts ?

AFAIK it should be correct, at least the info required by the scheduler
is reset to default (include/linux/topology.h) if DT nodes are
misconfigured. It applies on top of your series.

Thanks,
Lorenzo

-- >8 --
Subject: [PATCH] arm64: kernel: update topology discovery code

If the device tree does not contain topology information for a CPU,
kernel code should set-up topology information for the CPU according
to the default values defined in Documentation/cputopology.txt.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm64/kernel/topology.c |   34 +++++++++++++---------------------
 1 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index e85560a..22ce390 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -317,6 +317,18 @@ static void update_siblings_masks(unsigned int cpuid)
 	struct cpu_topology *cpu_topo, *cpuid_topo = &cpu_topology[cpuid];
 	int cpu;
 
+	if (cpuid_topo->cluster_id == -1) {
+		/*
+		 * DT does not contain topology information for this cpu
+		 * reset it to default behaviour
+		 */
+		pr_warn("CPU%u: No topology information configured\n", cpuid);
+		cpuid_topo->core_id = 0;
+		cpumask_set_cpu(cpuid, &cpuid_topo->core_sibling);
+		cpumask_set_cpu(cpuid, &cpuid_topo->thread_sibling);
+		return;
+	}
+
 	/* update core and thread sibling masks */
 	for_each_possible_cpu(cpu) {
 		cpu_topo = &cpu_topology[cpu];
@@ -339,14 +351,7 @@ static void update_siblings_masks(unsigned int cpuid)
 
 void store_cpu_topology(unsigned int cpuid)
 {
-	struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
-
-	/* Something should have picked a topology by the time we get here */
-	if (cpuid_topo->core_id == -1)
-		pr_warn("CPU%u: No topology information configured\n", cpuid);
-	else
-		update_siblings_masks(cpuid);
-
+	update_siblings_masks(cpuid);
 	update_cpu_power(cpuid);
 }
 
@@ -372,17 +377,4 @@ void __init init_cpu_topology(void)
 	}
 
 	parse_dt_topology();
-
-	/*
-	 * Assign all remaining CPUs to a cluster so the scheduler
-	 * doesn't get confused.
-	 */
-	for_each_possible_cpu(cpu) {
-		struct cpu_topology *cpu_topo = &cpu_topology[cpu];
-
-		if (cpu_topo->cluster_id == -1) {
-			cpu_topo->cluster_id = INT_MAX;
-			cpu_topo->core_id = cpu;
-		}
-	}
 }
-- 
1.7.5.4

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

[Mark Brown]

On Tue, Feb 25, 2014 at 04:54:11PM +0000, Lorenzo Pieralisi wrote:

> I have been clobbering the code with some random dts configs, running on
> top of the patch attached (to make code compliant with default if DT
> info is missing for some CPUs). Can you give it a go please and run your
> tests against it (I have just put it together) ? It should simplify things,
> thoughts ?

> AFAIK it should be correct, at least the info required by the scheduler
> is reset to default (include/linux/topology.h) if DT nodes are
> misconfigured. It applies on top of your series.

It seems to work fine for me, I'll resend with it squashed in though one
thing below which concerns me a bit.  

Can I suggest that given the sort of thing we're looking at here it
might be easier to apply the code as-is and then do any further work
incrementally?

> If the device tree does not contain topology information for a CPU,
> kernel code should set-up topology information for the CPU according
> to the default values defined in Documentation/cputopology.txt.

> +		pr_warn("CPU%u: No topology information configured\n", cpuid);
> +		cpuid_topo->core_id = 0;

I know this is what the document says and the scheduler copes but it
does make me a bit nervous if we end up with only some CPUs in this
undefined state.  I can't point to any practical problems and it's
going to be the scheduler's fault if it does but I wouldn't be surprised
if there were issues in cases where there is partial topology
information (as opposed to either full topology information or none).

Having said that like I keep saying it seems most sensible to fall back
to MPIDR if we don't have any better idea; it's guaranteed to be
available, may well be correct and helps systems that did a good job
with their hardware.
-------------- 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/20140226/3ac1e6bb/attachment-0001.sig>

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

[Lorenzo Pieralisi]

On Wed, Feb 26, 2014 at 12:50:52AM +0000, Mark Brown wrote:
> On Tue, Feb 25, 2014 at 04:54:11PM +0000, Lorenzo Pieralisi wrote:
> Can I suggest that given the sort of thing we're looking at here it
> might be easier to apply the code as-is and then do any further work
> incrementally?

I think we have enough information to make code complete now, at least
for the topology code, I prefer to merge what's considered to be final.

> > If the device tree does not contain topology information for a CPU,
> > kernel code should set-up topology information for the CPU according
> > to the default values defined in Documentation/cputopology.txt.
> 
> > +		pr_warn("CPU%u: No topology information configured\n", cpuid);
> > +		cpuid_topo->core_id = 0;
> 
> I know this is what the document says and the scheduler copes but it
> does make me a bit nervous if we end up with only some CPUs in this
> undefined state.  I can't point to any practical problems and it's
> going to be the scheduler's fault if it does but I wouldn't be surprised
> if there were issues in cases where there is partial topology
> information (as opposed to either full topology information or none).
> 
> Having said that like I keep saying it seems most sensible to fall back
> to MPIDR if we don't have any better idea; it's guaranteed to be
> available, may well be correct and helps systems that did a good job
> with their hardware.

You have a point. Unless someone feels strongly against this, I would
suggest falling back to MPIDR_EL1 if there is missing or wrong information in
DT.

To do that, you need to scrap the structures initialized from DT
altogether if there is missing or wrong DT information, it is either DT for
all CPUs or MPIDR_EL1 for all CPUs, we can't mix the approaches (and while
at it I'd rather parse the HW MPIDR_EL1 so that you get get the MT bit
from it, which is not present in cpu_logical_map).

Let's sum it up:

- if there is a cpu-map, it takes precedence. If there are missing bits
  of info in the cpu-map we WARN_ON (since this is a firmware bug,
  cpu-map overrides MPIDR_EL1 registers info, if the information in cpu-map
  is wrong or missing we should flag it up and that deserves a WARN_ON)
- if cpu-map is missing, we fall back to MPIDR_EL1 and print a message
  just describing that.

I noticed you will have to add some macros to read MPIDR_EL1 bits in the
process.

Thanks,
Lorenzo

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

[Mark Brown]

On Wed, Feb 26, 2014 at 12:32:08PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Feb 26, 2014 at 12:50:52AM +0000, Mark Brown wrote:

> > Can I suggest that given the sort of thing we're looking at here it
> > might be easier to apply the code as-is and then do any further work
> > incrementally?

> I think we have enough information to make code complete now, at least
> for the topology code, I prefer to merge what's considered to be final.

Hrm, in that case perhaps the first patch is OK to go?  The changes
still being discussed are to the DT binding handling and the addition of
a new parsing mechanism (for MPIDR, which I'd do as a separate patch).
This would be quite helpful, tweaks to the first patch typically cause
manual rebases against the DT patches which increases the stop energy.

> > Having said that like I keep saying it seems most sensible to fall back
> > to MPIDR if we don't have any better idea; it's guaranteed to be
> > available, may well be correct and helps systems that did a good job
> > with their hardware.

> You have a point. Unless someone feels strongly against this, I would
> suggest falling back to MPIDR_EL1 if there is missing or wrong information in
> DT.

Catalin seemed very concerned about any use at all of MPIDR, that's why
the code was removed originally.  Catalin?
-------------- 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/20140226/940eb385/attachment.sig>

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

[Catalin Marinas]

On Wed, Feb 26, 2014 at 02:46:10PM +0000, Mark Brown wrote:
> On Wed, Feb 26, 2014 at 12:32:08PM +0000, Lorenzo Pieralisi wrote:
> > On Wed, Feb 26, 2014 at 12:50:52AM +0000, Mark Brown wrote:
> > > Having said that like I keep saying it seems most sensible to fall back
> > > to MPIDR if we don't have any better idea; it's guaranteed to be
> > > available, may well be correct and helps systems that did a good job
> > > with their hardware.
> 
> > You have a point. Unless someone feels strongly against this, I would
> > suggest falling back to MPIDR_EL1 if there is missing or wrong information in
> > DT.
> 
> Catalin seemed very concerned about any use at all of MPIDR, that's why
> the code was removed originally.  Catalin?

My concern is that the MPIDR is just considered a unique ID. The ARMv8
relaxes the requirement so that it no longer needs to start at 0 and
increase monotonically. I checked with the architecture guys here and
they still expect the affinity hierarchy to be described by MPIDR but we
can have holes in the range for certain levels (i.e. an affinity level
may not start at 0 and may not even increase monotonically for
subsequent CPUs).

So we can either add a tolerant MPIDR parsing or we simply assume that
the topology is _flat_ when DT doesn't provide the information.

-- 
Catalin

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

[Lorenzo Pieralisi]

On Wed, Feb 26, 2014 at 03:48:58PM +0000, Catalin Marinas wrote:
> On Wed, Feb 26, 2014 at 02:46:10PM +0000, Mark Brown wrote:
> > On Wed, Feb 26, 2014 at 12:32:08PM +0000, Lorenzo Pieralisi wrote:
> > > On Wed, Feb 26, 2014 at 12:50:52AM +0000, Mark Brown wrote:
> > > > Having said that like I keep saying it seems most sensible to fall back
> > > > to MPIDR if we don't have any better idea; it's guaranteed to be
> > > > available, may well be correct and helps systems that did a good job
> > > > with their hardware.
> > 
> > > You have a point. Unless someone feels strongly against this, I would
> > > suggest falling back to MPIDR_EL1 if there is missing or wrong information in
> > > DT.
> > 
> > Catalin seemed very concerned about any use at all of MPIDR, that's why
> > the code was removed originally.  Catalin?
> 
> My concern is that the MPIDR is just considered a unique ID. The ARMv8
> relaxes the requirement so that it no longer needs to start at 0 and
> increase monotonically. I checked with the architecture guys here and
> they still expect the affinity hierarchy to be described by MPIDR but we
> can have holes in the range for certain levels (i.e. an affinity level
> may not start at 0 and may not even increase monotonically for
> subsequent CPUs).
> 
> So we can either add a tolerant MPIDR parsing or we simply assume that
> the topology is _flat_ when DT doesn't provide the information.

I think that as far as the topology structures are concerned we can fall
back to MPIDR_EL1 in case cpu-map is missing or incomplete, since even
if the affinity levels contain holes or are not sequential that should
be fine (but see below). After all we have cpu-map to correct things for
this same reason and that will take precedence over the MPIDR_EL1 anyway.

Macros below are required for the scheduler to work. The ids are arch
dependent (so generic code cannot rely on them being sequential or
follow any rule) and if we fall back to MPIDR_EL1 they will get initialized.

#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)

What would be wrong, is leaving the core_sibling and thread_sibling
masks empty and this CAN happen even on current arm32 code if the
cluster id field (ie MPIDR[15:8] for the sake of this discussion) is
different for different CPUs, and that's a bug that fortunately was
never hit (it is a totally valid MPIDR layout, a bit weird though).

On an arm32 4 cpus system with MPIDR layout {0x100, 0x200, 0x300, 0x400}
(which is allowed) the kernel would panic (since the thread_sibling and
core_sibling masks would end up empty).

So I agree with Catalin, either we allow a fall-back to MPIDR_EL1, with relaxed
parsing rules (but let's not forget what I wrote above), or we keep the
current set but with following changes:

- if s topology node for a cpu (or some cpus) is missing or wrong, the whole
  topology structure should be reverted to default (as in
  include/linux/topology.h Documentation/cputopology.txt). This solves Mark's
  concern about having some cpus with missing topology information.
  NB: My patch does not do that, it resets topology only for the misconfigured
  cpu.

That's the best we can do. I think that an MPIDR_EL1 fall back mechanism
should be put in place, as a separate patch or squashed to patch 1.

I would prefer the latter, but if you want to get the infrastructure in
first I think that's acceptable.

Apologies for all requested changes.

Thanks,
Lorenzo

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

[Mark Brown]

On Wed, Feb 26, 2014 at 03:48:58PM +0000, Catalin Marinas wrote:

> My concern is that the MPIDR is just considered a unique ID. The ARMv8
> relaxes the requirement so that it no longer needs to start at 0 and
> increase monotonically. I checked with the architecture guys here and
> they still expect the affinity hierarchy to be described by MPIDR but we
> can have holes in the range for certain levels (i.e. an affinity level
> may not start at 0 and may not even increase monotonically for
> subsequent CPUs).

I don't think anything has a problem with holes in the number range, the
only thing I can think of which imposes that requirement is the DT
binding but obviously that's not relevant if we are using MPIDR.  The
core topology documentation explicitly says that these are physical IDs
and entirely up to the architecture/platform.  It'd seem odd given that
we have hotplug support.

What problems do you anticipate?

> So we can either add a tolerant MPIDR parsing or we simply assume that
> the topology is _flat_ when DT doesn't provide the information.

Right, that's what the current code does but I'm not sure it's the best
option.
-------------- 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/20140227/02fdbbc0/attachment.sig>

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

[Mark Brown]

On Wed, Feb 26, 2014 at 05:50:04PM +0000, Lorenzo Pieralisi wrote:

> What would be wrong, is leaving the core_sibling and thread_sibling
> masks empty and this CAN happen even on current arm32 code if the
> cluster id field (ie MPIDR[15:8] for the sake of this discussion) is
> different for different CPUs, and that's a bug that fortunately was
> never hit (it is a totally valid MPIDR layout, a bit weird though).

This is a good point, I will make sure any MPIDR code handles it (we
should fix the 32 bit code as well of course, I'll try to look at that
too).

> - if s topology node for a cpu (or some cpus) is missing or wrong, the whole
>   topology structure should be reverted to default (as in
>   include/linux/topology.h Documentation/cputopology.txt). This solves Mark's
>   concern about having some cpus with missing topology information.
>   NB: My patch does not do that, it resets topology only for the misconfigured
>   cpu.

I would certainly prefer this, I'll do an update (though not today,
trains again).

> That's the best we can do. I think that an MPIDR_EL1 fall back mechanism
> should be put in place, as a separate patch or squashed to patch 1.

> I would prefer the latter, but if you want to get the infrastructure in
> first I think that's acceptable.

I'd certainly prefer to get the code merged and work incrementally, it'd
make life easier to avoid having to manage the rebases.
-------------- 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/20140227/25c17ebc/attachment.sig>

[PATCH 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.

No locking of the topology data is done since it is only modified during
CPU bringup with external serialisation from the SMP code.

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 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           | 11 +++++
 arch/arm64/kernel/topology.c      | 95 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 170 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 27bbcfc..fea7b47 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -164,6 +164,30 @@ config SMP
 
 	  If you don't know what to do here, say N.
 
+config 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 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 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 0000000..c8a47e8
--- /dev/null
+++ b/arch/arm64/include/asm/topology.h
@@ -0,0 +1,39 @@
+#ifndef __ASM_TOPOLOGY_H
+#define __ASM_TOPOLOGY_H
+
+#ifdef CONFIG_CPU_TOPOLOGY
+
+#include <linux/cpumask.h>
+
+struct cpu_topology {
+	int thread_id;
+	int core_id;
+	int cluster_id;
+	cpumask_t thread_sibling;
+	cpumask_t core_sibling;
+};
+
+extern struct cpu_topology cpu_topology[NR_CPUS];
+
+#define topology_physical_package_id(cpu)	(cpu_topology[cpu].cluster_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].cluster_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 2d4554b..252b621 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_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 7cfb92a..9660750 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 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
@@ -390,6 +397,10 @@ 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 0000000..0d94918
--- /dev/null
+++ b/arch/arm64/kernel/topology.c
@@ -0,0 +1,95 @@
+/*
+ * 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 cpu_topology 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 cpu_topology *cpu_topo, *cpuid_topo = &cpu_topology[cpuid];
+	int cpu;
+
+	if (cpuid_topo->cluster_id == -1) {
+		/*
+		 * DT does not contain topology information for this cpu
+		 * reset it to default behaviour
+		 */
+		pr_warn("CPU%u: No topology information configured\n", cpuid);
+		cpuid_topo->core_id = 0;
+		cpumask_set_cpu(cpuid, &cpuid_topo->core_sibling);
+		cpumask_set_cpu(cpuid, &cpuid_topo->thread_sibling);
+		return;
+	}
+
+	/* update core and thread sibling masks */
+	for_each_possible_cpu(cpu) {
+		cpu_topo = &cpu_topology[cpu];
+
+		if (cpuid_topo->cluster_id != cpu_topo->cluster_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);
+	}
+}
+
+void store_cpu_topology(unsigned int cpuid)
+{
+	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 cpu_topology *cpu_topo = &cpu_topology[cpu];
+
+		cpu_topo->thread_id = -1;
+		cpu_topo->core_id =  -1;
+		cpu_topo->cluster_id = -1;
+		cpumask_clear(&cpu_topo->core_sibling);
+		cpumask_clear(&cpu_topo->thread_sibling);
+	}
+}
-- 
1.9.0

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

[Mark Brown]

On Sat, Feb 22, 2014 at 12:26:48PM +0000, Lorenzo Pieralisi wrote:
> On Sat, Feb 22, 2014 at 02:06:02AM +0000, Mark Brown wrote:
> > On Fri, Feb 21, 2014 at 03:01:40PM +0000, Lorenzo Pieralisi wrote:

> > > If the DT does not contain a proper topology the scheduler seem to go for a
> > > toss. I tried to track it down and it seems it expects topology cpumasks to be
> > > initialized regardless (eg to possible mask), they cannot be left empty.

> > Could you be more specific please?  I didn't notice anything particular
> > in my testing but then the fact that it's a model does obscure a lot of
> > things.

> I will send you a backtrace, config file, commit.

> Problem is hit when CONFIG_SCHED_SMT is on and there is no cpu-map in
> the dts.

Interesting...  I did test this incrementally during development but
didn't see any issues, though as there have been many stylistic updates
so I have to confess it's probably been a while since I ran that test,
I'll take a look when I have a stable enough network connection to run
the models (I'm on a Shinkansen at the minute so my connection keeps
dropping out).

> What's a model ? And if you mean the processor model, what can it possibly
> obscure as long as this patch is concerned ?

A fast or foundation model.  Your only description was "went for a toss"
so I had no idea what the problem was, I was guessing that this was some
sort of issue with performance like only using one core or something.
If it's a crash then using the models won't make a difference but for
performance issues the emulation means it's not always apparent when
using the system if the kernel is performing poorly or if the emulation
is just slow.

> > > On top of that, the pr_info message is quite annoying and should be
> > > probably downgraded or removed altogether.

> > This was deliberate - since we are not willing to use the MPIDR
> > information to discover the topology we need to get the information into
> > the DT bindings in order to discover it.  Even in a SMP system there is
> > a difference in how closely attached the cores are so it seems like we
> > should be expecting a description of the topology.

> We have to make a decision. Either we rely on MPIDR_EL1 as a fallback or
> we barf on missing topology nodes (or we just set-up a flat topology if
> the cpu-map is missing and do not log anything).

Indeed.  My personal preference would be that we fall back to MPIDR if
we don't have topology information from the firmware (since it's always
possible that the silicon does the right thing) or failing that we
insist on topology information from the firmware.  Once systems are out
in the wild it's potentially painful to get the data added to DTs so
pushing for information in case we need it in the future seems like the
safest approach in cases like this where it's not going to be too much
work to provide.
-------------- 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/20140223/6988707f/attachment-0001.sig>

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

[Lorenzo Pieralisi]

On Sat, Feb 22, 2014 at 02:06:02AM +0000, Mark Brown wrote:
> On Fri, Feb 21, 2014 at 03:01:40PM +0000, Lorenzo Pieralisi wrote:
> 
> > If the DT does not contain a proper topology the scheduler seem to go for a
> > toss. I tried to track it down and it seems it expects topology cpumasks to be
> > initialized regardless (eg to possible mask), they cannot be left empty.
> 
> Could you be more specific please?  I didn't notice anything particular
> in my testing but then the fact that it's a model does obscure a lot of
> things.

I will send you a backtrace, config file, commit.

Problem is hit when CONFIG_SCHED_SMT is on and there is no cpu-map in
the dts.

What's a model ? And if you mean the processor model, what can it possibly
obscure as long as this patch is concerned ?

> > On top of that, the pr_info message is quite annoying and should be
> > probably downgraded or removed altogether.
> 
> This was deliberate - since we are not willing to use the MPIDR
> information to discover the topology we need to get the information into
> the DT bindings in order to discover it.  Even in a SMP system there is
> a difference in how closely attached the cores are so it seems like we
> should be expecting a description of the topology.

We have to make a decision. Either we rely on MPIDR_EL1 as a fallback or
we barf on missing topology nodes (or we just set-up a flat topology if
the cpu-map is missing and do not log anything).

Thanks,
Lorenzo

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

[Mark Brown]

On Fri, Feb 21, 2014 at 03:01:40PM +0000, Lorenzo Pieralisi wrote:

> If the DT does not contain a proper topology the scheduler seem to go for a
> toss. I tried to track it down and it seems it expects topology cpumasks to be
> initialized regardless (eg to possible mask), they cannot be left empty.

Could you be more specific please?  I didn't notice anything particular
in my testing but then the fact that it's a model does obscure a lot of
things.

> On top of that, the pr_info message is quite annoying and should be
> probably downgraded or removed altogether.

This was deliberate - since we are not willing to use the MPIDR
information to discover the topology we need to get the information into
the DT bindings in order to discover it.  Even in a SMP system there is
a difference in how closely attached the cores are so it seems like we
should be expecting a description of the topology.
-------------- 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/20140222/c5be7caa/attachment.sig>

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

[Lorenzo Pieralisi]

On Mon, Feb 10, 2014 at 01:02:01PM +0000, Mark Brown wrote:

[...]

> +void store_cpu_topology(unsigned int cpuid)
> +{
> +	struct cpu_topology *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);

If the DT does not contain a proper topology the scheduler seem to go for a
toss. I tried to track it down and it seems it expects topology cpumasks to be
initialized regardless (eg to possible mask), they cannot be left empty.

Either update_siblings_masks is called regardless or possible mask must
be copied to the topology masks.

I will have a thorough look to check if the scheduler has a fall-back
mechanism.

On top of that, the pr_info message is quite annoying and should be
probably downgraded or removed altogether.

Furthermore leaving core_id as -1 should be fine, but I have to have a
proper look into the scheduler topology code to countercheck that.

Lorenzo

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

[Mark Brown]

On Wed, Feb 12, 2014 at 10:27:16AM +0000, Catalin Marinas wrote:
> On Wed, Feb 12, 2014 at 08:04:54AM +0000, Vincent Guittot wrote:

> > The fields will be used to construct topology so we must ensure their
> > visibility

> I wonder whether you need spinlocks around the topology updating code.

It certainly feels a lot safer to have them; if we can't convince
ourselves that the code is safe without them then there's very little
cost in having them so we may as well err on the side of doing that -
does that seem reasonable?
-------------- 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/20140212/a4dd9428/attachment.sig>

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

[Catalin Marinas]

On Wed, Feb 12, 2014 at 08:04:54AM +0000, Vincent Guittot wrote:
> On 11 February 2014 15:07, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Tue, Feb 11, 2014 at 01:18:56PM +0000, Vincent Guittot wrote:
> >> On 11 February 2014 11:34, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Tue, Feb 11, 2014 at 08:15:19AM +0000, Vincent Guittot wrote:
> >> >> On 10 February 2014 17:46, Mark Brown <broonie@kernel.org> wrote:
> >> >> > On Mon, Feb 10, 2014 at 04:22:31PM +0000, Catalin Marinas wrote:
> >> >> >> On Mon, Feb 10, 2014 at 01:02:01PM +0000, Mark Brown wrote:
> >> >> >
> >> >> >> > +           if (cpu != cpuid)
> >> >> >> > +                   cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
> >> >> >> > +   }
> >> >> >> > +   smp_wmb();
> >> >> >
> >> >> >> I now noticed there are a couple of smp_wmb() calls in this patch. What
> >> >> >> are they for?
> >> >> >
> >> >> > To be honest I mostly cargo culted them from the ARM implementation; I
> >> >> > did look a bit but didn't fully dig into it - it seemed they were
> >> >> > required to ensure that the updates for the new CPU are visible over all
> >> >> > CPUs.  Vincent?
> >> >>
> >> >> Yes that's it. we must ensure that updates are made visible to other CPUs
> >> >
> >> > In relation to what? The smp_* barriers ensure ordering of observability
> >> > between a number of independent accesses, so you must be ensuring
> >> > ordering against something else. Also, you need to guarantee ordering on the
> >> > read-side too -- how is this achieved? I can't see any smp_rmb calls from a
> >> > quick grep, so I assume you're making use of address dependencies?
> >>
> >> The boot sequence ensures the rmb
> >
> > As Will said, smp_*mb() do not ensure absolute visibility, only relative
> > to subsequent memory accesses on the same processor. So just placing a
> 
> It's my time to be a bit confused, if smp_*mb() do not ensure absolute
> visibility on other CPUs, how can we ensure that ?

smb_wmb()/smb_rmb() do not provide any waiting, they are not
synchronisation primitives. You have to use spinlocks or some other
polling (and of course, barriers for relative ordering of memory
reads/writes).

> > barrier at the end of a function does not mean much, it only shows half
> > of the problem it is trying to solve.
> >
> > How are the secondary CPUs using this information? AFAICT, secondaries
> > call smp_store_cpu_info() which also go through each CPU in
> > update_siblings_mask(). Is there any race here that smp_wmb() is trying
> > to solve?
> 
> The fields will be used to construct topology so we must ensure their
> visibility

I wonder whether you need spinlocks around the topology updating code.

-- 
Catalin

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

[Vincent Guittot]

On 11 February 2014 15:07, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Tue, Feb 11, 2014 at 01:18:56PM +0000, Vincent Guittot wrote:
>> On 11 February 2014 11:34, Will Deacon <will.deacon@arm.com> wrote:
>> > On Tue, Feb 11, 2014 at 08:15:19AM +0000, Vincent Guittot wrote:
>> >> On 10 February 2014 17:46, Mark Brown <broonie@kernel.org> wrote:
>> >> > On Mon, Feb 10, 2014 at 04:22:31PM +0000, Catalin Marinas wrote:
>> >> >> On Mon, Feb 10, 2014 at 01:02:01PM +0000, Mark Brown wrote:
>> >> >
>> >> >> > +           if (cpu != cpuid)
>> >> >> > +                   cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
>> >> >> > +   }
>> >> >> > +   smp_wmb();
>> >> >
>> >> >> I now noticed there are a couple of smp_wmb() calls in this patch. What
>> >> >> are they for?
>> >> >
>> >> > To be honest I mostly cargo culted them from the ARM implementation; I
>> >> > did look a bit but didn't fully dig into it - it seemed they were
>> >> > required to ensure that the updates for the new CPU are visible over all
>> >> > CPUs.  Vincent?
>> >>
>> >> Yes that's it. we must ensure that updates are made visible to other CPUs
>> >
>> > In relation to what? The smp_* barriers ensure ordering of observability
>> > between a number of independent accesses, so you must be ensuring
>> > ordering against something else. Also, you need to guarantee ordering on the
>> > read-side too -- how is this achieved? I can't see any smp_rmb calls from a
>> > quick grep, so I assume you're making use of address dependencies?
>>
>> The boot sequence ensures the rmb
>
> As Will said, smp_*mb() do not ensure absolute visibility, only relative
> to subsequent memory accesses on the same processor. So just placing a

It's my time to be a bit confused, if smp_*mb() do not ensure absolute
visibility on other CPUs, how can we ensure that ?

> barrier at the end of a function does not mean much, it only shows half
> of the problem it is trying to solve.
>
> How are the secondary CPUs using this information? AFAICT, secondaries
> call smp_store_cpu_info() which also go through each CPU in
> update_siblings_mask(). Is there any race here that smp_wmb() is trying
> to solve?

The fields will be used to construct topology so we must ensure their
visibility

Vincent
>
> I guess for secondaries you could move the barrier just before
> set_cpu_online(), this way it is clear that we want any previous writes
> to become visible when this CPU is marked online. For the primary, any
> memory writes should become visible before the CPU is started. One
> synchronisation point is the pen release, depending on the smp_ops. I
> think that's already covered by code like arch/arm/mach-*/platsmp.c.
>
> So my proposal is to remove the smp_wmb() from topology.c and add it
> where it is relevant as described above. If we have some race in
> topology.c (like for example we may later decide to start more
> secondaries at the same time), it needs to be solved using spinlocks.
>
> --
> Catalin

[PATCH 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 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           | 11 +++++
 arch/arm64/kernel/topology.c      | 89 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 164 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 27bbcfc7202a..fea7b477676b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -164,6 +164,30 @@ config SMP
 
 	  If you don't know what to do here, say N.
 
+config 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 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 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..c8a47e8f452b
--- /dev/null
+++ b/arch/arm64/include/asm/topology.h
@@ -0,0 +1,39 @@
+#ifndef __ASM_TOPOLOGY_H
+#define __ASM_TOPOLOGY_H
+
+#ifdef CONFIG_CPU_TOPOLOGY
+
+#include <linux/cpumask.h>
+
+struct cpu_topology {
+	int thread_id;
+	int core_id;
+	int cluster_id;
+	cpumask_t thread_sibling;
+	cpumask_t core_sibling;
+};
+
+extern struct cpu_topology cpu_topology[NR_CPUS];
+
+#define topology_physical_package_id(cpu)	(cpu_topology[cpu].cluster_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].cluster_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..252b62181532 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_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 7cfb92a4ab66..9660750f34ba 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 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
@@ -390,6 +397,10 @@ 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..cbb8d432214e
--- /dev/null
+++ b/arch/arm64/kernel/topology.c
@@ -0,0 +1,89 @@
+/*
+ * 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 cpu_topology 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 cpu_topology *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->cluster_id != cpu_topo->cluster_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);
+	}
+}
+
+void store_cpu_topology(unsigned int cpuid)
+{
+	struct cpu_topology *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 cpu_topology *cpu_topo = &cpu_topology[cpu];
+
+		cpu_topo->thread_id = -1;
+		cpu_topo->core_id =  -1;
+		cpu_topo->cluster_id = -1;
+		cpumask_clear(&cpu_topo->core_sibling);
+		cpumask_clear(&cpu_topo->thread_sibling);
+	}
+}
-- 
1.9.0.rc3

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

[Mark Brown]

On Tue, Feb 11, 2014 at 03:46:04PM +0100, Vincent Guittot wrote:
> On 11 February 2014 15:07, Catalin Marinas <catalin.marinas@arm.com> wrote:

> > As Will said, smp_*mb() do not ensure absolute visibility, only relative
> > to subsequent memory accesses on the same processor. So just placing a
> > barrier at the end of a function does not mean much, it only shows half
> > of the problem it is trying to solve.

> OK, that's probably the shortcut that has been made, we want to drain
> the write buffer to make modification available to other cpus and I
> though smp_wmb and the associated mb(ishst) was there for that
> purpose.

It does also explain where I got to trying to figure out what exactly
the mechanism was!

> > So my proposal is to remove the smp_wmb() from topology.c and add it
> > where it is relevant as described above. If we have some race in
> > topology.c (like for example we may later decide to start more
> > secondaries at the same time), it needs to be solved using spinlocks.

I'll just repost a version dropping them from the topology series and
separately add the barriers elsewhere.  The 32 bit ARM implementation
probably ought to be fixed as well.
-------------- 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/20140211/af5fc293/attachment-0001.sig>

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

[Vincent Guittot]

On 11 February 2014 15:07, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Tue, Feb 11, 2014 at 01:18:56PM +0000, Vincent Guittot wrote:
>> On 11 February 2014 11:34, Will Deacon <will.deacon@arm.com> wrote:
>> > On Tue, Feb 11, 2014 at 08:15:19AM +0000, Vincent Guittot wrote:
>> >> On 10 February 2014 17:46, Mark Brown <broonie@kernel.org> wrote:
>> >> > On Mon, Feb 10, 2014 at 04:22:31PM +0000, Catalin Marinas wrote:
>> >> >> On Mon, Feb 10, 2014 at 01:02:01PM +0000, Mark Brown wrote:
>> >> >
>> >> >> > +           if (cpu != cpuid)
>> >> >> > +                   cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
>> >> >> > +   }
>> >> >> > +   smp_wmb();
>> >> >
>> >> >> I now noticed there are a couple of smp_wmb() calls in this patch. What
>> >> >> are they for?
>> >> >
>> >> > To be honest I mostly cargo culted them from the ARM implementation; I
>> >> > did look a bit but didn't fully dig into it - it seemed they were
>> >> > required to ensure that the updates for the new CPU are visible over all
>> >> > CPUs.  Vincent?
>> >>
>> >> Yes that's it. we must ensure that updates are made visible to other CPUs
>> >
>> > In relation to what? The smp_* barriers ensure ordering of observability
>> > between a number of independent accesses, so you must be ensuring
>> > ordering against something else. Also, you need to guarantee ordering on the
>> > read-side too -- how is this achieved? I can't see any smp_rmb calls from a
>> > quick grep, so I assume you're making use of address dependencies?
>>
>> The boot sequence ensures the rmb
>
> As Will said, smp_*mb() do not ensure absolute visibility, only relative
> to subsequent memory accesses on the same processor. So just placing a
> barrier at the end of a function does not mean much, it only shows half
> of the problem it is trying to solve.

OK, that's probably the shortcut that has been made, we want to drain
the write buffer to make modification available to other cpus and I
though smp_wmb and the associated mb(ishst) was there for that
purpose.

Vincent

>
> How are the secondary CPUs using this information? AFAICT, secondaries
> call smp_store_cpu_info() which also go through each CPU in
> update_siblings_mask(). Is there any race here that smp_wmb() is trying
> to solve?
>
> I guess for secondaries you could move the barrier just before
> set_cpu_online(), this way it is clear that we want any previous writes
> to become visible when this CPU is marked online. For the primary, any
> memory writes should become visible before the CPU is started. One
> synchronisation point is the pen release, depending on the smp_ops. I
> think that's already covered by code like arch/arm/mach-*/platsmp.c.
>
> So my proposal is to remove the smp_wmb() from topology.c and add it
> where it is relevant as described above. If we have some race in
> topology.c (like for example we may later decide to start more
> secondaries at the same time), it needs to be solved using spinlocks.
>
> --
> Catalin

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

[Catalin Marinas]

On Tue, Feb 11, 2014 at 01:18:56PM +0000, Vincent Guittot wrote:
> On 11 February 2014 11:34, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Feb 11, 2014 at 08:15:19AM +0000, Vincent Guittot wrote:
> >> On 10 February 2014 17:46, Mark Brown <broonie@kernel.org> wrote:
> >> > On Mon, Feb 10, 2014 at 04:22:31PM +0000, Catalin Marinas wrote:
> >> >> On Mon, Feb 10, 2014 at 01:02:01PM +0000, Mark Brown wrote:
> >> >
> >> >> > +           if (cpu != cpuid)
> >> >> > +                   cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
> >> >> > +   }
> >> >> > +   smp_wmb();
> >> >
> >> >> I now noticed there are a couple of smp_wmb() calls in this patch. What
> >> >> are they for?
> >> >
> >> > To be honest I mostly cargo culted them from the ARM implementation; I
> >> > did look a bit but didn't fully dig into it - it seemed they were
> >> > required to ensure that the updates for the new CPU are visible over all
> >> > CPUs.  Vincent?
> >>
> >> Yes that's it. we must ensure that updates are made visible to other CPUs
> >
> > In relation to what? The smp_* barriers ensure ordering of observability
> > between a number of independent accesses, so you must be ensuring
> > ordering against something else. Also, you need to guarantee ordering on the
> > read-side too -- how is this achieved? I can't see any smp_rmb calls from a
> > quick grep, so I assume you're making use of address dependencies?
> 
> The boot sequence ensures the rmb

As Will said, smp_*mb() do not ensure absolute visibility, only relative
to subsequent memory accesses on the same processor. So just placing a
barrier at the end of a function does not mean much, it only shows half
of the problem it is trying to solve.

How are the secondary CPUs using this information? AFAICT, secondaries
call smp_store_cpu_info() which also go through each CPU in
update_siblings_mask(). Is there any race here that smp_wmb() is trying
to solve?

I guess for secondaries you could move the barrier just before
set_cpu_online(), this way it is clear that we want any previous writes
to become visible when this CPU is marked online. For the primary, any
memory writes should become visible before the CPU is started. One
synchronisation point is the pen release, depending on the smp_ops. I
think that's already covered by code like arch/arm/mach-*/platsmp.c.

So my proposal is to remove the smp_wmb() from topology.c and add it
where it is relevant as described above. If we have some race in
topology.c (like for example we may later decide to start more
secondaries at the same time), it needs to be solved using spinlocks.

-- 
Catalin

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

[Vincent Guittot]

On 11 February 2014 11:34, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Feb 11, 2014 at 08:15:19AM +0000, Vincent Guittot wrote:
>> On 10 February 2014 17:46, Mark Brown <broonie@kernel.org> wrote:
>> > On Mon, Feb 10, 2014 at 04:22:31PM +0000, Catalin Marinas wrote:
>> >> On Mon, Feb 10, 2014 at 01:02:01PM +0000, Mark Brown wrote:
>> >
>> >> > +           if (cpu != cpuid)
>> >> > +                   cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
>> >> > +   }
>> >> > +   smp_wmb();
>> >
>> >> I now noticed there are a couple of smp_wmb() calls in this patch. What
>> >> are they for?
>> >
>> > To be honest I mostly cargo culted them from the ARM implementation; I
>> > did look a bit but didn't fully dig into it - it seemed they were
>> > required to ensure that the updates for the new CPU are visible over all
>> > CPUs.  Vincent?
>>
>> Yes that's it. we must ensure that updates are made visible to other CPUs
>
> In relation to what? The smp_* barriers ensure ordering of observability
> between a number of independent accesses, so you must be ensuring
> ordering against something else. Also, you need to guarantee ordering on the
> read-side too -- how is this achieved? I can't see any smp_rmb calls from a
> quick grep, so I assume you're making use of address dependencies?

The boot sequence ensures the rmb

Vincent

>
> /confused
>
> Will

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

[Will Deacon]

On Tue, Feb 11, 2014 at 08:15:19AM +0000, Vincent Guittot wrote:
> On 10 February 2014 17:46, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Feb 10, 2014 at 04:22:31PM +0000, Catalin Marinas wrote:
> >> On Mon, Feb 10, 2014 at 01:02:01PM +0000, Mark Brown wrote:
> >
> >> > +           if (cpu != cpuid)
> >> > +                   cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
> >> > +   }
> >> > +   smp_wmb();
> >
> >> I now noticed there are a couple of smp_wmb() calls in this patch. What
> >> are they for?
> >
> > To be honest I mostly cargo culted them from the ARM implementation; I
> > did look a bit but didn't fully dig into it - it seemed they were
> > required to ensure that the updates for the new CPU are visible over all
> > CPUs.  Vincent?
> 
> Yes that's it. we must ensure that updates are made visible to other CPUs

In relation to what? The smp_* barriers ensure ordering of observability
between a number of independent accesses, so you must be ensuring
ordering against something else. Also, you need to guarantee ordering on the
read-side too -- how is this achieved? I can't see any smp_rmb calls from a
quick grep, so I assume you're making use of address dependencies?

/confused

Will

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

[Vincent Guittot]

On 10 February 2014 17:46, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Feb 10, 2014 at 04:22:31PM +0000, Catalin Marinas wrote:
>> On Mon, Feb 10, 2014 at 01:02:01PM +0000, Mark Brown wrote:
>
>> > +           if (cpu != cpuid)
>> > +                   cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
>> > +   }
>> > +   smp_wmb();
>
>> I now noticed there are a couple of smp_wmb() calls in this patch. What
>> are they for?
>
> To be honest I mostly cargo culted them from the ARM implementation; I
> did look a bit but didn't fully dig into it - it seemed they were
> required to ensure that the updates for the new CPU are visible over all
> CPUs.  Vincent?

Yes that's it. we must ensure that updates are made visible to other CPUs

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

[Mark Brown]

On Mon, Feb 10, 2014 at 04:22:31PM +0000, Catalin Marinas wrote:
> On Mon, Feb 10, 2014 at 01:02:01PM +0000, Mark Brown wrote:

> > +		if (cpu != cpuid)
> > +			cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
> > +	}
> > +	smp_wmb();

> I now noticed there are a couple of smp_wmb() calls in this patch. What
> are they for?

To be honest I mostly cargo culted them from the ARM implementation; I
did look a bit but didn't fully dig into it - it seemed they were
required to ensure that the updates for the new CPU are visible over all
CPUs.  Vincent?
-------------- 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/20140210/fc8ec600/attachment-0001.sig>

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

[Catalin Marinas]

On Mon, Feb 10, 2014 at 01:02:01PM +0000, Mark Brown wrote:
> +static void update_siblings_masks(unsigned int cpuid)
> +{
> +	struct cpu_topology *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->cluster_id != cpu_topo->cluster_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();

I now noticed there are a couple of smp_wmb() calls in this patch. What
are they for?

-- 
Catalin

[PATCH 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 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           | 11 +++++
 arch/arm64/kernel/topology.c      | 91 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 166 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 27bbcfc7202a..fea7b477676b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -164,6 +164,30 @@ config SMP
 
 	  If you don't know what to do here, say N.
 
+config 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 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 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..c8a47e8f452b
--- /dev/null
+++ b/arch/arm64/include/asm/topology.h
@@ -0,0 +1,39 @@
+#ifndef __ASM_TOPOLOGY_H
+#define __ASM_TOPOLOGY_H
+
+#ifdef CONFIG_CPU_TOPOLOGY
+
+#include <linux/cpumask.h>
+
+struct cpu_topology {
+	int thread_id;
+	int core_id;
+	int cluster_id;
+	cpumask_t thread_sibling;
+	cpumask_t core_sibling;
+};
+
+extern struct cpu_topology cpu_topology[NR_CPUS];
+
+#define topology_physical_package_id(cpu)	(cpu_topology[cpu].cluster_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].cluster_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..252b62181532 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_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 7cfb92a4ab66..9660750f34ba 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 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
@@ -390,6 +397,10 @@ 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..a6d4ed2d69c0
--- /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 cpu_topology 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 cpu_topology *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->cluster_id != cpu_topo->cluster_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 cpu_topology *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 cpu_topology *cpu_topo = &cpu_topology[cpu];
+
+		cpu_topo->thread_id = -1;
+		cpu_topo->core_id =  -1;
+		cpu_topo->cluster_id = -1;
+		cpumask_clear(&cpu_topo->core_sibling);
+		cpumask_clear(&cpu_topo->thread_sibling);
+	}
+	smp_wmb();
+}
-- 
1.9.0.rc3

[PATCH 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 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           | 11 +++++
 arch/arm64/kernel/topology.c      | 91 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 166 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..02309c3eec33 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 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 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 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..c8a47e8f452b
--- /dev/null
+++ b/arch/arm64/include/asm/topology.h
@@ -0,0 +1,39 @@
+#ifndef __ASM_TOPOLOGY_H
+#define __ASM_TOPOLOGY_H
+
+#ifdef CONFIG_CPU_TOPOLOGY
+
+#include <linux/cpumask.h>
+
+struct cpu_topology {
+	int thread_id;
+	int core_id;
+	int cluster_id;
+	cpumask_t thread_sibling;
+	cpumask_t core_sibling;
+};
+
+extern struct cpu_topology cpu_topology[NR_CPUS];
+
+#define topology_physical_package_id(cpu)	(cpu_topology[cpu].cluster_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].cluster_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..252b62181532 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_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..faca699de62d 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 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,10 @@ 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..a6d4ed2d69c0
--- /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 cpu_topology 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 cpu_topology *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->cluster_id != cpu_topo->cluster_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 cpu_topology *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 cpu_topology *cpu_topo = &cpu_topology[cpu];
+
+		cpu_topo->thread_id = -1;
+		cpu_topo->core_id =  -1;
+		cpu_topo->cluster_id = -1;
+		cpumask_clear(&cpu_topo->core_sibling);
+		cpumask_clear(&cpu_topo->thread_sibling);
+	}
+	smp_wmb();
+}
-- 
1.8.5.2

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

[Vincent Guittot]

On 13 January 2014 18:44, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Mon, Jan 13, 2014 at 04:30:45PM +0000, Mark Brown wrote:
>> On Mon, Jan 13, 2014 at 04:10:59PM +0000, Lorenzo Pieralisi wrote:
>> > On Sun, Jan 12, 2014 at 07:20:38PM +0000, Mark Brown wrote:
>>
>> > > +struct cpu_topology {
>> > > + int thread_id;
>> > > + int core_id;
>> > > + int socket_id;
>>
>> > Is there any reason why we can't rename socket_id to cluster_id ? It won't
>> > change our lives but at least we kind of know what it means in ARM world.
>>
>> I really don't care, whatever you guys want.
>
> s/socket_id/cluster_id
>
> unless we have a compelling reason to keep the socket_id naming, I do
> not see it, given that cpu_topology is arch specific anyway.
>
> socket_id means really nothing in ARM world.
>
> Again, Vincent if you see a compelling reason to keep socket_id as in arm32
> that I am missing please shout.

I don't have any compellingreason, i have just used the same name than
other platform.

Vincent

>
>>
>> > > +#define mc_capable()     (cpu_topology[0].socket_id != -1)
>> > > +#define smt_capable()    (cpu_topology[0].thread_id != -1)
>>
>> > Are the two macros above still required in the kernel ? I can't see any
>> > usage at present.
>> >
>> > Vincent, do you know why they were not removed in commit:
>> >
>> > 8e7fbcbc22c12414bcc9dfdd683637f58fb32759
>> >
>> > I am certainly missing something.
>>
>> They're defined by a bunch of other architectures (including x86).  If I
>> had to guess I'd say the architectures are still providing the
>> information so we don't need to go round adding it again if someone
>> comes up with a use for it in the core.
>
> Yes, let's keep the macros, just wanted to make sure I got it right.
>
> Lorenzo
>

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

[Lorenzo Pieralisi]

On Mon, Jan 13, 2014 at 04:30:45PM +0000, Mark Brown wrote:
> On Mon, Jan 13, 2014 at 04:10:59PM +0000, Lorenzo Pieralisi wrote:
> > On Sun, Jan 12, 2014 at 07:20:38PM +0000, Mark Brown wrote:
> 
> > > +struct cpu_topology {
> > > +	int thread_id;
> > > +	int core_id;
> > > +	int socket_id;
> 
> > Is there any reason why we can't rename socket_id to cluster_id ? It won't
> > change our lives but at least we kind of know what it means in ARM world.
> 
> I really don't care, whatever you guys want.

s/socket_id/cluster_id

unless we have a compelling reason to keep the socket_id naming, I do
not see it, given that cpu_topology is arch specific anyway.

socket_id means really nothing in ARM world.

Again, Vincent if you see a compelling reason to keep socket_id as in arm32
that I am missing please shout.

> 
> > > +#define mc_capable()	(cpu_topology[0].socket_id != -1)
> > > +#define smt_capable()	(cpu_topology[0].thread_id != -1)
> 
> > Are the two macros above still required in the kernel ? I can't see any
> > usage at present.
> > 
> > Vincent, do you know why they were not removed in commit:
> > 
> > 8e7fbcbc22c12414bcc9dfdd683637f58fb32759
> > 
> > I am certainly missing something.
> 
> They're defined by a bunch of other architectures (including x86).  If I
> had to guess I'd say the architectures are still providing the
> information so we don't need to go round adding it again if someone
> comes up with a use for it in the core.

Yes, let's keep the macros, just wanted to make sure I got it right.

Lorenzo

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

[Lorenzo Pieralisi]

On Mon, Jan 13, 2014 at 04:44:06PM +0000, Vincent Guittot wrote:

[...]

> >> +extern struct cpu_topology 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)
> >
> > Are the two macros above still required in the kernel ? I can't see any
> > usage at present.
> >
> > Vincent, do you know why they were not removed in commit:
> >
> > 8e7fbcbc22c12414bcc9dfdd683637f58fb32759
> >
> > I am certainly missing something.
> 
> I think it was not planned to be used only by the scheduler but since
> 8e7fbcbc22c12414bcc9dfdd683637f58fb32759, we have reach a situation
> where nobody use them for the moment.

Ok, we can add the macros and eventually remove all of them in a later
patch if we think they must go then.

Lorenzo

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

[Vincent Guittot]

On 13 January 2014 17:10, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> [adding Vincent in CC, questions related to SCHED MC macros]
>
> Minor comments below.
>
> On Sun, Jan 12, 2014 at 07:20:38PM +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 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.
>
> "something something", one something is enough.
>
> [...]
>
>> new file mode 100644
>> index 000000000000..6f5270c65a6c
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/topology.h
>> @@ -0,0 +1,39 @@
>> +#ifndef __ASM_TOPOLOGY_H
>> +#define __ASM_TOPOLOGY_H
>> +
>> +#ifdef CONFIG_CPU_TOPOLOGY
>> +
>> +#include <linux/cpumask.h>
>> +
>> +struct cpu_topology {
>> +     int thread_id;
>> +     int core_id;
>> +     int socket_id;
>
> Is there any reason why we can't rename socket_id to cluster_id ? It won't
> change our lives but at least we kind of know what it means in ARM world.
>
>> +     cpumask_t thread_sibling;
>> +     cpumask_t core_sibling;
>> +};
>> +
>> +extern struct cpu_topology 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)
>
> Are the two macros above still required in the kernel ? I can't see any
> usage at present.
>
> Vincent, do you know why they were not removed in commit:
>
> 8e7fbcbc22c12414bcc9dfdd683637f58fb32759
>
> I am certainly missing something.

I think it was not planned to be used only by the scheduler but since
8e7fbcbc22c12414bcc9dfdd683637f58fb32759, we have reach a situation
where nobody use them for the moment.

>
>> +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..252b62181532 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_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);
>> +}
>
> __cpuinit has been (is being) removed from the kernel and probably should
> be removed from this definition too.
>
>> +
>>  /*
>>   * 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());
>> +
>> +
>
> Too many empty lines, one is enough.
>
>>       /*
>>        * 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..980019fefeff
>> --- /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>
>
> I have already commented on this. If this patchset is completely merged,
> that is one thing, if it is not we are adding include files for nothing.
> If you have time, and there should be given that the set missed the
> merge window it would be nice to split the includes, I will not nitpick
> any longer though, so it is up to you.
>
> [...]
>
>> +/*
>> + * 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 cpu_topology *cpu_topo = &(cpu_topology[cpu]);
>
> You do not need brackets, &cpu_topology[cpu] will do.
>
> Lorenzo
>

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

[Mark Brown]

On Mon, Jan 13, 2014 at 04:10:59PM +0000, Lorenzo Pieralisi wrote:
> On Sun, Jan 12, 2014 at 07:20:38PM +0000, Mark Brown wrote:

> > +struct cpu_topology {
> > +	int thread_id;
> > +	int core_id;
> > +	int socket_id;

> Is there any reason why we can't rename socket_id to cluster_id ? It won't
> change our lives but at least we kind of know what it means in ARM world.

I really don't care, whatever you guys want.

> > +#define mc_capable()	(cpu_topology[0].socket_id != -1)
> > +#define smt_capable()	(cpu_topology[0].thread_id != -1)

> Are the two macros above still required in the kernel ? I can't see any
> usage at present.
> 
> Vincent, do you know why they were not removed in commit:
> 
> 8e7fbcbc22c12414bcc9dfdd683637f58fb32759
> 
> I am certainly missing something.

They're defined by a bunch of other architectures (including x86).  If I
had to guess I'd say the architectures are still providing the
information so we don't need to go round adding it again if someone
comes up with a use for it in the core.
-------------- 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/c9185b05/attachment-0001.sig>

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

[Lorenzo Pieralisi]

[adding Vincent in CC, questions related to SCHED MC macros]

Minor comments below.

On Sun, Jan 12, 2014 at 07:20:38PM +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 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.

"something something", one something is enough.

[...]

> new file mode 100644
> index 000000000000..6f5270c65a6c
> --- /dev/null
> +++ b/arch/arm64/include/asm/topology.h
> @@ -0,0 +1,39 @@
> +#ifndef __ASM_TOPOLOGY_H
> +#define __ASM_TOPOLOGY_H
> +
> +#ifdef CONFIG_CPU_TOPOLOGY
> +
> +#include <linux/cpumask.h>
> +
> +struct cpu_topology {
> +	int thread_id;
> +	int core_id;
> +	int socket_id;

Is there any reason why we can't rename socket_id to cluster_id ? It won't
change our lives but at least we kind of know what it means in ARM world.

> +	cpumask_t thread_sibling;
> +	cpumask_t core_sibling;
> +};
> +
> +extern struct cpu_topology 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)

Are the two macros above still required in the kernel ? I can't see any
usage at present.

Vincent, do you know why they were not removed in commit:

8e7fbcbc22c12414bcc9dfdd683637f58fb32759

I am certainly missing something.

> +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..252b62181532 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_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);
> +}

__cpuinit has been (is being) removed from the kernel and probably should
be removed from this definition too.

> +
>  /*
>   * 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());
> +
> +

Too many empty lines, one is enough.

>  	/*
>  	 * 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..980019fefeff
> --- /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>

I have already commented on this. If this patchset is completely merged,
that is one thing, if it is not we are adding include files for nothing.
If you have time, and there should be given that the set missed the
merge window it would be nice to split the includes, I will not nitpick
any longer though, so it is up to you.

[...]

> +/*
> + * 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 cpu_topology *cpu_topo = &(cpu_topology[cpu]);

You do not need brackets, &cpu_topology[cpu] will do.

Lorenzo

[PATCH 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..02309c3eec33 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 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 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 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..6f5270c65a6c
--- /dev/null
+++ b/arch/arm64/include/asm/topology.h
@@ -0,0 +1,39 @@
+#ifndef __ASM_TOPOLOGY_H
+#define __ASM_TOPOLOGY_H
+
+#ifdef CONFIG_CPU_TOPOLOGY
+
+#include <linux/cpumask.h>
+
+struct cpu_topology {
+	int thread_id;
+	int core_id;
+	int socket_id;
+	cpumask_t thread_sibling;
+	cpumask_t core_sibling;
+};
+
+extern struct cpu_topology 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..252b62181532 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_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..980019fefeff
--- /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 cpu_topology 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 cpu_topology *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 cpu_topology *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 cpu_topology *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 1/4] arm64: topology: Implement basic CPU topology support

[Lorenzo Pieralisi]

On Wed, Jan 08, 2014 at 06:40:21PM +0000, Mark Brown wrote:
> On Wed, Jan 08, 2014 at 06:30:06PM +0000, Lorenzo Pieralisi wrote:
> > On Wed, Jan 08, 2014 at 05:10:10PM +0000, Mark Brown wrote:
> 
> > > +#include <linux/slab.h>
> 
> > Nitpick, <linux/slab.h> does not belong in this patch. There are other
> > includes that should be moved to the respective patch (percpu.h ?) where
> > they are actually used.
> 
> I can move this one but honestly I'm having an extremely difficult time
> summoning up the enthusiasm for looking for others and moving them - the
> practical impact will not be substantial.

Ok, it is your call and I understand I am being pedantic but from a logical
patch split perspective that's not correct and honestly it should take no more
than few minutes to move the includes where they actually belong.

Lorenzo

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

[Catalin Marinas]

On Wed, Jan 08, 2014 at 07:12:07PM +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 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.

Mark, for future patches, could you please add a cover letter with a
simple changelog and also add some versioning to the subject ([PATCH v*]).
It's getting hard to follow which series addresses what comments,
especially when you post more updates in the same day.

Thanks.

-- 
Catalin

[PATCH 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 249acb9da4e3..a53514cdf39c 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -162,6 +162,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 1cd339d5037b..b2fe6ba97080 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -19,6 +19,7 @@ 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_ARM64_CPU_SUSPEND)	+= sleep.o suspend.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 1/4] arm64: topology: Implement basic CPU topology support

[Mark Brown]

On Wed, Jan 08, 2014 at 06:30:06PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Jan 08, 2014 at 05:10:10PM +0000, Mark Brown wrote:

> > +#include <linux/slab.h>

> Nitpick, <linux/slab.h> does not belong in this patch. There are other
> includes that should be moved to the respective patch (percpu.h ?) where
> they are actually used.

I can move this one but honestly I'm having an extremely difficult time
summoning up the enthusiasm for looking for others and moving them - the
practical impact will not be substantial.
-------------- 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/20140108/6bf8d0e3/attachment.sig>

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

[Lorenzo Pieralisi]

On Wed, Jan 08, 2014 at 05:10:10PM +0000, Mark Brown wrote:

[...]

> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> new file mode 100644
> index 000000000000..853544f30a8b
> --- /dev/null
> +++ b/arch/arm64/kernel/topology.c
> @@ -0,0 +1,92 @@
> +/*
> + * 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 <linux/slab.h>

Nitpick, <linux/slab.h> does not belong in this patch. There are other
includes that should be moved to the respective patch (percpu.h ?) where
they are actually used.

Lorenzo

[PATCH 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      | 92 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 168 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 249acb9da4e3..a53514cdf39c 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -162,6 +162,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 1cd339d5037b..b2fe6ba97080 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -19,6 +19,7 @@ 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_ARM64_CPU_SUSPEND)	+= sleep.o suspend.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..853544f30a8b
--- /dev/null
+++ b/arch/arm64/kernel/topology.c
@@ -0,0 +1,92 @@
+/*
+ * 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 <linux/slab.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 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      | 92 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 168 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 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..853544f30a8b
--- /dev/null
+++ b/arch/arm64/kernel/topology.c
@@ -0,0 +1,92 @@
+/*
+ * 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 <linux/slab.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 1/4] arm64: topology: Implement basic CPU topology support

[Mark Brown]

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.
-------------- 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/20131218/8d42ba70/attachment.sig>

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

[Lorenzo Pieralisi]

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

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

[Mark Brown]

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.
-------------- 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/20131217/e28cb16c/attachment-0001.sig>

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

[Lorenzo Pieralisi]

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

[PATCH 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      | 95 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 171 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 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();
+}
-- 
1.8.5.1