Skip to contentSkip to conversation

Add CPU hotplug support for PAI PMU

PATCH Superseded Thomas Richter 2 participants 7 messages Aug 24 s390

v2 of 9 Superseded Latest is v9 →

lore
Share & tools
lore
#0 lore raw

v2 cover Add CPU hotplug support for PAI PMU

V4: Add findings from Sumanth and updated patch 3/3.
- Fix reference count for CPU hotplug remove while multiple per-task events are active.

V3: Add findings from Sumanth and updated patch 3/3.
- Refined Fixes: commit id and messages
- Handle cpuhp_setup_state() failure

V2: Add findings from Sumanth and updated patch 3/3.
- handle possible NULL pointer dereference in pai_event_destroy_cpu()
- handle possibe memory allocation failure in pai_online_cpu()
- add Reviewed-by: Sumanth Korikkar <[email protected]> to patch 1/3 and patch 2/3.

Add support for CPU hotplug add and remove for PAI PMU.

Patch 1: Change function argument list Patch 2: Move locking from CPU scope to event scope Patch 3: Add CPU hotplug handler

Thomas Richter (3):
s390/pai: Use PAI PMU index as parameter replacing event s390/pai: Move locking to event init and delete s390/pai: Support CPU hotplug for PMU PAI

 arch/s390/include/asm/pai.h |   1 -
 arch/s390/kernel/perf_pai.c | 178 +++++++++++++++++++++++++-----------
 2 files changed, 124 insertions(+), 55 deletions(-)
Signature · 1 line
2.55.0
#1 lore raw

PATCH v2 1/3 s390/pai: Use PAI PMU index as parameter replacing event

Use PAI PMU index value as function argument instead of pointer to struct perf_event. Only that index value is used inside functions pai_alloc_cpu() and pai_event_destroy_cpu(). No functional change.

---
 arch/s390/kernel/perf_pai.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)
arch/s390/kernel/perf_pai.c +12 -13
index cdb8006220ca..7c13f5586c79 100644
@@ -140,16 +140,14 @@ static void pai_free(struct pai_mapptr *mp)
 /* Adjust usage counters and remove allocated memory when all users are
  * gone.
  */
-static void pai_event_destroy_cpu(struct perf_event *event, int cpu)
+static void pai_event_destroy_cpu(int idx, int cpu)
 {
-	int idx = PAI_PMU_IDX(event);
 	struct pai_mapptr *mp = per_cpu_ptr(pai_root[idx].mapptr, cpu);
 	struct pai_map *cpump = mp->mapptr;
 
 	mutex_lock(&pai_reserve_mutex);
-	debug_sprintf_event(paidbg, 5, "%s event %#llx idx %d cpu %d users %d "
-			    "refcnt %u\n", __func__, event->attr.config, idx,
-			    event->cpu, cpump->active_events,
+	debug_sprintf_event(paidbg, 5, "%s users %d refcnt %u\n",
+			    __func__, cpump->active_events,
 			    refcount_read(&cpump->refcnt));
 	if (refcount_dec_and_test(&cpump->refcnt))
 		pai_free(mp);
@@ -159,17 +157,17 @@ static void pai_event_destroy_cpu(struct perf_event *event, int cpu)
 
 static void pai_event_destroy(struct perf_event *event)
 {
-	int cpu;
+	int cpu = 0, idx = PAI_PMU_IDX(event);
 
 	free_page(PAI_SAVE_AREA(event));
 	if (event->cpu == -1) {
 		struct cpumask *mask = PAI_CPU_MASK(event);
 
 		for_each_cpu(cpu, mask)
-			pai_event_destroy_cpu(event, cpu);
+			pai_event_destroy_cpu(idx, cpu);
 		kfree(mask);
 	} else {
-		pai_event_destroy_cpu(event, event->cpu);
+		pai_event_destroy_cpu(idx, event->cpu);
 	}
 }
 
@@ -241,12 +239,12 @@ static u64 paicrypt_getall(struct perf_event *event)
  *
  * Allocate the memory for the event.
  */
-static int pai_alloc_cpu(struct perf_event *event, int cpu)
+static int pai_alloc_cpu(int idx, int cpu)
 {
-	int rc, idx = PAI_PMU_IDX(event);
 	struct pai_map *cpump = NULL;
 	bool need_paiext_cb = false;
 	struct pai_mapptr *mp;
+	int rc;
 
 	mutex_lock(&pai_reserve_mutex);
 	/* Allocate root node */
@@ -318,6 +316,7 @@ static int pai_alloc_cpu(struct perf_event *event, int cpu)
 
 static int pai_alloc(struct perf_event *event)
 {
+	int idx = PAI_PMU_IDX(event);
 	struct cpumask *maskptr;
 	int cpu, rc = -ENOMEM;
 
@@ -326,10 +325,10 @@ static int pai_alloc(struct perf_event *event)
 		goto out;
 
 	for_each_online_cpu(cpu) {
-		rc = pai_alloc_cpu(event, cpu);
+		rc = pai_alloc_cpu(idx, cpu);
 		if (rc) {
 			for_each_cpu(cpu, maskptr)
-				pai_event_destroy_cpu(event, cpu);
+				pai_event_destroy_cpu(idx, cpu);
 			kfree(maskptr);
 			goto out;
 		}
@@ -392,7 +391,7 @@ static int pai_event_init(struct perf_event *event, int idx)
 	}
 
 	if (event->cpu >= 0)
-		rc = pai_alloc_cpu(event, event->cpu);
+		rc = pai_alloc_cpu(idx, event->cpu);
 	else
 		rc = pai_alloc(event);
 	if (rc) {
Signature · 1 line
2.55.0
#2 lore raw

Re: [PATCH 1/3] s390/pai: Use PAI PMU index as parameter replacing event

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

Use PAI PMU index value as function argument instead of pointer to struct perf_event. Only that index value is used inside functions pai_alloc_cpu() and pai_event_destroy_cpu(). No functional change.

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

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#3 lore raw

PATCH v2 2/3 s390/pai: Move locking to event init and delete

In reply to Thomas Richter

Move mutex locking from per CPU allocation to event allocation. No functional change.

---
 arch/s390/kernel/perf_pai.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)
arch/s390/kernel/perf_pai.c +12 -13
index 7c13f5586c79..52d9f654346a 100644
@@ -138,21 +138,19 @@ static void pai_free(struct pai_mapptr *mp)
 }
 
 /* Adjust usage counters and remove allocated memory when all users are
- * gone.
+ * gone. Called under mutex_lock.
  */
 static void pai_event_destroy_cpu(int idx, int cpu)
 {
 	struct pai_mapptr *mp = per_cpu_ptr(pai_root[idx].mapptr, cpu);
 	struct pai_map *cpump = mp->mapptr;
 
-	mutex_lock(&pai_reserve_mutex);
 	debug_sprintf_event(paidbg, 5, "%s users %d refcnt %u\n",
 			    __func__, cpump->active_events,
 			    refcount_read(&cpump->refcnt));
 	if (refcount_dec_and_test(&cpump->refcnt))
 		pai_free(mp);
 	pai_root_free(idx);
-	mutex_unlock(&pai_reserve_mutex);
 }
 
 static void pai_event_destroy(struct perf_event *event)
@@ -160,6 +158,7 @@ static void pai_event_destroy(struct perf_event *event)
 	int cpu = 0, idx = PAI_PMU_IDX(event);
 
 	free_page(PAI_SAVE_AREA(event));
+	mutex_lock(&pai_reserve_mutex);
 	if (event->cpu == -1) {
 		struct cpumask *mask = PAI_CPU_MASK(event);
 
@@ -169,6 +168,7 @@ static void pai_event_destroy(struct perf_event *event)
 	} else {
 		pai_event_destroy_cpu(idx, event->cpu);
 	}
+	mutex_unlock(&pai_reserve_mutex);
 }
 
 static void paicrypt_event_destroy(struct perf_event *event)
@@ -232,12 +232,10 @@ static u64 paicrypt_getall(struct perf_event *event)
 	return sum;
 }
 
-/* Check concurrent access of counting and sampling for crypto events.
- * This function is called in process context and it is save to block.
- * When the event initialization functions fails, no other call back will
- * be invoked.
- *
- * Allocate the memory for the event.
+/* Allocate all per-CPU data structures. This function is called in
+ * process context and can block. In case of error all partly allocated
+ * memory is released and the reference counters adjusted correctly.
+ * Called under mutex_lock.
  */
 static int pai_alloc_cpu(int idx, int cpu)
 {
@@ -246,11 +244,10 @@ static int pai_alloc_cpu(int idx, int cpu)
 	struct pai_mapptr *mp;
 	int rc;
 
-	mutex_lock(&pai_reserve_mutex);
 	/* Allocate root node */
 	rc = pai_root_alloc(idx);
 	if (rc)
-		goto unlock;
+		goto out;
 
 	/* Allocate node for this event */
 	mp = per_cpu_ptr(pai_root[idx].mapptr, cpu);
@@ -308,12 +305,12 @@ static int pai_alloc_cpu(int idx, int cpu)
 		 */
 		pai_root_free(idx);
 	}
-unlock:
-	mutex_unlock(&pai_reserve_mutex);
+out:
 	/* If rc is non-zero, no increment of counter/sampler was done. */
 	return rc;
 }
 
+/* Called under mutex_lock */
 static int pai_alloc(struct perf_event *event)
 {
 	int idx = PAI_PMU_IDX(event);
@@ -390,10 +387,12 @@ static int pai_event_init(struct perf_event *event, int idx)
 		}
 	}
 
+	mutex_lock(&pai_reserve_mutex);
 	if (event->cpu >= 0)
 		rc = pai_alloc_cpu(idx, event->cpu);
 	else
 		rc = pai_alloc(event);
+	mutex_unlock(&pai_reserve_mutex);
 	if (rc) {
 		free_page(PAI_SAVE_AREA(event));
 		goto out;
Signature · 1 line
2.55.0
#4 lore raw

Re: [PATCH 2/3] s390/pai: Move locking to event init and delete

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

Move mutex locking from per CPU allocation to event allocation. No functional change.

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

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#5 lore raw

PATCH v2 3/3 s390/pai: Support CPU hotplug for PMU PAI

In reply to Thomas Richter

The command 'perf stat -e pai_crypto/CRYPTO_ALL/ -- <command>' crashes the kernel when CPUs are hotplug added during that run.

Root cause is the missing allocation of per-CPU data structures for that new CPU. The allocation is dynamic and the first event that has task context creates such a structure for each online CPU. This is not sufficient. CPUs may be offline during event creation and can be set online during the perf run time. For example commands

 # echo 0 > /sys/devices/system/cpu/cpu1/online
 # perf stat -e cycles -i -- stress-ng -t10s --matrix X
 # sleep 1
 # echo 1 > /sys/devices/system/cpu/cpu1/online

Currently without a CPU hotplug handler, that new CPU has no per-CPU data infrastructure. The scheduler runs PMU call back function pai_add() to install the PMU support for that CPU before the task is being scheduled on that new CPU.
In pai_add() instructions

    mp = this_cpu_ptr(pai_root[idx].mapptr);
    cpump = mp->mapptr;

return a NULL pointer and the result is a kernel panic as variable cpump is used inside that function.

Add CPU hotplug support for CPU add and delete and create the necessary per-CPU data infrastructure during CPU hotplug add processing. Same for CPU hotplug remove.
This is done when the CPU is offline to ensure the data structures are available when CPU is made online and tasks are schedules on it.

#Cc: [email protected] # v6.19

Fixes 582cc1b28e8c ("s390/pai_ext: Enable per-task and system-wide sampling event")
Fixes 9f66572f2889 ("s390/pai_crypto: Enable per-task and system-wide sampling event")
---
 arch/s390/include/asm/pai.h |   1 -
 arch/s390/kernel/perf_pai.c | 160 ++++++++++++++++++++++++++----------
 2 files changed, 116 insertions(+), 45 deletions(-)
arch/s390/include/asm/pai.h +0 -1
index 534d0320e2aa..a3456a36aaa7 100644
@@ -76,7 +76,6 @@ static __always_inline void pai_kernel_exit(struct pt_regs *regs)
 }
 
 #define PAI_SAVE_AREA(x)	((x)->hw.event_base)
-#define PAI_CPU_MASK(x)		((x)->hw.addr_filters)
 #define PAI_PMU_IDX(x)		((x)->hw.last_tag)
 #define PAI_SWLIST(x)		(&(x)->hw.tp_list)
 
arch/s390/kernel/perf_pai.c +116 -44

258 lines not on this page · raw message

Signature · 1 line
2.55.0
#6 lore raw

Re: [PATCH 3/3 v2] s390/pai: Support CPU hotplug for PMU PAI

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Missing `cpus_read_lock()` during PAI event allocation and destruction causes race conditions with…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing cpus_read_lock() during PAI event allocation and destruction causes race conditions with the new CPU hotplug callbacks, leading to NULL pointer dereferences or memory leaks. --

--- Patch [3]: [PATCH 3/3 v2] s390/pai: Support CPU hotplug for PMU PAI --- commit 91620cb7064d1e27822672c053b2886bba4751a9
Author: Thomas Richter <[email protected]>

s390/pai: Support CPU hotplug for PMU PAI

This commit adds CPU hotplug support for the PAI PMU, creating per-CPU data structures during CPU add/remove to prevent panics when CPUs are hotplugged during perf runs.

Quoted 4 lines
arch/s390/kernel/perf_pai.c +0 -0
index 52d9f654346a2..04a9dafa8f371 100644

[ ... ]

arch/s390/kernel/perf_pai.c static void pai_event_destroy(struct perf_event *event) · line 183

Quoted 12 lines
arch/s390/kernel/perf_pai.c +3 -5
@@ -160,13 +178,11 @@ static void pai_event_destroy(struct perf_event *event)
 	free_page(PAI_SAVE_AREA(event));
 	mutex_lock(&pai_reserve_mutex);
 	if (event->cpu == -1) {
-		struct cpumask *mask = PAI_CPU_MASK(event);
-
-		for_each_cpu(cpu, mask)
-			pai_event_destroy_cpu(idx, cpu);
-		kfree(mask);
+		atomic_dec(&pai_root[idx].tskctx);
+		for_each_online_cpu(cpu)
+			pai_event_destroy_cpu(idx, cpu, false);

[Severity: High]
Does this code introduce a memory leak if a CPU is offlined concurrently?

If a CPU is offlined, it is removed from cpu_online_mask at CPUHP_BRINGUP_CPU. If a concurrent perf event destruction runs, for_each_online_cpu() will skip the offlining CPU, failing to decrement its refcnt.

When the offline process reaches pai_offline_cpu(), it reads tskctx as 0, so refcount_sub_and_test(0, ...) does not drop the reference, leaving the memory permanently leaked.

Should cpus_read_lock() be held during pai_event_destroy() to prevent this?

Quoted 6 lines
arch/s390/kernel/perf_pai.c +1 -1
 	} else {
-		pai_event_destroy_cpu(idx, event->cpu);
+		pai_event_destroy_cpu(idx, event->cpu, false);
 	}
 	mutex_unlock(&pai_reserve_mutex);
 }

[ ... ]

Quoted 17 lines
arch/s390/kernel/perf_pai.c +3 -4
@@ -322,24 +362,20 @@ static int pai_alloc(struct perf_event *event)
 		goto out;
 
 	for_each_online_cpu(cpu) {
-		rc = pai_alloc_cpu(idx, cpu);
+		rc = pai_alloc_cpu(idx, cpu, false);
 		if (rc) {
 			for_each_cpu(cpu, maskptr)
-				pai_event_destroy_cpu(idx, cpu);
-			kfree(maskptr);
-			goto out;
+				pai_event_destroy_cpu(idx, cpu, false);
+			goto undo;
 		}
 		cpumask_set_cpu(cpu, maskptr);
 	}
 

arch/s390/kernel/perf_pai.c static int pai_alloc(struct perf_event *event) · line 376

Quoted 10 lines
arch/s390/kernel/perf_pai.c +2 -7
-	/*
-	 * On error all cpumask are freed and all events have been destroyed.
-	 * Save of which CPUs data structures have been allocated for.
-	 * Release them in pai_event_destroy call back function
-	 * for this event.
-	 */
-	PAI_CPU_MASK(event) = maskptr;
 	rc = 0;
+	/* Trace per-task events for CPU hotplug. */
+	atomic_inc(&pai_root[idx].tskctx);

[Severity: High]
Can this race with CPU hotplug and lead to a NULL pointer dereference?

If CPU hotplug begins (e.g., via sysfs), pai_online_cpu() executes at CPUHP_BP_PREPARE_DYN. Since no per-task events exist (tskctx == 0), it skips allocation.

If a concurrent system-wide perf event is created here, pai_alloc() iterates cpu_online_mask. Since the new CPU is not yet in the online mask (which happens later at CPUHP_BRINGUP_CPU), it skips allocation for the new CPU, and then increments tskctx to 1.

When the CPU completes bring-up and a task schedules on it, will pai_add() dereference the unallocated per-CPU pointer and cause a kernel panic?

Quoted 5 lines
arch/s390/kernel/perf_pai.c +2 -0
+undo:
+	kfree(maskptr);
 out:
 	return rc;
 }

[ ... ]

arch/s390/kernel/perf_pai.c static int pai_event_init(struct perf_event *event, int idx) · line 431

Quoted 9 lines
arch/s390/kernel/perf_pai.c +1 -1
@@ -389,7 +425,7 @@ static int pai_event_init(struct perf_event *event, int idx)
 
 	mutex_lock(&pai_reserve_mutex);
 	if (event->cpu >= 0)
-		rc = pai_alloc_cpu(idx, event->cpu);
+		rc = pai_alloc_cpu(idx, event->cpu, false);
 	else
 		rc = pai_alloc(event);
 	mutex_unlock(&pai_reserve_mutex);

[Severity: High]
Does pai_event_init() need to hold cpus_read_lock() here?

Acquiring cpus_read_lock() before taking pai_reserve_mutex (to avoid ABBA deadlocks with the hotplug writer lock) would serialize event allocation with CPU hotplug operations, preventing the races mentioned above.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3