Re: [PATCH 6/8] perf sched: Fix thread reference leak in idle hist processing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] `timehist_get_thread()` leaks thread references on both success and error paths when `--idle-hist` is active.
- [High] The callchain root tree within `struct idle_thread_runtime` is not freed when idle threads are destroyed, leaking dynamically allocated callchain nodes.
--

commit b5d8b3caa1d8b8f8fc2ff742e34d77adc37bf655
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf sched: Fix thread reference leak in idle hist processing

timehist_sched_change_event() sets itr->last_thread to NULL at the end
of idle hist processing without calling thread__put() first.  The
thread reference was acquired via thread__get() in timehist_get_thread()
(line 2581), so every idle context switch leaks a thread reference when
--idle-hist is active.

Use thread__zput() to properly release the reference before clearing
the pointer.

> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index e47db1c5c6020..e2df69a624765 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2890,7 +2890,7 @@ static int timehist_sched_change_event(const struct perf_tool *tool,
>  			if (itr->cursor.nr)
>  				callchain_append(&itr->callchain, &itr->cursor, t - tprev);
>  
> -			itr->last_thread = NULL;
> +			thread__zput(itr->last_thread);

[Severity: High]
This isn't a bug introduced by this patch, but while we are fixing leaks
around idle thread handling, does timehist_get_thread() also leak thread
references when --idle-hist is active?

tools/perf/builtin-sched.c:timehist_get_thread() {
    ...
		if (sched->idle_hist) {
			struct thread *idle;
			struct idle_thread_runtime *itr;

			idle = get_idle_thread(sample->cpu);
			if (idle == NULL) {
				pr_err("Failed to get idle thread for cpu %d.\n", sample->cpu);
				return NULL;
			}
    ...
}

It appears get_idle_thread() returns a new reference via thread__get(). On
success, this local idle thread reference is never released. Also, on the
error paths returning NULL, it seems to bypass the caller's cleanup code and
permanently leak the task thread reference acquired earlier via
machine__findnew_thread().


[Severity: High]
This is a pre-existing issue, but does free_idle_threads() leak dynamically
allocated callchain nodes?

tools/perf/builtin-sched.c:free_idle_threads() {
    ...
		thread__put(idle);
    ...
}

The thread's private data destructor is set to free() via
thread__set_priv_destructor(free). While free() successfully frees the struct
idle_thread_runtime container, it doesn't appear to recursively free the
nodes embedded in the itr->callchain tree built by callchain_append(). Can
this lead to unbounded memory growth proportional to the trace size?

>  		}
>  
>  		if (!sched->summary_only)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605203316.1758661-1-acme@kernel.org?part=6