Re: [PATCH v3 1/2] powerpc/perf/hv-24x7: Add cpu hotplug support

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

On 7/7/20 10:26 AM, Michael Ellerman wrote:
> Madhavan Srinivasan <maddy@linux.ibm.com> writes:
>> On 7/6/20 8:43 AM, Michael Ellerman wrote:
>>> Kajol Jain <kjain@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 <kjain@linux.ibm.com>
>>>> Reviewed-by: Gautham R. Shenoy <ego@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.
>> We  did fail the PMU registration on failure of the hotplug
>> code (and yes error handling is much simpler), but on internal
>> review/discussion,
>> what came up was that, hv_24x7 PMU will still be usable without
>> the hotplug code (with "-C" option to perf tool command line).
> In theory yes.
>
> But in reality no one will ever test that case, so the code will easily
> bit rot.
>
> Even if it doesn't bit rot, you've now created another state the system
> can legally be in (hotplug init failed but PMU still probed), which you
> have to test, document & support.
>
> If the hotplug init fails then something is badly wrong, the best thing
> we can do is bail on the PMU initialisation and hope the rest of the
> system boots OK.

Yep agreed. Thanks for the comments mpe

Maddy

>
> cheers