[PATCH 0/2] perf: Generic hotplug/cpumask for system PMUs

[PATCH 0/2] perf: Generic hotplug/cpumask for system PMUs

[Robin Murphy]

Hi all,

Here's my take on finally trying to clean up the longstanding PMUs vs.
CPU hotplug can of worms. Patch #1 is the thing to look at; patch #2
is included as an example of how it simplifies my "favourite" driver
(in fact arm-cmn actually gets one of the least-negative diffstats on
offer, but it's the one I can test on a box under my desk). I'll save
the remaining bulk-conversion noise for a follow-up if and when.

For simplicity I've framed this around the system PMU use-case, since
consolidating their whole cpumask/migration business is the big win
here, and isn't all that easy to do in separate stages. However, the
design does also allow for any driver to now benefit from implementing
the init_cpu/exit_cpu ops in place of managing their own cpuhp state;
only the automatic filtering and context migration is tied to the new
cpumask-specific scope.

Thanks,
Robin.


Robin Murphy (2):
  perf: Generic hotplug/cpumask for system PMUs
  perf/arm-cmn: Switch to generic cpumask

 drivers/perf/arm-cmn.c     | 96 +++++++++-----------------------------
 include/linux/perf_event.h | 19 ++++++++
 kernel/events/core.c       | 76 ++++++++++++++++++++++++++----
 3 files changed, 108 insertions(+), 83 deletions(-)

-- 
2.54.0.dirty

[PATCH 1/2] perf: Generic hotplug/cpumask for system PMUs

[Robin Murphy]

Using the distinction of "uncore" PMUs being x86-style sliced across
standard ACPI topological scopes, and "system" PMUs being arbitrary
independent instances, while the former's PERF_PMU_SCOPE_SYS_WIDE has
some overlap with the latter, it still involves some assumptions which
don't quite carry over. System PMUs may be associated with irregular
subsets of CPUs, and they usually have overflow interrupts, which
require maintaining specific CPU affinity to avoid races against
reading or scheduling out events.

Fortunately, with the generic uncore machinery in place, it doesn't
take much to extend with a new PMU-defined scope to do the job for
system PMUs - all we really need is a way for the core code to retrieve
a per-PMU cpumask, which can simply be a pointer rather than needing a
function method, plus a couple of methods to cover the driver-specific
work currently done by all the many, many open-coded hotplug states.

Not only does this save whole swathes of boilerplate in drivers, it also
finally eliminates the fiddly race condition where depending on the
order in which cpuhp_setup_state/add_instance() is called relative to
perf_pmu_register(), one would theoretically risk either missing a
hotplug event, or attempting to migrate an uninitialised PMU.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 include/linux/perf_event.h | 19 ++++++++++
 kernel/events/core.c       | 76 +++++++++++++++++++++++++++++++++-----
 2 files changed, 85 insertions(+), 10 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 48d851fbd8ea..ff29c45eb082 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -317,6 +317,8 @@ enum perf_pmu_scope {
 	PERF_PMU_SCOPE_CLUSTER,
 	PERF_PMU_SCOPE_PKG,
 	PERF_PMU_SCOPE_SYS_WIDE,
+	/* Dynamically determined by PMU driver via pmu->cpumask */
+	PERF_PMU_SCOPE_CPUMASK,
 	PERF_PMU_MAX_SCOPE,
 };
 
@@ -350,6 +352,7 @@ struct pmu {
 	 * PMU scope
 	 */
 	unsigned int			scope;
+	const struct cpumask		*cpumask;
 
 	struct perf_cpu_pmu_context * __percpu *cpu_pmu_context;
 	atomic_t			exclusive_cnt; /* < 0: cpu; > 0: tsk */
@@ -589,6 +592,22 @@ struct pmu {
 	 * Check period value for PERF_EVENT_IOC_PERIOD ioctl.
 	 */
 	int (*check_period)		(struct perf_event *event, u64 value); /* optional */
+
+	/*
+	 * For PMUs to manage any private CPU-affine state, e.g. IRQs.
+	 *
+	 * Called from CPU hotplug. PMUs with PERF_PMU_SCOPE_CPUMASK may
+	 * return true if pmu->cpumask was updated and PMU context should be
+	 * be migrated to a new CPU; otherwise should return false.
+	 *
+	 * ->exit_cpu() runs from CPUHP_AP_PERF_OFFLINE context, unless @cpu
+	 * is already absent from pmu->cpumask
+	 * ->init_cpu() runs from CPUHP_AP_PERF_ONLINE context, unless @cpu is
+	 * already present in pmu->cpumask, plus once for each online CPU upon
+	 * initial registration
+	 */
+	bool (*exit_cpu)		(struct pmu *pmu, int cpu); /* optional */
+	bool (*init_cpu)		(struct pmu *pmu, int cpu); /* optional */
 };
 
 enum perf_addr_filter_action_t {
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 954c36e28101..506bdd3c572c 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -12651,6 +12651,13 @@ perf_event_mux_interval_ms_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(perf_event_mux_interval_ms);
 
+static void perf_scope_init_cpu(void *info)
+{
+	struct pmu *pmu = info;
+
+	pmu->init_cpu(pmu, smp_processor_id());
+}
+
 static inline const struct cpumask *perf_scope_cpu_topology_cpumask(unsigned int scope, int cpu)
 {
 	switch (scope) {
@@ -12664,9 +12671,11 @@ static inline const struct cpumask *perf_scope_cpu_topology_cpumask(unsigned int
 		return topology_core_cpumask(cpu);
 	case PERF_PMU_SCOPE_SYS_WIDE:
 		return cpu_online_mask;
+	case PERF_PMU_SCOPE_CPUMASK:
+		return cpumask_of(cpu);
+	default:
+		return NULL;
 	}
-
-	return NULL;
 }
 
 static inline struct cpumask *perf_scope_cpumask(unsigned int scope)
@@ -12682,16 +12691,26 @@ static inline struct cpumask *perf_scope_cpumask(unsigned int scope)
 		return perf_online_pkg_mask;
 	case PERF_PMU_SCOPE_SYS_WIDE:
 		return perf_online_sys_mask;
+	default:
+		return NULL;
 	}
+}
 
-	return NULL;
+static inline const struct cpumask *perf_pmu_cpumask(const struct pmu *pmu)
+{
+	switch (pmu->scope) {
+	case PERF_PMU_SCOPE_CPUMASK:
+		return pmu->cpumask;
+	default:
+		return perf_scope_cpumask(pmu->scope);
+	}
 }
 
 static ssize_t cpumask_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
 	struct pmu *pmu = dev_get_drvdata(dev);
-	struct cpumask *mask = perf_scope_cpumask(pmu->scope);
+	const struct cpumask *mask = perf_pmu_cpumask(pmu);
 
 	if (mask)
 		return cpumap_print_to_pagebuf(true, buf, mask);
@@ -12849,6 +12868,12 @@ int perf_pmu_register(struct pmu *_pmu, const char *name, int type)
 		      "Can not register a pmu with an invalid scope.\n"))
 		return -EINVAL;
 
+	if (pmu->scope == PERF_PMU_SCOPE_CPUMASK && !pmu->cpumask) {
+		if (WARN_ONCE(!pmu->init_cpu, "PMU must provide cpumask or init callback\n"))
+			return -EINVAL;
+		pmu->cpumask = cpu_none_mask;
+	}
+
 	pmu->name = name;
 
 	if (type >= 0)
@@ -12918,6 +12943,16 @@ int perf_pmu_register(struct pmu *_pmu, const char *name, int type)
 	INIT_LIST_HEAD(&pmu->events);
 	spin_lock_init(&pmu->events_lock);
 
+	/*
+	 * Finally, if appropriate give the PMU a chance to initialise any
+	 * of its own CPU-affine state. Note that we're serialised against
+	 * perf_event_{init,exit}_cpu() themselves by virtue of pmus_lock.
+	 */
+	if (pmu->init_cpu) {
+		for_each_online_cpu(cpu)
+			smp_call_function_single(cpu, perf_scope_init_cpu, pmu, 1);
+	}
+
 	/*
 	 * Now that the PMU is complete, make it visible to perf_try_init_event().
 	 */
@@ -13124,11 +13159,11 @@ static int perf_try_init_event(struct pmu *pmu, struct perf_event *event)
 
 	if (pmu->scope != PERF_PMU_SCOPE_NONE && event->cpu >= 0) {
 		const struct cpumask *cpumask;
-		struct cpumask *pmu_cpumask;
+		const struct cpumask *pmu_cpumask;
 		int cpu;
 
 		cpumask = perf_scope_cpu_topology_cpumask(pmu->scope, event->cpu);
-		pmu_cpumask = perf_scope_cpumask(pmu->scope);
+		pmu_cpumask = perf_pmu_cpumask(pmu);
 
 		ret = -ENODEV;
 		if (!pmu_cpumask || !cpumask)
@@ -13138,7 +13173,8 @@ static int perf_try_init_event(struct pmu *pmu, struct perf_event *event)
 		if (cpu >= nr_cpu_ids)
 			goto err_destroy;
 
-		event->event_caps |= PERF_EV_CAP_READ_SCOPE;
+		if (pmu->scope <= PERF_PMU_SCOPE_SYS_WIDE)
+			event->event_caps |= PERF_EV_CAP_READ_SCOPE;
 	}
 
 	return 0;
@@ -15169,7 +15205,7 @@ static void perf_event_clear_cpumask(unsigned int cpu)
 
 	cpumask_clear_cpu(cpu, perf_online_mask);
 
-	for (scope = PERF_PMU_SCOPE_NONE + 1; scope < PERF_PMU_MAX_SCOPE; scope++) {
+	for (scope = PERF_PMU_SCOPE_NONE + 1; scope <= PERF_PMU_SCOPE_SYS_WIDE; scope++) {
 		const struct cpumask *cpumask = perf_scope_cpu_topology_cpumask(scope, cpu);
 		struct cpumask *pmu_cpumask = perf_scope_cpumask(scope);
 
@@ -15186,10 +15222,19 @@ static void perf_event_clear_cpumask(unsigned int cpu)
 
 	/* migrate */
 	list_for_each_entry(pmu, &pmus, entry) {
+		bool cpumask_migrate = false;
+
+		if (pmu->init_cpu && (!pmu->cpumask || cpumask_test_cpu(cpu, pmu->cpumask)))
+			cpumask_migrate = pmu->exit_cpu(pmu, cpu);
+
 		if (pmu->scope == PERF_PMU_SCOPE_NONE ||
 		    WARN_ON_ONCE(pmu->scope >= PERF_PMU_MAX_SCOPE))
 			continue;
 
+		target[PERF_PMU_SCOPE_CPUMASK] = -1;
+		if (cpumask_migrate && !WARN_ON_ONCE(!pmu->cpumask))
+			target[PERF_PMU_SCOPE_CPUMASK] = cpumask_any_but(pmu->cpumask, cpu);
+
 		if (target[pmu->scope] >= 0 && target[pmu->scope] < nr_cpu_ids)
 			perf_pmu_migrate_context(pmu, cpu, target[pmu->scope]);
 	}
@@ -15227,6 +15272,7 @@ static void perf_event_setup_cpumask(unsigned int cpu)
 {
 	struct cpumask *pmu_cpumask;
 	unsigned int scope;
+	struct pmu *pmu;
 
 	/*
 	 * Early boot stage, the cpumask hasn't been set yet.
@@ -15234,7 +15280,7 @@ static void perf_event_setup_cpumask(unsigned int cpu)
 	 * Always unconditionally set the boot CPU for the perf_online_<domain>_masks.
 	 */
 	if (cpumask_empty(perf_online_mask)) {
-		for (scope = PERF_PMU_SCOPE_NONE + 1; scope < PERF_PMU_MAX_SCOPE; scope++) {
+		for (scope = PERF_PMU_SCOPE_NONE + 1; scope <= PERF_PMU_SCOPE_SYS_WIDE; scope++) {
 			pmu_cpumask = perf_scope_cpumask(scope);
 			if (WARN_ON_ONCE(!pmu_cpumask))
 				continue;
@@ -15243,7 +15289,7 @@ static void perf_event_setup_cpumask(unsigned int cpu)
 		goto end;
 	}
 
-	for (scope = PERF_PMU_SCOPE_NONE + 1; scope < PERF_PMU_MAX_SCOPE; scope++) {
+	for (scope = PERF_PMU_SCOPE_NONE + 1; scope <= PERF_PMU_SCOPE_CPUMASK; scope++) {
 		const struct cpumask *cpumask = perf_scope_cpu_topology_cpumask(scope, cpu);
 
 		pmu_cpumask = perf_scope_cpumask(scope);
@@ -15255,6 +15301,16 @@ static void perf_event_setup_cpumask(unsigned int cpu)
 		    cpumask_any_and(pmu_cpumask, cpumask) >= nr_cpu_ids)
 			cpumask_set_cpu(cpu, pmu_cpumask);
 	}
+
+	/* Allow migrating if the new CPU is preferable (e.g. NUMA locality) */
+	list_for_each_entry(pmu, &pmus, entry) {
+		bool cpumask_migrate = false;
+
+		if (pmu->init_cpu && (!pmu->cpumask || !cpumask_test_cpu(cpu, pmu->cpumask)))
+			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));
+	}
 end:
 	cpumask_set_cpu(cpu, perf_online_mask);
 }
-- 
2.54.0.dirty

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

[Robin Murphy]

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.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/perf/arm-cmn.c | 96 ++++++++++--------------------------------
 1 file changed, 23 insertions(+), 73 deletions(-)

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index 6e5cc4086a9e..10243ac6217d 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -358,17 +358,12 @@ struct arm_cmn {
 	struct arm_cmn_dtc *dtc;
 	unsigned int num_dtcs;
 
-	int cpu;
-	struct hlist_node cpuhp_node;
-
 	struct pmu pmu;
 	struct dentry *debug;
 };
 
 #define to_cmn(p)	container_of(p, struct arm_cmn, pmu)
 
-static int arm_cmn_hp_state;
-
 struct arm_cmn_nodeid {
 	u8 port;
 	u8 dev;
@@ -1322,17 +1317,6 @@ static const struct attribute_group arm_cmn_format_attrs_group = {
 	.attrs = arm_cmn_format_attrs,
 };
 
-static ssize_t arm_cmn_cpumask_show(struct device *dev,
-				    struct device_attribute *attr, char *buf)
-{
-	struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev));
-
-	return cpumap_print_to_pagebuf(true, buf, cpumask_of(cmn->cpu));
-}
-
-static struct device_attribute arm_cmn_cpumask_attr =
-		__ATTR(cpumask, 0444, arm_cmn_cpumask_show, NULL);
-
 static ssize_t arm_cmn_identifier_show(struct device *dev,
 				       struct device_attribute *attr, char *buf)
 {
@@ -1345,7 +1329,6 @@ static struct device_attribute arm_cmn_identifier_attr =
 		__ATTR(identifier, 0444, arm_cmn_identifier_show, NULL);
 
 static struct attribute *arm_cmn_other_attrs[] = {
-	&arm_cmn_cpumask_attr.attr,
 	&arm_cmn_identifier_attr.attr,
 	NULL,
 };
@@ -1779,10 +1762,6 @@ static int arm_cmn_event_init(struct perf_event *event)
 	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
 		return -EINVAL;
 
-	event->cpu = cmn->cpu;
-	if (event->cpu < 0)
-		return -EINVAL;
-
 	type = CMN_EVENT_TYPE(event);
 	/* DTC events (i.e. cycles) already have everything they need */
 	if (type == CMN_TYPE_DTC)
@@ -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);
+	return false;
 }
 
-static int arm_cmn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_node)
+static bool arm_cmn_exit_cpu(struct pmu *pmu, int cpu)
 {
-	struct arm_cmn *cmn;
+	struct arm_cmn *cmn = to_cmn(pmu);
+	int node = dev_to_node(cmn->dev);
 	unsigned int target;
-	int node;
-
-	cmn = hlist_entry_safe(cpuhp_node, struct arm_cmn, cpuhp_node);
-	if (cpu != cmn->cpu)
-		return 0;
-
-	node = dev_to_node(cmn->dev);
 
 	target = cpumask_any_and_but(cpumask_of_node(node), cpu_online_mask, cpu);
 	if (target >= nr_cpu_ids)
 		target = cpumask_any_but(cpu_online_mask, cpu);
 
 	if (target < nr_cpu_ids)
-		arm_cmn_migrate(cmn, target);
-
-	return 0;
+		return arm_cmn_migrate(cmn, target);
+	return false;
 }
 
 static irqreturn_t arm_cmn_handle_irq(int irq, void *dev_id)
@@ -2101,15 +2070,12 @@ static int arm_cmn_init_irqs(struct arm_cmn *cmn)
 				goto next;
 			}
 		}
+		/* Affinity will get set during PMU registration */
 		err = devm_request_irq(cmn->dev, irq, arm_cmn_handle_irq,
 				       IRQF_NOBALANCING | IRQF_NO_THREAD,
 				       dev_name(cmn->dev), &cmn->dtc[i]);
 		if (err)
 			return err;
-
-		err = irq_set_affinity(irq, cpumask_of(cmn->cpu));
-		if (err)
-			return err;
 	next:
 		; /* isn't C great? */
 	}
@@ -2565,7 +2531,6 @@ static int arm_cmn_probe(struct platform_device *pdev)
 
 	cmn->dev = &pdev->dev;
 	cmn->part = (unsigned long)device_get_match_data(cmn->dev);
-	cmn->cpu = cpumask_local_spread(0, dev_to_node(cmn->dev));
 	platform_set_drvdata(pdev, cmn);
 
 	cfg = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -2610,6 +2575,9 @@ static int arm_cmn_probe(struct platform_device *pdev)
 		.start_txn = arm_cmn_start_txn,
 		.commit_txn = arm_cmn_commit_txn,
 		.cancel_txn = arm_cmn_end_txn,
+		.scope = PERF_PMU_SCOPE_CPUMASK,
+		.init_cpu = arm_cmn_init_cpu,
+		.exit_cpu = arm_cmn_exit_cpu,
 	};
 
 	this_id = atomic_fetch_inc(&id);
@@ -2617,14 +2585,8 @@ static int arm_cmn_probe(struct platform_device *pdev)
 	if (!name)
 		return -ENOMEM;
 
-	err = cpuhp_state_add_instance(arm_cmn_hp_state, &cmn->cpuhp_node);
-	if (err)
-		return err;
-
 	err = perf_pmu_register(&cmn->pmu, name, -1);
-	if (err)
-		cpuhp_state_remove_instance_nocalls(arm_cmn_hp_state, &cmn->cpuhp_node);
-	else
+	if (!err)
 		arm_cmn_debugfs_init(cmn, this_id);
 
 	return err;
@@ -2637,7 +2599,6 @@ static void arm_cmn_remove(struct platform_device *pdev)
 	writel_relaxed(0, cmn->dtc[0].base + CMN_DT_DTC_CTL);
 
 	perf_pmu_unregister(&cmn->pmu);
-	cpuhp_state_remove_instance_nocalls(arm_cmn_hp_state, &cmn->cpuhp_node);
 	debugfs_remove(cmn->debug);
 }
 
@@ -2679,28 +2640,17 @@ static int __init arm_cmn_init(void)
 {
 	int ret;
 
-	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
-				      "perf/arm/cmn:online",
-				      arm_cmn_pmu_online_cpu,
-				      arm_cmn_pmu_offline_cpu);
-	if (ret < 0)
-		return ret;
-
-	arm_cmn_hp_state = ret;
 	arm_cmn_debugfs = debugfs_create_dir("arm-cmn", NULL);
 
 	ret = platform_driver_register(&arm_cmn_driver);
-	if (ret) {
-		cpuhp_remove_multi_state(arm_cmn_hp_state);
+	if (ret)
 		debugfs_remove(arm_cmn_debugfs);
-	}
 	return ret;
 }
 
 static void __exit arm_cmn_exit(void)
 {
 	platform_driver_unregister(&arm_cmn_driver);
-	cpuhp_remove_multi_state(arm_cmn_hp_state);
 	debugfs_remove(arm_cmn_debugfs);
 }
 
-- 
2.54.0.dirty

Re: [PATCH 1/2] perf: Generic hotplug/cpumask for system PMUs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Taking a CPU offline triggers a NULL pointer dereference if a PMU defines `init_cpu` but not `exit_cpu`.
- [High] CPU online context migration passes the new CPU as both source and destination to `perf_pmu_migrate_context`, causing a self-deadlock and failing to migrate events.
- [High] When a PMU is unregistered, the perf core fails to invoke the `pmu->exit_cpu()` callback for currently online CPUs, leaking state.
- [High] Missing bounds check in `perf_event_setup_cpumask` leads to an out-of-bounds per-CPU pointer access during migration.
- [High] `perf_pmu_register()` invokes the PMU's `init_cpu` callback in atomic IPI context, contradicting its documented thread context.
- [Medium] Secondary CPU hotplug triggers a `WARN_ON_ONCE` because `perf_event_setup_cpumask` incorrectly iterates up to `PERF_PMU_SCOPE_CPUMASK`.
- [Medium] PMUs using `PERF_PMU_SCOPE_CPUMASK` with static masks but no hotplug callbacks will never have their context migrated on CPU offline.
- [Medium] If `perf_pmu_register()` fails during the IDR allocation step, it leaks the CPU-affine state initialized via `pmu->init_cpu()`.
--

--- Patch [1]: [PATCH 1/2] perf: Generic hotplug/cpumask for system PMUs ---
commit aa795c7bb07889a3ee4d3e9c388a299913298dba
Author: Robin Murphy <robin.murphy@arm.com>

perf: Generic hotplug/cpumask for system PMUs

This commit extends the generic uncore machinery to handle system PMUs
by introducing a new PERF_PMU_SCOPE_CPUMASK scope. It allows the core
code to retrieve a per-PMU cpumask dynamically and adds optional callbacks
to manage CPU-affine state during hotplug operations.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index ba5bd6a78fe7b..505cbda2ac41f 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c

[ ... ]

> @@ -12920,6 +12945,16 @@ int perf_pmu_register(struct pmu *_pmu, const char *name, int type)
>  	INIT_LIST_HEAD(&pmu->events);
>  	spin_lock_init(&pmu->events_lock);
>  
> +	/*
> +	 * Finally, if appropriate give the PMU a chance to initialise any
> +	 * of its own CPU-affine state. Note that we're serialised against
> +	 * perf_event_{init,exit}_cpu() themselves by virtue of pmus_lock.
> +	 */
> +	if (pmu->init_cpu) {
> +		for_each_online_cpu(cpu)
> +			smp_call_function_single(cpu, perf_scope_init_cpu, pmu, 1);
> +	}
> +

[Severity: High]
Does executing init_cpu via smp_call_function_single() conflict with the
header documentation?

The documentation states init_cpu runs from CPUHP_AP_PERF_ONLINE context,
which is preemptible. However, smp_call_function_single() executes the
callback in atomic IPI context. If a driver sleeps here, will it cause a
panic?

[Severity: Medium]
If idr_cmpxchg() fails shortly after this initialization loop, does this
leak the newly allocated CPU-affine state?

There doesn't appear to be an error path to invoke exit_cpu on the online CPUs
to unwind this initialization.

[Severity: High]
While not explicitly in this diff, since this function sets up CPU-affine
state for a PMU during registration, is there a corresponding teardown step
missing?

It appears perf_pmu_unregister() doesn't invoke exit_cpu across the online
CPUs, which could leave this state dangling when a driver is unloaded.

>  	/*
>  	 * Now that the PMU is complete, make it visible to perf_try_init_event().
>  	 */

[ ... ]

> @@ -15189,10 +15225,19 @@ static void perf_event_clear_cpumask(unsigned int cpu)
>  
>  	/* migrate */
>  	list_for_each_entry(pmu, &pmus, entry) {
> +		bool cpumask_migrate = false;
> +
> +		if (pmu->init_cpu && (!pmu->cpumask || cpumask_test_cpu(cpu, pmu->cpumask)))
> +			cpumask_migrate = pmu->exit_cpu(pmu, cpu);
> +

[Severity: Critical]
Is there a risk of a NULL pointer dereference here?

This code checks if pmu->init_cpu is present, but then invokes
pmu->exit_cpu. Because both callbacks are optional, a PMU driver might
define init_cpu but leave exit_cpu as NULL.

>  		if (pmu->scope == PERF_PMU_SCOPE_NONE ||
>  		    WARN_ON_ONCE(pmu->scope >= PERF_PMU_MAX_SCOPE))
>  			continue;
>  
> +		target[PERF_PMU_SCOPE_CPUMASK] = -1;
> +		if (cpumask_migrate && !WARN_ON_ONCE(!pmu->cpumask))
> +			target[PERF_PMU_SCOPE_CPUMASK] = cpumask_any_but(pmu->cpumask, cpu);
> +

[Severity: Medium]
Does this strand PMU contexts for PMUs that use a static cpumask without
providing hotplug callbacks?

Since cpumask_migrate defaults to false and is only set if a callback
is invoked, target[PERF_PMU_SCOPE_CPUMASK] remains -1 and the migration
seems to be bypassed, leaving the context orphaned on the offline CPU.

>  		if (target[pmu->scope] >= 0 && target[pmu->scope] < nr_cpu_ids)
>  			perf_pmu_migrate_context(pmu, cpu, target[pmu->scope]);
>  	}

[ ... ]

> @@ -15246,7 +15292,7 @@ static void perf_event_setup_cpumask(unsigned int cpu)
>  		goto end;
>  	}
>  
> -	for (scope = PERF_PMU_SCOPE_NONE + 1; scope < PERF_PMU_MAX_SCOPE; scope++) {
> +	for (scope = PERF_PMU_SCOPE_NONE + 1; scope <= PERF_PMU_SCOPE_CPUMASK; scope++) {

[Severity: Medium]
Does iterating up to PERF_PMU_SCOPE_CPUMASK unconditionally trigger a
warning during secondary CPU hotplug?

For this scope, perf_scope_cpumask() explicitly returns NULL, which would
immediately trip the WARN_ON_ONCE(!pmu_cpumask || !cpumask) check later in
this loop.

>  		const struct cpumask *cpumask = perf_scope_cpu_topology_cpumask(scope, cpu);
>  
>  		pmu_cpumask = perf_scope_cpumask(scope);

[ ... ]

> @@ -15258,6 +15304,16 @@ static void perf_event_setup_cpumask(unsigned int cpu)
>  		    cpumask_any_and(pmu_cpumask, cpumask) >= nr_cpu_ids)
>  			cpumask_set_cpu(cpu, pmu_cpumask);
>  	}
> +
> +	/* Allow migrating if the new CPU is preferable (e.g. NUMA locality) */
> +	list_for_each_entry(pmu, &pmus, entry) {
> +		bool cpumask_migrate = false;
> +
> +		if (pmu->init_cpu && (!pmu->cpumask || !cpumask_test_cpu(cpu, pmu->cpumask)))
> +			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));
> +	}

[Severity: High]
Will this cause a self-deadlock in perf_pmu_migrate_context()?

The newly onlined CPU is passed as both the source CPU (cpu) and
potentially the destination CPU (via cpumask_any). This would lock the same
mutex twice in perf_pmu_migrate_context():

kernel/events/core.c:perf_pmu_migrate_context() {
    ...
    mutex_lock_double(&src_ctx->mutex, &dst_ctx->mutex);
    ...
}

Should this be migrating from the old CPU to the new CPU instead?

[Severity: High]
What happens if the PMU cpumask is empty here?

If init_cpu returns true but the PMU's mask is empty (for example, if it
defaulted to cpu_none_mask), cpumask_any() could return a value greater
than or equal to nr_cpu_ids. Passing this out-of-bounds value to
perf_pmu_migrate_context() might result in corrupting memory when it accesses
the per-CPU pointers.

>  end:
>  	cpumask_set_cpu(cpu, perf_online_mask);
>  }

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

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

[sashiko-bot]

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

Re: [PATCH 1/2] perf: Generic hotplug/cpumask for system PMUs

[Robin Murphy]

On 22/07/2026 6:38 pm, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
> - [Critical] Taking a CPU offline triggers a NULL pointer dereference if a PMU defines `init_cpu` but not `exit_cpu`.
> - [High] CPU online context migration passes the new CPU as both source and destination to `perf_pmu_migrate_context`, causing a self-deadlock and failing to migrate events.
> - [High] When a PMU is unregistered, the perf core fails to invoke the `pmu->exit_cpu()` callback for currently online CPUs, leaking state.
> - [High] Missing bounds check in `perf_event_setup_cpumask` leads to an out-of-bounds per-CPU pointer access during migration.
> - [High] `perf_pmu_register()` invokes the PMU's `init_cpu` callback in atomic IPI context, contradicting its documented thread context.
> - [Medium] Secondary CPU hotplug triggers a `WARN_ON_ONCE` because `perf_event_setup_cpumask` incorrectly iterates up to `PERF_PMU_SCOPE_CPUMASK`.
> - [Medium] PMUs using `PERF_PMU_SCOPE_CPUMASK` with static masks but no hotplug callbacks will never have their context migrated on CPU offline.
> - [Medium] If `perf_pmu_register()` fails during the IDR allocation step, it leaks the CPU-affine state initialized via `pmu->init_cpu()`.
> --
> 
> --- Patch [1]: [PATCH 1/2] perf: Generic hotplug/cpumask for system PMUs ---
> commit aa795c7bb07889a3ee4d3e9c388a299913298dba
> Author: Robin Murphy <robin.murphy@arm.com>
> 
> perf: Generic hotplug/cpumask for system PMUs
> 
> This commit extends the generic uncore machinery to handle system PMUs
> by introducing a new PERF_PMU_SCOPE_CPUMASK scope. It allows the core
> code to retrieve a per-PMU cpumask dynamically and adds optional callbacks
> to manage CPU-affine state during hotplug operations.
> 
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index ba5bd6a78fe7b..505cbda2ac41f 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
> 
> [ ... ]
> 
>> @@ -12920,6 +12945,16 @@ int perf_pmu_register(struct pmu *_pmu, const char *name, int type)
>>   	INIT_LIST_HEAD(&pmu->events);
>>   	spin_lock_init(&pmu->events_lock);
>>   
>> +	/*
>> +	 * Finally, if appropriate give the PMU a chance to initialise any
>> +	 * of its own CPU-affine state. Note that we're serialised against
>> +	 * perf_event_{init,exit}_cpu() themselves by virtue of pmus_lock.
>> +	 */
>> +	if (pmu->init_cpu) {
>> +		for_each_online_cpu(cpu)
>> +			smp_call_function_single(cpu, perf_scope_init_cpu, pmu, 1);
>> +	}
>> +
> 
> [Severity: High]
> Does executing init_cpu via smp_call_function_single() conflict with the
> header documentation?
> 
> The documentation states init_cpu runs from CPUHP_AP_PERF_ONLINE context,
> which is preemptible.

The documentation states it runs from there *and* once upon initial 
registration. This is the initial registration.

> However, smp_call_function_single() executes the
> callback in atomic IPI context. If a driver sleeps here, will it cause a
> panic?

Probably they just shouldn't sleep, since the vast majority of cases are 
basically only to invoke irq_set_affinity() anyway. Hence it didn't seem 
worth overcomplicating the initial design, but if something does turn 
out to really need a thread, it could be done.

> [Severity: Medium]
> If idr_cmpxchg() fails shortly after this initialization loop, does this
> leak the newly allocated CPU-affine state?
> 
> There doesn't appear to be an error path to invoke exit_cpu on the online CPUs
> to unwind this initialization.
> 
> [Severity: High]
> While not explicitly in this diff, since this function sets up CPU-affine
> state for a PMU during registration, is there a corresponding teardown step
> missing?
> 
> It appears perf_pmu_unregister() doesn't invoke exit_cpu across the online
> CPUs, which could leave this state dangling when a driver is unloaded.

The expectation is that drivers should be able to clean up without 
requiring explicit teardown calls. This cpu_init call is really just to 
allow picking _some_ initial affinity at the right point in the process 
to avoid the races, without having to have some additional dedicated 
method just for that.

>>   	/*
>>   	 * Now that the PMU is complete, make it visible to perf_try_init_event().
>>   	 */
> 
> [ ... ]
> 
>> @@ -15189,10 +15225,19 @@ static void perf_event_clear_cpumask(unsigned int cpu)
>>   
>>   	/* migrate */
>>   	list_for_each_entry(pmu, &pmus, entry) {
>> +		bool cpumask_migrate = false;
>> +
>> +		if (pmu->init_cpu && (!pmu->cpumask || cpumask_test_cpu(cpu, pmu->cpumask)))
>> +			cpumask_migrate = pmu->exit_cpu(pmu, cpu);
>> +
> 
> [Severity: Critical]
> Is there a risk of a NULL pointer dereference here?

Oops, just a copy-paste error.

> This code checks if pmu->init_cpu is present, but then invokes
> pmu->exit_cpu. Because both callbacks are optional, a PMU driver might
> define init_cpu but leave exit_cpu as NULL.
> 
>>   		if (pmu->scope == PERF_PMU_SCOPE_NONE ||
>>   		    WARN_ON_ONCE(pmu->scope >= PERF_PMU_MAX_SCOPE))
>>   			continue;
>>   
>> +		target[PERF_PMU_SCOPE_CPUMASK] = -1;
>> +		if (cpumask_migrate && !WARN_ON_ONCE(!pmu->cpumask))
>> +			target[PERF_PMU_SCOPE_CPUMASK] = cpumask_any_but(pmu->cpumask, cpu);
>> +
> 
> [Severity: Medium]
> Does this strand PMU contexts for PMUs that use a static cpumask without
> providing hotplug callbacks?
> 
> Since cpumask_migrate defaults to false and is only set if a callback
> is invoked, target[PERF_PMU_SCOPE_CPUMASK] remains -1 and the migration
> seems to be bypassed, leaving the context orphaned on the offline CPU.

The idea of a driver which sets PERF_PMU_SCOPE_CPUMASK but does not 
provide init_cpu/exit_cpu was kind of in case of CPU-coupled devices 
which want to provide per-CPU events for some fixed subset of CPUs. I'm 
not sure any such thing exists, but it seems like more work to actively 
forbid it than to leave the possibility open...

> 
>>   		if (target[pmu->scope] >= 0 && target[pmu->scope] < nr_cpu_ids)
>>   			perf_pmu_migrate_context(pmu, cpu, target[pmu->scope]);
>>   	}
> 
> [ ... ]
> 
>> @@ -15246,7 +15292,7 @@ static void perf_event_setup_cpumask(unsigned int cpu)
>>   		goto end;
>>   	}
>>   
>> -	for (scope = PERF_PMU_SCOPE_NONE + 1; scope < PERF_PMU_MAX_SCOPE; scope++) {
>> +	for (scope = PERF_PMU_SCOPE_NONE + 1; scope <= PERF_PMU_SCOPE_CPUMASK; scope++) {
> 
> [Severity: Medium]
> Does iterating up to PERF_PMU_SCOPE_CPUMASK unconditionally trigger a
> warning during secondary CPU hotplug?

Oops, could have sworn I fixed that... For these loops I initially had 
"< PERF_PMU_SCOPE_CPUMASK", decided that "<= PERF_PMU_SCOPE_SYS_WIDE" 
might be clearer, apparently somehow this one got stuck in-between.

> For this scope, perf_scope_cpumask() explicitly returns NULL, which would
> immediately trip the WARN_ON_ONCE(!pmu_cpumask || !cpumask) check later in
> this loop.
> 
>>   		const struct cpumask *cpumask = perf_scope_cpu_topology_cpumask(scope, cpu);
>>   
>>   		pmu_cpumask = perf_scope_cpumask(scope);
> 
> [ ... ]
> 
>> @@ -15258,6 +15304,16 @@ static void perf_event_setup_cpumask(unsigned int cpu)
>>   		    cpumask_any_and(pmu_cpumask, cpumask) >= nr_cpu_ids)
>>   			cpumask_set_cpu(cpu, pmu_cpumask);
>>   	}
>> +
>> +	/* Allow migrating if the new CPU is preferable (e.g. NUMA locality) */
>> +	list_for_each_entry(pmu, &pmus, entry) {
>> +		bool cpumask_migrate = false;
>> +
>> +		if (pmu->init_cpu && (!pmu->cpumask || !cpumask_test_cpu(cpu, pmu->cpumask)))
>> +			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));
>> +	}
> 
> [Severity: High]
> Will this cause a self-deadlock in perf_pmu_migrate_context()?
> 
> The newly onlined CPU is passed as both the source CPU (cpu) and
> potentially the destination CPU (via cpumask_any). This would lock the same
> mutex twice in perf_pmu_migrate_context():
> 
> kernel/events/core.c:perf_pmu_migrate_context() {
>      ...
>      mutex_lock_double(&src_ctx->mutex, &dst_ctx->mutex);
>      ...
> }
> 
> Should this be migrating from the old CPU to the new CPU instead?

Ooh, now that is a real bug. Indeed this isn't the same as the exit_cpu 
path, it needs to be in fully inverse order such that we sample the 
cpumask *before* init_cpu might update it.

> [Severity: High]
> What happens if the PMU cpumask is empty here?

Then the driver has done something egregiously wrong. However I guess 
since I do already have the WARN condition (for a misbehaving 
hotplug-only driver invalidly requesting migration), I may as well add 
cpumask_empty() to that as well.

Robin.

> If init_cpu returns true but the PMU's mask is empty (for example, if it
> defaulted to cpu_none_mask), cpumask_any() could return a value greater
> than or equal to nr_cpu_ids. Passing this out-of-bounds value to
> perf_pmu_migrate_context() might result in corrupting memory when it accesses
> the per-CPU pointers.
> 
>>   end:
>>   	cpumask_set_cpu(cpu, perf_online_mask);
>>   }
>