Skip to contentSkip to conversation

Add CPU hotplug support for PAI PMU

PATCH In review Thomas Richter 2 participants 6 messages Sep 1 – Sep 2

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v9 cover Add CPU hotplug support for PAI PMU

V9: Added suggestions from Heiko Carsten
- Changed coding style in patch 3.

V8: Add patch version 8 (v8) to each subject line

V7: Add findings from Jan Polensky
- Fixed spelling in code and commit messages.
- Added correct Cc: stable tag.

V6: Add findings from Sashiko
- Remove cpus_read_lock() and cpus_read_unlock() invocations from CPU hotplug handlers in patch 3.

V5: Add findings from Sashiko
- Add cpus_read_lock() and cpus_read_unlock() in patch 3.

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 | 182 +++++++++++++++++++++++++-----------
 2 files changed, 128 insertions(+), 55 deletions(-)
Signature · 1 line
2.55.0
#1 lore raw

PATCH v9 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 5c18c8b82ab7..109b15227d8f 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

PATCH v9 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 109b15227d8f..c333158cc945 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
#3 lore raw

PATCH v9 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 scheduled on it.

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 | 173 ++++++++++++++++++++++++++----------
 2 files changed, 126 insertions(+), 48 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 +126 -47

283 lines not on this page · raw message

Signature · 1 line
2.55.0
maintainer
#4 lore raw

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

Quoted 8 lines · Thomas Richter
arch/s390/kernel/perf_pai.c +6 -0
+	/* CPUHP_BP_PREPARE_DYN --> before CPU is brought online */
+	rc = cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "perf/pai:prepare",
+			       pai_online_cpu, pai_offline_cpu);
+	if (rc < 0)
+		goto out_debug;
+
 	if (!paipmu_setup()) {
 		/* No PMU registration, no need for debug buffer */

arch/s390/kernel/perf_pai.c static int __init pai_init(void) · line 1331

Quoted 10 lines
arch/s390/kernel/perf_pai.c +5 -3
-		debug_unregister_view(paidbg, &debug_sprintf_view);
-		debug_unregister(paidbg);
-		return -ENODEV;
+		rc = -ENODEV;
+		goto out_cpuhp;
 	}
 	return 0;
+
+out_cpuhp:
+	cpuhp_remove_state(rc);

This would now use "-ENODEV" as state for cpuhp_remove_state. I fixed it up since I suggested this code, so you can blame me in any case.

#5 lore raw

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

Quoted 14 lines · Heiko Carstens
Quoted 13 lines · Thomas Richter
arch/s390/kernel/perf_pai.c +8 -3
+	/* CPUHP_BP_PREPARE_DYN --> before CPU is brought online */
+	rc = cpuhp_setup_state(CPUHP_BP_PREPARE_DYN, "perf/pai:prepare",
+			       pai_online_cpu, pai_offline_cpu);
+	if (rc < 0)
+		goto out_debug;
+
 	if (!paipmu_setup()) {
 		/* No PMU registration, no need for debug buffer */
-		debug_unregister_view(paidbg, &debug_sprintf_view);
-		debug_unregister(paidbg);
-		return -ENODEV;
+		rc = -ENODEV;
+		goto out_cpuhp;
Quoted 8 lines

arch/s390/kernel/perf_pai.c static int __init pai_init(void) · line 1331

Quoted 5 lines
arch/s390/kernel/perf_pai.c +3 -0
 	}
 	return 0;
+
+out_cpuhp:
+	cpuhp_remove_state(rc);

This would now use "-ENODEV" as state for cpuhp_remove_state. I fixed it up since I suggested this code, so you can blame me in any case.

Ahhh right, thanks for fixing this for me.

Signature · 9 lines
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
IBM Deutschland Research & Development GmbH

Vorsitzender des Aufsichtsrats: Wolfgang Wendt

Geschäftsführung: David Faller

Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294