[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fU+nJreGEc1HVsYOL1Ba=YFE4G7OQqtQ=iwH=6+Crxssw@mail.gmail.com>
Date: Wed, 2 Jul 2025 20:08:35 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>,
LKML <linux-kernel@...r.kernel.org>, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH 4/8] perf sched: Fix thread leaks in 'perf sched timehist'
On Wed, Jul 2, 2025 at 6:49 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> Add missing thread__put() after machine__findnew_thread() or
> timehist_get_thread(). Also idle threads' last_thread should be
> refcounted properly.
>
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
Reviewed-by: Ian Rogers <irogers@...gle.com>
Thanks,
Ian
> ---
> tools/perf/builtin-sched.c | 48 +++++++++++++++++++++++++++++---------
> 1 file changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index b73989fb6acef8d6..83b5a85a91b7ffbd 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2313,8 +2313,10 @@ static void save_task_callchain(struct perf_sched *sched,
> return;
> }
>
> - if (!sched->show_callchain || sample->callchain == NULL)
> + if (!sched->show_callchain || sample->callchain == NULL) {
> + thread__put(thread);
> return;
> + }
>
> cursor = get_tls_callchain_cursor();
>
> @@ -2323,10 +2325,12 @@ static void save_task_callchain(struct perf_sched *sched,
> if (verbose > 0)
> pr_err("Failed to resolve callchain. Skipping\n");
>
> + thread__put(thread);
> return;
> }
>
> callchain_cursor_commit(cursor);
> + thread__put(thread);
>
> while (true) {
> struct callchain_cursor_node *node;
> @@ -2403,8 +2407,17 @@ static void free_idle_threads(void)
> return;
>
> for (i = 0; i < idle_max_cpu; ++i) {
> - if ((idle_threads[i]))
> - thread__delete(idle_threads[i]);
> + struct thread *idle = idle_threads[i];
> +
> + if (idle) {
> + struct idle_thread_runtime *itr;
> +
> + itr = thread__priv(idle);
> + if (itr)
> + thread__put(itr->last_thread);
> +
> + thread__delete(idle);
> + }
> }
>
> free(idle_threads);
> @@ -2441,7 +2454,7 @@ static struct thread *get_idle_thread(int cpu)
> }
> }
>
> - return idle_threads[cpu];
> + return thread__get(idle_threads[cpu]);
> }
>
> static void save_idle_callchain(struct perf_sched *sched,
> @@ -2496,7 +2509,8 @@ static struct thread *timehist_get_thread(struct perf_sched *sched,
> if (itr == NULL)
> return NULL;
>
> - itr->last_thread = thread;
> + thread__put(itr->last_thread);
> + itr->last_thread = thread__get(thread);
>
> /* copy task callchain when entering to idle */
> if (evsel__intval(evsel, sample, "next_pid") == 0)
> @@ -2567,6 +2581,7 @@ static void timehist_print_wakeup_event(struct perf_sched *sched,
> /* show wakeup unless both awakee and awaker are filtered */
> if (timehist_skip_sample(sched, thread, evsel, sample) &&
> timehist_skip_sample(sched, awakened, evsel, sample)) {
> + thread__put(thread);
> return;
> }
>
> @@ -2583,6 +2598,8 @@ static void timehist_print_wakeup_event(struct perf_sched *sched,
> printf("awakened: %s", timehist_get_commstr(awakened));
>
> printf("\n");
> +
> + thread__put(thread);
> }
>
> static int timehist_sched_wakeup_ignore(const struct perf_tool *tool __maybe_unused,
> @@ -2611,8 +2628,10 @@ static int timehist_sched_wakeup_event(const struct perf_tool *tool,
> return -1;
>
> tr = thread__get_runtime(thread);
> - if (tr == NULL)
> + if (tr == NULL) {
> + thread__put(thread);
> return -1;
> + }
>
> if (tr->ready_to_run == 0)
> tr->ready_to_run = sample->time;
> @@ -2622,6 +2641,7 @@ static int timehist_sched_wakeup_event(const struct perf_tool *tool,
> !perf_time__skip_sample(&sched->ptime, sample->time))
> timehist_print_wakeup_event(sched, evsel, sample, machine, thread);
>
> + thread__put(thread);
> return 0;
> }
>
> @@ -2649,6 +2669,7 @@ static void timehist_print_migration_event(struct perf_sched *sched,
>
> if (timehist_skip_sample(sched, thread, evsel, sample) &&
> timehist_skip_sample(sched, migrated, evsel, sample)) {
> + thread__put(thread);
> return;
> }
>
> @@ -2676,6 +2697,7 @@ static void timehist_print_migration_event(struct perf_sched *sched,
> printf(" cpu %d => %d", ocpu, dcpu);
>
> printf("\n");
> + thread__put(thread);
> }
>
> static int timehist_migrate_task_event(const struct perf_tool *tool,
> @@ -2695,8 +2717,10 @@ static int timehist_migrate_task_event(const struct perf_tool *tool,
> return -1;
>
> tr = thread__get_runtime(thread);
> - if (tr == NULL)
> + if (tr == NULL) {
> + thread__put(thread);
> return -1;
> + }
>
> tr->migrations++;
> tr->migrated = sample->time;
> @@ -2706,6 +2730,7 @@ static int timehist_migrate_task_event(const struct perf_tool *tool,
> timehist_print_migration_event(sched, evsel, sample,
> machine, thread);
> }
> + thread__put(thread);
>
> return 0;
> }
> @@ -2728,10 +2753,10 @@ static void timehist_update_task_prio(struct evsel *evsel,
> return;
>
> tr = thread__get_runtime(thread);
> - if (tr == NULL)
> - return;
> + if (tr != NULL)
> + tr->prio = next_prio;
>
> - tr->prio = next_prio;
> + thread__put(thread);
> }
>
> static int timehist_sched_change_event(const struct perf_tool *tool,
> @@ -2743,7 +2768,7 @@ static int timehist_sched_change_event(const struct perf_tool *tool,
> struct perf_sched *sched = container_of(tool, struct perf_sched, tool);
> struct perf_time_interval *ptime = &sched->ptime;
> struct addr_location al;
> - struct thread *thread;
> + struct thread *thread = NULL;
> struct thread_runtime *tr = NULL;
> u64 tprev, t = sample->time;
> int rc = 0;
> @@ -2867,6 +2892,7 @@ static int timehist_sched_change_event(const struct perf_tool *tool,
>
> evsel__save_time(evsel, sample->time, sample->cpu);
>
> + thread__put(thread);
> addr_location__exit(&al);
> return rc;
> }
> --
> 2.50.0.727.gbf7dc18ff4-goog
>
Powered by blists - more mailing lists