Add CPU hotplug support for PAI PMU
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
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
@@ -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
[email protected]
AI reviewRe: [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 functionspai_alloc_cpu()andpai_event_destroy_cpu(). No functional change.Signed-off-by Thomas Richter <[email protected]>
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
PATCH v2 2/3 s390/pai: Move locking to event init and delete
In reply to Thomas RichterMove 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
@@ -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
[email protected]
AI reviewRe: [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.
Signed-off-by Thomas Richter <[email protected]>
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
PATCH v2 3/3 s390/pai: Support CPU hotplug for PMU PAI
In reply to Thomas RichterThe 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
--- 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
@@ -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
[email protected]
AI reviewRe: [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
[ ... ]
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