Re: [PATCH v3 2/2] perf/arm-ccn: Clean up CPU hotplug handling

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 04/16/2019 04:24 PM, Robin Murphy wrote:
> Like arm-cci, arm-ccn has the same issue of disabling preemption around
> operations which can take mutexes. Again, remove the definite bug by
> simply not trying to fight the theoretical races. And since we are
> touching the hotplug handling code, take the opportunity to streamline
> it, as there's really no need to store a full-sized cpumask to keep
> track of a single CPU ID.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   drivers/perf/arm-ccn.c | 25 +++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/perf/arm-ccn.c b/drivers/perf/arm-ccn.c
> index 2ae76026e947..0bb52d9bdcf7 100644
> --- a/drivers/perf/arm-ccn.c
> +++ b/drivers/perf/arm-ccn.c
> @@ -167,7 +167,7 @@ struct arm_ccn_dt {
>   
>   	struct hrtimer hrtimer;
>   
> -	cpumask_t cpu;
> +	unsigned int cpu;
>   	struct hlist_node node;
>   
>   	struct pmu pmu;
> @@ -559,7 +559,7 @@ static ssize_t arm_ccn_pmu_cpumask_show(struct device *dev,
>   {
>   	struct arm_ccn *ccn = pmu_to_arm_ccn(dev_get_drvdata(dev));
>   
> -	return cpumap_print_to_pagebuf(true, buf, &ccn->dt.cpu);
> +	return cpumap_print_to_pagebuf(true, buf, cpumask_of(ccn->dt.cpu));
>   }
>   
>   static struct device_attribute arm_ccn_pmu_cpumask_attr =
> @@ -759,7 +759,7 @@ static int arm_ccn_pmu_event_init(struct perf_event *event)
>   	 * mitigate this, we enforce CPU assignment to one, selected
>   	 * processor (the one described in the "cpumask" attribute).
>   	 */
> -	event->cpu = cpumask_first(&ccn->dt.cpu);
> +	event->cpu = ccn->dt.cpu;
>   
>   	node_xp = CCN_CONFIG_NODE(event->attr.config);
>   	type = CCN_CONFIG_TYPE(event->attr.config);
> @@ -1215,15 +1215,15 @@ static int arm_ccn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
>   	struct arm_ccn *ccn = container_of(dt, struct arm_ccn, dt);
>   	unsigned int target;
>   
> -	if (!cpumask_test_and_clear_cpu(cpu, &dt->cpu))
> +	if (cpu != dt->cpu)
>   		return 0;
>   	target = cpumask_any_but(cpu_online_mask, cpu);
>   	if (target >= nr_cpu_ids)
>   		return 0;
>   	perf_pmu_migrate_context(&dt->pmu, cpu, target);
> -	cpumask_set_cpu(target, &dt->cpu);
> +	dt->cpu = target;
>   	if (ccn->irq)
> -		WARN_ON(irq_set_affinity_hint(ccn->irq, &dt->cpu) != 0);
> +		WARN_ON(irq_set_affinity_hint(ccn->irq, cpumask_of(dt->cpu)));
>   	return 0;
>   }
>   
> @@ -1299,29 +1299,30 @@ static int arm_ccn_pmu_init(struct arm_ccn *ccn)
>   	}
>   
>   	/* Pick one CPU which we will use to collect data from CCN... */
> -	cpumask_set_cpu(get_cpu(), &ccn->dt.cpu);
> +	ccn->dt.cpu = raw_smp_processor_id();
>   
>   	/* Also make sure that the overflow interrupt is handled by this CPU */
>   	if (ccn->irq) {
> -		err = irq_set_affinity_hint(ccn->irq, &ccn->dt.cpu);
> +		err = irq_set_affinity_hint(ccn->irq, cpumask_of(ccn->dt.cpu));
>   		if (err) {
>   			dev_err(ccn->dev, "Failed to set interrupt affinity!\n");
>   			goto error_set_affinity;
>   		}
>   	}
>   
> +	cpuhp_state_add_instance_nocalls(CPUHP_AP_PERF_ARM_CCN_ONLINE,
> +					 &ccn->dt.node);
> +
>   	err = perf_pmu_register(&ccn->dt.pmu, name, -1);
>   	if (err)
>   		goto error_pmu_register;
>   
> -	cpuhp_state_add_instance_nocalls(CPUHP_AP_PERF_ARM_CCN_ONLINE,
> -					 &ccn->dt.node);
> -	put_cpu();
>   	return 0;
>   
>   error_pmu_register:
> +	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_CCN_ONLINE,
> +					    &ccn->dt.node);
>   error_set_affinity:
> -	put_cpu();

Super minor nit: We don't need the error_set_affinity label anymore, as
we don't do anything here. Otherwise:

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>