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

[Robin Murphy]

On 16/04/2019 17:29, Suzuki K Poulose wrote:
> 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@xxxxxxx>
> > ---
> > Â 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:

Right, there were already somewhat-redundant labels to begin with, but 
since they remain consistently named for the failure conditions which 
lead to them (as opposed to the cleanup action that they take) I figured 
I would leave them be for this change.

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

Thanks!

Robin.