[PATCHSET 0/9] perf tools: Fixup for the --percentage change

[PATCHSET 0/9] perf tools: Fixup for the --percentage change

[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.

It's available at 'perf/percentage-v10' branch on my tree:

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

Any comments, review and testing are welcomed.

Thanks,
Namhyung


Namhyung Kim (9):
  perf report: Count number of entries and samples separately
  perf hists: Introduce hists__add_nr_events()
  perf tools: Account entry stats when it's added to the output tree
  perf tools: Introduce hists__inc_dump_events()
  perf hists: Add missing update on nr_non_filtered_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  | 27 ++++++--------
 tools/perf/builtin-report.c    | 51 +++++++++++++-------------
 tools/perf/builtin-top.c       |  4 ---
 tools/perf/ui/browsers/hists.c | 81 ++++++++++++++++++++++++++++--------------
 tools/perf/util/hist.c         | 53 ++++++++++++++++++++-------
 tools/perf/util/hist.h         |  7 ++++
 6 files changed, 137 insertions(+), 86 deletions(-)

-- 
1.9.2

[PATCH 1/9] perf report: Count number of entries and samples separately

[Namhyung Kim]

Those stats are counted 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.

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

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 76e2bb6cf571..375fc504386e 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -57,6 +57,8 @@ struct report {
 	const char		*cpu_list;
 	const char		*symbol_filter_str;
 	float			min_percent;
+	u64			nr_entries;
+	u64			nr_samples;
 	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
 };
 
@@ -121,6 +123,12 @@ static int report__add_mem_hist_entry(struct report *rep, struct addr_location *
 			goto out;
 	}
 
+	rep->nr_samples++;
+	if (he->stat.nr_events == 1) {
+		/* count new entries only */
+		rep->nr_entries++;
+	}
+
 	evsel->hists.stats.total_period += cost;
 	hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE);
 	if (!he->filtered)
@@ -176,6 +184,12 @@ static int report__add_branch_hist_entry(struct report *rep, struct addr_locatio
 					goto out;
 			}
 
+			rep->nr_samples++;
+			if (he->stat.nr_events == 1) {
+				/* count new entries only */
+				rep->nr_entries++;
+			}
+
 			evsel->hists.stats.total_period += 1;
 			hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE);
 			if (!he->filtered)
@@ -212,6 +226,12 @@ 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);
 
+	rep->nr_samples++;
+	if (he->stat.nr_events == 1) {
+		/* count new entries only */
+		rep->nr_entries++;
+	}
+
 	evsel->hists.stats.total_period += sample->period;
 	if (!he->filtered)
 		evsel->hists.stats.nr_non_filtered_samples++;
@@ -490,20 +510,8 @@ static u64 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 +520,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 &&
@@ -526,7 +533,7 @@ static u64 report__collapse_hists(struct report *rep)
 
 	ui_progress__finish();
 
-	return nr_samples;
+	return rep->nr_samples;
 }
 
 static int __cmd_report(struct report *rep)
-- 
1.9.2

[PATCH 2/9] perf hists: Introduce hists__add_nr_events()

[Namhyung Kim]

The hists__add_nr_events() is a helper function intended to be ran
inside of hists__inc_nr_entries().  However this requires other
changes that handled in the next patch.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 5a892477aa50..1c77714f668d 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -317,6 +317,27 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template)
 	return he;
 }
 
+static void __events_stats__add(struct events_stats *stats, u32 type, u32 val)
+{
+	stats->nr_events[0] += val;
+	stats->nr_events[type] += val;
+}
+
+void events_stats__inc(struct events_stats *stats, u32 type)
+{
+	__events_stats__add(stats, type, 1);
+}
+
+void hists__inc_nr_events(struct hists *hists, u32 type)
+{
+	__events_stats__add(&hists->stats, type, 1);
+}
+
+static void hists__add_nr_events(struct hists *hists, u32 type, u32 val)
+{
+	__events_stats__add(&hists->stats, type, val);
+}
+
 void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h)
 {
 	if (!h->filtered) {
@@ -805,17 +826,6 @@ void hists__filter_by_symbol(struct hists *hists)
 	}
 }
 
-void events_stats__inc(struct events_stats *stats, u32 type)
-{
-	++stats->nr_events[0];
-	++stats->nr_events[type];
-}
-
-void hists__inc_nr_events(struct hists *hists, u32 type)
-{
-	events_stats__inc(&hists->stats, type);
-}
-
 static struct hist_entry *hists__add_dummy_entry(struct hists *hists,
 						 struct hist_entry *pair)
 {
-- 
1.9.2

[PATCH 3/9] 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 | 26 ++++++++++----------------
 tools/perf/builtin-report.c   | 13 -------------
 tools/perf/builtin-top.c      |  4 ----
 tools/perf/util/hist.c        |  3 ++-
 4 files changed, 12 insertions(+), 34 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 0da603b79b61..469bd5fcb824 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -46,12 +46,11 @@ 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)
 {
 	struct hist_entry *he;
-	int ret;
 
 	if (ann->sym_hist_filter != NULL &&
 	    (al->sym == NULL ||
@@ -69,10 +68,7 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
 	if (he == NULL)
 		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;
+	return hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
 }
 
 static int process_sample_event(struct perf_tool *tool,
@@ -234,19 +230,17 @@ static int __cmd_annotate(struct perf_annotate *ann)
 	total_nr_samples = 0;
 	evlist__for_each(session->evlist, pos) {
 		struct hists *hists = &pos->hists;
-		u32 nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE];
 
-		if (nr_samples > 0) {
-			total_nr_samples += nr_samples;
-			hists__collapse_resort(hists, NULL);
-			hists__output_resort(hists);
+		hists__collapse_resort(hists, NULL);
+		hists__output_resort(hists);
 
-			if (symbol_conf.event_group &&
-			    !perf_evsel__is_group_leader(pos))
-				continue;
+		if (symbol_conf.event_group &&
+		    !perf_evsel__is_group_leader(pos))
+			continue;
 
-			hists__find_annotations(hists, pos, ann);
-		}
+		hists__find_annotations(hists, pos, ann);
+
+		total_nr_samples += hists->stats.nr_events[PERF_RECORD_SAMPLE];
 	}
 
 	if (total_nr_samples == 0) {
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 375fc504386e..5691f55ddd4d 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -129,10 +129,6 @@ static int report__add_mem_hist_entry(struct report *rep, struct addr_location *
 		rep->nr_entries++;
 	}
 
-	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,11 +185,6 @@ static int report__add_branch_hist_entry(struct report *rep, struct addr_locatio
 				/* count new entries only */
 				rep->nr_entries++;
 			}
-
-			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;
 	}
@@ -232,10 +223,6 @@ static int report__add_hist_entry(struct report *rep, struct perf_evsel *evsel,
 		rep->nr_entries++;
 	}
 
-	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/builtin-top.c b/tools/perf/builtin-top.c
index 37d30460bada..fb87c16b2038 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -252,10 +252,6 @@ static struct hist_entry *perf_evsel__add_hist_entry(struct perf_evsel *evsel,
 	if (he == NULL)
 		return NULL;
 
-	hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE);
-	if (!he->filtered)
-		evsel->hists.stats.nr_non_filtered_samples++;
-
 	return he;
 }
 
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 1c77714f668d..f955ae5a41c5 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -344,9 +344,11 @@ void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h)
 		hists__calc_col_len(hists, h);
 		hists->nr_non_filtered_entries++;
 		hists->stats.total_non_filtered_period += h->stat.period;
+		hists->stats.nr_non_filtered_samples += h->stat.nr_events;
 	}
 	hists->nr_entries++;
 	hists->stats.total_period += h->stat.period;
+	hists__add_nr_events(hists, PERF_RECORD_SAMPLE, h->stat.nr_events);
 }
 
 static u8 symbol__parent_filter(const struct symbol *parent)
@@ -414,7 +416,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:
-- 
1.9.2

[PATCH 4/9] perf tools: Introduce hists__inc_dump_events()

[Namhyung Kim]

Now event stats are accounted at the hists__output_resort(), but if
dump_trace is enabled (via -D option of perf report/annotate), the
perf will not go be there and output will miss the sample events
count.  The hists__inc_dump_events() will take care of it.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-annotate.c |  1 +
 tools/perf/builtin-report.c   |  3 +++
 tools/perf/util/hist.c        | 13 +++++++++++++
 tools/perf/util/hist.h        |  1 +
 4 files changed, 18 insertions(+)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 469bd5fcb824..be89afbacd39 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -68,6 +68,7 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
 	if (he == NULL)
 		return -ENOMEM;
 
+	hists__inc_dump_events(&evsel->hists);
 	return hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
 }
 
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 5691f55ddd4d..04258e131da4 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -124,6 +124,7 @@ static int report__add_mem_hist_entry(struct report *rep, struct addr_location *
 	}
 
 	rep->nr_samples++;
+	hists__inc_dump_events(&evsel->hists);
 	if (he->stat.nr_events == 1) {
 		/* count new entries only */
 		rep->nr_entries++;
@@ -181,6 +182,7 @@ static int report__add_branch_hist_entry(struct report *rep, struct addr_locatio
 			}
 
 			rep->nr_samples++;
+			hists__inc_dump_events(&evsel->hists);
 			if (he->stat.nr_events == 1) {
 				/* count new entries only */
 				rep->nr_entries++;
@@ -218,6 +220,7 @@ static int report__add_hist_entry(struct report *rep, struct perf_evsel *evsel,
 		err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
 
 	rep->nr_samples++;
+	hists__inc_dump_events(&evsel->hists);
 	if (he->stat.nr_events == 1) {
 		/* count new entries only */
 		rep->nr_entries++;
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index f955ae5a41c5..883340d7d43e 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -333,6 +333,19 @@ void hists__inc_nr_events(struct hists *hists, u32 type)
 	__events_stats__add(&hists->stats, type, 1);
 }
 
+void hists__inc_dump_events(struct hists *hists)
+{
+	if (!dump_trace)
+		return;
+
+	/*
+	 * If dump_trace is enabled, perf will exit before accounting
+	 * sample events during hists__output_resort().  Thus it needs to
+	 * be done separately.
+	 */
+	__events_stats__add(&hists->stats, PERF_RECORD_SAMPLE, 1);
+}
+
 static void hists__add_nr_events(struct hists *hists, u32 type, u32 val)
 {
 	__events_stats__add(&hists->stats, type, val);
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 5a0343eb22e2..185bd41be2ce 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -118,6 +118,7 @@ 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_nr_events(struct hists *hists, u32 type);
+void hists__inc_dump_events(struct hists *hists);
 void events_stats__inc(struct events_stats *stats, u32 type);
 size_t events_stats__fprintf(struct events_stats *stats, FILE *fp);
 
-- 
1.9.2

[PATCH 5/9] perf hists: Add missing update on nr_non_filtered_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().

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 883340d7d43e..7d51f599ae98 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -259,8 +259,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 6/9] 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 7/9] 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 8/9] 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 185bd41be2ce..9cfbfee24dc9 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 9/9] 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 | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 886eee0062e0..215f10429dda 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,6 +36,10 @@ 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;
@@ -51,6 +56,7 @@ 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 += browser->nr_callchain_rows;
 	hist_browser__refresh_dimensions(browser);
 	ui_browser__reset_index(&browser->b);
 }
@@ -205,14 +211,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 +295,31 @@ 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);
+
+	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 += browser->nr_callchain_rows;
 	/* Go to the start, we may be way after valid entries after a collapse */
 	ui_browser__reset_index(&browser->b);
 }
@@ -329,6 +345,7 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name,
 		browser->b.nr_entries = browser->nr_non_filtered_entries;
 	else
 		browser->b.nr_entries = browser->hists->nr_entries;
+	browser->b.nr_entries += browser->nr_callchain_rows;
 
 	hist_browser__refresh_dimensions(browser);
 	hists__browser_title(browser->hists, title, sizeof(title), ev_name);
@@ -352,6 +369,7 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name,
 				nr_entries = browser->hists->nr_entries;
 			}
 
+			nr_entries += browser->nr_callchain_rows;
 			ui_browser__update_nr_entries(&browser->b, nr_entries);
 
 			if (browser->hists->stats.nr_lost_warned !=
-- 
1.9.2

Re: [PATCHSET 0/9] perf tools: Fixup for the --percentage change

[Ingo Molnar]

* Namhyung Kim <namhyung@kernel.org> wrote:

> 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.
> 
> It's available at 'perf/percentage-v10' branch on my tree:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> 
> Any comments, review and testing are welcomed.
> 
> Thanks,
> Namhyung
> 
> 
> Namhyung Kim (9):
>   perf report: Count number of entries and samples separately
>   perf hists: Introduce hists__add_nr_events()
>   perf tools: Account entry stats when it's added to the output tree
>   perf tools: Introduce hists__inc_dump_events()
>   perf hists: Add missing update on nr_non_filtered_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  | 27 ++++++--------
>  tools/perf/builtin-report.c    | 51 +++++++++++++-------------
>  tools/perf/builtin-top.c       |  4 ---
>  tools/perf/ui/browsers/hists.c | 81 ++++++++++++++++++++++++++++--------------
>  tools/perf/util/hist.c         | 53 ++++++++++++++++++++-------
>  tools/perf/util/hist.h         |  7 ++++
>  6 files changed, 137 insertions(+), 86 deletions(-)

I gave it some quick testing and after fixing a trivial merge conflict 
in tools/lib/lockdep/Makefile all seems to be working fine.

But while looking at it I remembered one of my old UI complains about 
perf top and report, the hard to read nature of:

   Event count (approx.): 226958779

the values displayed are typically way too large to be easily human 
readable. More importantly, they are also nonsensical! That we have a 
sampling interval and can sum up all the intervals sampled has very 
little meaning to the overwhelming majority of humans looking at the 
data.

And printing that just spams the visual field and confuses people.

People care about the quality and speed of sampling itself, not 
directly the interval of sampling (which will often be variable with 
auto-freq sampling).

So instead of:

  Samples: 42K of event 'cycles', Event count (approx.): 226958779

How about only printing this in 'perf top' and 'perf report':

  Captured 42.1K 'cycles' event samples

Note the extra decimal (which helps monitor smaller changes as well), 
and note the different wording.

Thoughts?

Thanks,

	Ingo

Re: [PATCH 1/9] perf report: Count number of entries and samples separately

[Jiri Olsa]

On Tue, Apr 22, 2014 at 05:49:43PM +0900, Namhyung Kim wrote:
> Those stats are counted 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.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-report.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 76e2bb6cf571..375fc504386e 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -57,6 +57,8 @@ struct report {
>  	const char		*cpu_list;
>  	const char		*symbol_filter_str;
>  	float			min_percent;
> +	u64			nr_entries;
> +	u64			nr_samples;
>  	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
>  };
>  
> @@ -121,6 +123,12 @@ static int report__add_mem_hist_entry(struct report *rep, struct addr_location *
>  			goto out;
>  	}
>  
> +	rep->nr_samples++;
> +	if (he->stat.nr_events == 1) {
> +		/* count new entries only */
> +		rep->nr_entries++;
> +	}
> +
>  	evsel->hists.stats.total_period += cost;
>  	hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE);
>  	if (!he->filtered)
> @@ -176,6 +184,12 @@ static int report__add_branch_hist_entry(struct report *rep, struct addr_locatio
>  					goto out;
>  			}
>  
> +			rep->nr_samples++;
> +			if (he->stat.nr_events == 1) {
> +				/* count new entries only */
> +				rep->nr_entries++;
> +			}
> +
>  			evsel->hists.stats.total_period += 1;
>  			hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE);
>  			if (!he->filtered)
> @@ -212,6 +226,12 @@ 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);
>  
> +	rep->nr_samples++;
> +	if (he->stat.nr_events == 1) {
> +		/* count new entries only */
> +		rep->nr_entries++;
> +	}

smeels like we could use function for this ^^^

also it took me a while to figure out the reason for the condition,
maybe there could be more comment about that

thanks,
jirka

Re: [PATCH 2/9] perf hists: Introduce hists__add_nr_events()

[Jiri Olsa]

On Tue, Apr 22, 2014 at 05:49:44PM +0900, Namhyung Kim wrote:
> The hists__add_nr_events() is a helper function intended to be ran
> inside of hists__inc_nr_entries().  However this requires other
> changes that handled in the next patch.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/hist.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 5a892477aa50..1c77714f668d 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -317,6 +317,27 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template)
>  	return he;
>  }
>  
> +static void __events_stats__add(struct events_stats *stats, u32 type, u32 val)
> +{
> +	stats->nr_events[0] += val;
> +	stats->nr_events[type] += val;
> +}
> +
> +void events_stats__inc(struct events_stats *stats, u32 type)
> +{
> +	__events_stats__add(stats, type, 1);
> +}
> +
> +void hists__inc_nr_events(struct hists *hists, u32 type)
> +{
> +	__events_stats__add(&hists->stats, type, 1);
> +}
> +
> +static void hists__add_nr_events(struct hists *hists, u32 type, u32 val)
> +{
> +	__events_stats__add(&hists->stats, type, val);
> +}

adding static function with no usage breaks build..
we dont want that, eventhough it's fixed in the next commit

thanks,
jirka

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

[Jiri Olsa]

On Tue, Apr 22, 2014 at 05:49:45PM +0900, Namhyung Kim wrote:

SNIP

>  }
>  
>  static int process_sample_event(struct perf_tool *tool,
> @@ -234,19 +230,17 @@ static int __cmd_annotate(struct perf_annotate *ann)
>  	total_nr_samples = 0;
>  	evlist__for_each(session->evlist, pos) {
>  		struct hists *hists = &pos->hists;
> -		u32 nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE];
>  
> -		if (nr_samples > 0) {

so this condition of having some data is handled in the
resort I guess.. hm, it will just iterate 0 times ;-)

> -			total_nr_samples += nr_samples;
> -			hists__collapse_resort(hists, NULL);
> -			hists__output_resort(hists);
> +		hists__collapse_resort(hists, NULL);
> +		hists__output_resort(hists);
>  
> -			if (symbol_conf.event_group &&
> -			    !perf_evsel__is_group_leader(pos))
> -				continue;
> +		if (symbol_conf.event_group &&
> +		    !perf_evsel__is_group_leader(pos))
> +			continue;
>  
> -			hists__find_annotations(hists, pos, ann);
> -		}
> +		hists__find_annotations(hists, pos, ann);
> +
> +		total_nr_samples += hists->stats.nr_events[PERF_RECORD_SAMPLE];
>  	}
>  
>  	if (total_nr_samples == 0) {
>  

SNIP

> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 1c77714f668d..f955ae5a41c5 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -344,9 +344,11 @@ void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h)

I was wondering for a while about the name of this function
since it no longer increments only nr_entries.. but could not
think about any good replacement ;-)

We could also add hists__reset_nr_entries for the zeroing code.


>  		hists__calc_col_len(hists, h);

also having hists__calc_col_len called here seems strange

thanks,
jirka

Re: [PATCH 1/9] perf report: Count number of entries and samples separately

[Jiri Olsa]

On Tue, Apr 22, 2014 at 05:49:43PM +0900, Namhyung Kim wrote:
> Those stats are counted 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.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---

SNIP

>  
>  		/* Non-group events are considered as leader */
>  		if (symbol_conf.event_group &&
> @@ -526,7 +533,7 @@ static u64 report__collapse_hists(struct report *rep)
>  
>  	ui_progress__finish();
>  
> -	return nr_samples;
> +	return rep->nr_samples;

so this ^^^ is the only usage for rep->nr_samples right?

and the output from report__collapse_hists is used
as a bool if we have at least 1 sample:

        nr_samples = report__collapse_hists(rep);

        if (session_done())
                return 0;

        if (nr_samples == 0) {
                ui__error("The %s file has no samples!\n", file->path);
                return 0;
        }

I think report__collapse_hists does not need to return anything
and above code could use (rep->nr_entries > 0) instead, like:

        report__collapse_hists(rep);

        if (session_done())
                return 0;

        if (rep->nr_entries == 0) {
                ui__error("The %s file has no samples!\n", file->path);
                return 0;
        }

and we could get rid of rep->nr_samples

thanks,
jirka

Re: [PATCH 4/9] perf tools: Introduce hists__inc_dump_events()

[Jiri Olsa]

On Tue, Apr 22, 2014 at 05:49:46PM +0900, Namhyung Kim wrote:

SNIP

> index f955ae5a41c5..883340d7d43e 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -333,6 +333,19 @@ void hists__inc_nr_events(struct hists *hists, u32 type)
>  	__events_stats__add(&hists->stats, type, 1);
>  }
>  
> +void hists__inc_dump_events(struct hists *hists)
> +{
> +	if (!dump_trace)
> +		return;
> +
> +	/*
> +	 * If dump_trace is enabled, perf will exit before accounting
> +	 * sample events during hists__output_resort().  Thus it needs to
> +	 * be done separately.
> +	 */
> +	__events_stats__add(&hists->stats, PERF_RECORD_SAMPLE, 1);
> +}

hum, we already clear all the stats before resorting for output so why done
we call hists__inc_nr_entries from add_hist_entry? (at out label)

jirka

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

[Jiri Olsa]

On Tue, Apr 22, 2014 at 05:49:45PM +0900, Namhyung Kim wrote:

SNIP

>  
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 1c77714f668d..f955ae5a41c5 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -344,9 +344,11 @@ void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h)
>  		hists__calc_col_len(hists, h);
>  		hists->nr_non_filtered_entries++;
>  		hists->stats.total_non_filtered_period += h->stat.period;
> +		hists->stats.nr_non_filtered_samples += h->stat.nr_events;
>  	}
>  	hists->nr_entries++;
>  	hists->stats.total_period += h->stat.period;
> +	hists__add_nr_events(hists, PERF_RECORD_SAMPLE, h->stat.nr_events);

while at it.. could hists__remove_entry_filter call hists__inc_nr_entries too?

not sure about those 2 extra lines in hists__remove_entry_filter:

        if (h->ms.unfolded)
                hists->nr_non_filtered_entries += h->nr_rows;
        h->row_offset = 0;

maybe they should be added into hists__inc_nr_entries?


thanks,
jirka

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

[Jiri Olsa]

On Tue, Apr 22, 2014 at 05:49:51PM +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.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 38 ++++++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 886eee0062e0..215f10429dda 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,6 +36,10 @@ 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;
> @@ -51,6 +56,7 @@ 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 += browser->nr_callchain_rows;
>  	hist_browser__refresh_dimensions(browser);
>  	ui_browser__reset_index(&browser->b);
>  }
> @@ -205,14 +211,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;

does browser->b.nr_entries need to be updated in here?

it gets set in hist_browser__reset and hist_browser__run

> +		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;

ditto

> +		browser->nr_callchain_rows += he->nr_rows;
>  
>  		return true;
>  	}
> @@ -287,23 +295,31 @@ 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);
> +
> +	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 += browser->nr_callchain_rows;

ditto,

I think whole condition above could go away, it's the same as in the
hist_browser__run, which is going to be executed after this anyway..

just setup browser->nr_callchain_rows


thanks,
jirka

Re: [PATCHSET 0/9] perf tools: Fixup for the --percentage change

[Namhyung Kim]

Hi Ingo,

On Tue, 22 Apr 2014 11:55:57 +0200, Ingo Molnar wrote:
> I gave it some quick testing and after fixing a trivial merge conflict 
> in tools/lib/lockdep/Makefile all seems to be working fine.

Thanks for testing!

>
> But while looking at it I remembered one of my old UI complains about 
> perf top and report, the hard to read nature of:
>
>    Event count (approx.): 226958779
>
> the values displayed are typically way too large to be easily human 
> readable. More importantly, they are also nonsensical! That we have a 
> sampling interval and can sum up all the intervals sampled has very 
> little meaning to the overwhelming majority of humans looking at the 
> data.
>
> And printing that just spams the visual field and confuses people.
>
> People care about the quality and speed of sampling itself, not 
> directly the interval of sampling (which will often be variable with 
> auto-freq sampling).

You meant 'period' by 'interval', right?

There's --show-total-period option (should be equivalent to -F period
later) in perf report, so there might be people want to see the numbers
IMHO.

>
> So instead of:
>
>   Samples: 42K of event 'cycles', Event count (approx.): 226958779
>
> How about only printing this in 'perf top' and 'perf report':
>
>   Captured 42.1K 'cycles' event samples
>
> Note the extra decimal (which helps monitor smaller changes as well), 
> and note the different wording.
>
> Thoughts?

Well, I'm okay to add the extra decimal, but it seems that it only makes
sense when the unit is 'K'..

And I think it might be worth adding filtered sample count as well if
filtering is enabled something like:

  Captured 13.2K/42.1K 'cycles' event samples


Thanks,
Namhyung

Re: [PATCH 1/9] perf report: Count number of entries and samples separately

[Namhyung Kim]

Hi Jiri,

On Tue, 22 Apr 2014 16:51:03 +0200, Jiri Olsa wrote:
> On Tue, Apr 22, 2014 at 05:49:43PM +0900, Namhyung Kim wrote:
>> Those stats are counted 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.

[SNIP]
>> +	rep->nr_samples++;
>> +	if (he->stat.nr_events == 1) {
>> +		/* count new entries only */
>> +		rep->nr_entries++;
>> +	}
>
> smeels like we could use function for this ^^^
>
> also it took me a while to figure out the reason for the condition,
> maybe there could be more comment about that

Okay, so I changed it like below:

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++;
}

This also eliminated the unneeded nr_samples field.

Thanks,
Namhyung

Re: [PATCH 2/9] perf hists: Introduce hists__add_nr_events()

[Namhyung Kim]

On Tue, 22 Apr 2014 16:52:03 +0200, Jiri Olsa wrote:
> On Tue, Apr 22, 2014 at 05:49:44PM +0900, Namhyung Kim wrote:
>> +static void hists__add_nr_events(struct hists *hists, u32 type, u32 val)
>> +{
>> +	__events_stats__add(&hists->stats, type, val);
>> +}
>
> adding static function with no usage breaks build..
> we dont want that, eventhough it's fixed in the next commit

Oops, my bad.  Will fix!

Thanks,
Namhyung

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

[Namhyung Kim]

On Tue, 22 Apr 2014 16:54:49 +0200, Jiri Olsa wrote:
> On Tue, Apr 22, 2014 at 05:49:45PM +0900, Namhyung Kim wrote:
>
> SNIP
>
>>  }
>>  
>>  static int process_sample_event(struct perf_tool *tool,
>> @@ -234,19 +230,17 @@ static int __cmd_annotate(struct perf_annotate *ann)
>>  	total_nr_samples = 0;
>>  	evlist__for_each(session->evlist, pos) {
>>  		struct hists *hists = &pos->hists;
>> -		u32 nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE];
>>  
>> -		if (nr_samples > 0) {
>
> so this condition of having some data is handled in the
> resort I guess.. hm, it will just iterate 0 times ;-)

Right.  But I decided to keep this logic as is - will explain on other thread.

>
>> -			total_nr_samples += nr_samples;
>> -			hists__collapse_resort(hists, NULL);
>> -			hists__output_resort(hists);
>> +		hists__collapse_resort(hists, NULL);
>> +		hists__output_resort(hists);
>>  
>> -			if (symbol_conf.event_group &&
>> -			    !perf_evsel__is_group_leader(pos))
>> -				continue;
>> +		if (symbol_conf.event_group &&
>> +		    !perf_evsel__is_group_leader(pos))
>> +			continue;
>>  
>> -			hists__find_annotations(hists, pos, ann);
>> -		}
>> +		hists__find_annotations(hists, pos, ann);
>> +
>> +		total_nr_samples += hists->stats.nr_events[PERF_RECORD_SAMPLE];
>>  	}
>>  
>>  	if (total_nr_samples == 0) {
>>  
>
> SNIP
>
>> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
>> index 1c77714f668d..f955ae5a41c5 100644
>> --- a/tools/perf/util/hist.c
>> +++ b/tools/perf/util/hist.c
>> @@ -344,9 +344,11 @@ void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h)
>
> I was wondering for a while about the name of this function
> since it no longer increments only nr_entries.. but could not
> think about any good replacement ;-)
>
> We could also add hists__reset_nr_entries for the zeroing code.

How about hists__inc_stats() and hists__reset_stats()?

>
>
>>  		hists__calc_col_len(hists, h);
>
> also having hists__calc_col_len called here seems strange

Agreed.  I'll move this out of the function.

Thanks,
Namhyung

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

[Namhyung Kim]

On Tue, 22 Apr 2014 19:10:49 +0200, Jiri Olsa wrote:
> On Tue, Apr 22, 2014 at 05:49:45PM +0900, Namhyung Kim wrote:
>
> SNIP
>
>>  
>> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
>> index 1c77714f668d..f955ae5a41c5 100644
>> --- a/tools/perf/util/hist.c
>> +++ b/tools/perf/util/hist.c
>> @@ -344,9 +344,11 @@ void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h)
>>  		hists__calc_col_len(hists, h);
>>  		hists->nr_non_filtered_entries++;
>>  		hists->stats.total_non_filtered_period += h->stat.period;
>> +		hists->stats.nr_non_filtered_samples += h->stat.nr_events;
>>  	}
>>  	hists->nr_entries++;
>>  	hists->stats.total_period += h->stat.period;
>> +	hists__add_nr_events(hists, PERF_RECORD_SAMPLE, h->stat.nr_events);
>
> while at it.. could hists__remove_entry_filter call hists__inc_nr_entries too?

Nope, I (and Arnaldo) want to the default stats invariant - they are not
changed by any filter.  As you can see hists__remove_entry_filter() only
updates non-filtered stats.

>
> not sure about those 2 extra lines in hists__remove_entry_filter:
>
>         if (h->ms.unfolded)
>                 hists->nr_non_filtered_entries += h->nr_rows;
>         h->row_offset = 0;
>
> maybe they should be added into hists__inc_nr_entries?

Hmm.. this seems like a mix of generic code and UI-specific code.  The
->nr_rows, ->row_offset and ->ms.unfolded are only used by TUI browser
code AFAIK and it takes the folding state into account.

I think we should just make it folded to make things simple - and
finally get rid of the UI-specific bits in the generic code.

Thanks,
Namhyung

Re: [PATCH 4/9] perf tools: Introduce hists__inc_dump_events()

[Namhyung Kim]

On Tue, 22 Apr 2014 18:53:55 +0200, Jiri Olsa wrote:
> On Tue, Apr 22, 2014 at 05:49:46PM +0900, Namhyung Kim wrote:
>
> SNIP
>
>> index f955ae5a41c5..883340d7d43e 100644
>> --- a/tools/perf/util/hist.c
>> +++ b/tools/perf/util/hist.c
>> @@ -333,6 +333,19 @@ void hists__inc_nr_events(struct hists *hists, u32 type)
>>  	__events_stats__add(&hists->stats, type, 1);
>>  }
>>  
>> +void hists__inc_dump_events(struct hists *hists)
>> +{
>> +	if (!dump_trace)
>> +		return;
>> +
>> +	/*
>> +	 * If dump_trace is enabled, perf will exit before accounting
>> +	 * sample events during hists__output_resort().  Thus it needs to
>> +	 * be done separately.
>> +	 */
>> +	__events_stats__add(&hists->stats, PERF_RECORD_SAMPLE, 1);
>> +}
>
> hum, we already clear all the stats before resorting for output so why done
> we call hists__inc_nr_entries from add_hist_entry? (at out label)

In short, because it could be seen by perf top's display thread before
the entry actually moves in the output tree.

Let me explain what this series does again.

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.

Thoughts?

Thanks,
Namhyung

Re: [PATCHSET 0/9] perf tools: Fixup for the --percentage change

[Ingo Molnar]

* Namhyung Kim <namhyung@kernel.org> wrote:

> Hi Ingo,
> 
> On Tue, 22 Apr 2014 11:55:57 +0200, Ingo Molnar wrote:
> > I gave it some quick testing and after fixing a trivial merge conflict 
> > in tools/lib/lockdep/Makefile all seems to be working fine.
> 
> Thanks for testing!
> 
> >
> > But while looking at it I remembered one of my old UI complains about 
> > perf top and report, the hard to read nature of:
> >
> >    Event count (approx.): 226958779
> >
> > the values displayed are typically way too large to be easily human 
> > readable. More importantly, they are also nonsensical! That we have a 
> > sampling interval and can sum up all the intervals sampled has very 
> > little meaning to the overwhelming majority of humans looking at the 
> > data.
> >
> > And printing that just spams the visual field and confuses people.
> >
> > People care about the quality and speed of sampling itself, not 
> > directly the interval of sampling (which will often be variable with 
> > auto-freq sampling).
> 
> You meant 'period' by 'interval', right?

Yeah.

> There's --show-total-period option (should be equivalent to -F period
> later) in perf report, so there might be people want to see the numbers
> IMHO.
> 
> >
> > So instead of:
> >
> >   Samples: 42K of event 'cycles', Event count (approx.): 226958779
> >
> > How about only printing this in 'perf top' and 'perf report':
> >
> >   Captured 42.1K 'cycles' event samples
> >
> > Note the extra decimal (which helps monitor smaller changes as well), 
> > and note the different wording.
> >
> > Thoughts?
> 
> Well, I'm okay to add the extra decimal, but it seems that it only makes
> sense when the unit is 'K'..
> 
> And I think it might be worth adding filtered sample count as well if
> filtering is enabled something like:
> 
>   Captured 13.2K/42.1K 'cycles' event samples

Yeah. Maybe make it:

    Filtered 13.2K out of 42.1K 'cycles' event samples

or so.

Thanks,

	Ingo

Re: [PATCHSET 0/9] perf tools: Fixup for the --percentage change

[Namhyung Kim]

Hi Ingo,

On Wed, 23 Apr 2014 08:09:38 +0200, Ingo Molnar wrote:
> * Namhyung Kim <namhyung@kernel.org> wrote:
>> Well, I'm okay to add the extra decimal, but it seems that it only makes
>> sense when the unit is 'K'..
>> 
>> And I think it might be worth adding filtered sample count as well if
>> filtering is enabled something like:
>> 
>>   Captured 13.2K/42.1K 'cycles' event samples
>
> Yeah. Maybe make it:
>
>     Filtered 13.2K out of 42.1K 'cycles' event samples

Looks better to me.

Thanks,
Namhyung