[PATCHSET 0/7] perf tools: Fixup for the --percentage change (v2)

[PATCHSET 0/7] perf tools: Fixup for the --percentage change (v2)

[Namhyung Kim]

Hello,

This patchset tries to fix bugs in percentage handling which is
recently changed.  The perf top with symbol filter could cause a
segfault (NULL pointer dereference) if the filter found no entry.

In this patchset, I moved accounting of various histogram stats to be
calculated at the time it actually shown to users.  Although I tested
it on my system for a while, it needs more testing since it'll affect
behaviors of many commands/usages.

There're three main fields in the hists->stats to affect the output:
number of samples, number of hist entries and total periods.

Also there're three stages to process samples: at first, samples are
converted to a hist entry and added to the input tree, and then they are
moved to the collapsed tree if needed, and finally they're moved to the
output tree to be shown to user.

The (part of) stats are accounted when samples are added to the input
tree and then reset before moving to the output tree, and re-counted
during insertion to the output tree.

I can see some reason to do it this way but it's basically not necessary
and could make a problem in multi-threaded programs like perf top.

The perf report does all these passes sequentially in a single thread so
it seems no problem.  But perf top uses two threads - one for gathering
samples (in the input tree) and another for (collapsing and) moving them
to the output tree.  Thus accounting stat in parallel can result in an
inaccurate stats and the output.

So I'd like to get rid of the accounting on the input stage as you can
see it just gets dropped before doing output resort.  I originally make
the all three stats are accounted when doing output resort but changed
mind to account number of samples in the input stage and others in the
output stage.  Because it'd make more sense accounting number of events
(sample event) in the input stage (as all other events are also
accounted in the input stage) and it'd make less changes in code.

So yes, it has a same problem of inaccurate number of samples, but its
impact should be smaller than other stats - seeing increasing sample
count (could be slightly inaccurate) without new entries in the browser.


You can get this on the 'perf/percentage-v11' branch (yes, v11 - as
it's continued from my previous series) in my tree

  git://git.kenrel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git


Any comments, review and testing are welcomed.

Thanks,
Namhyung


Namhyung Kim (7):
  perf report: Count number of entries separately
  perf tools: Account entry stats when it's added to the output tree
  perf hists: Add missing update on filtered stats in
    hists__decay_entries()
  perf ui/tui: Fix off-by-one in hist_browser__update_nr_entries()
  perf ui/tui: Rename hist_browser__update_nr_entries()
  perf top/tui: Update nr_entries properly after a filter is applied
  perf hists/tui: Count callchain rows separately

 tools/perf/builtin-annotate.c  |  3 +-
 tools/perf/builtin-diff.c      | 23 ++++++-----
 tools/perf/builtin-report.c    | 58 ++++++++++++---------------
 tools/perf/ui/browsers/hists.c | 86 ++++++++++++++++++++++++++--------------
 tools/perf/util/hist.c         | 89 +++++++++++++++++++++++++++---------------
 tools/perf/util/hist.h         |  9 ++++-
 6 files changed, 162 insertions(+), 106 deletions(-)

-- 
1.9.2

[PATCH 1/7] perf report: Count number of entries separately

[Namhyung Kim]

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 <namhyung@kernel.org>
---
 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 76e2bb6cf571..aed52036468d 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;
 	}
-- 
1.9.2

[PATCH 2/7] perf tools: Account entry stats when it's added to the output tree

[Namhyung Kim]

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 <namhyung@kernel.org>
---
 tools/perf/builtin-annotate.c |  3 +-
 tools/perf/builtin-diff.c     | 23 ++++++++-----
 tools/perf/builtin-report.c   | 18 +++-------
 tools/perf/util/hist.c        | 78 ++++++++++++++++++++++++++-----------------
 tools/perf/util/hist.h        |  3 +-
 5 files changed, 69 insertions(+), 56 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 0da603b79b61..d30d2c2e2a7a 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 6ef80f22c1e2..f3b10dcf6838 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;
 }
 
@@ -573,10 +578,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) {
@@ -586,7 +588,10 @@ 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);
+
+		if (!he->filtered)
+			hists__calc_col_len(hists, he);
 	}
 }
 
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index aed52036468d..4bdcc040b216 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -85,6 +85,10 @@ static void report__inc_stats(struct report *rep, struct hist_entry *he)
 	 */
 	if (he->stat.nr_events == 1)
 		rep->nr_entries++;
+
+	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 +139,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 +189,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 +224,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 5a892477aa50..3ebc3cd09f42 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -317,17 +317,6 @@ 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)
-{
-	if (!h->filtered) {
-		hists__calc_col_len(hists, h);
-		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)
@@ -393,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:
@@ -659,6 +647,35 @@ static void __hists__insert_output_entry(struct rb_root *entries,
 	rb_insert_color(&he->rb_node, entries);
 }
 
+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 *he)
+{
+	hists->nr_non_filtered_entries++;
+	hists->stats.total_non_filtered_period += he->stat.period;
+}
+
+void hists__inc_stats(struct hists *hists, struct hist_entry *he)
+{
+	if (!he->filtered)
+		hists__inc_filter_stats(hists, he);
+
+	hists->nr_entries++;
+	hists->stats.total_period += he->stat.period;
+}
+
 void hists__output_resort(struct hists *hists)
 {
 	struct rb_root *root;
@@ -676,9 +693,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) {
@@ -686,7 +701,10 @@ 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);
+
+		if (!n->filtered)
+			hists__calc_col_len(hists, n);
 	}
 }
 
@@ -697,13 +715,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;
+	/* force fold unfiltered entry for simplicity */
+	h->ms.unfolded = false;
 	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);
 }
 
@@ -724,11 +742,11 @@ 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);
 
+	hists->stats.nr_non_filtered_samples = 0;
+
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 
@@ -758,11 +776,11 @@ 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);
 
+	hists->stats.nr_non_filtered_samples = 0;
+
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 
@@ -790,11 +808,11 @@ 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);
 
+	hists->stats.nr_non_filtered_samples = 0;
+
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 
@@ -853,7 +871,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 5a0343eb22e2..ef1ad7a948c0 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -116,7 +116,8 @@ 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__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);
 size_t events_stats__fprintf(struct events_stats *stats, FILE *fp);
-- 
1.9.2

Re: [PATCH 2/7] perf tools: Account entry stats when it's added to the output tree

[Jiri Olsa]

On Wed, Apr 23, 2014 at 04:00:03PM +0900, Namhyung Kim wrote:
> 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.

sry to be PITA, but need to ask you to split the patch,
otherwise I'd get beaten for that ;-)

I can see several separated changes here:
 * remove stats for input tree (as stated in changelog)
 * hists__inc_nr_entries -> hists__inc_stats rename
 * add hists__reset_stats function
 * move hists__calc_col_len out of hists__inc_nr_entries


> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---

SNIP

>  
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index aed52036468d..4bdcc040b216 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -85,6 +85,10 @@ static void report__inc_stats(struct report *rep, struct hist_entry *he)
>  	 */
>  	if (he->stat.nr_events == 1)
>  		rep->nr_entries++;
> +
> +	hists__inc_nr_events(he->hists, PERF_RECORD_SAMPLE);
> +	if (!he->filtered)
> +		he->hists->stats.nr_non_filtered_samples++;

could you put here some comments as for the diff command
I mean why is this exception in stats counting for input tree

thanks,
jirka

Re: [PATCH 2/7] perf tools: Account entry stats when it's added to the output tree

[Namhyung Kim]

Hi Jiri,

2014-04-23 (수), 15:35 +0200, Jiri Olsa:
> On Wed, Apr 23, 2014 at 04:00:03PM +0900, Namhyung Kim wrote:
> > 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.
> 
> sry to be PITA, but need to ask you to split the patch,
> otherwise I'd get beaten for that ;-)

No problem.  I'll split it in the next spin.

> 
> I can see several separated changes here:
>  * remove stats for input tree (as stated in changelog)
>  * hists__inc_nr_entries -> hists__inc_stats rename
>  * add hists__reset_stats function
>  * move hists__calc_col_len out of hists__inc_nr_entries
> 
> 
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> 
> SNIP
> 
> >  
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index aed52036468d..4bdcc040b216 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -85,6 +85,10 @@ static void report__inc_stats(struct report *rep, struct hist_entry *he)
> >  	 */
> >  	if (he->stat.nr_events == 1)
> >  		rep->nr_entries++;
> > +
> > +	hists__inc_nr_events(he->hists, PERF_RECORD_SAMPLE);
> > +	if (!he->filtered)
> > +		he->hists->stats.nr_non_filtered_samples++;
> 
> could you put here some comments as for the diff command
> I mean why is this exception in stats counting for input tree

Will do as well.

Thanks,
Namhyung

[PATCH 3/7] perf hists: Add missing update on filtered stats in hists__decay_entries()

[Namhyung Kim]

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 <namhyung@kernel.org>
---
 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 3ebc3cd09f42..fb17bf22433b 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);
 		}
 	}
 }
-- 
1.9.2

[PATCH 4/7] perf ui/tui: Fix off-by-one in hist_browser__update_nr_entries()

[Namhyung Kim]

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 <namhyung@kernel.org>
---
 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 4d416984c59d..311226edae12 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;
-- 
1.9.2

[PATCH 5/7] perf ui/tui: Rename hist_browser__update_nr_entries()

[Namhyung Kim]

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 <namhyung@kernel.org>
---
 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 311226edae12..769295bf2c10 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 @@ close_file_and_continue:
 	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);
-- 
1.9.2

[PATCH 6/7] perf top/tui: Update nr_entries properly after a filter is applied

[Namhyung Kim]

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 <namhyung@kernel.org>
---
 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 769295bf2c10..886eee0062e0 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 ef1ad7a948c0..38c3e874c164 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);
-- 
1.9.2

[PATCH 7/7] perf hists/tui: Count callchain rows separately

[Namhyung Kim]

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.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 57 +++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 886eee0062e0..5b81c0d2fec5 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 */
@@ -50,7 +67,7 @@ static void hist_browser__refresh_dimensions(struct hist_browser *browser)
 static void hist_browser__reset(struct hist_browser *browser)
 {
 	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 +222,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 +306,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 +348,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 +365,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 !=
-- 
1.9.2

Re: [PATCH 7/7] perf hists/tui: Count callchain rows separately

[Jiri Olsa]

On Wed, Apr 23, 2014 at 04:00:08PM +0900, Namhyung Kim wrote:
> 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.

SNIP

hi,
I needed following patch, otherwise following test case would segfault

- record with -g
- report - unfold one symbol
- search for nonsense

the reason is that after unfolding the symbol the nr_callchain_rows
will get some number, which will cause the hist_browser__nr_entries
function return (entries != 0) actually (entries == nr_callchain_rows)
even if there's no entry passing the filter

fixing that by unfolding everything after the filter and reseting
the nr_callchain_rows number

feel free to merge this with your change if you agree ;-)

thanks,
jirka


---
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 5b81c0d..1cf5f45 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -312,6 +312,8 @@ __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 	struct rb_node *nd;
 	struct hists *hists = browser->hists;
 
+	browser->nr_callchain_rows = 0;
+
 	for (nd = rb_first(&hists->entries);
 	     (nd = hists__filter_entries(nd, hists, browser->min_pcnt)) != NULL;
 	     nd = rb_next(nd)) {
@@ -323,7 +325,6 @@ __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 
 static void hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 {
-	browser->nr_callchain_rows = 0;
 	__hist_browser__set_folding(browser, unfold);
 
 	browser->b.nr_entries = hist_browser__nr_entries(browser);
@@ -1507,6 +1508,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 					delay_secs * 2) == K_ENTER) {
 				hists->symbol_filter_str = *buf ? buf : NULL;
 				hists__filter_by_symbol(hists);
+				__hist_browser__set_folding(browser, false);
 				hist_browser__reset(browser);
 			}
 			continue;

Re: [PATCH 7/7] perf hists/tui: Count callchain rows separately

[Namhyung Kim]

2014-04-23 (수), 16:29 +0200, Jiri Olsa:
> On Wed, Apr 23, 2014 at 04:00:08PM +0900, Namhyung Kim wrote:
> > 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.
> 
> SNIP
> 
> hi,
> I needed following patch, otherwise following test case would segfault
> 
> - record with -g
> - report - unfold one symbol
> - search for nonsense
> 
> the reason is that after unfolding the symbol the nr_callchain_rows
> will get some number, which will cause the hist_browser__nr_entries
> function return (entries != 0) actually (entries == nr_callchain_rows)
> even if there's no entry passing the filter

Argh, right..  Thanks for spotting this.  I guess other filters (thread,
dso) have same problem.

> 
> fixing that by unfolding everything after the filter and reseting
> the nr_callchain_rows number

Hmm.. I think hists__remove_entry_filter() already folds them so how
about just setting ->nr_callchain_rows to 0 in hist_browser__reset()
instead?

Thanks,
Namhyung

> 
> feel free to merge this with your change if you agree ;-)
> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 5b81c0d..1cf5f45 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -312,6 +312,8 @@ __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
>  	struct rb_node *nd;
>  	struct hists *hists = browser->hists;
>  
> +	browser->nr_callchain_rows = 0;
> +
>  	for (nd = rb_first(&hists->entries);
>  	     (nd = hists__filter_entries(nd, hists, browser->min_pcnt)) != NULL;
>  	     nd = rb_next(nd)) {
> @@ -323,7 +325,6 @@ __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
>  
>  static void hist_browser__set_folding(struct hist_browser *browser, bool unfold)
>  {
> -	browser->nr_callchain_rows = 0;
>  	__hist_browser__set_folding(browser, unfold);
>  
>  	browser->b.nr_entries = hist_browser__nr_entries(browser);
> @@ -1507,6 +1508,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  					delay_secs * 2) == K_ENTER) {
>  				hists->symbol_filter_str = *buf ? buf : NULL;
>  				hists__filter_by_symbol(hists);
> +				__hist_browser__set_folding(browser, false);
>  				hist_browser__reset(browser);
>  			}
>  			continue;

Re: [PATCH 7/7] perf hists/tui: Count callchain rows separately

[Jiri Olsa]

On Wed, Apr 23, 2014 at 11:57:33PM +0900, Namhyung Kim wrote:
> 2014-04-23 (수), 16:29 +0200, Jiri Olsa:
> > On Wed, Apr 23, 2014 at 04:00:08PM +0900, Namhyung Kim wrote:
> > > 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.
> > 
> > SNIP
> > 
> > hi,
> > I needed following patch, otherwise following test case would segfault
> > 
> > - record with -g
> > - report - unfold one symbol
> > - search for nonsense
> > 
> > the reason is that after unfolding the symbol the nr_callchain_rows
> > will get some number, which will cause the hist_browser__nr_entries
> > function return (entries != 0) actually (entries == nr_callchain_rows)
> > even if there's no entry passing the filter
> 
> Argh, right..  Thanks for spotting this.  I guess other filters (thread,
> dso) have same problem.
> 
> > 
> > fixing that by unfolding everything after the filter and reseting
> > the nr_callchain_rows number
> 
> Hmm.. I think hists__remove_entry_filter() already folds them so how
> about just setting ->nr_callchain_rows to 0 in hist_browser__reset()
> instead?

ok, sounds good

thanks,
jirka