From 58c311da9cec97d7a665156a726bd1653384c65c Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Tue, 22 Apr 2014 09:47:25 +0900 Subject: [PATCH 01/11] perf report: Count number of entries separately The hists->nr_entries is counted in multiple places so that they can confuse readers of the code. This is a preparation of later change and do not intend any functional difference. Note that report__collapse_hists() now changed to return nothing since its return value (nr_samples) is only for checking if there's any data in the input file and this can be acheived by checking ->nr_entries. Signed-off-by: Namhyung Kim Link: http://lkml.kernel.org/r/1398327843-31845-2-git-send-email-namhyung@kernel.org Signed-off-by: Jiri Olsa --- tools/perf/builtin-report.c | 42 +++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c index 76e2bb6cf571a..aed52036468da 100644 --- a/tools/perf/builtin-report.c +++ b/tools/perf/builtin-report.c @@ -57,6 +57,7 @@ struct report { const char *cpu_list; const char *symbol_filter_str; float min_percent; + u64 nr_entries; DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS); }; @@ -75,6 +76,17 @@ static int report__config(const char *var, const char *value, void *cb) return perf_default_config(var, value, cb); } +static void report__inc_stats(struct report *rep, struct hist_entry *he) +{ + /* + * The @he is either of a newly created one or an existing one + * merging current sample. We only want to count a new one so + * checking ->nr_events being 1. + */ + if (he->stat.nr_events == 1) + rep->nr_entries++; +} + static int report__add_mem_hist_entry(struct report *rep, struct addr_location *al, struct perf_sample *sample, struct perf_evsel *evsel) { @@ -121,6 +133,8 @@ static int report__add_mem_hist_entry(struct report *rep, struct addr_location * goto out; } + report__inc_stats(rep, he); + evsel->hists.stats.total_period += cost; hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE); if (!he->filtered) @@ -176,6 +190,8 @@ static int report__add_branch_hist_entry(struct report *rep, struct addr_locatio goto out; } + report__inc_stats(rep, he); + evsel->hists.stats.total_period += 1; hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE); if (!he->filtered) @@ -212,6 +228,8 @@ static int report__add_hist_entry(struct report *rep, struct perf_evsel *evsel, if (ui__has_annotation()) err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr); + report__inc_stats(rep, he); + evsel->hists.stats.total_period += sample->period; if (!he->filtered) evsel->hists.stats.nr_non_filtered_samples++; @@ -486,24 +504,12 @@ static int report__browse_hists(struct report *rep) return ret; } -static u64 report__collapse_hists(struct report *rep) +static void report__collapse_hists(struct report *rep) { struct ui_progress prog; struct perf_evsel *pos; - u64 nr_samples = 0; - /* - * Count number of histogram entries to use when showing progress, - * reusing nr_samples variable. - */ - evlist__for_each(rep->session->evlist, pos) - nr_samples += pos->hists.nr_entries; - ui_progress__init(&prog, nr_samples, "Merging related events..."); - /* - * Count total number of samples, will be used to check if this - * session had any. - */ - nr_samples = 0; + ui_progress__init(&prog, rep->nr_entries, "Merging related events..."); evlist__for_each(rep->session->evlist, pos) { struct hists *hists = &pos->hists; @@ -512,7 +518,6 @@ static u64 report__collapse_hists(struct report *rep) hists->symbol_filter_str = rep->symbol_filter_str; hists__collapse_resort(hists, &prog); - nr_samples += hists->stats.nr_events[PERF_RECORD_SAMPLE]; /* Non-group events are considered as leader */ if (symbol_conf.event_group && @@ -525,14 +530,11 @@ static u64 report__collapse_hists(struct report *rep) } ui_progress__finish(); - - return nr_samples; } static int __cmd_report(struct report *rep) { int ret; - u64 nr_samples; struct perf_session *session = rep->session; struct perf_evsel *pos; struct perf_data_file *file = session->file; @@ -572,12 +574,12 @@ static int __cmd_report(struct report *rep) } } - nr_samples = report__collapse_hists(rep); + report__collapse_hists(rep); if (session_done()) return 0; - if (nr_samples == 0) { + if (rep->nr_entries == 0) { ui__error("The %s file has no samples!\n", file->path); return 0; } From 6263835a1b1ad137f3c26a1383c0487a9388d06e Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Thu, 24 Apr 2014 16:21:46 +0900 Subject: [PATCH 02/11] perf hists: Rename hists__inc_stats() The existing hists__inc_nr_entries() is a misnomer as it's not only increasing ->nr_entries but also other stats. So rename it to more general hists__inc_stats(). Signed-off-by: Namhyung Kim Link: http://lkml.kernel.org/r/1398327843-31845-3-git-send-email-namhyung@kernel.org Signed-off-by: Jiri Olsa --- tools/perf/builtin-diff.c | 2 +- tools/perf/util/hist.c | 6 +++--- tools/perf/util/hist.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c index 6ef80f22c1e2f..0e46fa1b5ca0f 100644 --- a/tools/perf/builtin-diff.c +++ b/tools/perf/builtin-diff.c @@ -586,7 +586,7 @@ static void hists__compute_resort(struct hists *hists) next = rb_next(&he->rb_node_in); insert_hist_entry_by_compute(&hists->entries, he, compute); - hists__inc_nr_entries(hists, he); + hists__inc_stats(hists, he); } } diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index 5a892477aa50b..12d6c1bd761d2 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -317,7 +317,7 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template) return he; } -void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h) +void hists__inc_stats(struct hists *hists, struct hist_entry *h) { if (!h->filtered) { hists__calc_col_len(hists, h); @@ -686,7 +686,7 @@ void hists__output_resort(struct hists *hists) next = rb_next(&n->rb_node_in); __hists__insert_output_entry(&hists->entries, n, min_callchain_hits); - hists__inc_nr_entries(hists, n); + hists__inc_stats(hists, n); } } @@ -853,7 +853,7 @@ static struct hist_entry *hists__add_dummy_entry(struct hists *hists, he->hists = hists; rb_link_node(&he->rb_node_in, parent, p); rb_insert_color(&he->rb_node_in, root); - hists__inc_nr_entries(hists, he); + hists__inc_stats(hists, he); he->dummy = true; } out: diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h index 5a0343eb22e2e..51478c94d976d 100644 --- a/tools/perf/util/hist.h +++ b/tools/perf/util/hist.h @@ -116,7 +116,7 @@ void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel); void hists__output_recalc_col_len(struct hists *hists, int max_rows); u64 hists__total_period(struct hists *hists); -void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h); +void hists__inc_stats(struct hists *hists, struct hist_entry *h); void hists__inc_nr_events(struct hists *hists, u32 type); void events_stats__inc(struct events_stats *stats, u32 type); size_t events_stats__fprintf(struct events_stats *stats, FILE *fp); From ae993efc9c6bd109b027d2799a442892067e9230 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Thu, 24 Apr 2014 16:25:19 +0900 Subject: [PATCH 03/11] perf hists: Move column length calculation out of hists__inc_stats() It's not the part of logic of hists__inc_stats() so it'd be better to move it out of the function. Signed-off-by: Namhyung Kim Link: http://lkml.kernel.org/r/1398327843-31845-4-git-send-email-namhyung@kernel.org Signed-off-by: Jiri Olsa --- tools/perf/builtin-diff.c | 3 +++ tools/perf/util/hist.c | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c index 0e46fa1b5ca0f..c9cc771f182a5 100644 --- a/tools/perf/builtin-diff.c +++ b/tools/perf/builtin-diff.c @@ -587,6 +587,9 @@ static void hists__compute_resort(struct hists *hists) insert_hist_entry_by_compute(&hists->entries, he, compute); hists__inc_stats(hists, he); + + if (!he->filtered) + hists__calc_col_len(hists, he); } } diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index 12d6c1bd761d2..f5b388e50265a 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -320,7 +320,6 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template) void hists__inc_stats(struct hists *hists, struct hist_entry *h) { if (!h->filtered) { - hists__calc_col_len(hists, h); hists->nr_non_filtered_entries++; hists->stats.total_non_filtered_period += h->stat.period; } @@ -687,6 +686,9 @@ void hists__output_resort(struct hists *hists) __hists__insert_output_entry(&hists->entries, n, min_callchain_hits); hists__inc_stats(hists, n); + + if (!n->filtered) + hists__calc_col_len(hists, n); } } From 9283ba9bd77a6940ecad8721429131d773c704b8 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Thu, 24 Apr 2014 16:37:26 +0900 Subject: [PATCH 04/11] perf hists: Add a couple of hists stat helper functions Add hists__{reset,inc}_[filter_]stats() functions to cleanup accesses to hist stats (for output). Note that number of samples in the stat is not handled here since it belongs to the input stage. Signed-off-by: Namhyung Kim Link: http://lkml.kernel.org/r/1398327843-31845-5-git-send-email-namhyung@kernel.org Signed-off-by: Jiri Olsa --- tools/perf/builtin-diff.c | 5 +--- tools/perf/util/hist.c | 59 +++++++++++++++++++++++++-------------- tools/perf/util/hist.h | 1 + 3 files changed, 40 insertions(+), 25 deletions(-) diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c index c9cc771f182a5..52d91ac4e6c8d 100644 --- a/tools/perf/builtin-diff.c +++ b/tools/perf/builtin-diff.c @@ -573,10 +573,7 @@ static void hists__compute_resort(struct hists *hists) hists->entries = RB_ROOT; next = rb_first(root); - hists->nr_entries = 0; - hists->nr_non_filtered_entries = 0; - hists->stats.total_period = 0; - hists->stats.total_non_filtered_period = 0; + hists__reset_stats(hists); hists__reset_col_len(hists); while (next != NULL) { diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index f5b388e50265a..b675857883a2a 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -317,16 +317,6 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template) return he; } -void hists__inc_stats(struct hists *hists, struct hist_entry *h) -{ - if (!h->filtered) { - hists->nr_non_filtered_entries++; - hists->stats.total_non_filtered_period += h->stat.period; - } - hists->nr_entries++; - hists->stats.total_period += h->stat.period; -} - static u8 symbol__parent_filter(const struct symbol *parent) { if (symbol_conf.exclude_other && parent == NULL) @@ -632,6 +622,35 @@ static int hist_entry__sort_on_period(struct hist_entry *a, return ret; } +static void hists__reset_filter_stats(struct hists *hists) +{ + hists->nr_non_filtered_entries = 0; + hists->stats.total_non_filtered_period = 0; +} + +void hists__reset_stats(struct hists *hists) +{ + hists->nr_entries = 0; + hists->stats.total_period = 0; + + hists__reset_filter_stats(hists); +} + +static void hists__inc_filter_stats(struct hists *hists, struct hist_entry *h) +{ + hists->nr_non_filtered_entries++; + hists->stats.total_non_filtered_period += h->stat.period; +} + +void hists__inc_stats(struct hists *hists, struct hist_entry *h) +{ + if (!h->filtered) + hists__inc_filter_stats(hists, h); + + hists->nr_entries++; + hists->stats.total_period += h->stat.period; +} + static void __hists__insert_output_entry(struct rb_root *entries, struct hist_entry *he, u64 min_callchain_hits) @@ -675,9 +694,7 @@ void hists__output_resort(struct hists *hists) next = rb_first(root); hists->entries = RB_ROOT; - hists->nr_non_filtered_entries = 0; - hists->stats.total_period = 0; - hists->stats.total_non_filtered_period = 0; + hists__reset_stats(hists); hists__reset_col_len(hists); while (next) { @@ -699,13 +716,13 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h if (h->filtered) return; - ++hists->nr_non_filtered_entries; if (h->ms.unfolded) hists->nr_non_filtered_entries += h->nr_rows; h->row_offset = 0; - hists->stats.total_non_filtered_period += h->stat.period; + hists->stats.nr_non_filtered_samples += h->stat.nr_events; + hists__inc_filter_stats(hists, h); hists__calc_col_len(hists, h); } @@ -726,9 +743,9 @@ void hists__filter_by_dso(struct hists *hists) { struct rb_node *nd; - hists->nr_non_filtered_entries = 0; - hists->stats.total_non_filtered_period = 0; hists->stats.nr_non_filtered_samples = 0; + + hists__reset_filter_stats(hists); hists__reset_col_len(hists); for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) { @@ -760,9 +777,9 @@ void hists__filter_by_thread(struct hists *hists) { struct rb_node *nd; - hists->nr_non_filtered_entries = 0; - hists->stats.total_non_filtered_period = 0; hists->stats.nr_non_filtered_samples = 0; + + hists__reset_filter_stats(hists); hists__reset_col_len(hists); for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) { @@ -792,9 +809,9 @@ void hists__filter_by_symbol(struct hists *hists) { struct rb_node *nd; - hists->nr_non_filtered_entries = 0; - hists->stats.total_non_filtered_period = 0; hists->stats.nr_non_filtered_samples = 0; + + hists__reset_filter_stats(hists); hists__reset_col_len(hists); for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) { diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h index 51478c94d976d..ef1ad7a948c07 100644 --- a/tools/perf/util/hist.h +++ b/tools/perf/util/hist.h @@ -116,6 +116,7 @@ void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel); void hists__output_recalc_col_len(struct hists *hists, int max_rows); u64 hists__total_period(struct hists *hists); +void hists__reset_stats(struct hists *hists); void hists__inc_stats(struct hists *hists, struct hist_entry *h); void hists__inc_nr_events(struct hists *hists, u32 type); void events_stats__inc(struct events_stats *stats, u32 type); From 87e90f43285f4096e9ba5fc18b05c2e04caf3fab Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Thu, 24 Apr 2014 16:44:16 +0900 Subject: [PATCH 05/11] perf hists: Collapse expanded callchains after filter is applied When a filter is applied a hist entry checks whether its callchain was folded and account it to the output stat. But this is rather hacky and only TUI-specific. Simply fold the callchains for the entry looks like a simpler and more generic solution IMHO. Signed-off-by: Namhyung Kim Link: http://lkml.kernel.org/r/1398327843-31845-6-git-send-email-namhyung@kernel.org Signed-off-by: Jiri Olsa --- tools/perf/util/hist.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index b675857883a2a..8d5cfcc3bc631 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -716,8 +716,8 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h if (h->filtered) return; - if (h->ms.unfolded) - hists->nr_non_filtered_entries += h->nr_rows; + /* force fold unfiltered entry for simplicity */ + h->ms.unfolded = false; h->row_offset = 0; hists->stats.nr_non_filtered_samples += h->stat.nr_events; From 820bc81f4cdaac09a8f25040d3a20d86f3da292b Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Tue, 22 Apr 2014 11:44:21 +0900 Subject: [PATCH 06/11] perf tools: Account entry stats when it's added to the output tree Currently, accounting each sample is done in multiple places - once when adding them to the input tree, other when adding them to the output tree. It's not only confusing but also can cause a subtle problem since concurrent processing like in perf top might see the updated stats before adding entries into the output tree - like seeing more (blank) lines at the end and/or slight inaccurate percentage. To fix this, only account the entries when it's moved into the output tree so that they cannot be seen prematurely. There're some exceptional cases here and there - they should be addressed separately with comments. Signed-off-by: Namhyung Kim Link: http://lkml.kernel.org/r/1398327843-31845-7-git-send-email-namhyung@kernel.org Signed-off-by: Jiri Olsa --- tools/perf/builtin-annotate.c | 3 +-- tools/perf/builtin-diff.c | 13 +++++++++---- tools/perf/builtin-report.c | 24 ++++++++++-------------- tools/perf/util/hist.c | 1 - 4 files changed, 20 insertions(+), 21 deletions(-) diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c index 0da603b79b617..d30d2c2e2a7a3 100644 --- a/tools/perf/builtin-annotate.c +++ b/tools/perf/builtin-annotate.c @@ -46,7 +46,7 @@ struct perf_annotate { }; static int perf_evsel__add_sample(struct perf_evsel *evsel, - struct perf_sample *sample, + struct perf_sample *sample __maybe_unused, struct addr_location *al, struct perf_annotate *ann) { @@ -70,7 +70,6 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel, return -ENOMEM; ret = hist_entry__inc_addr_samples(he, evsel->idx, al->addr); - evsel->hists.stats.total_period += sample->period; hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE); return ret; } diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c index 52d91ac4e6c8d..f3b10dcf68380 100644 --- a/tools/perf/builtin-diff.c +++ b/tools/perf/builtin-diff.c @@ -341,11 +341,16 @@ static int diff__process_sample_event(struct perf_tool *tool __maybe_unused, return -1; } - if (al.filtered == 0) { - evsel->hists.stats.total_non_filtered_period += sample->period; - evsel->hists.nr_non_filtered_entries++; - } + /* + * The total_period is updated here before going to the output + * tree since normally only the baseline hists will call + * hists__output_resort() and precompute needs the total + * period in order to sort entries by percentage delta. + */ evsel->hists.stats.total_period += sample->period; + if (!al.filtered) + evsel->hists.stats.total_non_filtered_period += sample->period; + return 0; } diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c index aed52036468da..89c95289fd510 100644 --- a/tools/perf/builtin-report.c +++ b/tools/perf/builtin-report.c @@ -85,6 +85,16 @@ static void report__inc_stats(struct report *rep, struct hist_entry *he) */ if (he->stat.nr_events == 1) rep->nr_entries++; + + /* + * Only counts number of samples at this stage as it's more + * natural to do it here and non-sample events are also + * counted in perf_session_deliver_event(). The dump_trace + * requires this info is ready before going to the output tree. + */ + hists__inc_nr_events(he->hists, PERF_RECORD_SAMPLE); + if (!he->filtered) + he->hists->stats.nr_non_filtered_samples++; } static int report__add_mem_hist_entry(struct report *rep, struct addr_location *al, @@ -135,10 +145,6 @@ static int report__add_mem_hist_entry(struct report *rep, struct addr_location * report__inc_stats(rep, he); - evsel->hists.stats.total_period += cost; - hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE); - if (!he->filtered) - evsel->hists.stats.nr_non_filtered_samples++; err = hist_entry__append_callchain(he, sample); out: return err; @@ -189,13 +195,7 @@ static int report__add_branch_hist_entry(struct report *rep, struct addr_locatio if (err) goto out; } - report__inc_stats(rep, he); - - evsel->hists.stats.total_period += 1; - hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE); - if (!he->filtered) - evsel->hists.stats.nr_non_filtered_samples++; } else goto out; } @@ -230,10 +230,6 @@ static int report__add_hist_entry(struct report *rep, struct perf_evsel *evsel, report__inc_stats(rep, he); - evsel->hists.stats.total_period += sample->period; - if (!he->filtered) - evsel->hists.stats.nr_non_filtered_samples++; - hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE); out: return err; } diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index 8d5cfcc3bc631..6d0d2d75db68e 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -382,7 +382,6 @@ static struct hist_entry *add_hist_entry(struct hists *hists, if (!he) return NULL; - hists->nr_entries++; rb_link_node(&he->rb_node_in, parent, p); rb_insert_color(&he->rb_node_in, hists->entries_in); out: From 3186b6815d49b5e0defbd884223da3778edb59fc Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Tue, 22 Apr 2014 13:44:23 +0900 Subject: [PATCH 07/11] perf hists: Add missing update on filtered stats in hists__decay_entries() When a filter is used for perf top, its hists->nr_non_filtered_entries was not updated after it removed an entry in hists__decay_entries(). Also hists->stats.total_non_filtered_period was missed too. Signed-off-by: Namhyung Kim Link: http://lkml.kernel.org/r/1398327843-31845-8-git-send-email-namhyung@kernel.org Signed-off-by: Jiri Olsa --- tools/perf/util/hist.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index 6d0d2d75db68e..7f0236cea4fe5 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -225,14 +225,18 @@ static void he_stat__decay(struct he_stat *he_stat) static bool hists__decay_entry(struct hists *hists, struct hist_entry *he) { u64 prev_period = he->stat.period; + u64 diff; if (prev_period == 0) return true; he_stat__decay(&he->stat); + diff = prev_period - he->stat.period; + + hists->stats.total_period -= diff; if (!he->filtered) - hists->stats.total_period -= prev_period - he->stat.period; + hists->stats.total_non_filtered_period -= diff; return he->stat.period == 0; } @@ -259,8 +263,11 @@ void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel) if (sort__need_collapse) rb_erase(&n->rb_node_in, &hists->entries_collapsed); - hist_entry__free(n); --hists->nr_entries; + if (!n->filtered) + --hists->nr_non_filtered_entries; + + hist_entry__free(n); } } } From c481f9301183260a78e55fa4d70d977b68c81846 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Tue, 22 Apr 2014 13:56:11 +0900 Subject: [PATCH 08/11] perf ui/tui: Fix off-by-one in hist_browser__update_nr_entries() The nr_entries variable is increased inside the loop in the function but it always count the first entry regardless of it's filtered or not; caused an off-by-one error. It'd become a problem especially there's no entry at all - it'd get a segfault during referencing a NULL pointer. Signed-off-by: Namhyung Kim Link: http://lkml.kernel.org/r/1398327843-31845-9-git-send-email-namhyung@kernel.org Signed-off-by: Jiri Olsa --- tools/perf/ui/browsers/hists.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c index 4d416984c59d1..311226edae128 100644 --- a/tools/perf/ui/browsers/hists.c +++ b/tools/perf/ui/browsers/hists.c @@ -1348,10 +1348,10 @@ static void hist_browser__update_pcnt_entries(struct hist_browser *hb) u64 nr_entries = 0; struct rb_node *nd = rb_first(&hb->hists->entries); - while (nd) { + while ((nd = hists__filter_entries(nd, hb->hists, + hb->min_pcnt)) != NULL) { nr_entries++; - nd = hists__filter_entries(rb_next(nd), hb->hists, - hb->min_pcnt); + nd = rb_next(nd); } hb->nr_pcnt_entries = nr_entries; From 112f761fc0b43def377af889f8cd242df6af9e34 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Tue, 22 Apr 2014 14:05:35 +0900 Subject: [PATCH 09/11] perf ui/tui: Rename hist_browser__update_nr_entries() Rename ->nr_pcnt_entries and hist_browser__update_pcnt_entries() to ->nr_non_filtered_entries and hist_browser__update_nr_entries() since it's now used for filtering as well. Signed-off-by: Namhyung Kim Link: http://lkml.kernel.org/r/1398327843-31845-10-git-send-email-namhyung@kernel.org Signed-off-by: Jiri Olsa --- tools/perf/ui/browsers/hists.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c index 311226edae128..769295bf2c104 100644 --- a/tools/perf/ui/browsers/hists.c +++ b/tools/perf/ui/browsers/hists.c @@ -26,13 +26,14 @@ struct hist_browser { int print_seq; bool show_dso; float min_pcnt; - u64 nr_pcnt_entries; + u64 nr_non_filtered_entries; }; extern void hist_browser__init_hpp(void); static int hists__browser_title(struct hists *hists, char *bf, size_t size, const char *ev_name); +static void hist_browser__update_nr_entries(struct hist_browser *hb); static void hist_browser__refresh_dimensions(struct hist_browser *browser) { @@ -310,8 +311,6 @@ static void ui_browser__warn_lost_events(struct ui_browser *browser) "Or reduce the sampling frequency."); } -static void hist_browser__update_pcnt_entries(struct hist_browser *hb); - static int hist_browser__run(struct hist_browser *browser, const char *ev_name, struct hist_browser_timer *hbt) { @@ -322,7 +321,7 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name, browser->b.entries = &browser->hists->entries; browser->b.nr_entries = browser->hists->nr_entries; if (browser->min_pcnt) - browser->b.nr_entries = browser->nr_pcnt_entries; + browser->b.nr_entries = browser->nr_non_filtered_entries; hist_browser__refresh_dimensions(browser); hists__browser_title(browser->hists, title, sizeof(title), ev_name); @@ -340,8 +339,8 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name, hbt->timer(hbt->arg); if (browser->min_pcnt) { - hist_browser__update_pcnt_entries(browser); - nr_entries = browser->nr_pcnt_entries; + hist_browser__update_nr_entries(browser); + nr_entries = browser->nr_non_filtered_entries; } else { nr_entries = browser->hists->nr_entries; } @@ -1343,7 +1342,7 @@ static int switch_data_file(void) return ret; } -static void hist_browser__update_pcnt_entries(struct hist_browser *hb) +static void hist_browser__update_nr_entries(struct hist_browser *hb) { u64 nr_entries = 0; struct rb_node *nd = rb_first(&hb->hists->entries); @@ -1354,7 +1353,7 @@ static void hist_browser__update_pcnt_entries(struct hist_browser *hb) nd = rb_next(nd); } - hb->nr_pcnt_entries = nr_entries; + hb->nr_non_filtered_entries = nr_entries; } static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events, @@ -1411,7 +1410,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events, if (min_pcnt) { browser->min_pcnt = min_pcnt; - hist_browser__update_pcnt_entries(browser); + hist_browser__update_nr_entries(browser); } fstack = pstack__new(2); From 268397cb2a47ce6e1c0298d9de1762143867f9d3 Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Tue, 22 Apr 2014 14:49:31 +0900 Subject: [PATCH 10/11] perf top/tui: Update nr_entries properly after a filter is applied The hist_browser__reset() is only called right after a filter is applied so it needs to udpate browser->nr_entries properly. We cannot use hists->nr_non_filtered_entreis directly since it's possible that such entries are also filtered out by minimum percentage limit. In addition when a filter is used for perf top, hist browser's nr_entries field was not updated after applying the filter. But it needs to be updated as new samples are coming. Signed-off-by: Namhyung Kim Link: http://lkml.kernel.org/r/1398327843-31845-11-git-send-email-namhyung@kernel.org Signed-off-by: Jiri Olsa --- tools/perf/ui/browsers/hists.c | 20 ++++++++++++++++---- tools/perf/util/hist.h | 6 ++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c index 769295bf2c104..886eee0062e0e 100644 --- a/tools/perf/ui/browsers/hists.c +++ b/tools/perf/ui/browsers/hists.c @@ -35,6 +35,11 @@ static int hists__browser_title(struct hists *hists, char *bf, size_t size, const char *ev_name); static void hist_browser__update_nr_entries(struct hist_browser *hb); +static bool hist_browser__has_filter(struct hist_browser *hb) +{ + return hists__has_filter(hb->hists) || hb->min_pcnt; +} + static void hist_browser__refresh_dimensions(struct hist_browser *browser) { /* 3 == +/- toggle symbol before actual hist_entry rendering */ @@ -44,7 +49,8 @@ static void hist_browser__refresh_dimensions(struct hist_browser *browser) static void hist_browser__reset(struct hist_browser *browser) { - browser->b.nr_entries = browser->hists->nr_entries; + hist_browser__update_nr_entries(browser); + browser->b.nr_entries = browser->nr_non_filtered_entries; hist_browser__refresh_dimensions(browser); ui_browser__reset_index(&browser->b); } @@ -319,9 +325,10 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name, int delay_secs = hbt ? hbt->refresh : 0; browser->b.entries = &browser->hists->entries; - browser->b.nr_entries = browser->hists->nr_entries; - if (browser->min_pcnt) + if (hist_browser__has_filter(browser)) browser->b.nr_entries = browser->nr_non_filtered_entries; + else + browser->b.nr_entries = browser->hists->nr_entries; hist_browser__refresh_dimensions(browser); hists__browser_title(browser->hists, title, sizeof(title), ev_name); @@ -338,7 +345,7 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name, u64 nr_entries; hbt->timer(hbt->arg); - if (browser->min_pcnt) { + if (hist_browser__has_filter(browser)) { hist_browser__update_nr_entries(browser); nr_entries = browser->nr_non_filtered_entries; } else { @@ -1347,6 +1354,11 @@ static void hist_browser__update_nr_entries(struct hist_browser *hb) u64 nr_entries = 0; struct rb_node *nd = rb_first(&hb->hists->entries); + if (hb->min_pcnt == 0) { + hb->nr_non_filtered_entries = hb->hists->nr_non_filtered_entries; + return; + } + while ((nd = hists__filter_entries(nd, hb->hists, hb->min_pcnt)) != NULL) { nr_entries++; diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h index ef1ad7a948c07..38c3e874c164a 100644 --- a/tools/perf/util/hist.h +++ b/tools/perf/util/hist.h @@ -129,6 +129,12 @@ void hists__filter_by_dso(struct hists *hists); void hists__filter_by_thread(struct hists *hists); void hists__filter_by_symbol(struct hists *hists); +static inline bool hists__has_filter(struct hists *hists) +{ + return hists->thread_filter || hists->dso_filter || + hists->symbol_filter_str; +} + u16 hists__col_len(struct hists *hists, enum hist_column col); void hists__set_col_len(struct hists *hists, enum hist_column col, u16 len); bool hists__new_col_len(struct hists *hists, enum hist_column col, u16 len); From c3b789527b236873557f53740ceac47747e0e1cb Mon Sep 17 00:00:00 2001 From: Namhyung Kim Date: Tue, 22 Apr 2014 15:56:17 +0900 Subject: [PATCH 11/11] perf hists/tui: Count callchain rows separately When TUI hist browser expands/collapses callchains it accounted number of callchain nodes into total entries to show. However this code ignores filtering so that it can make the cursor go to out of screen. Thanks to Jiri Olsa for pointing out a bug (and a fix) in the code. Signed-off-by: Namhyung Kim Link: http://lkml.kernel.org/r/1398327843-31845-12-git-send-email-namhyung@kernel.org Signed-off-by: Jiri Olsa --- tools/perf/ui/browsers/hists.c | 63 +++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c index 886eee0062e0e..b0861e3e50a58 100644 --- a/tools/perf/ui/browsers/hists.c +++ b/tools/perf/ui/browsers/hists.c @@ -27,6 +27,7 @@ struct hist_browser { bool show_dso; float min_pcnt; u64 nr_non_filtered_entries; + u64 nr_callchain_rows; }; extern void hist_browser__init_hpp(void); @@ -35,11 +36,27 @@ static int hists__browser_title(struct hists *hists, char *bf, size_t size, const char *ev_name); static void hist_browser__update_nr_entries(struct hist_browser *hb); +static struct rb_node *hists__filter_entries(struct rb_node *nd, + struct hists *hists, + float min_pcnt); + static bool hist_browser__has_filter(struct hist_browser *hb) { return hists__has_filter(hb->hists) || hb->min_pcnt; } +static u32 hist_browser__nr_entries(struct hist_browser *hb) +{ + u32 nr_entries; + + if (hist_browser__has_filter(hb)) + nr_entries = hb->nr_non_filtered_entries; + else + nr_entries = hb->hists->nr_entries; + + return nr_entries + hb->nr_callchain_rows; +} + static void hist_browser__refresh_dimensions(struct hist_browser *browser) { /* 3 == +/- toggle symbol before actual hist_entry rendering */ @@ -49,8 +66,14 @@ static void hist_browser__refresh_dimensions(struct hist_browser *browser) static void hist_browser__reset(struct hist_browser *browser) { + /* + * The hists__remove_entry_filter() already folds non-filtered + * entries so we can assume it has 0 callchain rows. + */ + browser->nr_callchain_rows = 0; + hist_browser__update_nr_entries(browser); - browser->b.nr_entries = browser->nr_non_filtered_entries; + browser->b.nr_entries = hist_browser__nr_entries(browser); hist_browser__refresh_dimensions(browser); ui_browser__reset_index(&browser->b); } @@ -205,14 +228,16 @@ static bool hist_browser__toggle_fold(struct hist_browser *browser) struct hist_entry *he = browser->he_selection; hist_entry__init_have_children(he); - browser->hists->nr_entries -= he->nr_rows; + browser->b.nr_entries -= he->nr_rows; + browser->nr_callchain_rows -= he->nr_rows; if (he->ms.unfolded) he->nr_rows = callchain__count_rows(&he->sorted_chain); else he->nr_rows = 0; - browser->hists->nr_entries += he->nr_rows; - browser->b.nr_entries = browser->hists->nr_entries; + + browser->b.nr_entries += he->nr_rows; + browser->nr_callchain_rows += he->nr_rows; return true; } @@ -287,23 +312,27 @@ static void hist_entry__set_folding(struct hist_entry *he, bool unfold) he->nr_rows = 0; } -static void hists__set_folding(struct hists *hists, bool unfold) +static void +__hist_browser__set_folding(struct hist_browser *browser, bool unfold) { struct rb_node *nd; + struct hists *hists = browser->hists; - hists->nr_entries = 0; - - for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) { + for (nd = rb_first(&hists->entries); + (nd = hists__filter_entries(nd, hists, browser->min_pcnt)) != NULL; + nd = rb_next(nd)) { struct hist_entry *he = rb_entry(nd, struct hist_entry, rb_node); hist_entry__set_folding(he, unfold); - hists->nr_entries += 1 + he->nr_rows; + browser->nr_callchain_rows += he->nr_rows; } } static void hist_browser__set_folding(struct hist_browser *browser, bool unfold) { - hists__set_folding(browser->hists, unfold); - browser->b.nr_entries = browser->hists->nr_entries; + browser->nr_callchain_rows = 0; + __hist_browser__set_folding(browser, unfold); + + browser->b.nr_entries = hist_browser__nr_entries(browser); /* Go to the start, we may be way after valid entries after a collapse */ ui_browser__reset_index(&browser->b); } @@ -325,10 +354,7 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name, int delay_secs = hbt ? hbt->refresh : 0; browser->b.entries = &browser->hists->entries; - if (hist_browser__has_filter(browser)) - browser->b.nr_entries = browser->nr_non_filtered_entries; - else - browser->b.nr_entries = browser->hists->nr_entries; + browser->b.nr_entries = hist_browser__nr_entries(browser); hist_browser__refresh_dimensions(browser); hists__browser_title(browser->hists, title, sizeof(title), ev_name); @@ -345,13 +371,10 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name, u64 nr_entries; hbt->timer(hbt->arg); - if (hist_browser__has_filter(browser)) { + if (hist_browser__has_filter(browser)) hist_browser__update_nr_entries(browser); - nr_entries = browser->nr_non_filtered_entries; - } else { - nr_entries = browser->hists->nr_entries; - } + nr_entries = hist_browser__nr_entries(browser); ui_browser__update_nr_entries(&browser->b, nr_entries); if (browser->hists->stats.nr_lost_warned !=