[PATCHSET 0/8] perf sched: Fix various memory leaks

[PATCHSET 0/8] perf sched: Fix various memory leaks

[Namhyung Kim]

Hello,

While running the new perf sched test with ASAN, it fails due to
memory leaks.  They comes mostly from missing thread__put() but
sometimes it needs to release other data structures.

Fix those leaks and add more subcommands to the test.

Thanks,
Namhyung


Namhyung Kim (8):
  perf sched: Make sure it frees the usage string
  perf sched: Free thread->priv using priv_destructor
  perf sched: Fix memory leaks in 'perf sched map'
  perf sched: Fix thread leaks in 'perf sched timehist'
  perf sched: Fix memory leaks for evsel->priv in timehist
  perf sched: Use RC_CHK_EQUAL() to compare pointers
  perf sched: Fix memory leaks in 'perf sched latency'
  perf test: Add more test cases to sched test

 tools/perf/builtin-sched.c      | 147 +++++++++++++++++++++++---------
 tools/perf/tests/shell/sched.sh |  39 +++++++--
 tools/perf/util/evsel.c         |  11 +++
 tools/perf/util/evsel.h         |   2 +
 4 files changed, 153 insertions(+), 46 deletions(-)

-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH 1/8] perf sched: Make sure it frees the usage string

[Namhyung Kim]

The parse_options_subcommand() allocates the usage string based on the
given subcommands.  So it should reach the end of the function to free
the string to prevent memory leaks.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-sched.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 26ece6e9bfd167b3..b7bbfad0ed600eee 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3902,9 +3902,9 @@ int cmd_sched(int argc, const char **argv)
 	 * Aliased to 'perf script' for now:
 	 */
 	if (!strcmp(argv[0], "script")) {
-		return cmd_script(argc, argv);
+		ret = cmd_script(argc, argv);
 	} else if (strlen(argv[0]) > 2 && strstarts("record", argv[0])) {
-		return __cmd_record(argc, argv);
+		ret = __cmd_record(argc, argv);
 	} else if (strlen(argv[0]) > 2 && strstarts("latency", argv[0])) {
 		sched.tp_handler = &lat_ops;
 		if (argc > 1) {
@@ -3913,7 +3913,7 @@ int cmd_sched(int argc, const char **argv)
 				usage_with_options(latency_usage, latency_options);
 		}
 		setup_sorting(&sched, latency_options, latency_usage);
-		return perf_sched__lat(&sched);
+		ret = perf_sched__lat(&sched);
 	} else if (!strcmp(argv[0], "map")) {
 		if (argc) {
 			argc = parse_options(argc, argv, map_options, map_usage, 0);
@@ -3924,13 +3924,14 @@ int cmd_sched(int argc, const char **argv)
 				sched.map.task_names = strlist__new(sched.map.task_name, NULL);
 				if (sched.map.task_names == NULL) {
 					fprintf(stderr, "Failed to parse task names\n");
-					return -1;
+					ret = -1;
+					goto out;
 				}
 			}
 		}
 		sched.tp_handler = &map_ops;
 		setup_sorting(&sched, latency_options, latency_usage);
-		return perf_sched__map(&sched);
+		ret = perf_sched__map(&sched);
 	} else if (strlen(argv[0]) > 2 && strstarts("replay", argv[0])) {
 		sched.tp_handler = &replay_ops;
 		if (argc) {
@@ -3938,7 +3939,7 @@ int cmd_sched(int argc, const char **argv)
 			if (argc)
 				usage_with_options(replay_usage, replay_options);
 		}
-		return perf_sched__replay(&sched);
+		ret = perf_sched__replay(&sched);
 	} else if (!strcmp(argv[0], "timehist")) {
 		if (argc) {
 			argc = parse_options(argc, argv, timehist_options,
@@ -3954,19 +3955,19 @@ int cmd_sched(int argc, const char **argv)
 				parse_options_usage(NULL, timehist_options, "w", true);
 			if (sched.show_next)
 				parse_options_usage(NULL, timehist_options, "n", true);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto out;
 		}
 		ret = symbol__validate_sym_arguments();
-		if (ret)
-			return ret;
-
-		return perf_sched__timehist(&sched);
+		if (!ret)
+			ret = perf_sched__timehist(&sched);
 	} else {
 		usage_with_options(sched_usage, sched_options);
 	}
 
+out:
 	/* free usage string allocated by parse_options_subcommand */
 	free((void *)sched_usage[0]);
 
-	return 0;
+	return ret;
 }
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH 2/8] perf sched: Free thread->priv using priv_destructor

[Namhyung Kim]

In many perf sched subcommand saves priv data structure in the thread
but it forgot to free them.  As it's an opaque type with 'void *', it
needs to register that knows how to free the data.  In this case, just
regular 'free()' is fine.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-sched.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index b7bbfad0ed600eee..fa4052e040201105 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3898,6 +3898,8 @@ int cmd_sched(int argc, const char **argv)
 	if (!argc)
 		usage_with_options(sched_usage, sched_options);
 
+	thread__set_priv_destructor(free);
+
 	/*
 	 * Aliased to 'perf script' for now:
 	 */
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH 3/8] perf sched: Fix memory leaks in 'perf sched map'

[Namhyung Kim]

It maintains per-cpu pointers for the current thread but it doesn't
release the refcounts.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-sched.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index fa4052e040201105..b73989fb6acef8d6 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1634,6 +1634,7 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel,
 	const char *color = PERF_COLOR_NORMAL;
 	char stimestamp[32];
 	const char *str;
+	int ret = -1;
 
 	BUG_ON(this_cpu.cpu >= MAX_CPUS || this_cpu.cpu < 0);
 
@@ -1664,17 +1665,20 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel,
 	sched_in = map__findnew_thread(sched, machine, -1, next_pid);
 	sched_out = map__findnew_thread(sched, machine, -1, prev_pid);
 	if (sched_in == NULL || sched_out == NULL)
-		return -1;
+		goto out;
 
 	tr = thread__get_runtime(sched_in);
-	if (tr == NULL) {
-		thread__put(sched_in);
-		return -1;
-	}
+	if (tr == NULL)
+		goto out;
+
+	thread__put(sched->curr_thread[this_cpu.cpu]);
+	thread__put(sched->curr_out_thread[this_cpu.cpu]);
 
 	sched->curr_thread[this_cpu.cpu] = thread__get(sched_in);
 	sched->curr_out_thread[this_cpu.cpu] = thread__get(sched_out);
 
+	ret = 0;
+
 	str = thread__comm_str(sched_in);
 	new_shortname = 0;
 	if (!tr->shortname[0]) {
@@ -1769,12 +1773,10 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel,
 	color_fprintf(stdout, color, "\n");
 
 out:
-	if (sched->map.task_name)
-		thread__put(sched_out);
-
+	thread__put(sched_out);
 	thread__put(sched_in);
 
-	return 0;
+	return ret;
 }
 
 static int process_sched_switch_event(const struct perf_tool *tool,
@@ -3556,10 +3558,10 @@ static int perf_sched__map(struct perf_sched *sched)
 
 	sched->curr_out_thread = calloc(MAX_CPUS, sizeof(*(sched->curr_out_thread)));
 	if (!sched->curr_out_thread)
-		return rc;
+		goto out_free_curr_thread;
 
 	if (setup_cpus_switch_event(sched))
-		goto out_free_curr_thread;
+		goto out_free_curr_out_thread;
 
 	if (setup_map_cpus(sched))
 		goto out_free_cpus_switch_event;
@@ -3590,7 +3592,14 @@ static int perf_sched__map(struct perf_sched *sched)
 out_free_cpus_switch_event:
 	free_cpus_switch_event(sched);
 
+out_free_curr_out_thread:
+	for (int i = 0; i < MAX_CPUS; i++)
+		thread__put(sched->curr_out_thread[i]);
+	zfree(&sched->curr_out_thread);
+
 out_free_curr_thread:
+	for (int i = 0; i < MAX_CPUS; i++)
+		thread__put(sched->curr_thread[i]);
 	zfree(&sched->curr_thread);
 	return rc;
 }
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH 4/8] perf sched: Fix thread leaks in 'perf sched timehist'

[Namhyung Kim]

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@kernel.org>
---
 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

[PATCH 5/8] perf sched: Fix memory leaks for evsel->priv in timehist

[Namhyung Kim]

It uses evsel->priv to save per-cpu timing information.  It should be
freed when the evsel is released.

Add the priv destructor for evsel same as thread to handle that.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-sched.c | 12 ++++++++++++
 tools/perf/util/evsel.c    | 11 +++++++++++
 tools/perf/util/evsel.h    |  2 ++
 3 files changed, 25 insertions(+)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 83b5a85a91b7ffbd..a6eb0462dd5be20f 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -2020,6 +2020,16 @@ static u64 evsel__get_time(struct evsel *evsel, u32 cpu)
 	return r->last_time[cpu];
 }
 
+static void timehist__evsel_priv_destructor(void *priv)
+{
+	struct evsel_runtime *r = priv;
+
+	if (r) {
+		free(r->last_time);
+		free(r);
+	}
+}
+
 static int comm_width = 30;
 
 static char *timehist_get_commstr(struct thread *thread)
@@ -3314,6 +3324,8 @@ static int perf_sched__timehist(struct perf_sched *sched)
 
 	setup_pager();
 
+	evsel__set_priv_destructor(timehist__evsel_priv_destructor);
+
 	/* prefer sched_waking if it is captured */
 	if (evlist__find_tracepoint_by_name(session->evlist, "sched:sched_waking"))
 		handlers[1].handler = timehist_sched_wakeup_ignore;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 9c50c3960487feee..3896a04d90af71f3 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1657,6 +1657,15 @@ static void evsel__free_config_terms(struct evsel *evsel)
 	free_config_terms(&evsel->config_terms);
 }
 
+static void (*evsel__priv_destructor)(void *priv);
+
+void evsel__set_priv_destructor(void (*destructor)(void *priv))
+{
+	assert(evsel__priv_destructor == NULL);
+
+	evsel__priv_destructor = destructor;
+}
+
 void evsel__exit(struct evsel *evsel)
 {
 	assert(list_empty(&evsel->core.node));
@@ -1687,6 +1696,8 @@ void evsel__exit(struct evsel *evsel)
 	hashmap__free(evsel->per_pkg_mask);
 	evsel->per_pkg_mask = NULL;
 	zfree(&evsel->metric_events);
+	if (evsel__priv_destructor)
+		evsel__priv_destructor(evsel->priv);
 	perf_evsel__object.fini(evsel);
 	if (evsel__tool_event(evsel) == TOOL_PMU__EVENT_SYSTEM_TIME ||
 	    evsel__tool_event(evsel) == TOOL_PMU__EVENT_USER_TIME)
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 6dbc9690e0c9258c..b84ee274602d7d2b 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -280,6 +280,8 @@ void evsel__init(struct evsel *evsel, struct perf_event_attr *attr, int idx);
 void evsel__exit(struct evsel *evsel);
 void evsel__delete(struct evsel *evsel);
 
+void evsel__set_priv_destructor(void (*destructor)(void *priv));
+
 struct callchain_param;
 
 void evsel__config(struct evsel *evsel, struct record_opts *opts,
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH 6/8] perf sched: Use RC_CHK_EQUAL() to compare pointers

[Namhyung Kim]

So that it can check two pointers to the same object properly when
REFCNT_CHECKING is on.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-sched.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index a6eb0462dd5be20f..087d4eaba5f7160d 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -994,7 +994,7 @@ thread_atoms_search(struct rb_root_cached *root, struct thread *thread,
 		else if (cmp < 0)
 			node = node->rb_right;
 		else {
-			BUG_ON(thread != atoms->thread);
+			BUG_ON(!RC_CHK_EQUAL(thread, atoms->thread));
 			return atoms;
 		}
 	}
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH 7/8] perf sched: Fix memory leaks in 'perf sched latency'

[Namhyung Kim]

The work_atoms should be freed after use.  Add free_work_atoms() to
make sure to release all.  It should use list_splice_init() when merging
atoms to prevent accessing invalid pointers.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-sched.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 087d4eaba5f7160d..4bbebd6ef2e4a791 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -1111,6 +1111,21 @@ add_sched_in_event(struct work_atoms *atoms, u64 timestamp)
 	atoms->nb_atoms++;
 }
 
+static void free_work_atoms(struct work_atoms *atoms)
+{
+	struct work_atom *atom, *tmp;
+
+	if (atoms == NULL)
+		return;
+
+	list_for_each_entry_safe(atom, tmp, &atoms->work_list, list) {
+		list_del(&atom->list);
+		free(atom);
+	}
+	thread__zput(atoms->thread);
+	free(atoms);
+}
+
 static int latency_switch_event(struct perf_sched *sched,
 				struct evsel *evsel,
 				struct perf_sample *sample,
@@ -3426,13 +3441,13 @@ static void __merge_work_atoms(struct rb_root_cached *root, struct work_atoms *d
 			this->total_runtime += data->total_runtime;
 			this->nb_atoms += data->nb_atoms;
 			this->total_lat += data->total_lat;
-			list_splice(&data->work_list, &this->work_list);
+			list_splice_init(&data->work_list, &this->work_list);
 			if (this->max_lat < data->max_lat) {
 				this->max_lat = data->max_lat;
 				this->max_lat_start = data->max_lat_start;
 				this->max_lat_end = data->max_lat_end;
 			}
-			zfree(&data);
+			free_work_atoms(data);
 			return;
 		}
 	}
@@ -3511,7 +3526,6 @@ static int perf_sched__lat(struct perf_sched *sched)
 		work_list = rb_entry(next, struct work_atoms, node);
 		output_lat_thread(sched, work_list);
 		next = rb_next(next);
-		thread__zput(work_list->thread);
 	}
 
 	printf(" -----------------------------------------------------------------------------------------------------------------\n");
@@ -3525,6 +3539,13 @@ static int perf_sched__lat(struct perf_sched *sched)
 
 	rc = 0;
 
+	while ((next = rb_first_cached(&sched->sorted_atom_root))) {
+		struct work_atoms *data;
+
+		data = rb_entry(next, struct work_atoms, node);
+		rb_erase_cached(next, &sched->sorted_atom_root);
+		free_work_atoms(data);
+	}
 out_free_cpus_switch_event:
 	free_cpus_switch_event(sched);
 	return rc;
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH 8/8] perf test: Add more test cases to sched test

[Namhyung Kim]

  $ sudo ./perf test -vv 92
   92: perf sched tests:
  --- start ---
  test child forked, pid 1360101
  Sched record
  pid 1360105's current affinity list: 0-3
  pid 1360105's new affinity list: 0
  pid 1360107's current affinity list: 0-3
  pid 1360107's new affinity list: 0
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 4.330 MB /tmp/__perf_test_sched.perf.data.b3319 (12246 samples) ]
  Sched latency
  Sched script
  Sched map
  Sched timehist
  Samples of sched_switch event do not have callchains.
  ---- end(0) ----
   92: perf sched tests                                                : Ok

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/sched.sh | 39 ++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/tools/perf/tests/shell/sched.sh b/tools/perf/tests/shell/sched.sh
index c030126d1a0c918d..b9b81eaf856e6555 100755
--- a/tools/perf/tests/shell/sched.sh
+++ b/tools/perf/tests/shell/sched.sh
@@ -56,38 +56,61 @@ cleanup_noploops() {
   kill "$PID1" "$PID2"
 }
 
-test_sched_latency() {
-  echo "Sched latency"
+test_sched_record() {
+  echo "Sched record"
 
   start_noploops
 
   perf sched record --no-inherit -o "${perfdata}" sleep 1
+
+  cleanup_noploops
+}
+
+test_sched_latency() {
+  echo "Sched latency"
+
   if ! perf sched latency -i "${perfdata}" | grep -q perf-noploop
   then
     echo "Sched latency [Failed missing output]"
     err=1
   fi
-
-  cleanup_noploops
 }
 
 test_sched_script() {
   echo "Sched script"
 
-  start_noploops
-
-  perf sched record --no-inherit -o "${perfdata}" sleep 1
   if ! perf sched script -i "${perfdata}" | grep -q perf-noploop
   then
     echo "Sched script [Failed missing output]"
     err=1
   fi
+}
 
-  cleanup_noploops
+test_sched_map() {
+  echo "Sched map"
+
+  if ! perf sched map -i "${perfdata}" | grep -q perf-noploop
+  then
+    echo "Sched map [Failed missing output]"
+    err=1
+  fi
+}
+
+test_sched_timehist() {
+  echo "Sched timehist"
+
+  if ! perf sched timehist -i "${perfdata}" | grep -q perf-noploop
+  then
+    echo "Sched timehist [Failed missing output]"
+    err=1
+  fi
 }
 
+test_sched_record
 test_sched_latency
 test_sched_script
+test_sched_map
+test_sched_timehist
 
 cleanup
 exit $err
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH 1/8] perf sched: Make sure it frees the usage string

[Ian Rogers]

On Wed, Jul 2, 2025 at 6:49 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The parse_options_subcommand() allocates the usage string based on the
> given subcommands.  So it should reach the end of the function to free
> the string to prevent memory leaks.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-sched.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 26ece6e9bfd167b3..b7bbfad0ed600eee 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -3902,9 +3902,9 @@ int cmd_sched(int argc, const char **argv)
>          * Aliased to 'perf script' for now:
>          */
>         if (!strcmp(argv[0], "script")) {
> -               return cmd_script(argc, argv);
> +               ret = cmd_script(argc, argv);
>         } else if (strlen(argv[0]) > 2 && strstarts("record", argv[0])) {
> -               return __cmd_record(argc, argv);
> +               ret = __cmd_record(argc, argv);
>         } else if (strlen(argv[0]) > 2 && strstarts("latency", argv[0])) {
>                 sched.tp_handler = &lat_ops;
>                 if (argc > 1) {
> @@ -3913,7 +3913,7 @@ int cmd_sched(int argc, const char **argv)
>                                 usage_with_options(latency_usage, latency_options);
>                 }
>                 setup_sorting(&sched, latency_options, latency_usage);
> -               return perf_sched__lat(&sched);
> +               ret = perf_sched__lat(&sched);
>         } else if (!strcmp(argv[0], "map")) {
>                 if (argc) {
>                         argc = parse_options(argc, argv, map_options, map_usage, 0);
> @@ -3924,13 +3924,14 @@ int cmd_sched(int argc, const char **argv)
>                                 sched.map.task_names = strlist__new(sched.map.task_name, NULL);
>                                 if (sched.map.task_names == NULL) {
>                                         fprintf(stderr, "Failed to parse task names\n");
> -                                       return -1;
> +                                       ret = -1;
> +                                       goto out;
>                                 }
>                         }
>                 }
>                 sched.tp_handler = &map_ops;
>                 setup_sorting(&sched, latency_options, latency_usage);
> -               return perf_sched__map(&sched);
> +               ret = perf_sched__map(&sched);
>         } else if (strlen(argv[0]) > 2 && strstarts("replay", argv[0])) {
>                 sched.tp_handler = &replay_ops;
>                 if (argc) {
> @@ -3938,7 +3939,7 @@ int cmd_sched(int argc, const char **argv)
>                         if (argc)
>                                 usage_with_options(replay_usage, replay_options);
>                 }
> -               return perf_sched__replay(&sched);
> +               ret = perf_sched__replay(&sched);
>         } else if (!strcmp(argv[0], "timehist")) {
>                 if (argc) {
>                         argc = parse_options(argc, argv, timehist_options,
> @@ -3954,19 +3955,19 @@ int cmd_sched(int argc, const char **argv)
>                                 parse_options_usage(NULL, timehist_options, "w", true);
>                         if (sched.show_next)
>                                 parse_options_usage(NULL, timehist_options, "n", true);
> -                       return -EINVAL;
> +                       ret = -EINVAL;
> +                       goto out;
>                 }
>                 ret = symbol__validate_sym_arguments();
> -               if (ret)
> -                       return ret;
> -
> -               return perf_sched__timehist(&sched);
> +               if (!ret)
> +                       ret = perf_sched__timehist(&sched);
>         } else {
>                 usage_with_options(sched_usage, sched_options);
>         }
>
> +out:
>         /* free usage string allocated by parse_options_subcommand */
>         free((void *)sched_usage[0]);
>
> -       return 0;
> +       return ret;
>  }
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH 2/8] perf sched: Free thread->priv using priv_destructor

[Ian Rogers]

On Wed, Jul 2, 2025 at 6:49 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> In many perf sched subcommand saves priv data structure in the thread
> but it forgot to free them.  As it's an opaque type with 'void *', it
> needs to register that knows how to free the data.  In this case, just
> regular 'free()' is fine.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-sched.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index b7bbfad0ed600eee..fa4052e040201105 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -3898,6 +3898,8 @@ int cmd_sched(int argc, const char **argv)
>         if (!argc)
>                 usage_with_options(sched_usage, sched_options);
>
> +       thread__set_priv_destructor(free);
> +
>         /*
>          * Aliased to 'perf script' for now:
>          */
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH 3/8] perf sched: Fix memory leaks in 'perf sched map'

[Ian Rogers]

On Wed, Jul 2, 2025 at 6:49 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It maintains per-cpu pointers for the current thread but it doesn't
> release the refcounts.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-sched.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index fa4052e040201105..b73989fb6acef8d6 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -1634,6 +1634,7 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel,
>         const char *color = PERF_COLOR_NORMAL;
>         char stimestamp[32];
>         const char *str;
> +       int ret = -1;
>
>         BUG_ON(this_cpu.cpu >= MAX_CPUS || this_cpu.cpu < 0);
>
> @@ -1664,17 +1665,20 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel,
>         sched_in = map__findnew_thread(sched, machine, -1, next_pid);
>         sched_out = map__findnew_thread(sched, machine, -1, prev_pid);
>         if (sched_in == NULL || sched_out == NULL)
> -               return -1;
> +               goto out;
>
>         tr = thread__get_runtime(sched_in);
> -       if (tr == NULL) {
> -               thread__put(sched_in);
> -               return -1;
> -       }
> +       if (tr == NULL)
> +               goto out;
> +
> +       thread__put(sched->curr_thread[this_cpu.cpu]);
> +       thread__put(sched->curr_out_thread[this_cpu.cpu]);
>
>         sched->curr_thread[this_cpu.cpu] = thread__get(sched_in);
>         sched->curr_out_thread[this_cpu.cpu] = thread__get(sched_out);
>
> +       ret = 0;
> +
>         str = thread__comm_str(sched_in);
>         new_shortname = 0;
>         if (!tr->shortname[0]) {
> @@ -1769,12 +1773,10 @@ static int map_switch_event(struct perf_sched *sched, struct evsel *evsel,
>         color_fprintf(stdout, color, "\n");
>
>  out:
> -       if (sched->map.task_name)
> -               thread__put(sched_out);
> -
> +       thread__put(sched_out);
>         thread__put(sched_in);
>
> -       return 0;
> +       return ret;
>  }
>
>  static int process_sched_switch_event(const struct perf_tool *tool,
> @@ -3556,10 +3558,10 @@ static int perf_sched__map(struct perf_sched *sched)
>
>         sched->curr_out_thread = calloc(MAX_CPUS, sizeof(*(sched->curr_out_thread)));
>         if (!sched->curr_out_thread)
> -               return rc;
> +               goto out_free_curr_thread;
>
>         if (setup_cpus_switch_event(sched))
> -               goto out_free_curr_thread;
> +               goto out_free_curr_out_thread;
>
>         if (setup_map_cpus(sched))
>                 goto out_free_cpus_switch_event;
> @@ -3590,7 +3592,14 @@ static int perf_sched__map(struct perf_sched *sched)
>  out_free_cpus_switch_event:
>         free_cpus_switch_event(sched);
>
> +out_free_curr_out_thread:
> +       for (int i = 0; i < MAX_CPUS; i++)
> +               thread__put(sched->curr_out_thread[i]);
> +       zfree(&sched->curr_out_thread);
> +
>  out_free_curr_thread:
> +       for (int i = 0; i < MAX_CPUS; i++)
> +               thread__put(sched->curr_thread[i]);
>         zfree(&sched->curr_thread);
>         return rc;
>  }
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH 4/8] perf sched: Fix thread leaks in 'perf sched timehist'

[Ian Rogers]

On Wed, Jul 2, 2025 at 6:49 PM Namhyung Kim <namhyung@kernel.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@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.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
>

Re: [PATCH 5/8] perf sched: Fix memory leaks for evsel->priv in timehist

[Ian Rogers]

On Wed, Jul 2, 2025 at 6:49 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It uses evsel->priv to save per-cpu timing information.  It should be
> freed when the evsel is released.
>
> Add the priv destructor for evsel same as thread to handle that.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-sched.c | 12 ++++++++++++
>  tools/perf/util/evsel.c    | 11 +++++++++++
>  tools/perf/util/evsel.h    |  2 ++
>  3 files changed, 25 insertions(+)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 83b5a85a91b7ffbd..a6eb0462dd5be20f 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2020,6 +2020,16 @@ static u64 evsel__get_time(struct evsel *evsel, u32 cpu)
>         return r->last_time[cpu];
>  }
>
> +static void timehist__evsel_priv_destructor(void *priv)
> +{
> +       struct evsel_runtime *r = priv;
> +
> +       if (r) {
> +               free(r->last_time);
> +               free(r);
> +       }
> +}
> +
>  static int comm_width = 30;
>
>  static char *timehist_get_commstr(struct thread *thread)
> @@ -3314,6 +3324,8 @@ static int perf_sched__timehist(struct perf_sched *sched)
>
>         setup_pager();
>
> +       evsel__set_priv_destructor(timehist__evsel_priv_destructor);
> +
>         /* prefer sched_waking if it is captured */
>         if (evlist__find_tracepoint_by_name(session->evlist, "sched:sched_waking"))
>                 handlers[1].handler = timehist_sched_wakeup_ignore;
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 9c50c3960487feee..3896a04d90af71f3 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1657,6 +1657,15 @@ static void evsel__free_config_terms(struct evsel *evsel)
>         free_config_terms(&evsel->config_terms);
>  }
>
> +static void (*evsel__priv_destructor)(void *priv);
> +
> +void evsel__set_priv_destructor(void (*destructor)(void *priv))
> +{
> +       assert(evsel__priv_destructor == NULL);
> +
> +       evsel__priv_destructor = destructor;
> +}
> +
>  void evsel__exit(struct evsel *evsel)
>  {
>         assert(list_empty(&evsel->core.node));
> @@ -1687,6 +1696,8 @@ void evsel__exit(struct evsel *evsel)
>         hashmap__free(evsel->per_pkg_mask);
>         evsel->per_pkg_mask = NULL;
>         zfree(&evsel->metric_events);
> +       if (evsel__priv_destructor)
> +               evsel__priv_destructor(evsel->priv);
>         perf_evsel__object.fini(evsel);
>         if (evsel__tool_event(evsel) == TOOL_PMU__EVENT_SYSTEM_TIME ||
>             evsel__tool_event(evsel) == TOOL_PMU__EVENT_USER_TIME)
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 6dbc9690e0c9258c..b84ee274602d7d2b 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -280,6 +280,8 @@ void evsel__init(struct evsel *evsel, struct perf_event_attr *attr, int idx);
>  void evsel__exit(struct evsel *evsel);
>  void evsel__delete(struct evsel *evsel);
>
> +void evsel__set_priv_destructor(void (*destructor)(void *priv));
> +
>  struct callchain_param;
>
>  void evsel__config(struct evsel *evsel, struct record_opts *opts,
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH 6/8] perf sched: Use RC_CHK_EQUAL() to compare pointers

[Ian Rogers]

On Wed, Jul 2, 2025 at 6:49 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> So that it can check two pointers to the same object properly when
> REFCNT_CHECKING is on.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-sched.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index a6eb0462dd5be20f..087d4eaba5f7160d 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -994,7 +994,7 @@ thread_atoms_search(struct rb_root_cached *root, struct thread *thread,
>                 else if (cmp < 0)
>                         node = node->rb_right;
>                 else {
> -                       BUG_ON(thread != atoms->thread);
> +                       BUG_ON(!RC_CHK_EQUAL(thread, atoms->thread));
>                         return atoms;
>                 }
>         }
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH 7/8] perf sched: Fix memory leaks in 'perf sched latency'

[Ian Rogers]

On Wed, Jul 2, 2025 at 6:49 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The work_atoms should be freed after use.  Add free_work_atoms() to
> make sure to release all.  It should use list_splice_init() when merging
> atoms to prevent accessing invalid pointers.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-sched.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 087d4eaba5f7160d..4bbebd6ef2e4a791 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -1111,6 +1111,21 @@ add_sched_in_event(struct work_atoms *atoms, u64 timestamp)
>         atoms->nb_atoms++;
>  }
>
> +static void free_work_atoms(struct work_atoms *atoms)
> +{
> +       struct work_atom *atom, *tmp;
> +
> +       if (atoms == NULL)
> +               return;
> +
> +       list_for_each_entry_safe(atom, tmp, &atoms->work_list, list) {
> +               list_del(&atom->list);
> +               free(atom);
> +       }
> +       thread__zput(atoms->thread);
> +       free(atoms);
> +}
> +
>  static int latency_switch_event(struct perf_sched *sched,
>                                 struct evsel *evsel,
>                                 struct perf_sample *sample,
> @@ -3426,13 +3441,13 @@ static void __merge_work_atoms(struct rb_root_cached *root, struct work_atoms *d
>                         this->total_runtime += data->total_runtime;
>                         this->nb_atoms += data->nb_atoms;
>                         this->total_lat += data->total_lat;
> -                       list_splice(&data->work_list, &this->work_list);
> +                       list_splice_init(&data->work_list, &this->work_list);
>                         if (this->max_lat < data->max_lat) {
>                                 this->max_lat = data->max_lat;
>                                 this->max_lat_start = data->max_lat_start;
>                                 this->max_lat_end = data->max_lat_end;
>                         }
> -                       zfree(&data);
> +                       free_work_atoms(data);
>                         return;
>                 }
>         }
> @@ -3511,7 +3526,6 @@ static int perf_sched__lat(struct perf_sched *sched)
>                 work_list = rb_entry(next, struct work_atoms, node);
>                 output_lat_thread(sched, work_list);
>                 next = rb_next(next);
> -               thread__zput(work_list->thread);
>         }
>
>         printf(" -----------------------------------------------------------------------------------------------------------------\n");
> @@ -3525,6 +3539,13 @@ static int perf_sched__lat(struct perf_sched *sched)
>
>         rc = 0;
>
> +       while ((next = rb_first_cached(&sched->sorted_atom_root))) {
> +               struct work_atoms *data;
> +
> +               data = rb_entry(next, struct work_atoms, node);
> +               rb_erase_cached(next, &sched->sorted_atom_root);
> +               free_work_atoms(data);
> +       }
>  out_free_cpus_switch_event:
>         free_cpus_switch_event(sched);
>         return rc;
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCH 8/8] perf test: Add more test cases to sched test

[Ian Rogers]

On Wed, Jul 2, 2025 at 6:49 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
>   $ sudo ./perf test -vv 92
>    92: perf sched tests:
>   --- start ---
>   test child forked, pid 1360101
>   Sched record
>   pid 1360105's current affinity list: 0-3
>   pid 1360105's new affinity list: 0
>   pid 1360107's current affinity list: 0-3
>   pid 1360107's new affinity list: 0
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 4.330 MB /tmp/__perf_test_sched.perf.data.b3319 (12246 samples) ]
>   Sched latency
>   Sched script
>   Sched map
>   Sched timehist
>   Samples of sched_switch event do not have callchains.
>   ---- end(0) ----
>    92: perf sched tests                                                : Ok
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/tests/shell/sched.sh | 39 ++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/tests/shell/sched.sh b/tools/perf/tests/shell/sched.sh
> index c030126d1a0c918d..b9b81eaf856e6555 100755
> --- a/tools/perf/tests/shell/sched.sh
> +++ b/tools/perf/tests/shell/sched.sh
> @@ -56,38 +56,61 @@ cleanup_noploops() {
>    kill "$PID1" "$PID2"
>  }
>
> -test_sched_latency() {
> -  echo "Sched latency"
> +test_sched_record() {
> +  echo "Sched record"
>
>    start_noploops
>
>    perf sched record --no-inherit -o "${perfdata}" sleep 1
> +
> +  cleanup_noploops
> +}
> +
> +test_sched_latency() {
> +  echo "Sched latency"
> +
>    if ! perf sched latency -i "${perfdata}" | grep -q perf-noploop
>    then
>      echo "Sched latency [Failed missing output]"
>      err=1
>    fi
> -
> -  cleanup_noploops
>  }
>
>  test_sched_script() {
>    echo "Sched script"
>
> -  start_noploops
> -
> -  perf sched record --no-inherit -o "${perfdata}" sleep 1
>    if ! perf sched script -i "${perfdata}" | grep -q perf-noploop
>    then
>      echo "Sched script [Failed missing output]"
>      err=1
>    fi
> +}
>
> -  cleanup_noploops
> +test_sched_map() {
> +  echo "Sched map"
> +
> +  if ! perf sched map -i "${perfdata}" | grep -q perf-noploop
> +  then
> +    echo "Sched map [Failed missing output]"
> +    err=1
> +  fi
> +}
> +
> +test_sched_timehist() {
> +  echo "Sched timehist"
> +
> +  if ! perf sched timehist -i "${perfdata}" | grep -q perf-noploop
> +  then
> +    echo "Sched timehist [Failed missing output]"
> +    err=1
> +  fi
>  }
>
> +test_sched_record
>  test_sched_latency
>  test_sched_script
> +test_sched_map
> +test_sched_timehist
>
>  cleanup
>  exit $err
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCHSET 0/8] perf sched: Fix various memory leaks

[Ian Rogers]

On Wed, Jul 2, 2025 at 6:49 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> While running the new perf sched test with ASAN, it fails due to
> memory leaks.  They comes mostly from missing thread__put() but
> sometimes it needs to release other data structures.
>
> Fix those leaks and add more subcommands to the test.

Thanks Namhyung, it may be worth adding Fixes tags for the benefit of backports.

Tested-by: Ian Rogers <irogers@google.com>

Ian




> Thanks,
> Namhyung
>
>
> Namhyung Kim (8):
>   perf sched: Make sure it frees the usage string
>   perf sched: Free thread->priv using priv_destructor
>   perf sched: Fix memory leaks in 'perf sched map'
>   perf sched: Fix thread leaks in 'perf sched timehist'
>   perf sched: Fix memory leaks for evsel->priv in timehist
>   perf sched: Use RC_CHK_EQUAL() to compare pointers
>   perf sched: Fix memory leaks in 'perf sched latency'
>   perf test: Add more test cases to sched test
>
>  tools/perf/builtin-sched.c      | 147 +++++++++++++++++++++++---------
>  tools/perf/tests/shell/sched.sh |  39 +++++++--
>  tools/perf/util/evsel.c         |  11 +++
>  tools/perf/util/evsel.h         |   2 +
>  4 files changed, 153 insertions(+), 46 deletions(-)
>
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

Re: [PATCHSET 0/8] perf sched: Fix various memory leaks

[Namhyung Kim]

On Wed, Jul 02, 2025 at 08:45:18PM -0700, Ian Rogers wrote:
> On Wed, Jul 2, 2025 at 6:49 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hello,
> >
> > While running the new perf sched test with ASAN, it fails due to
> > memory leaks.  They comes mostly from missing thread__put() but
> > sometimes it needs to release other data structures.
> >
> > Fix those leaks and add more subcommands to the test.
> 
> Thanks Namhyung, it may be worth adding Fixes tags for the benefit of backports.

I was too lazy to find the commits. ;-p

> 
> Tested-by: Ian Rogers <irogers@google.com>

Thanks for your review and test!
Namhyung

Re: [PATCHSET 0/8] perf sched: Fix various memory leaks

[Namhyung Kim]

On Wed, 02 Jul 2025 18:49:34 -0700, Namhyung Kim wrote:
> While running the new perf sched test with ASAN, it fails due to
> memory leaks.  They comes mostly from missing thread__put() but
> sometimes it needs to release other data structures.
> 
> Fix those leaks and add more subcommands to the test.
> 
> Thanks,
> Namhyung
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung