Re: [PATCH] s390/cpum_cf: Handle CPU hotplug add and delete 6

Thank you for your contribution! Sashiko AI review found 2 potential issue(=
s) to consider:
- [High] The CPU hotplug online handler allocates `cpuhw` with a hardcoded =
reference count of 1, ignoring the actual number of active users tracked by=
 `tskcnt`. When any single active per-task event closes, it drops the newly=
 onlined CPU's reference count to 0, prematurely freeing the infrastructure.
- [High] Unconditional freeing of CPU-bound event infrastructure during CPU=
 hotplug offline permanently breaks those events upon subsequent online.
--

--- Patch [1]: [PATCH] s390/cpum_cf: Handle CPU hotplug add and delete 6 ---
commit 8c8ba5511cad2b2a0060e9a629b6a2e2df504f2d
Author: Thomas Richter <[email protected]>

s390/cpum_cf: Handle CPU hotplug add and delete 6

This commit changes the CPUMF PMU initialization to track the number of
active task-context events and allocate per-CPU infrastructure during CPU
hotplug add when these events are present. This prevents a NULL pointer
dereference panic when a per-task process is scheduled on a newly onlined
CPU that wasn't present during the initial perf event creation.

> diff --git a/arch/s390/kernel/perf_cpum_cf.c b/arch/s390/kernel/perf_cpum=
_cf.c
> index 2076ac22e2c49..c94334a4cfc4b 100644
> --- a/arch/s390/kernel/perf_cpum_cf.c
> +++ b/arch/s390/kernel/perf_cpum_cf.c
[ ... ]
> @@ -1105,13 +1136,13 @@ static int cpum_cf_online_cpu(unsigned int cpu)
>  {
>  	int rc =3D 0;
> =20
> -	/*
> -	 * Ignore notification for perf_event_open().
> -	 * Handle only /dev/hwctr device sessions.
> -	 */
>  	mutex_lock(&cfset_ctrset_mutex);
> -	if (refcount_read(&cfset_opencnt)) {
> +	/* Allocate per-CPU infrastructure when per-task context active. */
> +	mutex_lock(&pmc_reserve_mutex);
> +	if (atomic_read(&cpu_cf_root.tskcnt))
>  		rc =3D cpum_cf_alloc_cpu(cpu);

[Severity: High]
Does this code assign a reference count of 1 to the newly allocated
infrastructure regardless of how many active users are tracked by tskcnt?

If tskcnt is greater than 1, and a single task event closes,
cpum_cf_free() will decrement the reference count on all online CPUs.

Would this drop the reference count on this newly onlined CPU to 0, prematu=
rely
freeing the infrastructure while other events or /dev/hwctr sessions are
still active?

> +	mutex_unlock(&pmc_reserve_mutex);
> +	if (refcount_read(&cfset_opencnt)) {
>  		if (!rc)
>  			cfset_online_cpu(cpu);
>  	}
> @@ -1130,13 +1161,12 @@ static int cpum_cf_offline_cpu(unsigned int cpu)
>  	 * perf_event_open() created events. Perf common code triggers event
>  	 * destruction when the event file descriptor is closed.
>  	 *
> -	 * Handle only /dev/hwctr device sessions.
> +	 * Handle /dev/hwctr device sessions.
>  	 */
>  	mutex_lock(&cfset_ctrset_mutex);
> -	if (refcount_read(&cfset_opencnt)) {
> +	if (refcount_read(&cfset_opencnt))
>  		cfset_offline_cpu(cpu);
> -		cpum_cf_free_cpu(cpu);
> -	}
> +	cpum_cf_free(cpu);

[Severity: High]
Does this unconditional call to cpum_cf_free() permanently break CPU-bound
events when the CPU goes offline and comes back online?

CPU-bound events allocate infrastructure via cpum_cf_alloc() with a specific
CPU, taking the else branch which does not increment tskcnt:

arch/s390/kernel/perf_cpum_cf.c:cpum_cf_alloc() {
    ...
	if (cpu =3D=3D -1) {
		...
		if (!rc)
			atomic_inc(&cpu_cf_root.tskcnt);
	} else {
		rc =3D cpum_cf_alloc_cpu(cpu);
	}
    ...
}

When the CPU comes back online, cpum_cf_online_cpu() checks tskcnt:

arch/s390/kernel/perf_cpum_cf.c:cpum_cf_online_cpu() {
    ...
	if (atomic_read(&cpu_cf_root.tskcnt))
		rc =3D cpum_cf_alloc_cpu(cpu);
    ...
}

Since tskcnt was never incremented for the CPU-bound event, the
infrastructure is not reallocated. When the perf core attempts to restart t=
he
event on the new CPU, cpumf_pmu_add() will return -ENODEV.

>  	mutex_unlock(&cfset_ctrset_mutex);
>  	return 0;
>  }

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803161651.3803=
[email protected]?part=3D1