On Mon, 15 May 2017, Madhavan Srinivasan wrote:

On Wednesday 10 May 2017 05:39 PM, Thomas Gleixner wrote:

quoted

On Thu, 4 May 2017, Anju T Sudhakar wrote:

quoted

+	/*
+	 * Update the cpumask with the target cpu and
+	 * migrate the context if needed
+	 */
+	if (target >= 0 && target <= nr_cpu_ids) {
+		cpumask_set_cpu(target, &nest_imc_cpumask);
+		nest_change_cpu_context(cpu, target);
+	}

What disables the perf context if this was the last CPU on the node?

My bad. i did not understand this. Is this regarding the updates
of the "flags" in the perf_event and hw_perf_event structs?

Sorry, there is nothing to understand. It was me misreading it.

quoted

quoted

+static int nest_pmu_cpumask_init(void)
+{
+	const struct cpumask *l_cpumask;
+	int cpu, nid;
+	int *cpus_opal_rc;
+
+	if (!cpumask_empty(&nest_imc_cpumask))
+		return 0;

What's that for? Paranoia engineering?

No.  The idea here is to generate the cpu_mask attribute
field only for the first "nest" pmu and use the same
for other "nest" units.

Why is nest_pmu_cpumask_init() called more than once? If it is then you
should have a proper flag marking it initialiazed rather than making a
completely obscure check for a cpu mask. That check could be empty for the
second invocation when the first invocation fails.

quoted

But this whole function is completely overengineered. If you make that
nest_init() part of the online function then this works also for nodes
which come online later and it simplifies to:

static int ppc_nest_imc_cpu_online(unsigned int cpu)
{
	const struct cpumask *l_cpumask;
	static struct cpumask tmp_mask;
	int res;

	/* Get the cpumask of this node */
	l_cpumask = cpumask_of_node(cpu_to_node(cpu));

	/*
	 * If this is not the first online CPU on this node, then IMC is
	 * initialized already.
	 */
	if (cpumask_and(&tmp_mask, l_cpumask, &nest_imc_cpumask))
		return 0;

	/*
	 * If this fails, IMC is not usable.
	 *
	 * FIXME: Add a understandable comment what this actually does
	 * and why it can fail.
	 */
	res = opal_imc_counters_stop(OPAL_IMC_COUNTERS_NEST);
	if (res)
		return res;

	/* Make this CPU the designated target for counter collection */
	cpumask_set_cpu(cpu, &nest_imc_cpumask);
	nest_change_cpu_context(-1, cpu);
	return 0;
}

static int nest_pmu_cpumask_init(void)
{
	return cpuhp_setup_state(CPUHP_AP_PERF_POWERPC_NEST_ONLINE,
				 "perf/powerpc/imc:online",
				 ppc_nest_imc_cpu_online,
				 ppc_nest_imc_cpu_offline);
}

Hmm?

Yes this make sense. But we need to first designate a cpu in each
chip at init setup and use opal api to disable the engine in the same.
So probably, after cpuhp_setup_state, can we do that?

Errm. That's what ppc_nest_imc_cpu_online() does.

It checks whether this is the first online cpu on a node and if yes, it
calls opal_imc_counters_stop() and sets that cpu in nest_imc_cpumask.

cpuhp_setup_state() invokes the callback on each online CPU. Seo evrything
is set up proper after that.

Thanks,

	tglx