Kajol Jain <kj...@linux.ibm.com> writes: > Patch here adds cpu hotplug functions to hv_24x7 pmu. > A new cpuhp_state "CPUHP_AP_PERF_POWERPC_HV_24x7_ONLINE" enum > is added. > > The online callback function updates the cpumask only if its > empty. As the primary intention of adding hotplug support > is to designate a CPU to make HCALL to collect the > counter data. > > The offline function test and clear corresponding cpu in a cpumask > and update cpumask to any other active cpu. > > Signed-off-by: Kajol Jain <kj...@linux.ibm.com> > Reviewed-by: Gautham R. Shenoy <e...@linux.vnet.ibm.com> > --- > arch/powerpc/perf/hv-24x7.c | 45 +++++++++++++++++++++++++++++++++++++ > include/linux/cpuhotplug.h | 1 + > 2 files changed, 46 insertions(+) > > diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c > index db213eb7cb02..ce4739e2b407 100644 > --- a/arch/powerpc/perf/hv-24x7.c > +++ b/arch/powerpc/perf/hv-24x7.c > @@ -31,6 +31,8 @@ static int interface_version; > /* Whether we have to aggregate result data for some domains. */ > static bool aggregate_result_elements; > > +static cpumask_t hv_24x7_cpumask; > + > static bool domain_is_valid(unsigned domain) > { > switch (domain) { > @@ -1641,6 +1643,44 @@ static struct pmu h_24x7_pmu = { > .capabilities = PERF_PMU_CAP_NO_EXCLUDE, > }; > > +static int ppc_hv_24x7_cpu_online(unsigned int cpu) > +{ > + /* Make this CPU the designated target for counter collection */
The comment implies every newly onlined CPU will become the target, but actually it's only the first onlined CPU. So I think the comment needs updating, or you could just drop the comment, I think the code is fairly clear by itself. > + if (cpumask_empty(&hv_24x7_cpumask)) > + cpumask_set_cpu(cpu, &hv_24x7_cpumask); > + > + return 0; > +} > + > +static int ppc_hv_24x7_cpu_offline(unsigned int cpu) > +{ > + int target = -1; No need to initialise target, you assign to it unconditionally below. > + /* Check if exiting cpu is used for collecting 24x7 events */ > + if (!cpumask_test_and_clear_cpu(cpu, &hv_24x7_cpumask)) > + return 0; > + > + /* Find a new cpu to collect 24x7 events */ > + target = cpumask_last(cpu_active_mask); Any reason to use cpumask_last() vs cpumask_first(), or a randomly chosen CPU? > + if (target < 0 || target >= nr_cpu_ids) > + return -1; > + > + /* Migrate 24x7 events to the new target */ > + cpumask_set_cpu(target, &hv_24x7_cpumask); > + perf_pmu_migrate_context(&h_24x7_pmu, cpu, target); > + > + return 0; > +} > + > +static int hv_24x7_cpu_hotplug_init(void) > +{ > + return cpuhp_setup_state(CPUHP_AP_PERF_POWERPC_HV_24x7_ONLINE, > + "perf/powerpc/hv_24x7:online", > + ppc_hv_24x7_cpu_online, > + ppc_hv_24x7_cpu_offline); > +} > + > static int hv_24x7_init(void) > { > int r; > @@ -1685,6 +1725,11 @@ static int hv_24x7_init(void) > if (r) > return r; > > + /* init cpuhotplug */ > + r = hv_24x7_cpu_hotplug_init(); > + if (r) > + pr_err("hv_24x7: CPU hotplug init failed\n"); > + The hotplug initialisation shouldn't fail unless something is badly wrong. I think you should just fail initialisation of the entire PMU if that happens, which will make the error handling in the next patch much simpler. cheers > r = perf_pmu_register(&h_24x7_pmu, h_24x7_pmu.name, -1); > if (r) > return r;