| Message ID | 1433260778-26497-4-git-send-email-maddy@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, 2015-06-02 at 21:29 +0530, Madhavan Srinivasan wrote: > Patch adds cpu hotplug support. First online cpu in a node is picked as > designated thread to read the Nest pmu counter data, and at the time of > hotplug, next online cpu from the same node is picked up. I'm not sure I understand this commit message. I think I understand the first half - I think you're trying to say: "At boot, the first online CPU in a node is picked as the designated thread to read the Nest PMU counter data." I'm not sure I understand the second half: "picked up" how and for what? (I did eventually figure it out by reading the patch, but it'd be really nice to have it spelled out nicely in the commit message.) > +static void nest_exit_cpu(int cpu) > +{ > + int i, nid, target = -1; > + const struct cpumask *l_cpumask; > + int src_chipid; > + > + if (!cpumask_test_and_clear_cpu(cpu, &cpu_mask_nest_pmu)) > + return; > + > + nid = cpu_to_node(cpu); > + src_chipid = topology_physical_package_id(cpu); > + 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; > + } > + } Some comments here would really help. I think you're looking for the first CPU that's (a) not the cpu you're removing and (b) on the same physical package, so sharing the same nest, but it took me a lot of staring at the code to figure it out. > + > + cpumask_set_cpu(target, &cpu_mask_nest_pmu); > + nest_change_cpu_context (cpu, target); > + return; Return is redundant here and in several other functions in this patch. > +} > + > +static void nest_init_cpu(int cpu) > +{ > + int i, src_chipid; > + > + src_chipid = topology_physical_package_id(cpu); > + for_each_cpu(i, &cpu_mask_nest_pmu) > + if (src_chipid == topology_physical_package_id(i)) > + return; > + > + cpumask_set_cpu(cpu, &cpu_mask_nest_pmu); > + nest_change_cpu_context ( -1, cpu); Weird extra spaces here. > + return; > +} This function could also do with a comment: AFAICT, you've structured the function so that it only calls nest_change_cpu_context if you've picked up a cpu on a physical package that previously didn't have a nest pmu thread on it. > + > +static int nest_cpu_notifier(struct notifier_block *self, > + unsigned long action, void *hcpu) > +{ > + unsigned int cpu = (long)hcpu; What's with this cast? You cast it to a long and then assign it to an unsigned int? > + > + switch (action & ~CPU_TASKS_FROZEN) { > + case CPU_DOWN_FAILED: Is it necessary to move the thread back if the CPU fails to go down? You've moved it to another online CPU already; what's the benefit of paying the time-penalty to move it back? > + case CPU_STARTING: > + nest_init_cpu(cpu); > + break; > + case CPU_DOWN_PREPARE: > + nest_exit_cpu(cpu); > + break; > + default: > + break; > + } > + > + return NOTIFY_OK; > +} > Now, I don't know the details of CPU hotplug _at all_, so this may be stupid, but what happens if you hotplug a lot of CPUs all at once? Is everything properly serialised or is this going to race and end up with either multiple cpus trying to do PMU or no cpus? Regards, Daniel Axtens
On Wednesday 03 June 2015 05:08 AM, Daniel Axtens wrote: > On Tue, 2015-06-02 at 21:29 +0530, Madhavan Srinivasan wrote: >> Patch adds cpu hotplug support. First online cpu in a node is picked as >> designated thread to read the Nest pmu counter data, and at the time of >> hotplug, next online cpu from the same node is picked up. > I'm not sure I understand this commit message. I think I understand the > first half - I think you're trying to say: "At boot, the first online I will rephrase it. > CPU in a node is picked as the designated thread to read the Nest PMU > counter data." I'm not sure I understand the second half: "picked up" > how and for what? When the designated thread is hotplugged, next online cpu in the same node is picked up as the designated thread to read the PMU counter data. > (I did eventually figure it out by reading the patch, but it'd be really > nice to have it spelled out nicely in the commit message.) Sure. Will fix the commit message. >> +static void nest_exit_cpu(int cpu) >> +{ >> + int i, nid, target = -1; >> + const struct cpumask *l_cpumask; >> + int src_chipid; >> + >> + if (!cpumask_test_and_clear_cpu(cpu, &cpu_mask_nest_pmu)) >> + return; >> + >> + nid = cpu_to_node(cpu); >> + src_chipid = topology_physical_package_id(cpu); >> + 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; >> + } >> + } > Some comments here would really help. I think you're looking for the > first CPU that's (a) not the cpu you're removing and (b) on the same > physical package, so sharing the same nest, but it took me a lot of > staring at the code to figure it out. My bad. I will comment it. >> + >> + cpumask_set_cpu(target, &cpu_mask_nest_pmu); >> + nest_change_cpu_context (cpu, target); >> + return; > Return is redundant here and in several other functions in this patch. Ok. >> +} >> + >> +static void nest_init_cpu(int cpu) >> +{ >> + int i, src_chipid; >> + >> + src_chipid = topology_physical_package_id(cpu); >> + for_each_cpu(i, &cpu_mask_nest_pmu) >> + if (src_chipid == topology_physical_package_id(i)) >> + return; >> + >> + cpumask_set_cpu(cpu, &cpu_mask_nest_pmu); >> + nest_change_cpu_context ( -1, cpu); > Weird extra spaces here. Yes. Nice catch. Will fix it. >> + return; >> +} > This function could also do with a comment: AFAICT, you've structured > the function so that it only calls nest_change_cpu_context if you've > picked up a cpu on a physical package that previously didn't have a nest > pmu thread on it. > >> + >> +static int nest_cpu_notifier(struct notifier_block *self, >> + unsigned long action, void *hcpu) >> +{ >> + unsigned int cpu = (long)hcpu; > What's with this cast? You cast it to a long and then assign it to an > unsigned int? Facepalm. My bad, will fix it. >> + >> + switch (action & ~CPU_TASKS_FROZEN) { >> + case CPU_DOWN_FAILED: > Is it necessary to move the thread back if the CPU fails to go down? No. not need. > You've moved it to another online CPU already; what's the benefit of > paying the time-penalty to move it back? Why should go through that. Because, there is no restriction saying only the first cpu has to read it, why should we complicate it further instead of moving to another cpu in the same node. >> + case CPU_STARTING: >> + nest_init_cpu(cpu); >> + break; >> + case CPU_DOWN_PREPARE: >> + nest_exit_cpu(cpu); >> + break; >> + default: >> + break; >> + } >> + >> + return NOTIFY_OK; >> +} >> > Now, I don't know the details of CPU hotplug _at all_, so this may be > stupid, but what happens if you hotplug a lot of CPUs all at once? Is > everything properly serialised or is this going to race and end up with > either multiple cpus trying to do PMU or no cpus? I did test the code with hotplug test. If all the cpus in the node is offlined, then we will have no cpus designated for that node. Thanks for review Maddy > Regards, > Daniel Axtens > >
diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c index d4413bb..3e7010e 100644 --- a/arch/powerpc/perf/nest-pmu.c +++ b/arch/powerpc/perf/nest-pmu.c @@ -30,6 +30,86 @@ static struct attribute_group cpumask_nest_pmu_attr_group = { .attrs = cpumask_nest_pmu_attrs, }; +static void nest_init(void *dummy) +{ + opal_nest_ima_control(P8_NEST_ENGINE_START); +} + +static void nest_change_cpu_context(int old_cpu, int new_cpu) +{ + int i; + + for (i=0; per_nestpmu_arr[i] != NULL; i++) + perf_pmu_migrate_context(&per_nestpmu_arr[i]->pmu, + old_cpu, new_cpu); + return; +} + +static void nest_exit_cpu(int cpu) +{ + int i, nid, target = -1; + const struct cpumask *l_cpumask; + int src_chipid; + + if (!cpumask_test_and_clear_cpu(cpu, &cpu_mask_nest_pmu)) + return; + + nid = cpu_to_node(cpu); + src_chipid = topology_physical_package_id(cpu); + 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; + } + } + + cpumask_set_cpu(target, &cpu_mask_nest_pmu); + nest_change_cpu_context (cpu, target); + return; +} + +static void nest_init_cpu(int cpu) +{ + int i, src_chipid; + + src_chipid = topology_physical_package_id(cpu); + for_each_cpu(i, &cpu_mask_nest_pmu) + if (src_chipid == topology_physical_package_id(i)) + return; + + cpumask_set_cpu(cpu, &cpu_mask_nest_pmu); + nest_change_cpu_context ( -1, cpu); + return; +} + +static int nest_cpu_notifier(struct notifier_block *self, + unsigned long action, void *hcpu) +{ + unsigned int cpu = (long)hcpu; + + switch (action & ~CPU_TASKS_FROZEN) { + case CPU_DOWN_FAILED: + case CPU_STARTING: + 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) { const struct cpumask *l_cpumask; @@ -47,6 +127,10 @@ void cpumask_chip(void) cpumask_set_cpu(cpu, &cpu_mask_nest_pmu); } + 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(); }
Patch adds cpu hotplug support. First online cpu in a node is picked as designated thread to read the Nest pmu counter data, and at the time of hotplug, next online cpu from the same node is picked up. Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@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> --- arch/powerpc/perf/nest-pmu.c | 84 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+)