Skip to contentSkip to conversation

perf: Generic hotplug/cpumask for system PMUs

PATCH In review Robin Murphy 2 participants 5 messages Jul 24 – Jul 28

Subsystems (3)
perf events Arm PMU profiling and debugging Arm interconnect PMU drivers
lore
Share & tools
lore

Robin Murphy

author maintainer
#0 lore raw

v2 cover perf: Generic hotplug/cpumask for system PMUs

v1: https://lore.kernel.org/linux-perf-users/[email protected]/

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.

v2 fixes the definite functional mishaps from v1; as for the comments and other subtleties I'd rather know what real human maintainers think. I'm still not entirely sure about trying to put ->exit_cpu() calls in perf_pmu_free() for the sake of symmetry - it looks like it would be a little fiddly to get right, and as far as the system PMU drivers go, the ones which aren't already using _nocalls variants for remove/ probe-failure teardown anyway look to be just wrong and making exactly the mistakes I'm trying to clean up: doing a bunch of busywork chasing around the affinity of an IRQ that's about to be freed, plus calling perf_pmu_migrate_context() with an unregistered PMU! I do see at least arm-spe, coresight-trbe and riscv-sbi actually doing some kind of vaguely meaningful per-CPU disable, so perhaps the question of whether it's common enough to be generic or simple enough to open-code can wait for a future generic hotplug conversion of those?

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       | 78 +++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 83 deletions(-)
Signature · 1 line
2.54.0.dirty
author maintainer
#1 lore raw

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

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 <[email protected]>

---

v2: Fix copy-paste boo-boos, setup_cpumask migration logic

 include/linux/perf_event.h | 19 ++++++++++
 kernel/events/core.c       | 78 +++++++++++++++++++++++++++++++++-----
 2 files changed, 87 insertions(+), 10 deletions(-)
include/linux/perf_event.h +19 -0
index 48d851fbd8ea..ff29c45eb082 100644
@@ -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 {
kernel/events/core.c +68 -10

172 lines not on this page · raw message

Signature · 1 line
2.54.0.dirty
AI review
#2 lore raw
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] Missing teardown of per-CPU state on PMU unregister and error paths. - [High] The `init_cpu`…

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing teardown of per-CPU state on PMU unregister and error paths.
- [High] The init_cpu callback is executed in inconsistent contexts: hardirq context during PMU registration, but preemptible thread context during CPU hotplug.
- [High] perf_pmu_register iterates over cpu_online_mask instead of perf_online_mask under pmus_lock, causing races with CPU hotplug. --

--- Patch [1]: [PATCH v2 1/2] perf: Generic hotplug/cpumask for system PMUs --- commit b04553873017de2f8ea25f74f3ddf8a646a7b19d
Author: Robin Murphy <[email protected]>

perf: Generic hotplug/cpumask for system PMUs

This commit introduces a generic uncore machinery extension for system PMUs. It adds PERF_PMU_SCOPE_CPUMASK to allow PMUs to dynamically determine their cpumask. It also introduces init_cpu and exit_cpu callbacks to manage private CPU-affine state during CPU hotplug and PMU registration.

Quoted 4 lines
kernel/events/core.c +0 -0
index ba5bd6a78fe7b..3d8ccdfdd5707 100644

[ ... ]

kernel/events/core.c int perf_pmu_register(struct pmu *_pmu, const char *name, int type) · line 12955

Quoted 12 lines
kernel/events/core.c +8 -0
@@ -12920,6 +12945,17 @@ 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 pick an initial CPU
+	 * affinity for its cpumask, or initialise any general per-CPU state.
+	 * Note that we're serialised against perf_event_{init,exit}_cpu()
+	 * themselves here by virtue of pmus_lock.
+	 */
+	if (pmu->init_cpu) {
+		for_each_online_cpu(cpu)

[Severity: High]
Does this iteration over cpu_online_mask race with CPU hotplug?

Since pmus_lock serializes against perf_event_exit_cpu() and perf_event_init_cpu(), which update perf_online_mask, iterating over cpu_online_mask here can lead to desynchronization.

If a CPU is going offline, it may have been removed from perf_online_mask but is still in cpu_online_mask, resulting in an init_cpu call without a corresponding exit_cpu call. Similarly, a CPU coming online might be initialized here and then double-initialized in perf_event_setup_cpumask().

Should this use perf_online_mask instead?

kernel/events/core.c int perf_pmu_register(struct pmu *_pmu, const char *name, int type) · line 12954

Quoted 1 line
kernel/events/core.c +1 -0
+			smp_call_function_single(cpu, perf_scope_init_cpu, pmu, 1);

[Severity: High]
Will calling the init_cpu callback via smp_call_function_single() here crash PMU drivers migrating to this API?

This executes the callback in an IPI (hardirq) context with interrupts disabled. However, during CPU hotplug, perf_event_setup_cpumask() calls init_cpu directly in preemptible CPUHP_AP_PERF_ONLINE thread context.

If PMU drivers perform sleeping operations like request_irq() or kzalloc(..., GFP_KERNEL) in their init_cpu callback, this IPI context will cause a "BUG: scheduling while atomic" panic during driver probe.

kernel/events/core.c int perf_pmu_register(struct pmu *_pmu, const char *name, int type) · line 12959

Quoted 5 lines
kernel/events/core.c +2 -0
+	}
+
 	/*
 	 * Now that the PMU is complete, make it visible to perf_try_init_event().
 	 */

[Severity: Critical]
Does the error path following this initialization leak the per-CPU state?

When a PMU is registered, init_cpu() is invoked for all online CPUs to initialize per-CPU state. If perf_pmu_register() fails immediately after this at idr_cmpxchg(), or when the driver module is later unloaded and perf_pmu_unregister() is called, the framework does not iterate over online CPUs to call exit_cpu().

Because this new API bypasses the standard cpuhp_remove_state(), per-CPU resources will be permanently leaked, potentially causing a Use-After-Free panic when the module unloads and active interrupts point to freed code.

[ ... ]

kernel/events/core.c static void perf_event_setup_cpumask(unsigned int cpu) · line 15315

Quoted 12 lines
kernel/events/core.c +8 -0
@@ -15258,6 +15305,17 @@ 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) {
+		int prev_cpu = pmu->cpumask ? cpumask_any(pmu->cpumask) : nr_cpu_ids;
+		bool cpumask_migrate = false;
+
+		if (pmu->init_cpu && (!pmu->cpumask || !cpumask_test_cpu(cpu, pmu->cpumask)))
+			cpumask_migrate = pmu->init_cpu(pmu, cpu);

[Severity: High]
This is the second half of the API contract mismatch mentioned above.

Calling init_cpu() here executes in a preemptible thread context, while perf_pmu_register() calls it from a hardirq IPI context. Can this difference in context cause issues for PMU drivers that expect a consistent execution environment?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author maintainer
#3 lore raw
In reply to Robin Murphy

On 24/07/2026 6:48 pm, Robin Murphy wrote:
[...]

kernel/events/core.c int perf_pmu_register(struct pmu *_pmu, const char *name, int type) · line 12953

Quoted 12 lines
kernel/events/core.c +8 -0
@@ -12918,6 +12943,17 @@ 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 pick an initial CPU
+	 * affinity for its cpumask, or initialise any general per-CPU state.
+	 * Note that we're serialised against perf_event_{init,exit}_cpu()
+	 * themselves here by virtue of pmus_lock.
+	 */
+	if (pmu->init_cpu) {
+		for_each_online_cpu(cpu)

Having thought about it, while the drivers I want to convert should all be fine with a potential double-init, I guess it is true that an outgoing CPU in between CPUHP_AP_PERF_ONLINE and CPUHP_TEARDOWN_CPU would still be in cpu_online_mask with interrupts enabled, and thus potentially able to be picked by the driver callback, and thus end up inaccessible unless and until that CPU comes back again. So indeed for true race elimination this probably should use perf_online_mask. I'll save spamming a v3 until I get some feedback that the overall idea is worth pursuing at all, though.

(But I shall continue to ignore the rest of the twaddle from word-salad models which apparently can't differentiate "initialise" from "allocate"...)

Thanks,
Robin.

Quoted 6 lines
kernel/events/core.c +3 -0
+			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().
  	 */
author maintainer
#4 lore raw

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

In reply to 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 <[email protected]>

---

v2: No change.

 drivers/perf/arm-cmn.c | 96 ++++++++++--------------------------------
 1 file changed, 23 insertions(+), 73 deletions(-)
drivers/perf/arm-cmn.c +23 -73
index 6e5cc4086a9e..10243ac6217d 100644
@@ -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);
 }
Signature · 1 line
2.54.0.dirty