by Madhavan Srinivasan

[permalink] [raw]

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

On Monday 22 June 2015 02:45 PM, Madhavan Srinivasan wrote:
>
> 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 <[email protected]>
>>> Cc: Benjamin Herrenschmidt <[email protected]>
>>> Cc: Paul Mackerras <[email protected]>
>>> Cc: Anton Blanchard <[email protected]>
>>> Cc: Sukadev Bhattiprolu <[email protected]>
>>> Cc: Anshuman Khandual <[email protected]>
>>> Cc: Stephane Eranian <[email protected]>
>>> Cc: Preeti U Murthy <[email protected]>
>>> Cc: Ingo Molnar <[email protected]>
>>> Cc: Peter Zijlstra <[email protected]>
>>> Signed-off-by: Madhavan Srinivasan <[email protected]>
>>> ---
>>> +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.
I guess, we dont need to init again, since we dont stop the pore engine,
we are ok.

>>> +}
>>> +
>>> +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
> _______________________________________________
> Linuxppc-dev mailing list
> [email protected]
> https://lists.ozlabs.org/listinfo/linuxppc-dev