Re: [PATCH 2/2] perf/arm-cmn: Switch to generic cpumask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Generic PMU CPU hotplug online migration is broken, causing a self-deadlock and failing to migrate events.
--

--- Patch [2]: [PATCH 2/2] perf/arm-cmn: Switch to generic cpumask ---
commit e67db05bf209b086de25b5674a2cd9a5ee2ab5f4
Author: Robin Murphy <robin.murphy@arm.com>

perf/arm-cmn: Switch to generic cpumask

With perf core now providing appropriately-sequenced hotplug callbacks
via the new PMU scope, all the state management boilerplate can go,
while the cpumask also takes care of enforcing event affinity.

> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 6e5cc4086a9e2..10243ac6217d4 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c

[ ... ]

> @@ -2008,48 +1987,38 @@ static int arm_cmn_commit_txn(struct pmu *pmu)
>  	return 0;
>  }
>  
> -static void arm_cmn_migrate(struct arm_cmn *cmn, unsigned int cpu)
> +static bool arm_cmn_migrate(struct arm_cmn *cmn, unsigned int cpu)
>  {
> -	unsigned int i;
> -
> -	perf_pmu_migrate_context(&cmn->pmu, cmn->cpu, cpu);
> -	for (i = 0; i < cmn->num_dtcs; i++)
> -		irq_set_affinity(cmn->dtc[i].irq, cpumask_of(cpu));
> -	cmn->cpu = cpu;
> +	cmn->pmu.cpumask = cpumask_of(cpu);
> +	for (int i = 0; i < cmn->num_dtcs; i++)
> +		irq_set_affinity(cmn->dtc[i].irq, cmn->pmu.cpumask);
> +	return true;
>  }
>  
> -static int arm_cmn_pmu_online_cpu(unsigned int cpu, struct hlist_node *cpuhp_node)
> +static bool arm_cmn_init_cpu(struct pmu *pmu, int cpu)
>  {
> -	struct arm_cmn *cmn;
> -	int node;
> +	struct arm_cmn *cmn = to_cmn(pmu);
> +	int node = dev_to_node(cmn->dev);
>  
> -	cmn = hlist_entry_safe(cpuhp_node, struct arm_cmn, cpuhp_node);
> -	node = dev_to_node(cmn->dev);
> -	if (cpu_to_node(cmn->cpu) != node && cpu_to_node(cpu) == node)
> -		arm_cmn_migrate(cmn, cpu);
> -	return 0;
> +	if (!cpumask_intersects(pmu->cpumask, cpumask_of_node(node)) &&
> +	    (cpu_to_node(cpu) == node || cpumask_empty(pmu->cpumask)))
> +		return arm_cmn_migrate(cmn, cpu);

[Severity: Critical]
Does this cause a self-deadlock during CPU hotplug online operations?

When arm_cmn_init_cpu() calls arm_cmn_migrate(), it updates cmn->pmu.cpumask
to the new CPU and returns true.

The core framework in kernel/events/core.c:perf_event_setup_cpumask() sees
the true return value and triggers a migration:

    cpumask_migrate = pmu->init_cpu(pmu, cpu);
    if (cpumask_migrate && !WARN_ON_ONCE(!pmu->cpumask))
        perf_pmu_migrate_context(pmu, cpu, cpumask_any(pmu->cpumask));

Because pmu->cpumask was just updated by the driver, cpumask_any() evaluates
to the new CPU, meaning the source and destination CPUs passed to
perf_pmu_migrate_context() are exactly the same.

When perf_pmu_migrate_context() resolves src_ctx and dst_ctx, they will point
to the same per-CPU context. It then attempts to double lock the same
non-recursive mutex:

    mutex_lock_double(&src_ctx->mutex, &dst_ctx->mutex);

Is it possible to migrate events from the previous CPU if the driver
already overwrote pmu->cpumask before returning to the core framework?

> +	return false;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784653319.git.robin.murphy@arm.com?part=2