Add CPU hotplug support for PAI PMU
Subsystems (2)
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
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
@@ -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
PATCH v9 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
PATCH v9 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 scheduled on it.
--- 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
@@ -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
Heiko Carstens
maintainerRe: [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.
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 forcpuhp_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