From 26353a61b977e57b58dd3555bc0422fea46c5ad6 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Mon, 1 Apr 2013 20:35:17 +0900 Subject: perf hists: Fix an invalid memory free on he->branch_info The branch info was allocated for the whole stack and passed matching hist entry for each level during processing samples. Thus when a hist entry tries to free its branch info like in hists__collapse_insert_entry it'll face following error. *** glibc detected *** perf: munmap_chunk(): invalid pointer: 0x00000000014e9d20 *** ======= Backtrace: ========= /lib64/libc.so.6[0x387d47ae16] perf[0x4923bd] perf(cmd_report+0xd68)[0x432a08] perf[0x41a663] perf(main+0x58f)[0x419eaf] /lib64/libc.so.6(__libc_start_main+0xf5)[0x387d421735] perf[0x419f95] Fix it by allocating and copying branch info for each new hist entry. Signed-off-by: Namhyung Kim Cc: Andi Kleen Cc: David Ahern Cc: Ingo Molnar Cc: Jiri Olsa Cc: Paul Mackerras Cc: Peter Zijlstra Cc: Stephane Eranian Link: http://lkml.kernel.org/r/1364816125-12212-2-git-send-email-namhyung@kernel.org Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/hist.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'tools/perf/util/hist.c') diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index 6b32721f829a..9438d576459d 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -292,6 +292,20 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template) he->ms.map->referenced = true; if (he->branch_info) { + /* + * This branch info is (a part of) allocated from + * machine__resolve_bstack() and will be freed after + * adding new entries. So we need to save a copy. + */ + he->branch_info = malloc(sizeof(*he->branch_info)); + if (he->branch_info == NULL) { + free(he); + return NULL; + } + + memcpy(he->branch_info, template->branch_info, + sizeof(*he->branch_info)); + if (he->branch_info->from.map) he->branch_info->from.map->referenced = true; if (he->branch_info->to.map) -- cgit v1.2.3 From ceb2acbc2c1387c8785b3c98b482f5a2b89447c3 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Mon, 1 Apr 2013 20:35:18 +0900 Subject: perf hists: Free unused mem info of a matched hist entry The mem info is shared between matched entries so one should be freed. Signed-off-by: Namhyung Kim Cc: Andi Kleen Cc: David Ahern Cc: Ingo Molnar Cc: Jiri Olsa Cc: Paul Mackerras Cc: Peter Zijlstra Cc: Stephane Eranian Link: http://lkml.kernel.org/r/1364816125-12212-3-git-send-email-namhyung@kernel.org Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/hist.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'tools/perf/util/hist.c') diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index 9438d576459d..514fc0470e38 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -374,6 +374,12 @@ static struct hist_entry *add_hist_entry(struct hists *hists, if (!cmp) { he_stat__add_period(&he->stat, period, weight); + /* + * This mem info was allocated from machine__resolve_mem + * and will not be used anymore. + */ + free(entry->mem_info); + /* If the map of an existing hist_entry has * become out-of-date due to an exec() or * similar, update it. Otherwise we will -- cgit v1.2.3 From ded19d57a621e92a27a05972949ad3230f84d0b0 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Mon, 1 Apr 2013 20:35:19 +0900 Subject: perf report: Fix alignment of symbol column when -v is given When -v option is given, the symbol sort key prints its address also but it wasn't properly aligned since hists__calc_col_len() misses the additional part. Also it missed 2 spaces for 0x prefix when printing. $ perf report --stdio -v -s sym # Samples: 133 of event 'cycles' # Event count (approx.): 50536717 # # Overhead Symbol # ........ .............................. # 12.20% 0xffffffff81384c50 v [k] intel_idle 7.62% 0xffffffff8170976a v [k] ftrace_caller 7.02% 0x2d986d B [.] 0x00000000002d986d Signed-off-by: Namhyung Kim Cc: Andi Kleen Cc: David Ahern Cc: Ingo Molnar Cc: Jiri Olsa Cc: Paul Mackerras Cc: Peter Zijlstra Cc: Stephane Eranian Link: http://lkml.kernel.org/r/1364816125-12212-4-git-send-email-namhyung@kernel.org Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/hist.c | 26 +++++++++++++++----------- tools/perf/util/sort.c | 2 +- 2 files changed, 16 insertions(+), 12 deletions(-) (limited to 'tools/perf/util/hist.c') diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index 514fc0470e38..72b4eec820c3 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -70,9 +70,17 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h) int symlen; u16 len; - if (h->ms.sym) - hists__new_col_len(hists, HISTC_SYMBOL, h->ms.sym->namelen + 4); - else { + /* + * +4 accounts for '[x] ' priv level info + * +2 accounts for 0x prefix on raw addresses + * +3 accounts for ' y ' symtab origin info + */ + if (h->ms.sym) { + symlen = h->ms.sym->namelen + 4; + if (verbose) + symlen += BITS_PER_LONG / 4 + 2 + 3; + hists__new_col_len(hists, HISTC_SYMBOL, symlen); + } else { symlen = unresolved_col_width + 4 + 2; hists__new_col_len(hists, HISTC_SYMBOL, symlen); hists__set_unres_dso_col_len(hists, HISTC_DSO); @@ -91,12 +99,10 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h) hists__new_col_len(hists, HISTC_PARENT, h->parent->namelen); if (h->branch_info) { - /* - * +4 accounts for '[x] ' priv level info - * +2 account of 0x prefix on raw addresses - */ if (h->branch_info->from.sym) { symlen = (int)h->branch_info->from.sym->namelen + 4; + if (verbose) + symlen += BITS_PER_LONG / 4 + 2 + 3; hists__new_col_len(hists, HISTC_SYMBOL_FROM, symlen); symlen = dso__name_len(h->branch_info->from.map->dso); @@ -109,6 +115,8 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h) if (h->branch_info->to.sym) { symlen = (int)h->branch_info->to.sym->namelen + 4; + if (verbose) + symlen += BITS_PER_LONG / 4 + 2 + 3; hists__new_col_len(hists, HISTC_SYMBOL_TO, symlen); symlen = dso__name_len(h->branch_info->to.map->dso); @@ -121,10 +129,6 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h) } if (h->mem_info) { - /* - * +4 accounts for '[x] ' priv level info - * +2 account of 0x prefix on raw addresses - */ if (h->mem_info->daddr.sym) { symlen = (int)h->mem_info->daddr.sym->namelen + 4 + unresolved_col_width + 2; diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c index 5f52d492590c..16d5e38befe5 100644 --- a/tools/perf/util/sort.c +++ b/tools/perf/util/sort.c @@ -194,7 +194,7 @@ static int _hist_entry__sym_snprintf(struct map *map, struct symbol *sym, if (verbose) { char o = map ? dso__symtab_origin(map->dso) : '!'; ret += repsep_snprintf(bf, size, "%-#*llx %c ", - BITS_PER_LONG / 4, ip, o); + BITS_PER_LONG / 4 + 2, ip, o); } ret += repsep_snprintf(bf + ret, size - ret, "[%c] ", level); -- cgit v1.2.3 From 3a5714f8b58913ded4d9e90abdd30e7e5993f863 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Tue, 14 May 2013 11:09:01 +0900 Subject: perf top: Get rid of *_threaded() functions Those _threaded() functions are needed to make hist tree handling thread-safe, but AFAICS the only thing it does is forcing it to use the intermediate 'collapsed' tree. This can be acheived by setting sort__need_collapse to 1 in cmd_top() so no need to keep those _threaded() variants. Signed-off-by: Namhyung Kim Acked-by: Jiri Olsa Cc: David Ahern Cc: Ingo Molnar Cc: Jiri Olsa Cc: Paul Mackerras Cc: Peter Zijlstra Cc: Stephane Eranian Link: http://lkml.kernel.org/r/1368497347-9628-4-git-send-email-namhyung@kernel.org Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-top.c | 23 +++++++++++++---------- tools/perf/util/hist.c | 44 ++++++-------------------------------------- tools/perf/util/hist.h | 4 ---- 3 files changed, 19 insertions(+), 52 deletions(-) (limited to 'tools/perf/util/hist.c') diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c index 81adcafbac8f..5cd41ec43ce1 100644 --- a/tools/perf/builtin-top.c +++ b/tools/perf/builtin-top.c @@ -284,11 +284,11 @@ static void perf_top__print_sym_table(struct perf_top *top) return; } - hists__collapse_resort_threaded(&top->sym_evsel->hists); - hists__output_resort_threaded(&top->sym_evsel->hists); - hists__decay_entries_threaded(&top->sym_evsel->hists, - top->hide_user_symbols, - top->hide_kernel_symbols); + hists__collapse_resort(&top->sym_evsel->hists); + hists__output_resort(&top->sym_evsel->hists); + hists__decay_entries(&top->sym_evsel->hists, + top->hide_user_symbols, + top->hide_kernel_symbols); hists__output_recalc_col_len(&top->sym_evsel->hists, top->print_entries - printed); putchar('\n'); @@ -549,11 +549,11 @@ static void perf_top__sort_new_samples(void *arg) if (t->evlist->selected != NULL) t->sym_evsel = t->evlist->selected; - hists__collapse_resort_threaded(&t->sym_evsel->hists); - hists__output_resort_threaded(&t->sym_evsel->hists); - hists__decay_entries_threaded(&t->sym_evsel->hists, - t->hide_user_symbols, - t->hide_kernel_symbols); + hists__collapse_resort(&t->sym_evsel->hists); + hists__output_resort(&t->sym_evsel->hists); + hists__decay_entries(&t->sym_evsel->hists, + t->hide_user_symbols, + t->hide_kernel_symbols); } static void *display_thread_tui(void *arg) @@ -1126,6 +1126,9 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused) if (setup_sorting() < 0) usage_with_options(top_usage, options); + /* display thread wants entries to be collapsed in a different tree */ + sort__need_collapse = 1; + if (top.use_stdio) use_browser = 0; else if (top.use_tui) diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index 72b4eec820c3..7e0fa628e9ab 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -240,8 +240,7 @@ static bool hists__decay_entry(struct hists *hists, struct hist_entry *he) return he->stat.period == 0; } -static void __hists__decay_entries(struct hists *hists, bool zap_user, - bool zap_kernel, bool threaded) +void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel) { struct rb_node *next = rb_first(&hists->entries); struct hist_entry *n; @@ -260,7 +259,7 @@ static void __hists__decay_entries(struct hists *hists, bool zap_user, !n->used) { rb_erase(&n->rb_node, &hists->entries); - if (sort__need_collapse || threaded) + if (sort__need_collapse) rb_erase(&n->rb_node_in, &hists->entries_collapsed); hist_entry__free(n); @@ -269,17 +268,6 @@ static void __hists__decay_entries(struct hists *hists, bool zap_user, } } -void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel) -{ - return __hists__decay_entries(hists, zap_user, zap_kernel, false); -} - -void hists__decay_entries_threaded(struct hists *hists, - bool zap_user, bool zap_kernel) -{ - return __hists__decay_entries(hists, zap_user, zap_kernel, true); -} - /* * histogram, sorted on item, collects periods */ @@ -613,13 +601,13 @@ static void hists__apply_filters(struct hists *hists, struct hist_entry *he) hists__filter_entry_by_symbol(hists, he); } -static void __hists__collapse_resort(struct hists *hists, bool threaded) +void hists__collapse_resort(struct hists *hists) { struct rb_root *root; struct rb_node *next; struct hist_entry *n; - if (!sort__need_collapse && !threaded) + if (!sort__need_collapse) return; root = hists__get_rotate_entries_in(hists); @@ -641,16 +629,6 @@ static void __hists__collapse_resort(struct hists *hists, bool threaded) } } -void hists__collapse_resort(struct hists *hists) -{ - return __hists__collapse_resort(hists, false); -} - -void hists__collapse_resort_threaded(struct hists *hists) -{ - return __hists__collapse_resort(hists, true); -} - /* * reverse the map, sort on period. */ @@ -737,7 +715,7 @@ static void __hists__insert_output_entry(struct rb_root *entries, rb_insert_color(&he->rb_node, entries); } -static void __hists__output_resort(struct hists *hists, bool threaded) +void hists__output_resort(struct hists *hists) { struct rb_root *root; struct rb_node *next; @@ -746,7 +724,7 @@ static void __hists__output_resort(struct hists *hists, bool threaded) min_callchain_hits = hists->stats.total_period * (callchain_param.min_percent / 100); - if (sort__need_collapse || threaded) + if (sort__need_collapse) root = &hists->entries_collapsed; else root = hists->entries_in; @@ -767,16 +745,6 @@ static void __hists__output_resort(struct hists *hists, bool threaded) } } -void hists__output_resort(struct hists *hists) -{ - return __hists__output_resort(hists, false); -} - -void hists__output_resort_threaded(struct hists *hists) -{ - return __hists__output_resort(hists, true); -} - static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h, enum hist_filter filter) { diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h index 6be88dc12b9a..bd81d799a1bf 100644 --- a/tools/perf/util/hist.h +++ b/tools/perf/util/hist.h @@ -104,13 +104,9 @@ struct hist_entry *__hists__add_mem_entry(struct hists *self, u64 weight); void hists__output_resort(struct hists *self); -void hists__output_resort_threaded(struct hists *hists); void hists__collapse_resort(struct hists *self); -void hists__collapse_resort_threaded(struct hists *hists); void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel); -void hists__decay_entries_threaded(struct hists *hists, bool zap_user, - bool zap_kernel); void hists__output_recalc_col_len(struct hists *hists, int max_rows); void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h); -- cgit v1.2.3 From 27a0dcb7adb52473dd98d285a46b764b9219d303 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Tue, 14 May 2013 11:09:02 +0900 Subject: perf hists: Move locking to its call-sites It's a preparation patch to eliminate unneeded locking in the perf report path. Signed-off-by: Namhyung Kim Acked-by: Jiri Olsa Cc: David Ahern Cc: Ingo Molnar Cc: Jiri Olsa Cc: Paul Mackerras Cc: Peter Zijlstra Cc: Stephane Eranian Link: http://lkml.kernel.org/r/1368497347-9628-5-git-send-email-namhyung@kernel.org Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-report.c | 26 ++++++++++++++------------ tools/perf/builtin-top.c | 3 +++ tools/perf/util/hist.c | 6 +----- 3 files changed, 18 insertions(+), 17 deletions(-) (limited to 'tools/perf/util/hist.c') diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c index d45bf9b0361d..63febd24e912 100644 --- a/tools/perf/builtin-report.c +++ b/tools/perf/builtin-report.c @@ -297,6 +297,7 @@ static int process_sample_event(struct perf_tool *tool, { struct perf_report *rep = container_of(tool, struct perf_report, tool); struct addr_location al; + int ret; if (perf_event__preprocess_sample(event, machine, &al, sample, rep->annotate_init) < 0) { @@ -311,28 +312,29 @@ static int process_sample_event(struct perf_tool *tool, if (rep->cpu_list && !test_bit(sample->cpu, rep->cpu_bitmap)) return 0; + pthread_mutex_lock(&evsel->hists.lock); + if (sort__mode == SORT_MODE__BRANCH) { - if (perf_report__add_branch_hist_entry(tool, &al, sample, - evsel, machine)) { + ret = perf_report__add_branch_hist_entry(tool, &al, sample, + evsel, machine); + if (ret < 0) pr_debug("problem adding lbr entry, skipping event\n"); - return -1; - } } else if (rep->mem_mode == 1) { - if (perf_report__add_mem_hist_entry(tool, &al, sample, - evsel, machine, event)) { + ret = perf_report__add_mem_hist_entry(tool, &al, sample, + evsel, machine, event); + if (ret < 0) pr_debug("problem adding mem entry, skipping event\n"); - return -1; - } } else { if (al.map != NULL) al.map->dso->hit = 1; - if (perf_evsel__add_hist_entry(evsel, &al, sample, machine)) { + ret = perf_evsel__add_hist_entry(evsel, &al, sample, machine); + if (ret < 0) pr_debug("problem incrementing symbol period, skipping event\n"); - return -1; - } } - return 0; + pthread_mutex_unlock(&evsel->hists.lock); + + return ret; } static int process_read_event(struct perf_tool *tool, diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c index 5cd41ec43ce1..c2c973476479 100644 --- a/tools/perf/builtin-top.c +++ b/tools/perf/builtin-top.c @@ -245,8 +245,11 @@ static struct hist_entry *perf_evsel__add_hist_entry(struct perf_evsel *evsel, { struct hist_entry *he; + pthread_mutex_lock(&evsel->hists.lock); he = __hists__add_entry(&evsel->hists, al, NULL, sample->period, sample->weight); + pthread_mutex_unlock(&evsel->hists.lock); + if (he == NULL) return NULL; diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index 7e0fa628e9ab..b11a6cfdb414 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -347,8 +347,6 @@ static struct hist_entry *add_hist_entry(struct hists *hists, struct hist_entry *he; int cmp; - pthread_mutex_lock(&hists->lock); - p = &hists->entries_in->rb_node; while (*p != NULL) { @@ -394,14 +392,12 @@ static struct hist_entry *add_hist_entry(struct hists *hists, he = hist_entry__new(entry); if (!he) - goto out_unlock; + return NULL; rb_link_node(&he->rb_node_in, parent, p); rb_insert_color(&he->rb_node_in, hists->entries_in); out: hist_entry__add_cpumode_period(he, al->cpumode, period); -out_unlock: - pthread_mutex_unlock(&hists->lock); return he; } -- cgit v1.2.3