Re: [PATCH v2 7/7]powerpc/powernv: nest pmu cpumask and cpu hotplug support

[Madhavan Srinivasan <maddy@linux.vnet.ibm.com>]

On Tuesday 16 June 2015 11:58 AM, Preeti U Murthy wrote:
> 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 <paulus@samba.org>
>> Cc: Anton Blanchard <anton@samba.org>
>> Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>> Cc: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>> Cc: Stephane Eranian <eranian@google.com>
>> Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> ---
>> +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.
Ok will change it.

>> +	 * 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 ?
I was hoping it to be, but i will change  comment to 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.
>
Make sense. I can separate it out.

>> +	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 ?
I guess i was way too cautious :) Will change it

>> +		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.
Looks to be right. let me try it out.

> 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 ?
Yes. I missed that. We should init. Nice catch.

>> +}
>> +
>> +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.
Ok sure. Will do that.

>> +		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() ?

Ok.
>> +{
>> +	const struct cpumask *l_cpumask;
>> +	int cpu, nid;
>> +
>> +	if (!cpumask_empty(&cpu_mask_nest_pmu))
> When can this condition become true ?
My bad. This code ended up here from the initial RFC patch, but after
reviewing this again, i dont think this is needed. Once again nice catch.

>> +		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