[PATCH 0/4] perf: Manage single-CPU PMU affinity in perf core

[PATCH 0/4] perf: Manage single-CPU PMU affinity in perf core

[Haris Okanovic]

About 25 PMU drivers, nearly all under drivers/perf, drive their PMU from
a single CPU, and each implements the same logic independently: pick a CPU
at probe with cpumask_local_spread(), cpumask_any(), cpumask_any_and() or
similar, register a CPUHP state to handle hotplug migration, bind every
event to that CPU in event_init(), and publish it via a 'cpumask'
attribute.

Besides the duplicated code, that has two costs. On systems with many
PMUs, the arbitrary choice tends to land all of them on the same CPU,
concentrating overhead there. The chosen CPU also cannot be changed
afterwards, except via hotplug, which is awkward on systems that confine
background kernel work to housekeeping CPUs or otherwise isolate PMUs
from latency sensitive work.

These patches move the choice into perf core as a new "device" pmu::scope.
A driver describes which CPUs are eligible with an optional
filter_allowed_cpus() callback; core intersects that with the online CPUs,
picks one with cpumask_any_distribute() so instances spread out, migrates
events on hotplug, and reports the result through an optional migrate_cpu()
so the driver can follow with interrupt affinity.

'cpumask' is made writable for device scope, so users can also choose the
owning CPU deliberately, e.g. to move a PMU onto a housekeeping CPU. The
attribute is read-only or absent in other scopes. Reusing it was the
minimal change, and I'm happy to add a separate attribute instead if you
would rather 'cpumask' stayed read-only everywhere.

Only arm-cmn is converted in this series, as an example and because it is
what I can test today. I have prototype conversions for the other 24
drivers locally and will post them if the core approach looks right, but I
have no access to most of that hardware and will need help testing.

The series also carries two synchronization changes for events left behind
by migration: the __perf_pmu_remove() fix in patch 1 and the sequence
counter mechanism in patch 2. Neither problem is new. Under hotplug,
leftover events are torn down with the dying CPU, which limits the damage.
Migrating between two online CPUs leaves nothing to clean them up, so they
matter more once users can request migration. I'm open to suggestions on
different or better ways to close these races.

Tested arm-cmn on two platforms with CONFIG_PROVE_LOCKING=y:

  AWS m9g.metal-48xl CMN S3,  one mesh,   192 CPUs
  AWS m8g.metal-48xl CMN-700, two meshes, 192 CPUs

  - Events multiplex on the configured CPU, and the two meshes are placed
    on distinct CPUs at registration.
  - Offlining the owning CPU migrates every open event and updates the
    attribute; nothing is left behind on the dying CPU.
  - Writing 'cpumask' migrates open events, and offlining the vacated CPU
    afterwards strands nothing. Bad input is rejected with EINVAL, ERANGE
    or ENODEV without moving the PMU.
  - Hammering perf_event_open() from 64 threads while flipping 'cpumask'
    between two CPUs does not strand events.
  - No lockdep reports on either platform.

This supersedes the arm-cmn-only approach, following Robin Murphy's
suggestion that CPU affinity be generalized into perf core:

  https://lore.kernel.org/linux-arm-kernel/2d70be7b-12c0-46e0-b308-744a3159388a@arm.com/

Haris Okanovic (4):
  perf/core: Fix perf_pmu_migrate_context() leaving events behind
  perf/core: Add PERF_PMU_SCOPE_DEVICE
  perf/core: Allow userspace to select a device PMU's CPU
  perf/arm-cmn: Convert to PERF_PMU_SCOPE_DEVICE

 drivers/perf/arm-cmn.c     | 104 ++++------------
 include/linux/perf_event.h |  26 ++++
 kernel/events/core.c       | 247 +++++++++++++++++++++++++++++++++----
 3 files changed, 274 insertions(+), 103 deletions(-)

-- 
Haris Okanovic
AWS Graviton

[PATCH 1/4] perf/core: Fix perf_pmu_migrate_context() leaving events behind

[Haris Okanovic]

__perf_pmu_remove() walks the group leaders of a CPU and PMU with
perf_event_groups_for_cpu_pmu(), removing each one as it goes.
perf_remove_from_context() takes the leader out of the groups rbtree,
after which rb_next() on that node returns NULL, prematurely ending the
walk. Other groups stay behind in the source context.

On CPU hotplug, they are then torn down with the dying CPU. They keep
running until teardown, while the PMU's interrupt and migrated events
are on a new CPU, which drivers rely on never happening.

Walk ctx->event_list instead and pick out the PMU's events by their
pmu_ctx.

Fixes: bd2756811766 ("perf: Rewrite core context handling")
Signed-off-by: Haris Okanovic <harisokn@amazon.com>
---
 kernel/events/core.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7846d70be57f6..7caba4d528301 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -14394,23 +14394,18 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
 }
 EXPORT_SYMBOL_GPL(perf_event_create_kernel_counter);
 
-static void __perf_pmu_remove(struct perf_event_context *ctx,
-			      int cpu, struct pmu *pmu,
-			      struct perf_event_groups *groups,
+static void __perf_pmu_remove(struct perf_event_context *ctx, struct pmu *pmu,
 			      struct list_head *events)
 {
-	struct perf_event *event, *sibling;
+	struct perf_event *event, *tmp;
+
+	list_for_each_entry_safe(event, tmp, &ctx->event_list, event_entry) {
+		if (event->pmu_ctx->pmu != pmu)
+			continue;
 
-	perf_event_groups_for_cpu_pmu(event, groups, cpu, pmu) {
 		perf_remove_from_context(event, 0);
 		put_pmu_ctx(event->pmu_ctx);
 		list_add(&event->migrate_entry, events);
-
-		for_each_sibling_event(sibling, event) {
-			perf_remove_from_context(sibling, 0);
-			put_pmu_ctx(sibling->pmu_ctx);
-			list_add(&sibling->migrate_entry, events);
-		}
 	}
 }
 
@@ -14486,8 +14481,7 @@ void perf_pmu_migrate_context(struct pmu *pmu, int src_cpu, int dst_cpu)
 	 */
 	mutex_lock_double(&src_ctx->mutex, &dst_ctx->mutex);
 
-	__perf_pmu_remove(src_ctx, src_cpu, pmu, &src_ctx->pinned_groups, &events);
-	__perf_pmu_remove(src_ctx, src_cpu, pmu, &src_ctx->flexible_groups, &events);
+	__perf_pmu_remove(src_ctx, pmu, &events);
 
 	if (!list_empty(&events)) {
 		/*
-- 
Haris Okanovic
AWS Graviton

Re: [PATCH 1/4] perf/core: Fix perf_pmu_migrate_context() leaving events behind

[sashiko-bot]

> __perf_pmu_remove() walks the group leaders of a CPU and PMU with
> perf_event_groups_for_cpu_pmu(), removing each one as it goes.
> perf_remove_from_context() takes the leader out of the groups rbtree,
> after which rb_next() on that node returns NULL, prematurely ending the
> walk. Other groups stay behind in the source context.
> 
> On CPU hotplug, they are then torn down with the dying CPU. They keep
> running until teardown, while the PMU's interrupt and migrated events
> are on a new CPU, which drivers rely on never happening.
> 
> Walk ctx->event_list instead and pick out the PMU's events by their
> pmu_ctx.
> 
> Fixes: bd2756811766 ("perf: Rewrite core context handling")
> Signed-off-by: Haris Okanovic <harisokn@amazon.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013185603.1347265-1-harisokn@amazon.com?part=1

[PATCH 2/4] perf/core: Add PERF_PMU_SCOPE_DEVICE

[Haris Okanovic]

Add a "device" pmu::scope for PMUs counted on a single CPU drawn
from a driver-supplied set instead of a fixed topology.

This mechanism is implemented by new callbacks: filter_allowed_cpus()
is invoked to constrain the running CPU set to an allowed set for a
particular device; migrate_cpu() is used to perform any device specific
migration steps. Both are optional, e.g. for devices that can run on
any CPU or have no additional migration steps.

Placement initially happens in perf_pmu_register() before the PMU
becomes visible, and is subsequently re-evaluated on hotplug events.
A new CPU selection is made by cpumask_any_distribute() to spread out
load from multiple devices, but only when the previous CPU selection is
offline or no longer allowed by filter_allowed_cpus().

PMU events of these devices follow the chosen CPU. They are initially
bound to a CPU in perf_try_init_event() then reassigned by
perf_pmu_migrate_context() when a new CPU is selected.

The initial CPU assignment can race with migration, leaving a stranded
event. A sequence counter is added to close this gap without additional
locks. A count in struct pmu increments on each migration and is copied
to struct perf_event on init. The copied count is checked again before
the event is installed, under the same context mutex, which causes
perf_event_open() and perf_event_create_kernel_counter() to return -EBUSY
during migration periods.

Signed-off-by: Haris Okanovic <harisokn@amazon.com>
---
 include/linux/perf_event.h |  26 ++++++
 kernel/events/core.c       | 181 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 202 insertions(+), 5 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 2d5e7687875b0..99c88ec99b786 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -317,6 +317,7 @@ enum perf_pmu_scope {
 	PERF_PMU_SCOPE_CLUSTER,
 	PERF_PMU_SCOPE_PKG,
 	PERF_PMU_SCOPE_SYS_WIDE,
+	PERF_PMU_SCOPE_DEVICE,
 	PERF_PMU_MAX_SCOPE,
 };
 
@@ -351,6 +352,15 @@ struct pmu {
 	 */
 	unsigned int			scope;
 
+	/* The CPU this PMU counts on for PERF_PMU_SCOPE_DEVICE */
+	int				cpu;
+
+	/*
+	 * Ticks before and after changes to pmu::cpu, so that an event
+	 * being created can tell whether the PMU moved under it.
+	 */
+	unsigned int			migrate_seq;
+
 	struct perf_cpu_pmu_context * __percpu *cpu_pmu_context;
 	atomic_t			exclusive_cnt; /* < 0: cpu; > 0: tsk */
 	int				task_ctx_nr;
@@ -589,6 +599,16 @@ struct pmu {
 	 * Check period value for PERF_EVENT_IOC_PERIOD ioctl.
 	 */
 	int (*check_period)		(struct perf_event *event, u64 value); /* optional */
+
+	/*
+	 * Narrow @allowed_cpus (a set of candidate CPUs) down to the ones this
+	 * PMU can be counted on. Called on PERF_PMU_SCOPE_DEVICE PMUs when CPU
+	 * assignment needs to be reconsidered.
+	 */
+	int (*filter_allowed_cpus)(struct pmu *pmu, struct cpumask *allowed_cpus); /* optional */
+
+	/* Called when a PERF_PMU_SCOPE_DEVICE PMU is assigned a different CPU */
+	void (*migrate_cpu)(struct pmu *pmu, unsigned int cpu); /* optional */
 };
 
 enum perf_addr_filter_action_t {
@@ -855,6 +875,12 @@ struct perf_event {
 	int				oncpu;
 	int				cpu;
 
+	/*
+	 * Copy of pmu::migrate_seq on event init to detect migration of PMU
+	 * under PERF_PMU_SCOPE_DEVICE.
+	 */
+	unsigned int			pmu_migrate_seq;
+
 	struct list_head		owner_entry;
 	struct task_struct		*owner;
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7caba4d528301..0d4e2c6d71584 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -470,6 +470,7 @@ static cpumask_var_t perf_online_die_mask;
 static cpumask_var_t perf_online_cluster_mask;
 static cpumask_var_t perf_online_pkg_mask;
 static cpumask_var_t perf_online_sys_mask;
+static cpumask_var_t perf_device_scope_mask;
 static struct kmem_cache *perf_event_cache;
 
 #ifdef CONFIG_PERF_GUEST_MEDIATED_PMU
@@ -12684,12 +12685,93 @@ static inline struct cpumask *perf_scope_cpumask(unsigned int scope)
 	return NULL;
 }
 
+/*
+ * Move a PERF_PMU_SCOPE_DEVICE PMU to one of @requested_cpus.
+ *
+ * Uses the pre-allocated perf_device_scope_mask global to avoid
+ * allocation during hotplug; protected by pmus_lock.
+ */
+static int perf_pmu_set_cpu(struct pmu *pmu,
+			    const struct cpumask *requested_cpus)
+{
+	struct cpumask *allowed_cpus = perf_device_scope_mask;
+	int new_cpu, ret;
+
+	lockdep_assert_held(&pmus_lock);
+
+	/* A PMU can only be counted on a CPU that perf has online. */
+	cpumask_and(allowed_cpus, requested_cpus, perf_online_mask);
+	if (cpumask_empty(allowed_cpus))
+		return -ENODEV;
+
+	if (pmu->filter_allowed_cpus) {
+		ret = pmu->filter_allowed_cpus(pmu, allowed_cpus);
+		if (ret)
+			return ret;
+
+		if (cpumask_empty(allowed_cpus))
+			return -ENODEV;
+	}
+
+	/* Stay where we are for as long as that CPU is still allowed. */
+	if ((unsigned int)pmu->cpu < nr_cpu_ids &&
+	    cpumask_test_cpu(pmu->cpu, allowed_cpus))
+		return 0;
+
+	/* Spreads PMUs across allowed CPUs. */
+	new_cpu = cpumask_any_distribute(allowed_cpus);
+
+	WRITE_ONCE(pmu->migrate_seq, pmu->migrate_seq + 1);
+	smp_wmb(); /* pairs with the smp_rmb() in perf_try_init_event() */
+
+	/*
+	 * A PMU being placed for the first time (pmu->cpu = -1) has no events
+	 * yet, and no CPU to migrate them from.
+	 */
+	if ((unsigned int)pmu->cpu < nr_cpu_ids)
+		perf_pmu_migrate_context(pmu, pmu->cpu, new_cpu);
+
+	if (pmu->migrate_cpu)
+		pmu->migrate_cpu(pmu, new_cpu);
+
+	pmu->cpu = new_cpu;
+
+	smp_wmb(); /* pairs with the smp_rmb() in perf_try_init_event() */
+	WRITE_ONCE(pmu->migrate_seq, pmu->migrate_seq + 1);
+
+	return 0;
+}
+
+/* Did the PMU move after event was initialized? */
+static bool perf_event_pmu_moved(struct perf_event *event)
+{
+	struct pmu *pmu = event->pmu;
+
+	if (pmu->scope != PERF_PMU_SCOPE_DEVICE)
+		return false;
+
+	/*
+	 * An odd seq means the event was created during a move, so treat it
+	 * as moved.
+	 */
+	return (event->pmu_migrate_seq & 1) ||
+	       READ_ONCE(pmu->migrate_seq) != event->pmu_migrate_seq;
+}
+
 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);
+	struct cpumask *mask;
 
+	/* These PMU devices count on a single CPU, see pmu::cpu. */
+	if (pmu->scope == PERF_PMU_SCOPE_DEVICE) {
+		if ((unsigned int)pmu->cpu >= nr_cpu_ids)
+			return 0;
+		return sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(cpumask_of(pmu->cpu)));
+	}
+
+	mask = perf_scope_cpumask(pmu->scope);
 	if (mask)
 		return sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(mask));
 	return 0;
@@ -12846,6 +12928,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 (WARN_ONCE(pmu->scope != PERF_PMU_SCOPE_DEVICE &&
+		      (pmu->filter_allowed_cpus || pmu->migrate_cpu),
+		      "Can not register a pmu with CPU callbacks outside device scope.\n"))
+		return -EINVAL;
+
+	pmu->cpu = -1; /* chosen below for PERF_PMU_SCOPE_DEVICE */
 	pmu->name = name;
 
 	if (type >= 0)
@@ -12915,6 +13003,14 @@ int perf_pmu_register(struct pmu *_pmu, const char *name, int type)
 	INIT_LIST_HEAD(&pmu->events);
 	spin_lock_init(&pmu->events_lock);
 
+	/* Pick the initial CPU to count on */
+	if (pmu->scope == PERF_PMU_SCOPE_DEVICE) {
+		int ret = perf_pmu_set_cpu(pmu, perf_online_mask);
+
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * Now that the PMU is complete, make it visible to perf_try_init_event().
 	 */
@@ -13120,7 +13216,18 @@ static int perf_try_init_event(struct pmu *pmu, struct perf_event *event)
 		goto err_destroy;
 	}
 
-	if (pmu->scope != PERF_PMU_SCOPE_NONE && event->cpu >= 0) {
+	if (pmu->scope == PERF_PMU_SCOPE_DEVICE) {
+		/* A device is not per-task; its counters cannot follow one. */
+		if (event->attach_state & PERF_ATTACH_TASK) {
+			ret = -EINVAL;
+			goto err_destroy;
+		}
+
+		event->pmu_migrate_seq = READ_ONCE(pmu->migrate_seq);
+		smp_rmb(); /* pairs with the smp_wmb()s in perf_pmu_set_cpu() */
+
+		event->cpu = pmu->cpu;
+	} else if (pmu->scope != PERF_PMU_SCOPE_NONE && event->cpu >= 0) {
 		const struct cpumask *cpumask;
 		struct cpumask *pmu_cpumask;
 		int cpu;
@@ -14072,6 +14179,12 @@ SYSCALL_DEFINE5(perf_event_open,
 		}
 	}
 
+	/* The PMU may have moved since it was picked, retry if so. */
+	if (perf_event_pmu_moved(event)) {
+		err = -EBUSY;
+		goto err_locked;
+	}
+
 	if (group_leader) {
 		err = -EINVAL;
 
@@ -14369,6 +14482,12 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
 		}
 	}
 
+	/* The PMU may have moved since it was picked, retry if so. */
+	if (perf_event_pmu_moved(event)) {
+		err = -EBUSY;
+		goto err_pmu_ctx;
+	}
+
 	if (!exclusive_event_installable(event, ctx)) {
 		err = -EBUSY;
 		goto err_pmu_ctx;
@@ -15129,6 +15248,7 @@ static void __init perf_event_init_all_cpus(void)
 	zalloc_cpumask_var(&perf_online_cluster_mask, GFP_KERNEL);
 	zalloc_cpumask_var(&perf_online_pkg_mask, GFP_KERNEL);
 	zalloc_cpumask_var(&perf_online_sys_mask, GFP_KERNEL);
+	zalloc_cpumask_var(&perf_device_scope_mask, GFP_KERNEL);
 
 
 	for_each_possible_cpu(cpu) {
@@ -15165,6 +15285,36 @@ static void perf_swevent_init_cpu(unsigned int cpu)
 	mutex_unlock(&swhash->hlist_mutex);
 }
 
+static void perf_pmu_rebind(struct pmu *pmu)
+{
+	int ret;
+
+	/*
+	 * An empty mask legitimately happens for the last CPU in the
+	 * perf_reboot() teardown, when there is nothing left to migrate to.
+	 */
+	if (cpumask_empty(perf_online_mask))
+		return;
+
+	/* Failing to pick out of every online CPU is a PMU bug. */
+	ret = perf_pmu_set_cpu(pmu, perf_online_mask);
+	WARN_ON_ONCE(ret);
+}
+
+static void perf_pmu_rebind_all(void)
+{
+	struct pmu *pmu;
+
+	lockdep_assert_held(&pmus_lock);
+
+	list_for_each_entry(pmu, &pmus, entry) {
+		if (pmu->scope != PERF_PMU_SCOPE_DEVICE)
+			continue;
+
+		perf_pmu_rebind(pmu);
+	}
+}
+
 #if defined CONFIG_HOTPLUG_CPU || defined CONFIG_KEXEC_CORE
 static void __perf_event_exit_context(void *__info)
 {
@@ -15188,10 +15338,17 @@ 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++) {
-		const struct cpumask *cpumask = perf_scope_cpu_topology_cpumask(scope, cpu);
-		struct cpumask *pmu_cpumask = perf_scope_cpumask(scope);
+		const struct cpumask *cpumask;
+		struct cpumask *pmu_cpumask;
 
 		target[scope] = -1;
+
+		if (scope == PERF_PMU_SCOPE_DEVICE)
+			continue;
+
+		cpumask = perf_scope_cpu_topology_cpumask(scope, cpu);
+		pmu_cpumask = perf_scope_cpumask(scope);
+
 		if (WARN_ON_ONCE(!pmu_cpumask || !cpumask))
 			continue;
 
@@ -15208,6 +15365,12 @@ static void perf_event_clear_cpumask(unsigned int cpu)
 		    WARN_ON_ONCE(pmu->scope >= PERF_PMU_MAX_SCOPE))
 			continue;
 
+		if (pmu->scope == PERF_PMU_SCOPE_DEVICE) {
+			if (pmu->cpu == (int)cpu)
+				perf_pmu_rebind(pmu);
+			continue;
+		}
+
 		if (target[pmu->scope] >= 0 && target[pmu->scope] < nr_cpu_ids)
 			perf_pmu_migrate_context(pmu, cpu, target[pmu->scope]);
 	}
@@ -15253,6 +15416,9 @@ static void perf_event_setup_cpumask(unsigned int cpu)
 	 */
 	if (cpumask_empty(perf_online_mask)) {
 		for (scope = PERF_PMU_SCOPE_NONE + 1; scope < PERF_PMU_MAX_SCOPE; scope++) {
+			if (scope == PERF_PMU_SCOPE_DEVICE)
+				continue;
+
 			pmu_cpumask = perf_scope_cpumask(scope);
 			if (WARN_ON_ONCE(!pmu_cpumask))
 				continue;
@@ -15262,8 +15428,12 @@ static void perf_event_setup_cpumask(unsigned int cpu)
 	}
 
 	for (scope = PERF_PMU_SCOPE_NONE + 1; scope < PERF_PMU_MAX_SCOPE; scope++) {
-		const struct cpumask *cpumask = perf_scope_cpu_topology_cpumask(scope, cpu);
+		const struct cpumask *cpumask;
+
+		if (scope == PERF_PMU_SCOPE_DEVICE)
+			continue;
 
+		cpumask = perf_scope_cpu_topology_cpumask(scope, cpu);
 		pmu_cpumask = perf_scope_cpumask(scope);
 
 		if (WARN_ON_ONCE(!pmu_cpumask || !cpumask))
@@ -15292,6 +15462,7 @@ int perf_event_init_cpu(unsigned int cpu)
 	mutex_lock(&ctx->mutex);
 	cpuctx->online = 1;
 	mutex_unlock(&ctx->mutex);
+	perf_pmu_rebind_all();
 	mutex_unlock(&pmus_lock);
 
 	return 0;
-- 
Haris Okanovic
AWS Graviton

Re: [PATCH 2/4] perf/core: Add PERF_PMU_SCOPE_DEVICE

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] perf: WARN_ON_ONCE DoS during CPU hotplug for restricted device PMUs
- [Low] perf: spurious -ENODEV instead of -EBUSY during PMU migration
--

commit 97f2867d7b192326f20b5dc11e93789631bd5a2f
Author: Haris Okanovic <harisokn@amazon.com>

perf/core: Add PERF_PMU_SCOPE_DEVICE

Add a "device" pmu::scope for PMUs counted on a single CPU drawn
from a driver-supplied set instead of a fixed topology.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index dacc7375506a7..6274fea465a38 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
[ ... ]
> @@ -14109,6 +14216,12 @@ SYSCALL_DEFINE5(perf_event_open,
>  		}
>  	}
>  
> +	/* The PMU may have moved since it was picked, retry if so. */
> +	if (perf_event_pmu_moved(event)) {
> +		err = -EBUSY;
> +		goto err_locked;
> +	}
> +

[Severity: Low]
During a PMU migration caused by a CPU hotplug, a concurrent
sys_perf_event_open() could see cpuctx->online == 0. Does the existing
check for !cpuctx->online (which returns -ENODEV) eclipse this new
perf_event_pmu_moved() check, breaking the intent to return -EBUSY?

[ ... ]
> @@ -14406,6 +14519,12 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
>  		}
>  	}
>  
> +	/* The PMU may have moved since it was picked, retry if so. */
> +	if (perf_event_pmu_moved(event)) {
> +		err = -EBUSY;
> +		goto err_pmu_ctx;
> +	}
> +

[Severity: Low]
Similarly, does the !cpuctx->online check in perf_event_create_kernel_counter()
hide the -EBUSY return path when a PMU migrates due to CPU hotplug?

[ ... ]
> @@ -15202,6 +15322,36 @@ static void perf_swevent_init_cpu(unsigned int cpu)
>  	mutex_unlock(&swhash->hlist_mutex);
>  }
>  
> +static void perf_pmu_rebind(struct pmu *pmu)
> +{
> +	int ret;
> +
> +	/*
> +	 * An empty mask legitimately happens for the last CPU in the
> +	 * perf_reboot() teardown, when there is nothing left to migrate to.
> +	 */
> +	if (cpumask_empty(perf_online_mask))
> +		return;
> +
> +	/* Failing to pick out of every online CPU is a PMU bug. */
> +	ret = perf_pmu_set_cpu(pmu, perf_online_mask);
> +	WARN_ON_ONCE(ret);
> +}

[Severity: High]
If a device PMU implements filter_allowed_cpus() to restrict its valid execution
CPUs, and a user hot-unplugs all of those restricted CPUs while keeping at least
one other system CPU online, perf_pmu_set_cpu() accurately returns -ENODEV.
Will this legitimately return -ENODEV and trigger the WARN_ON_ONCE() in
perf_pmu_rebind(), leading to a denial of service if panic_on_warn is enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013185603.1347265-1-harisokn@amazon.com?part=2

[PATCH 3/4] perf/core: Allow userspace to select a device PMU's CPU

[Haris Okanovic]

perf core picks the CPU a "device" PMU (PERF_PMU_SCOPE_DEVICE) is
counted on when the PMU registers, and perf_try_init_event() then binds
every event to it. That choice is only revisited on CPU hotplug and
there is no way for users to set it explicitly, so in practice the PMU
stays wherever it was first placed.

All of the PMU's recurring work therefore lands on one CPU. This is
problematic on systems which reserve particular CPUs for latency
sensitive work or confine background activity to a chosen set of
housekeeping CPUs.

Make the 'cpumask' attribute writable for device scope, allowing users
to rebind PMU devices outside of hotplug by writing an arbitrary CPU
set. The same migration rules apply: user chosen CPU sets must contain
an online CPU that's accepted by the PMU driver's filter. Writes fail
with -ENODEV when none of the given CPUs satisfy these constraints.

The attribute expresses a preference rather than a guarantee. Hotplug
may still move the PMU, e.g. when the user chosen CPU is offlined.

Signed-off-by: Haris Okanovic <harisokn@amazon.com>
---
 kernel/events/core.c | 46 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 0d4e2c6d71584..fe048704ef4a1 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -12777,7 +12777,40 @@ static ssize_t cpumask_show(struct device *dev, struct device_attribute *attr,
 	return 0;
 }
 
-static DEVICE_ATTR_RO(cpumask);
+/*
+ * Move a device PMU to one of the CPUs written here. The PMU gets the final
+ * say: the write fails if it cannot be counted on any of them.
+ */
+static ssize_t cpumask_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct pmu *pmu = dev_get_drvdata(dev);
+	cpumask_var_t requested;
+	ssize_t ret;
+
+	if (pmu->scope != PERF_PMU_SCOPE_DEVICE)
+		return -EOPNOTSUPP;
+
+	if (!zalloc_cpumask_var(&requested, GFP_KERNEL))
+		return -ENOMEM;
+
+	ret = cpulist_parse(buf, requested);
+	if (ret)
+		goto out;
+
+	mutex_lock(&pmus_lock);
+	ret = perf_pmu_set_cpu(pmu, requested);
+	mutex_unlock(&pmus_lock);
+
+	if (!ret)
+		ret = count;
+out:
+	free_cpumask_var(requested);
+
+	return ret;
+}
+
+static DEVICE_ATTR_RW(cpumask);
 
 static struct attribute *pmu_dev_attrs[] = {
 	&dev_attr_type.attr,
@@ -12796,8 +12829,15 @@ static umode_t pmu_dev_is_visible(struct kobject *kobj, struct attribute *a, int
 		return 0;
 
 	/* cpumask */
-	if (n == 3 && pmu->scope == PERF_PMU_SCOPE_NONE)
-		return 0;
+	if (n == 3) {
+		/* Writable, the core picks the CPU. See cpumask_store(). */
+		if (pmu->scope == PERF_PMU_SCOPE_DEVICE)
+			return a->mode;
+		if (pmu->scope == PERF_PMU_SCOPE_NONE)
+			return 0;
+		/* A scope cpumask follows the CPU topology, read-only. */
+		return 0444;
+	}
 
 	return a->mode;
 }
-- 
Haris Okanovic
AWS Graviton

Re: [PATCH 3/4] perf/core: Allow userspace to select a device PMU's CPU

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf/core: AB-BA deadlock between pmus_lock and sysfs active reference
--

commit ad9a1746e052ae447d4db3655e5a566ce9de63d5
Author: Haris Okanovic <harisokn@amazon.com>

perf/core: Allow userspace to select a device PMU's CPU

Allow userspace to select the CPU a device PMU is counted on by making the
'cpumask' sysfs attribute writable. This enables rebinding PMU devices
outside of hotplug events by writing an arbitrary CPU set.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c

[ ... ]

> +/*
> + * Move a device PMU to one of the CPUs written here. The PMU gets the final
> + * say: the write fails if it cannot be counted on any of them.
> + */
> +static ssize_t cpumask_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct pmu *pmu = dev_get_drvdata(dev);
> +	cpumask_var_t requested;
> +	ssize_t ret;
> +
> +	if (pmu->scope != PERF_PMU_SCOPE_DEVICE)
> +		return -EOPNOTSUPP;
> +
> +	if (!zalloc_cpumask_var(&requested, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	ret = cpulist_parse(buf, requested);
> +	if (ret)
> +		goto out;
> +
> +	mutex_lock(&pmus_lock);
> +	ret = perf_pmu_set_cpu(pmu, requested);
> +	mutex_unlock(&pmus_lock);
> +
> +	if (!ret)
> +		ret = count;
> +out:
> +	free_cpumask_var(requested);
> +
> +	return ret;
> +}
> +
> +static DEVICE_ATTR_RW(cpumask);

[Severity: High]
Could this result in an AB-BA deadlock between pmus_lock and the sysfs active
reference?

Since cpumask_store() is called from a sysfs store operation, it implicitly
holds a kernfs active reference while waiting for pmus_lock.

However, perf_pmu_register() holds pmus_lock while calling pmu_dev_alloc():

kernel/events/core.c:perf_pmu_register() {
    ...
    guard(mutex)(&pmus_lock);
    ...
    if (pmu_bus_running && !pmu->dev) {
        int ret = pmu_dev_alloc(pmu);
        if (ret)
            return ret;
    }
    ...
}

Inside pmu_dev_alloc(), if sysfs_update_groups() fails after device_add() has
exposed the sysfs attributes, the error path calls device_del():

kernel/events/core.c:pmu_dev_alloc() {
    ...
    ret = device_add(pmu->dev);
    if (ret)
        goto free_dev;

    if (pmu->attr_update) {
        ret = sysfs_update_groups(&pmu->dev->kobj, pmu->attr_update);
        if (ret)
            goto del_dev;
    }
    ...
del_dev:
    device_del(pmu->dev);
    ...
}

Because device_del() waits for all active sysfs references to drain, if a
concurrent userspace write is executing cpumask_store() and blocking on
pmus_lock, would device_del() wait indefinitely for the active reference to be
released, resulting in an AB-BA deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013185603.1347265-1-harisokn@amazon.com?part=3

[PATCH 4/4] perf/arm-cmn: Convert to PERF_PMU_SCOPE_DEVICE

[Haris Okanovic]

Set pmu::scope to PERF_PMU_SCOPE_DEVICE and remove the driver-tracked cpu,
hotplug hooks, and the 'cpumask' attribute, since perf core does this
for device scope PMUs.

Define a filter_allowed_cpus() callback to keep preference for CPUs local
to the interconnect's NUMA node, and a migrate_cpu() callback to update
DTC interrupt affinity on migration.

Signed-off-by: Haris Okanovic <harisokn@amazon.com>
---
 drivers/perf/arm-cmn.c | 104 +++++++++--------------------------------
 1 file changed, 22 insertions(+), 82 deletions(-)

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index 5378fba916cf5..997ee5adf3f8e 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -388,17 +388,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;
@@ -1511,17 +1506,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 sysfs_emit(buf, "%*pbl\n", cpumask_pr_args(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)
 {
@@ -1534,7 +1518,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,
 };
@@ -1981,11 +1964,7 @@ static int arm_cmn_event_init(struct perf_event *event)
 	if (event->attr.type != event->pmu->type)
 		return -ENOENT;
 
-	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
-		return -EINVAL;
-
-	event->cpu = cmn->cpu;
-	if (event->cpu < 0)
+	if (is_sampling_event(event))
 		return -EINVAL;
 
 	type = CMN_EVENT_TYPE(event);
@@ -2243,48 +2222,26 @@ static int arm_cmn_commit_txn(struct pmu *pmu)
 	return 0;
 }
 
-static void arm_cmn_migrate(struct arm_cmn *cmn, unsigned int cpu)
+static int arm_cmn_filter_allowed_cpus(struct pmu *pmu, struct cpumask *allowed_cpus)
 {
-	unsigned int i;
+	struct arm_cmn *cmn = to_cmn(pmu);
+	int node = dev_to_node(cmn->dev);
 
-	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;
-}
+	/* Any CPU can drive the CMN, but prefer local ones */
+	if (node != NUMA_NO_NODE &&
+	    cpumask_intersects(allowed_cpus, cpumask_of_node(node)))
+		cpumask_and(allowed_cpus, allowed_cpus, cpumask_of_node(node));
 
-static int arm_cmn_pmu_online_cpu(unsigned int cpu, struct hlist_node *cpuhp_node)
-{
-	struct arm_cmn *cmn;
-	int node;
-
-	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;
+	return cpumask_empty(allowed_cpus) ? -ENODEV : 0;
 }
 
-static int arm_cmn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_node)
+static void arm_cmn_migrate_cpu(struct pmu *pmu, unsigned int cpu)
 {
-	struct arm_cmn *cmn;
-	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);
+	struct arm_cmn *cmn = to_cmn(pmu);
+	unsigned int i;
 
-	return 0;
+	for (i = 0; i < cmn->num_dtcs; i++)
+		WARN_ON_ONCE(irq_set_affinity(cmn->dtc[i].irq, cpumask_of(cpu)));
 }
 
 static irqreturn_t arm_cmn_handle_irq(int irq, void *dev_id)
@@ -2341,10 +2298,6 @@ static int arm_cmn_init_irqs(struct arm_cmn *cmn)
 				       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? */
 	}
@@ -2843,7 +2796,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);
@@ -2877,6 +2829,7 @@ static int arm_cmn_probe(struct platform_device *pdev)
 		.attr_groups = arm_cmn_attr_groups,
 		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
 		.task_ctx_nr = perf_invalid_context,
+		.scope = PERF_PMU_SCOPE_DEVICE,
 		.pmu_enable = arm_cmn_pmu_enable,
 		.pmu_disable = arm_cmn_pmu_disable,
 		.event_init = arm_cmn_event_init,
@@ -2888,6 +2841,8 @@ 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,
+		.filter_allowed_cpus = arm_cmn_filter_allowed_cpus,
+		.migrate_cpu = arm_cmn_migrate_cpu,
 	};
 
 	this_id = atomic_fetch_inc(&id);
@@ -2895,17 +2850,13 @@ 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);
+	err = perf_pmu_register(&cmn->pmu, name, -1);
 	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
-		arm_cmn_debugfs_init(cmn, this_id);
+	arm_cmn_debugfs_init(cmn, this_id);
 
-	return err;
+	return 0;
 }
 
 static void arm_cmn_remove(struct platform_device *pdev)
@@ -2915,7 +2866,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);
 }
 
@@ -2958,28 +2908,18 @@ 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);
 }
 
-- 
Haris Okanovic
AWS Graviton

Re: [PATCH 4/4] perf/arm-cmn: Convert to PERF_PMU_SCOPE_DEVICE

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf/arm-cmn: missing hardware disable on probe error path
--

commit baed79d02fccb15f4bbc09054f99c378f1c3c918
Author: Haris Okanovic <harisokn@amazon.com>

perf/arm-cmn: Convert to PERF_PMU_SCOPE_DEVICE

This commit sets the ARM CMN PMU scope to PERF_PMU_SCOPE_DEVICE and removes
the driver-tracked cpu, hotplug hooks, and the cpumask attribute. It also
defines filter_allowed_cpus() and migrate_cpu() callbacks to keep preferences
for local NUMA node CPUs.

> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index b162de3d9d169..00ffe45963244 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
[ ... ]
> @@ -2881,17 +2836,13 @@ 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);
> +	err = perf_pmu_register(&cmn->pmu, name, -1);
>  	if (err)
>  		return err;

[Severity: Medium]
If perf_pmu_register() fails here, does this return without explicitly
disabling the hardware, leaving it in an active state?

In arm_cmn_remove(), we see the driver disables the hardware:

    writel_relaxed(0, cmn->dtc[0].base + CMN_DT_DTC_CTL);

Since arm_cmn_init_dtcs() was called earlier in arm_cmn_probe(), should there
be an error handling path to write 0 to CMN_DT_DTC_CTL if registration fails,
to avoid leaking hardware state?

>  
> -	err = perf_pmu_register(&cmn->pmu, name, -1);
> -	if (err)
> -		cpuhp_state_remove_instance_nocalls(arm_cmn_hp_state, &cmn->cpuhp_node);
> -	else
> -		arm_cmn_debugfs_init(cmn, this_id);
> +	arm_cmn_debugfs_init(cmn, this_id);
>  
> -	return err;
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013185603.1347265-1-harisokn@amazon.com?part=4