From bffb5b0c0976aa46aaa961dd19a47c9d6301cfe1 Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Thu, 8 Jun 2023 16:28:17 -0700 Subject: [PATCH] perf map/maps/thread: Changes to reference counting Fix missed reference count gets and puts as detected with leak sanitizer and reference count checking. Signed-off-by: Ian Rogers Cc: Adrian Hunter Cc: Alexander Shishkin Cc: Ali Saidi Cc: Andi Kleen Cc: Athira Rajeev Cc: Brian Robbins Cc: Changbin Du Cc: Dmitrii Dolgov <9erthalion6@gmail.com> Cc: Fangrui Song Cc: German Gomez Cc: Ingo Molnar Cc: Ivan Babrou Cc: James Clark Cc: Jing Zhang Cc: Jiri Olsa Cc: John Garry Cc: K Prateek Nayak Cc: Kan Liang Cc: Leo Yan Cc: Liam Howlett Cc: Mark Rutland Cc: Miguel Ojeda Cc: Mike Leach Cc: Namhyung Kim Cc: Naveen N. Rao Cc: Peter Zijlstra Cc: Ravi Bangoria Cc: Sean Christopherson Cc: Steinar H. Gunderson Cc: Suzuki Poulouse Cc: Wenyu Liu Cc: Will Deacon Cc: Yang Jihong Cc: Ye Xingchen Cc: Yuan Can Cc: coresight@lists.linaro.org Cc: linux-arm-kernel@lists.infradead.org Link: https://lore.kernel.org/r/20230608232823.4027869-21-irogers@google.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/callchain.c | 28 ++++++++++++++++++++++------ tools/perf/util/event.c | 3 +++ tools/perf/util/hist.c | 6 ++++-- tools/perf/util/machine.c | 29 +++++++++++++++++------------ 4 files changed, 46 insertions(+), 20 deletions(-) diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c index b0dafc7..909f62b 100644 --- a/tools/perf/util/callchain.c +++ b/tools/perf/util/callchain.c @@ -590,6 +590,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor) call->ip = cursor_node->ip; call->ms = cursor_node->ms; call->ms.map = map__get(call->ms.map); + call->ms.maps = maps__get(call->ms.maps); call->srcline = cursor_node->srcline; if (cursor_node->branch) { @@ -649,6 +650,7 @@ add_child(struct callchain_node *parent, list_for_each_entry_safe(call, tmp, &new->val, list) { list_del_init(&call->list); map__zput(call->ms.map); + maps__zput(call->ms.maps); free(call); } free(new); @@ -1010,10 +1012,16 @@ merge_chain_branch(struct callchain_cursor *cursor, int err = 0; list_for_each_entry_safe(list, next_list, &src->val, list) { - callchain_cursor_append(cursor, list->ip, &list->ms, - false, NULL, 0, 0, 0, list->srcline); + struct map_symbol ms = { + .maps = maps__get(list->ms.maps), + .map = map__get(list->ms.map), + }; + callchain_cursor_append(cursor, list->ip, &ms, false, NULL, 0, 0, 0, list->srcline); list_del_init(&list->list); + map__zput(ms.map); + maps__zput(ms.maps); map__zput(list->ms.map); + maps__zput(list->ms.maps); free(list); } @@ -1065,9 +1073,11 @@ int callchain_cursor_append(struct callchain_cursor *cursor, } node->ip = ip; + maps__zput(node->ms.maps); map__zput(node->ms.map); node->ms = *ms; - node->ms.map = map__get(node->ms.map); + node->ms.maps = maps__get(ms->maps); + node->ms.map = map__get(ms->map); node->branch = branch; node->nr_loop_iter = nr_loop_iter; node->iter_cycles = iter_cycles; @@ -1114,7 +1124,8 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node * { struct machine *machine = maps__machine(node->ms.maps); - al->maps = node->ms.maps; + maps__put(al->maps); + al->maps = maps__get(node->ms.maps); map__put(al->map); al->map = map__get(node->ms.map); al->sym = node->ms.sym; @@ -1127,7 +1138,7 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node * if (al->map == NULL) goto out; } - if (al->maps == machine__kernel_maps(machine)) { + if (RC_CHK_ACCESS(al->maps) == RC_CHK_ACCESS(machine__kernel_maps(machine))) { if (machine__is_host(machine)) { al->cpumode = PERF_RECORD_MISC_KERNEL; al->level = 'k'; @@ -1460,12 +1471,14 @@ static void free_callchain_node(struct callchain_node *node) list_for_each_entry_safe(list, tmp, &node->parent_val, list) { list_del_init(&list->list); map__zput(list->ms.map); + maps__zput(list->ms.maps); free(list); } list_for_each_entry_safe(list, tmp, &node->val, list) { list_del_init(&list->list); map__zput(list->ms.map); + maps__zput(list->ms.maps); free(list); } @@ -1551,6 +1564,7 @@ out: list_for_each_entry_safe(chain, new, &head, list) { list_del_init(&chain->list); map__zput(chain->ms.map); + maps__zput(chain->ms.maps); free(chain); } return -ENOMEM; @@ -1596,8 +1610,10 @@ void callchain_cursor_reset(struct callchain_cursor *cursor) cursor->nr = 0; cursor->last = &cursor->first; - for (node = cursor->first; node != NULL; node = node->next) + for (node = cursor->first; node != NULL; node = node->next) { map__zput(node->ms.map); + maps__zput(node->ms.maps); + } } void callchain_param_setup(u64 sample_type, const char *arch) diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c index 2fcfba3..3860b0c7 100644 --- a/tools/perf/util/event.c +++ b/tools/perf/util/event.c @@ -711,6 +711,9 @@ int machine__resolve(struct machine *machine, struct addr_location *al, if (thread__is_filtered(thread)) al->filtered |= (1 << HIST_FILTER__THREAD); + thread__put(thread); + thread = NULL; + al->sym = NULL; al->cpu = sample->cpu; al->socket = -1; diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index dfda52d..fb218b3 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -450,6 +450,7 @@ static int hist_entry__init(struct hist_entry *he, memset(&he->stat, 0, sizeof(he->stat)); } + he->ms.maps = maps__get(he->ms.maps); he->ms.map = map__get(he->ms.map); if (he->branch_info) { @@ -497,7 +498,7 @@ static int hist_entry__init(struct hist_entry *he, } INIT_LIST_HEAD(&he->pairs.node); - thread__get(he->thread); + he->thread = thread__get(he->thread); he->hroot_in = RB_ROOT_CACHED; he->hroot_out = RB_ROOT_CACHED; @@ -523,6 +524,7 @@ err_infos: map__put(he->mem_info->daddr.ms.map); } err: + maps__zput(he->ms.maps); map__zput(he->ms.map); zfree(&he->stat_acc); return -ENOMEM; @@ -611,7 +613,6 @@ static struct hist_entry *hists__findnew_entry(struct hists *hists, * keys were used. */ cmp = hist_entry__cmp(he, entry); - if (!cmp) { if (sample_self) { he_stat__add_period(&he->stat, period); @@ -1309,6 +1310,7 @@ void hist_entry__delete(struct hist_entry *he) struct hist_entry_ops *ops = he->ops; thread__zput(he->thread); + maps__zput(he->ms.maps); map__zput(he->ms.map); if (he->branch_info) { diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index 359ef6b..bdad4b8 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -539,7 +539,7 @@ static void machine__update_thread_pid(struct machine *machine, goto out_err; if (thread__maps(th) == thread__maps(leader)) - return; + goto out_put; if (thread__maps(th)) { /* @@ -579,7 +579,7 @@ __threads__get_last_match(struct threads *threads, struct machine *machine, machine__update_thread_pid(machine, th, pid); return thread__get(th); } - + thread__put(threads->last_match); threads->last_match = NULL; } @@ -601,7 +601,8 @@ threads__get_last_match(struct threads *threads, struct machine *machine, static void __threads__set_last_match(struct threads *threads, struct thread *th) { - threads->last_match = th; + thread__put(threads->last_match); + threads->last_match = thread__get(th); } static void @@ -664,7 +665,6 @@ static struct thread *____machine__findnew_thread(struct machine *machine, rb_link_node(&nd->rb_node, parent, p); rb_insert_color_cached(&nd->rb_node, &threads->entries, leftmost); - /* * We have to initialize maps separately after rb tree is updated. * @@ -673,6 +673,7 @@ static struct thread *____machine__findnew_thread(struct machine *machine, * the rb tree. */ if (thread__init_maps(th, machine)) { + pr_err("Thread init failed thread %d\n", pid); rb_erase_cached(&nd->rb_node, &threads->entries); RB_CLEAR_NODE(&nd->rb_node); free(nd); @@ -682,11 +683,10 @@ static struct thread *____machine__findnew_thread(struct machine *machine, /* * It is now in the rbtree, get a ref */ - thread__get(th); threads__set_last_match(threads, th); ++threads->nr; - return th; + return thread__get(th); } struct thread *__machine__findnew_thread(struct machine *machine, pid_t pid, pid_t tid) @@ -2321,7 +2321,7 @@ static int add_callchain_ip(struct thread *thread, struct iterations *iter, u64 branch_from) { - struct map_symbol ms; + struct map_symbol ms = {}; struct addr_location al; int nr_loop_iter = 0, err = 0; u64 iter_cycles = 0; @@ -2395,6 +2395,8 @@ static int add_callchain_ip(struct thread *thread, iter_cycles, branch_from, srcline); out: addr_location__exit(&al); + maps__put(ms.maps); + map__put(ms.map); return err; } @@ -3089,6 +3091,7 @@ static int append_inlines(struct callchain_cursor *cursor, struct map_symbol *ms struct dso *dso; u64 addr; int ret = 1; + struct map_symbol ilist_ms; if (!symbol_conf.inline_name || !map || !sym) return ret; @@ -3105,18 +3108,20 @@ static int append_inlines(struct callchain_cursor *cursor, struct map_symbol *ms inlines__tree_insert(&dso->inlined_nodes, inline_node); } + ilist_ms = (struct map_symbol) { + .maps = maps__get(ms->maps), + .map = map__get(map), + }; list_for_each_entry(ilist, &inline_node->val, list) { - struct map_symbol ilist_ms = { - .maps = ms->maps, - .map = map, - .sym = ilist->symbol, - }; + ilist_ms.sym = ilist->symbol; ret = callchain_cursor_append(cursor, ip, &ilist_ms, false, NULL, 0, 0, 0, ilist->srcline); if (ret != 0) return ret; } + map__put(ilist_ms.map); + maps__put(ilist_ms.maps); return ret; } -- 2.7.4