On 06/11/2015 10:47 AM, Madhavan Srinivasan wrote:

Adds cpumask attribute to be used by each nest pmu since nest
units are per-chip. Only one cpu (first online cpu) from each node/chip
is designated to read counters.

On cpu hotplug, dying cpu is checked to see whether it is one of the
designated cpus, if yes, next online cpu from the same node/chip is designated
as new cpu to read counters.

Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Anton Blanchard <redacted>
Cc: Sukadev Bhattiprolu <redacted>
Cc: Anshuman Khandual <redacted>
Cc: Stephane Eranian <redacted>
Cc: Preeti U Murthy <redacted>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Madhavan Srinivasan <redacted>
---
+static void nest_change_cpu_context(int old_cpu, int new_cpu)
+{
+	int i;
+
+	if (new_cpu >= 0) {
+		for (i = 0; per_nest_pmu_arr[i] != NULL; i++)
+			perf_pmu_migrate_context(&per_nest_pmu_arr[i]->pmu,
+							old_cpu, new_cpu);
+	}
+}
+
+static void nest_exit_cpu(int cpu)
+{
+	int i, nid, target = -1;
+	const struct cpumask *l_cpumask;
+	int src_chipid;
+
+	/*
+	 * Check in the designated list for this cpu. Dont bother
+	 * if not one of them.
+	 */
+	if (!cpumask_test_and_clear_cpu(cpu, &cpu_mask_nest_pmu))
+		return;
+
+	/*
+	 * Now that this cpu is one of the designated,
+	 * find a new cpu a) which is not dying and

This comment is not right. nest_exit_cpu() is called in the hotplug
path, so another cpu cannot be dying in parallel. Hotplug operations are
done serially. The comment ought to be "a) which is online" instead.

+	 * b) is in same node/chip.

node is not the same as a chip right ? And you are interested in cpus on
the same chip alone. So shouldn't the above comment be b) in the same chip ?

+	 */
+	nid = cpu_to_node(cpu);
+	src_chipid = topology_physical_package_id(cpu);

What is the relation between a node and a chip ? Can't we have a
function which returns the cpumask of a chip straight away, since that
is what you seem to be more interested in ? You can then simply choose
the next cpu in this cpumask rather than going through the below loop.

+	l_cpumask = cpumask_of_node(nid);
+	for_each_cpu(i, l_cpumask) {
+		if (i == cpu)
+			continue;
+		if (src_chipid == topology_physical_package_id(i)) {
+			target = i;
+			break;
+		}
+	}
+
+	/*
+	 * Update the cpumask with the target cpu and
+	 * migrate the context if needed
+	 */
+	if (target >= 0) {

You already check for target >= 0 here. So you don't need to check for
new_cpu >= 0 in nest_change_cpu_context() above ?

+		cpumask_set_cpu(target, &cpu_mask_nest_pmu);
+		nest_change_cpu_context (cpu, target);
+	}
+}
+
+static void nest_init_cpu(int cpu)
+{
+	int i, src_chipid;
+
+	/*
+	 * Search for any existing designated thread from
+	 * the incoming cpu's node/chip. If found, do nothing.
+	 */
+	src_chipid = topology_physical_package_id(cpu);
+	for_each_cpu(i, &cpu_mask_nest_pmu)
+		if (src_chipid == topology_physical_package_id(i))
+			return;
+
+	/*
+	 * Make incoming cpu as a designated thread for
+	 * this node/chip
+	 */
+	cpumask_set_cpu(cpu, &cpu_mask_nest_pmu);

Why can't we simply check if cpu is the first one coming online in the
chip and designate it as the cpu_mask_nest_pmu for that chip ? If it is
not the first cpu, it means that another cpu in the same chip already
took over this duty and it needn't bother.

And shouldn't we also call nest_init() on this cpu, just like you do in
cpumask_chip() on all cpu_mask_nest_pmu cpus ?

+}
+
+static int nest_cpu_notifier(struct notifier_block *self,
+				unsigned long action, void *hcpu)
+{
+	long cpu = (long)hcpu;
+
+	switch (action & ~CPU_TASKS_FROZEN) {
+	case CPU_DOWN_FAILED:

Why do we need to handle the DOWN_FAILED case ? In DOWN_PREPARE, you
have ensured that the function moves on to another cpu. So even if the
offline failed, its no issue. The duty is safely taken over.

+	case CPU_STARTING:

I would suggest initializing nest in the CPU_ONLINE stage. This is
because CPU_STARTING phase can fail. In that case, we will be
unnecessarily initializing nest pre-maturely. CPU_ONLINE phase assures
that the cpu is successfully online and you can then initiate nest.

+		nest_init_cpu(cpu);
+		break;
+	case CPU_DOWN_PREPARE:
+		nest_exit_cpu(cpu);
+		break;
+	default:
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block nest_cpu_nb = {
+	.notifier_call  = nest_cpu_notifier,
+	.priority       = CPU_PRI_PERF + 1,
+};
+
+void cpumask_chip(void)

This name ^^ is not apt IMO. You are initiating the cpumask necessary
for nest pmu. So why not call it nest_pmu_cpumask_init() ?

+{
+	const struct cpumask *l_cpumask;
+	int cpu, nid;
+
+	if (!cpumask_empty(&cpu_mask_nest_pmu))

When can this condition become true ?

+		return;
+
+	cpu_notifier_register_begin();
+
+	/*
+	 * Nest PMUs are per-chip counters. So designate a cpu
+	 * from each node/chip for counter collection.
+	 */
+	for_each_online_node(nid) {
+		l_cpumask = cpumask_of_node(nid);
+
+		/* designate first online cpu in this node */
+		cpu = cpumask_first(l_cpumask);
+		cpumask_set_cpu(cpu, &cpu_mask_nest_pmu);
+	}
+
+	/* Initialize Nest PMUs in each node using designated cpus */
+	on_each_cpu_mask(&cpu_mask_nest_pmu, (smp_call_func_t)nest_init, NULL, 1);
+
+	__register_cpu_notifier(&nest_cpu_nb);
+
+	cpu_notifier_register_done();
+}

Regards
Preeti U Murthy