[PATCHSET 0/9] perf tools: Update on filtered entries' percentage output (v7)

[PATCHSET 0/9] perf tools: Update on filtered entries' percentage output (v7)

[Namhyung Kim]

Hello,

I added --percentage option to perf report to control display of
percentage of filtered entries.

 usage: perf report [<options>]

        --percentage <relative|absolute>
                          how to display percentage of filtered entries

"relative" means it's relative to filtered entries only so that the
sum of shown entries will be always 100%.  "absolute" means it retains
original value before and after the filter applied.  In patch 9/9, I
made the "absolute" as default since it makes more sense IMHO.
    
      $ perf report -s comm
      # Overhead       Command
      # ........  ............
      #
          74.19%           cc1
           7.61%           gcc
           6.11%            as
           4.35%            sh
           4.14%          make
           1.13%        fixdep
      ...
    
      $ perf report -s comm -c cc1,gcc --percentage absolute
      # Overhead       Command
      # ........  ............
      #
          74.19%           cc1
           7.61%           gcc
    
      $ perf report -s comm -c cc1,gcc --percentage relative
      # Overhead       Command
      # ........  ............
      #
          90.69%           cc1
           9.31%           gcc
    
Note that it has zero effect if no filter was applied.

 * changes in v7:
  - rename ->nr_filtered_* to ->nr_non_filtered_* (Arnaldo)
  - remove an unneeded alignment change (Arnaldo)

 * changes in v6:
  - fix a bug in --stdio group report
  - reuse __hpp__fmt() function in TUI/GTK
  - add Acked-by from Jiri

 * changes in v5:
  - fix 0 samples in relative percent output (Jiri)
  - factor hists__total_period function (Jiri)
  - share config parsing code with option parser (Jiri)

 * changes in v4:
  - support perf top and perf diff also  (Jiri)
  - add HIST_FILTER__HOST/GUEST  (Jiri)
  - retain both of filtered and total stats  (Arnaldo)
  - add 'F' hotkey on TUI  (Jiri)
  - rename config variable to have "hist." prefix


You can get this on the 'perf/percentage-v7' branch in my tree

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

Any comments are welcome, thanks
Namhyung


Namhyung Kim (9):
  perf tools: Pass evsel to hpp->header/width functions explicitly
  perf tools: Count periods of filtered entries separately
  perf hists: Add support for showing relative percentage
  perf report: Add --percentage option
  perf top: Add --percentage option
  perf diff: Add --percentage option
  perf tools: Add hist.percentage config option
  perf ui/tui: Add 'F' hotkey to toggle percentage output
  perf tools: Show absolute percentage by default

 tools/perf/Documentation/perf-diff.txt   | 21 ++++++++--
 tools/perf/Documentation/perf-report.txt | 24 ++++++++---
 tools/perf/Documentation/perf-top.txt    | 18 +++++++--
 tools/perf/builtin-diff.c                | 39 ++++++++++++------
 tools/perf/builtin-report.c              | 24 +++++++++--
 tools/perf/builtin-top.c                 |  2 +
 tools/perf/ui/browsers/hists.c           | 39 ++++++++++++++----
 tools/perf/ui/gtk/hists.c                | 14 +++----
 tools/perf/ui/hist.c                     | 32 +++++++--------
 tools/perf/ui/stdio/hist.c               |  5 +--
 tools/perf/util/config.c                 |  4 ++
 tools/perf/util/event.c                  | 22 +++++------
 tools/perf/util/hist.c                   | 68 ++++++++++++++++++++++----------
 tools/perf/util/hist.h                   | 25 +++++++++++-
 tools/perf/util/symbol.h                 |  5 ++-
 15 files changed, 242 insertions(+), 100 deletions(-)

-- 
1.7.11.7

[PATCH 1/9] perf tools: Pass evsel to hpp->header/width functions explicitly

[Namhyung Kim]

Those functions need evsel to investigate event group and it's passed
via hpp->ptr.  However as it can be missed easily so it's better to
pass it via an argument IMHO.

Cc: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-diff.c  |  7 ++++---
 tools/perf/ui/gtk/hists.c  |  3 +--
 tools/perf/ui/hist.c       | 24 ++++++++++--------------
 tools/perf/ui/stdio/hist.c |  5 ++---
 tools/perf/util/hist.h     |  6 ++++--
 5 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index a77e31246c00..204fffe22532 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -952,8 +952,8 @@ static int hpp__entry_global(struct perf_hpp_fmt *_fmt, struct perf_hpp *hpp,
 				 dfmt->header_width, buf);
 }
 
-static int hpp__header(struct perf_hpp_fmt *fmt,
-		       struct perf_hpp *hpp)
+static int hpp__header(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
+		       struct perf_evsel *evsel __maybe_unused)
 {
 	struct diff_hpp_fmt *dfmt =
 		container_of(fmt, struct diff_hpp_fmt, fmt);
@@ -963,7 +963,8 @@ static int hpp__header(struct perf_hpp_fmt *fmt,
 }
 
 static int hpp__width(struct perf_hpp_fmt *fmt,
-		      struct perf_hpp *hpp __maybe_unused)
+		      struct perf_hpp *hpp __maybe_unused,
+		      struct perf_evsel *evsel __maybe_unused)
 {
 	struct diff_hpp_fmt *dfmt =
 		container_of(fmt, struct diff_hpp_fmt, fmt);
diff --git a/tools/perf/ui/gtk/hists.c b/tools/perf/ui/gtk/hists.c
index 7912dab6e5fb..e395ef9b0ae0 100644
--- a/tools/perf/ui/gtk/hists.c
+++ b/tools/perf/ui/gtk/hists.c
@@ -165,7 +165,6 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 	struct perf_hpp hpp = {
 		.buf		= s,
 		.size		= sizeof(s),
-		.ptr		= hists_to_evsel(hists),
 	};
 
 	nr_cols = 0;
@@ -192,7 +191,7 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 	col_idx = 0;
 
 	perf_hpp__for_each_format(fmt) {
-		fmt->header(fmt, &hpp);
+		fmt->header(fmt, &hpp, hists_to_evsel(hists));
 
 		gtk_tree_view_insert_column_with_attributes(GTK_TREE_VIEW(view),
 							    -1, ltrim(s),
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index ac39313a3f0c..0f403b83e9d1 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -118,29 +118,27 @@ int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
 
 #define __HPP_HEADER_FN(_type, _str, _min_width, _unit_width) 		\
 static int hpp__header_##_type(struct perf_hpp_fmt *fmt __maybe_unused,	\
-			       struct perf_hpp *hpp)			\
+			       struct perf_hpp *hpp,			\
+			       struct perf_evsel *evsel)		\
 {									\
 	int len = _min_width;						\
 									\
-	if (symbol_conf.event_group) {					\
-		struct perf_evsel *evsel = hpp->ptr;			\
-									\
+	if (symbol_conf.event_group)					\
 		len = max(len, evsel->nr_members * _unit_width);	\
-	}								\
+									\
 	return scnprintf(hpp->buf, hpp->size, "%*s", len, _str);	\
 }
 
 #define __HPP_WIDTH_FN(_type, _min_width, _unit_width) 			\
 static int hpp__width_##_type(struct perf_hpp_fmt *fmt __maybe_unused,	\
-			      struct perf_hpp *hpp __maybe_unused)	\
+			      struct perf_hpp *hpp __maybe_unused,	\
+			      struct perf_evsel *evsel)			\
 {									\
 	int len = _min_width;						\
 									\
-	if (symbol_conf.event_group) {					\
-		struct perf_evsel *evsel = hpp->ptr;			\
-									\
+	if (symbol_conf.event_group)					\
 		len = max(len, evsel->nr_members * _unit_width);	\
-	}								\
+									\
 	return len;							\
 }
 
@@ -329,15 +327,13 @@ unsigned int hists__sort_list_width(struct hists *hists)
 	struct perf_hpp_fmt *fmt;
 	struct sort_entry *se;
 	int i = 0, ret = 0;
-	struct perf_hpp dummy_hpp = {
-		.ptr	= hists_to_evsel(hists),
-	};
+	struct perf_hpp dummy_hpp;
 
 	perf_hpp__for_each_format(fmt) {
 		if (i)
 			ret += 2;
 
-		ret += fmt->width(fmt, &dummy_hpp);
+		ret += fmt->width(fmt, &dummy_hpp, hists_to_evsel(hists));
 	}
 
 	list_for_each_entry(se, &hist_entry__sort_list, list)
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 9bad89228472..d59893edf031 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -379,7 +379,6 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 	struct perf_hpp dummy_hpp = {
 		.buf	= bf,
 		.size	= sizeof(bf),
-		.ptr	= hists_to_evsel(hists),
 	};
 	bool first = true;
 	size_t linesz;
@@ -398,7 +397,7 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 		else
 			first = false;
 
-		fmt->header(fmt, &dummy_hpp);
+		fmt->header(fmt, &dummy_hpp, hists_to_evsel(hists));
 		fprintf(fp, "%s", bf);
 	}
 
@@ -443,7 +442,7 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 		else
 			first = false;
 
-		width = fmt->width(fmt, &dummy_hpp);
+		width = fmt->width(fmt, &dummy_hpp, hists_to_evsel(hists));
 		for (i = 0; i < width; i++)
 			fprintf(fp, ".");
 	}
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 9e1cada45bb8..0c76bf972736 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -132,8 +132,10 @@ struct perf_hpp {
 };
 
 struct perf_hpp_fmt {
-	int (*header)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp);
-	int (*width)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp);
+	int (*header)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
+		      struct perf_evsel *evsel);
+	int (*width)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
+		     struct perf_evsel *evsel);
 	int (*color)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 		     struct hist_entry *he);
 	int (*entry)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
-- 
1.7.11.7

[PATCH 2/9] perf tools: Count periods of filtered entries separately

[Namhyung Kim]

Currently if a sample was filtered by command line option, it just
dropped.  But this affects final output in that the percentage can be
different since the filtered entries were not included to the total.

But user might want to see the original percentages when filter
applied so add new total_filtered_period filed to hists->stats in
order to be controlled by user later.  It doesn't change the current
behavior yet.

Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c |  2 +-
 tools/perf/util/event.c     | 22 ++++++++++------------
 tools/perf/util/hist.c      | 10 ++--------
 tools/perf/util/hist.h      | 10 ++++++++++
 tools/perf/util/symbol.h    |  2 +-
 5 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index d882b6f96411..5f7f1a332186 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -223,7 +223,7 @@ static int process_sample_event(struct perf_tool *tool,
 		return -1;
 	}
 
-	if (al.filtered || (rep->hide_unresolved && al.sym == NULL))
+	if (rep->hide_unresolved && al.sym == NULL)
 		return 0;
 
 	if (rep->cpu_list && !test_bit(sample->cpu, rep->cpu_bitmap))
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 55eebe936513..45859f5995db 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -3,6 +3,7 @@
 #include "debug.h"
 #include "machine.h"
 #include "sort.h"
+#include "hist.h"
 #include "string.h"
 #include "strlist.h"
 #include "thread.h"
@@ -662,7 +663,7 @@ void thread__find_addr_map(struct thread *thread,
 	al->thread = thread;
 	al->addr = addr;
 	al->cpumode = cpumode;
-	al->filtered = false;
+	al->filtered = 0;
 
 	if (machine == NULL) {
 		al->map = NULL;
@@ -688,11 +689,11 @@ void thread__find_addr_map(struct thread *thread,
 		if ((cpumode == PERF_RECORD_MISC_GUEST_USER ||
 			cpumode == PERF_RECORD_MISC_GUEST_KERNEL) &&
 			!perf_guest)
-			al->filtered = true;
+			al->filtered |= (1 << HIST_FILTER__GUEST);
 		if ((cpumode == PERF_RECORD_MISC_USER ||
 			cpumode == PERF_RECORD_MISC_KERNEL) &&
 			!perf_host)
-			al->filtered = true;
+			al->filtered |= (1 << HIST_FILTER__HOST);
 
 		return;
 	}
@@ -749,9 +750,6 @@ int perf_event__preprocess_sample(const union perf_event *event,
 	if (thread == NULL)
 		return -1;
 
-	if (thread__is_filtered(thread))
-		goto out_filtered;
-
 	dump_printf(" ... thread: %s:%d\n", thread__comm_str(thread), thread->tid);
 	/*
 	 * Have we already created the kernel maps for this machine?
@@ -766,6 +764,10 @@ int perf_event__preprocess_sample(const union perf_event *event,
 
 	thread__find_addr_map(thread, machine, cpumode, MAP__FUNCTION,
 			      sample->ip, al);
+
+	if (thread__is_filtered(thread))
+		al->filtered |= (1 << HIST_FILTER__THREAD);
+
 	dump_printf(" ...... dso: %s\n",
 		    al->map ? al->map->dso->long_name :
 			al->level == 'H' ? "[hypervisor]" : "<not found>");
@@ -781,7 +783,7 @@ int perf_event__preprocess_sample(const union perf_event *event,
 			       (dso->short_name != dso->long_name &&
 				strlist__has_entry(symbol_conf.dso_list,
 						   dso->long_name)))))
-			goto out_filtered;
+			al->filtered |= (1 << HIST_FILTER__DSO);
 
 		al->sym = map__find_symbol(al->map, al->addr,
 					   machine->symbol_filter);
@@ -790,11 +792,7 @@ int perf_event__preprocess_sample(const union perf_event *event,
 	if (symbol_conf.sym_list &&
 		(!al->sym || !strlist__has_entry(symbol_conf.sym_list,
 						al->sym->name)))
-		goto out_filtered;
-
-	return 0;
+		al->filtered |= (1 << HIST_FILTER__SYMBOL);
 
-out_filtered:
-	al->filtered = true;
 	return 0;
 }
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 0466efa71140..aa13aa9826d9 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -13,13 +13,6 @@ static bool hists__filter_entry_by_thread(struct hists *hists,
 static bool hists__filter_entry_by_symbol(struct hists *hists,
 					  struct hist_entry *he);
 
-enum hist_filter {
-	HIST_FILTER__DSO,
-	HIST_FILTER__THREAD,
-	HIST_FILTER__PARENT,
-	HIST_FILTER__SYMBOL,
-};
-
 struct callchain_param	callchain_param = {
 	.mode	= CHAIN_GRAPH_REL,
 	.min_percent = 0.5,
@@ -330,6 +323,7 @@ void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h)
 		hists__calc_col_len(hists, h);
 		++hists->nr_entries;
 		hists->stats.total_period += h->stat.period;
+		hists->stats.total_non_filtered_period += h->stat.period;
 	}
 }
 
@@ -429,7 +423,7 @@ struct hist_entry *__hists__add_entry(struct hists *hists,
 			.weight = weight,
 		},
 		.parent = sym_parent,
-		.filtered = symbol__parent_filter(sym_parent),
+		.filtered = symbol__parent_filter(sym_parent) | al->filtered,
 		.hists	= hists,
 		.branch_info = bi,
 		.mem_info = mi,
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 0c76bf972736..a149f1adaae4 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -14,6 +14,15 @@ struct hist_entry;
 struct addr_location;
 struct symbol;
 
+enum hist_filter {
+	HIST_FILTER__DSO,
+	HIST_FILTER__THREAD,
+	HIST_FILTER__PARENT,
+	HIST_FILTER__SYMBOL,
+	HIST_FILTER__GUEST,
+	HIST_FILTER__HOST,
+};
+
 /*
  * The kernel collects the number of events it couldn't send in a stretch and
  * when possible sends this number in a PERF_RECORD_LOST event. The number of
@@ -28,6 +37,7 @@ struct symbol;
  */
 struct events_stats {
 	u64 total_period;
+	u64 total_non_filtered_period;
 	u64 total_lost;
 	u64 total_invalid_chains;
 	u32 nr_events[PERF_RECORD_HEADER_MAX];
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 2553ae04b788..501e4e722e8e 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -186,7 +186,7 @@ struct addr_location {
 	struct symbol *sym;
 	u64	      addr;
 	char	      level;
-	bool	      filtered;
+	u8	      filtered;
 	u8	      cpumode;
 	s32	      cpu;
 };
-- 
1.7.11.7

[PATCH 3/9] perf hists: Add support for showing relative percentage

[Namhyung Kim]

When filtering by thread, dso or symbol on TUI it also update total
period so that the output shows different result than no filter - the
percentage changed to relative to filtered entries only.  Sometimes
this is not desired since users might expect same results with filter.

So new filtered_* fields to hists->stats to count them separately.
They'll be controlled/used by user later.

Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c |  6 ++++++
 tools/perf/util/hist.c      | 19 +++++++++++++++----
 tools/perf/util/hist.h      |  2 ++
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 5f7f1a332186..95580a1e122e 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -121,6 +121,8 @@ static int report__add_mem_hist_entry(struct report *rep, struct addr_location *
 
 	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;
@@ -170,6 +172,8 @@ static int report__add_branch_hist_entry(struct report *rep, struct addr_locatio
 
 			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;
 	}
@@ -201,6 +205,8 @@ static int report__add_hist_entry(struct report *rep, struct perf_evsel *evsel,
 
 	err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
 	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 aa13aa9826d9..42df44178c4a 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -675,8 +675,8 @@ void hists__output_resort(struct hists *hists)
 	next = rb_first(root);
 	hists->entries = RB_ROOT;
 
-	hists->nr_entries = 0;
-	hists->stats.total_period = 0;
+	hists->nr_entries = hists->nr_non_filtered_entries = 0;
+	hists->stats.total_period = hists->stats.total_non_filtered_period = 0;
 	hists__reset_col_len(hists);
 
 	while (next) {
@@ -695,12 +695,17 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 	if (h->filtered)
 		return;
 
-	++hists->nr_entries;
-	if (h->ms.unfolded)
+	hists->nr_entries++;
+	hists->nr_non_filtered_entries++;
+	if (h->ms.unfolded) {
 		hists->nr_entries += h->nr_rows;
+		hists->nr_non_filtered_entries += h->nr_rows;
+	}
 	h->row_offset = 0;
 	hists->stats.total_period += h->stat.period;
+	hists->stats.total_non_filtered_period += h->stat.period;
 	hists->stats.nr_events[PERF_RECORD_SAMPLE] += h->stat.nr_events;
+	hists->stats.nr_non_filtered_samples += h->stat.nr_events;
 
 	hists__calc_col_len(hists, h);
 }
@@ -723,7 +728,9 @@ void hists__filter_by_dso(struct hists *hists)
 	struct rb_node *nd;
 
 	hists->nr_entries = hists->stats.total_period = 0;
+	hists->nr_non_filtered_entries = hists->stats.total_non_filtered_period = 0;
 	hists->stats.nr_events[PERF_RECORD_SAMPLE] = 0;
+	hists->stats.nr_non_filtered_samples = 0;
 	hists__reset_col_len(hists);
 
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
@@ -756,7 +763,9 @@ void hists__filter_by_thread(struct hists *hists)
 	struct rb_node *nd;
 
 	hists->nr_entries = hists->stats.total_period = 0;
+	hists->nr_non_filtered_entries = hists->stats.total_non_filtered_period = 0;
 	hists->stats.nr_events[PERF_RECORD_SAMPLE] = 0;
+	hists->stats.nr_non_filtered_samples = 0;
 	hists__reset_col_len(hists);
 
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
@@ -787,7 +796,9 @@ void hists__filter_by_symbol(struct hists *hists)
 	struct rb_node *nd;
 
 	hists->nr_entries = hists->stats.total_period = 0;
+	hists->nr_non_filtered_entries = hists->stats.total_non_filtered_period = 0;
 	hists->stats.nr_events[PERF_RECORD_SAMPLE] = 0;
+	hists->stats.nr_non_filtered_samples = 0;
 	hists__reset_col_len(hists);
 
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index a149f1adaae4..213551469f36 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -41,6 +41,7 @@ struct events_stats {
 	u64 total_lost;
 	u64 total_invalid_chains;
 	u32 nr_events[PERF_RECORD_HEADER_MAX];
+	u32 nr_non_filtered_samples;
 	u32 nr_lost_warned;
 	u32 nr_unknown_events;
 	u32 nr_invalid_chains;
@@ -84,6 +85,7 @@ struct hists {
 	struct rb_root		entries;
 	struct rb_root		entries_collapsed;
 	u64			nr_entries;
+	u64			nr_non_filtered_entries;
 	const struct thread	*thread_filter;
 	const struct dso	*dso_filter;
 	const char		*uid_filter_str;
-- 
1.7.11.7

[PATCH 4/9] perf report: Add --percentage option

[Namhyung Kim]

The --percentage option is for controlling overhead percentage
displayed.  It can only receive either of "relative" or "absolute".

"relative" means it's relative to filtered entries only so that the
sum of shown entries will be always 100%.  "absolute" means it retains
the original value before and after the filter is applied.

  $ perf report -s comm
  # Overhead       Command
  # ........  ............
  #
      74.19%           cc1
       7.61%           gcc
       6.11%            as
       4.35%            sh
       4.14%          make
       1.13%        fixdep
  ...

  $ perf report -s comm -c cc1,gcc --percentage absolute
  # Overhead       Command
  # ........  ............
  #
      74.19%           cc1
       7.61%           gcc

  $ perf report -s comm -c cc1,gcc --percentage relative
  # Overhead       Command
  # ........  ............
  #
      90.69%           cc1
       9.31%           gcc

Note that it has zero effect if no filter was applied.

Suggested-by: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-report.txt | 24 +++++++++++++++-----
 tools/perf/builtin-report.c              | 30 +++++++++++++++++++++++--
 tools/perf/ui/browsers/hists.c           | 35 ++++++++++++++++++++++-------
 tools/perf/ui/gtk/hists.c                | 11 +++++----
 tools/perf/ui/hist.c                     |  8 +++----
 tools/perf/util/hist.c                   | 38 ++++++++++++++++----------------
 tools/perf/util/hist.h                   |  1 +
 tools/perf/util/symbol.c                 |  1 +
 tools/perf/util/symbol.h                 |  3 ++-
 9 files changed, 105 insertions(+), 46 deletions(-)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index 8eab8a4bdeb8..09af66298564 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -25,10 +25,6 @@ OPTIONS
 --verbose::
         Be more verbose. (show symbol address, etc)
 
--d::
---dsos=::
-	Only consider symbols in these dsos. CSV that understands
-	file://filename entries.
 -n::
 --show-nr-samples::
 	Show the number of samples for each symbol
@@ -42,11 +38,18 @@ OPTIONS
 -c::
 --comms=::
 	Only consider symbols in these comms. CSV that understands
-	file://filename entries.
+	file://filename entries.  This option will affect the percentage of
+	the overhead column.  See --percentage for more info.
+-d::
+--dsos=::
+	Only consider symbols in these dsos. CSV that understands
+	file://filename entries.  This option will affect the percentage of
+	the overhead column.  See --percentage for more info.
 -S::
 --symbols=::
 	Only consider these symbols. CSV that understands
-	file://filename entries.
+	file://filename entries.  This option will affect the percentage of
+	the overhead column.  See --percentage for more info.
 
 --symbol-filter=::
 	Only show symbols that match (partially) with this filter.
@@ -237,6 +240,15 @@ OPTIONS
 	Do not show entries which have an overhead under that percent.
 	(Default: 0).
 
+--percentage::
+	Determine how to display the overhead percentage of filtered entries.
+	Filters can be applied by --comms, --dsos and/or --symbols options and
+	Zoom operations on the TUI (thread, dso, etc).
+
+	"relative" means it's relative to filtered entries only so that the
+	sum of shown entries will be always 100%.  "absolute" means it retains
+	the original value before and after the filter is applied.
+
 --header::
 	Show header information in the perf.data file.  This includes
 	various information like hostname, OS and perf version, cpu/mem
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 95580a1e122e..b8eb8c6431aa 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -335,6 +335,11 @@ static size_t hists__fprintf_nr_sample_events(struct hists *hists, struct report
 	char buf[512];
 	size_t size = sizeof(buf);
 
+	if (symbol_conf.filter_relative) {
+		nr_samples = hists->stats.nr_non_filtered_samples;
+		nr_events = hists->stats.total_non_filtered_period;
+	}
+
 	if (perf_evsel__is_group_event(evsel)) {
 		struct perf_evsel *pos;
 
@@ -342,8 +347,13 @@ static size_t hists__fprintf_nr_sample_events(struct hists *hists, struct report
 		evname = buf;
 
 		for_each_group_member(pos, evsel) {
-			nr_samples += pos->hists.stats.nr_events[PERF_RECORD_SAMPLE];
-			nr_events += pos->hists.stats.total_period;
+			if (symbol_conf.filter_relative) {
+				nr_samples += pos->hists.stats.nr_non_filtered_samples;
+				nr_events += pos->hists.stats.total_non_filtered_period;
+			} else {
+				nr_samples += pos->hists.stats.nr_events[PERF_RECORD_SAMPLE];
+				nr_events += pos->hists.stats.total_period;
+			}
 		}
 	}
 
@@ -699,6 +709,20 @@ parse_percent_limit(const struct option *opt, const char *str,
 	return 0;
 }
 
+static int
+parse_percentage(const struct option *opt __maybe_unused, const char *str,
+		 int unset __maybe_unused)
+{
+	if (!strcmp(str, "relative"))
+		symbol_conf.filter_relative = true;
+	else if (!strcmp(str, "absolute"))
+		symbol_conf.filter_relative = false;
+	else
+		return -1;
+
+	return 0;
+}
+
 int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	struct perf_session *session;
@@ -821,6 +845,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_BOOLEAN(0, "mem-mode", &report.mem_mode, "mem access profile"),
 	OPT_CALLBACK(0, "percent-limit", &report, "percent",
 		     "Don't show entries under that percent", parse_percent_limit),
+	OPT_CALLBACK(0, "percentage", NULL, "relative|absolute",
+		     "how to display percentage of filtered entries", parse_percentage),
 	OPT_END()
 	};
 	struct perf_data_file file = {
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 7ec871af3f6f..7ad11477a0f5 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -769,12 +769,15 @@ static unsigned int hist_browser__refresh(struct ui_browser *browser)
 
 	for (nd = browser->top; nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-		float percent = h->stat.period * 100.0 /
-					hb->hists->stats.total_period;
+		u64 total = hists__total_period(h->hists);
+		float percent = 0.0;
 
 		if (h->filtered)
 			continue;
 
+		if (total)
+			percent = h->stat.period * 100.0 / total;
+
 		if (percent < hb->min_pcnt)
 			continue;
 
@@ -792,8 +795,11 @@ static struct rb_node *hists__filter_entries(struct rb_node *nd,
 {
 	while (nd != NULL) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-		float percent = h->stat.period * 100.0 /
-					hists->stats.total_period;
+		u64 total = hists__total_period(hists);
+		float percent = 0.0;
+
+		if (total)
+			percent = h->stat.period * 100.0 / total;
 
 		if (percent < min_pcnt)
 			return NULL;
@@ -813,8 +819,11 @@ static struct rb_node *hists__filter_prev_entries(struct rb_node *nd,
 {
 	while (nd != NULL) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-		float percent = h->stat.period * 100.0 /
-					hists->stats.total_period;
+		u64 total = hists__total_period(hists);
+		float percent = 0.0;
+
+		if (total)
+			percent = h->stat.period * 100.0 / total;
 
 		if (!h->filtered && percent >= min_pcnt)
 			return nd;
@@ -1189,6 +1198,11 @@ static int hists__browser_title(struct hists *hists, char *bf, size_t size,
 	char buf[512];
 	size_t buflen = sizeof(buf);
 
+	if (symbol_conf.filter_relative) {
+		nr_samples = hists->stats.nr_non_filtered_samples;
+		nr_events = hists->stats.total_non_filtered_period;
+	}
+
 	if (perf_evsel__is_group_event(evsel)) {
 		struct perf_evsel *pos;
 
@@ -1196,8 +1210,13 @@ static int hists__browser_title(struct hists *hists, char *bf, size_t size,
 		ev_name = buf;
 
 		for_each_group_member(pos, evsel) {
-			nr_samples += pos->hists.stats.nr_events[PERF_RECORD_SAMPLE];
-			nr_events += pos->hists.stats.total_period;
+			if (symbol_conf.filter_relative) {
+				nr_samples += pos->hists.stats.nr_non_filtered_samples;
+				nr_events += pos->hists.stats.total_non_filtered_period;
+			} else {
+				nr_samples += pos->hists.stats.nr_events[PERF_RECORD_SAMPLE];
+				nr_events += pos->hists.stats.total_period;
+			}
 		}
 	}
 
diff --git a/tools/perf/ui/gtk/hists.c b/tools/perf/ui/gtk/hists.c
index e395ef9b0ae0..91f10f3f6dd1 100644
--- a/tools/perf/ui/gtk/hists.c
+++ b/tools/perf/ui/gtk/hists.c
@@ -228,12 +228,15 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
 		GtkTreeIter iter;
-		float percent = h->stat.period * 100.0 /
-					hists->stats.total_period;
+		u64 total = hists__total_period(h->hists);
+		float percent = 0.0;
 
 		if (h->filtered)
 			continue;
 
+		if (total)
+			percent = h->stat.period * 100.0 / total;
+
 		if (percent < min_pcnt)
 			continue;
 
@@ -261,12 +264,8 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 		}
 
 		if (symbol_conf.use_callchain && sort__has_sym) {
-			u64 total;
-
 			if (callchain_param.mode == CHAIN_GRAPH_REL)
 				total = h->stat.period;
-			else
-				total = hists->stats.total_period;
 
 			perf_gtk__add_callchain(&h->sorted_chain, store, &iter,
 						sym_col, total);
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 0f403b83e9d1..0912805c08f4 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -32,10 +32,10 @@ int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
 
 	if (fmt_percent) {
 		double percent = 0.0;
+		u64 total = hists__total_period(hists);
 
-		if (hists->stats.total_period)
-			percent = 100.0 * get_field(he) /
-				  hists->stats.total_period;
+		if (total)
+			percent = 100.0 * get_field(he) / total;
 
 		ret += hpp__call_print_fn(hpp, print_fn, fmt, percent);
 	} else
@@ -50,7 +50,7 @@ int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
 
 		list_for_each_entry(pair, &he->pairs.head, pairs.node) {
 			u64 period = get_field(pair);
-			u64 total = pair->hists->stats.total_period;
+			u64 total = hists__total_period(pair->hists);
 
 			if (!total)
 				continue;
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 42df44178c4a..71078de683fe 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -321,10 +321,11 @@ void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h)
 {
 	if (!h->filtered) {
 		hists__calc_col_len(hists, h);
-		++hists->nr_entries;
-		hists->stats.total_period += h->stat.period;
+		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)
@@ -675,8 +676,9 @@ void hists__output_resort(struct hists *hists)
 	next = rb_first(root);
 	hists->entries = RB_ROOT;
 
-	hists->nr_entries = hists->nr_non_filtered_entries = 0;
-	hists->stats.total_period = hists->stats.total_non_filtered_period = 0;
+	hists->nr_non_filtered_entries = 0;
+	hists->stats.total_period = 0;
+	hists->stats.total_non_filtered_period = 0;
 	hists__reset_col_len(hists);
 
 	while (next) {
@@ -695,16 +697,11 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 	if (h->filtered)
 		return;
 
-	hists->nr_entries++;
 	hists->nr_non_filtered_entries++;
-	if (h->ms.unfolded) {
-		hists->nr_entries += h->nr_rows;
+	if (h->ms.unfolded)
 		hists->nr_non_filtered_entries += h->nr_rows;
-	}
 	h->row_offset = 0;
-	hists->stats.total_period += h->stat.period;
 	hists->stats.total_non_filtered_period += h->stat.period;
-	hists->stats.nr_events[PERF_RECORD_SAMPLE] += h->stat.nr_events;
 	hists->stats.nr_non_filtered_samples += h->stat.nr_events;
 
 	hists__calc_col_len(hists, h);
@@ -727,9 +724,8 @@ void hists__filter_by_dso(struct hists *hists)
 {
 	struct rb_node *nd;
 
-	hists->nr_entries = hists->stats.total_period = 0;
-	hists->nr_non_filtered_entries = hists->stats.total_non_filtered_period = 0;
-	hists->stats.nr_events[PERF_RECORD_SAMPLE] = 0;
+	hists->nr_non_filtered_entries = 0;
+	hists->stats.total_non_filtered_period = 0;
 	hists->stats.nr_non_filtered_samples = 0;
 	hists__reset_col_len(hists);
 
@@ -762,9 +758,8 @@ void hists__filter_by_thread(struct hists *hists)
 {
 	struct rb_node *nd;
 
-	hists->nr_entries = hists->stats.total_period = 0;
-	hists->nr_non_filtered_entries = hists->stats.total_non_filtered_period = 0;
-	hists->stats.nr_events[PERF_RECORD_SAMPLE] = 0;
+	hists->nr_non_filtered_entries = 0;
+	hists->stats.total_non_filtered_period = 0;
 	hists->stats.nr_non_filtered_samples = 0;
 	hists__reset_col_len(hists);
 
@@ -795,9 +790,8 @@ void hists__filter_by_symbol(struct hists *hists)
 {
 	struct rb_node *nd;
 
-	hists->nr_entries = hists->stats.total_period = 0;
-	hists->nr_non_filtered_entries = hists->stats.total_non_filtered_period = 0;
-	hists->stats.nr_events[PERF_RECORD_SAMPLE] = 0;
+	hists->nr_non_filtered_entries = 0;
+	hists->stats.total_non_filtered_period = 0;
 	hists->stats.nr_non_filtered_samples = 0;
 	hists__reset_col_len(hists);
 
@@ -943,3 +937,9 @@ int hists__link(struct hists *leader, struct hists *other)
 
 	return 0;
 }
+
+u64 hists__total_period(struct hists *hists)
+{
+	return symbol_conf.filter_relative ? hists->stats.total_non_filtered_period :
+		hists->stats.total_period;
+}
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 213551469f36..3191496bd3b7 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -115,6 +115,7 @@ void hists__collapse_resort(struct hists *hists, struct ui_progress *prog);
 void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel);
 void hists__output_recalc_col_len(struct hists *hists, int max_rows);
 
+u64 hists__total_period(struct hists *hists);
 void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h);
 void hists__inc_nr_events(struct hists *hists, u32 type);
 void events_stats__inc(struct events_stats *stats, u32 type);
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 0ada68b3b096..233681e304dd 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -33,6 +33,7 @@ struct symbol_conf symbol_conf = {
 	.try_vmlinux_path = true,
 	.annotate_src	  = true,
 	.demangle	  = true,
+	.filter_relative  = true,
 	.symfs            = "",
 };
 
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 501e4e722e8e..ae94e006a52d 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -115,7 +115,8 @@ struct symbol_conf {
 			annotate_asm_raw,
 			annotate_src,
 			event_group,
-			demangle;
+			demangle,
+			filter_relative;
 	const char	*vmlinux_name,
 			*kallsyms_name,
 			*source_prefix,
-- 
1.7.11.7

[PATCH 5/9] perf top: Add --percentage option

[Namhyung Kim]

The --percentage option is for controlling overhead percentage
displayed.  It can only receive either of "relative" or "absolute".
Move the parser callback function into a common location since it's
used by multiple commands now.

For more information, please see previous commit same thing done to
"perf report".

Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-top.txt | 18 +++++++++++++++---
 tools/perf/builtin-report.c           | 16 +---------------
 tools/perf/builtin-top.c              |  2 ++
 tools/perf/util/hist.c                | 13 +++++++++++++
 tools/perf/util/hist.h                |  5 +++++
 5 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
index cdd8d4946dba..0acf54f203f0 100644
--- a/tools/perf/Documentation/perf-top.txt
+++ b/tools/perf/Documentation/perf-top.txt
@@ -124,13 +124,16 @@ Default is to monitor all CPUS.
 	Show a column with the sum of periods.
 
 --dsos::
-	Only consider symbols in these dsos.
+	Only consider symbols in these dsos.  This option will affect the
+	percentage of the overhead column.  See --percentage for more info.
 
 --comms::
-	Only consider symbols in these comms.
+	Only consider symbols in these comms.  This option will affect the
+	percentage of the overhead column.  See --percentage for more info.
 
 --symbols::
-	Only consider these symbols.
+	Only consider these symbols.  This option will affect the
+	percentage of the overhead column.  See --percentage for more info.
 
 -M::
 --disassembler-style=:: Set disassembler style for objdump.
@@ -166,6 +169,15 @@ Default is to monitor all CPUS.
 	Do not show entries which have an overhead under that percent.
 	(Default: 0).
 
+--percentage::
+	Determine how to display the overhead percentage of filtered entries.
+	Filters can be applied by --comms, --dsos and/or --symbols options and
+	Zoom operations on the TUI (thread, dso, etc).
+
+	"relative" means it's relative to filtered entries only so that the
+	sum of shown entries will be always 100%. "absolute" means it retains
+	the original value before and after the filter is applied.
+
 INTERACTIVE PROMPTING KEYS
 --------------------------
 
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index b8eb8c6431aa..8fa36182d87c 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -709,20 +709,6 @@ parse_percent_limit(const struct option *opt, const char *str,
 	return 0;
 }
 
-static int
-parse_percentage(const struct option *opt __maybe_unused, const char *str,
-		 int unset __maybe_unused)
-{
-	if (!strcmp(str, "relative"))
-		symbol_conf.filter_relative = true;
-	else if (!strcmp(str, "absolute"))
-		symbol_conf.filter_relative = false;
-	else
-		return -1;
-
-	return 0;
-}
-
 int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	struct perf_session *session;
@@ -846,7 +832,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_CALLBACK(0, "percent-limit", &report, "percent",
 		     "Don't show entries under that percent", parse_percent_limit),
 	OPT_CALLBACK(0, "percentage", NULL, "relative|absolute",
-		     "how to display percentage of filtered entries", parse_percentage),
+		     "how to display percentage of filtered entries", parse_filter_percentage),
 	OPT_END()
 	};
 	struct perf_data_file file = {
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index ed99ec4a309f..18a2467bdf00 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1114,6 +1114,8 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_STRING('u', "uid", &target->uid_str, "user", "user to profile"),
 	OPT_CALLBACK(0, "percent-limit", &top, "percent",
 		     "Don't show entries under that percent", parse_percent_limit),
+	OPT_CALLBACK(0, "percentage", NULL, "relative|absolute",
+		     "How to display percentage of filtered entries", parse_filter_percentage),
 	OPT_END()
 	};
 	const char * const top_usage[] = {
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 71078de683fe..6e7eb53d4bcc 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -943,3 +943,16 @@ u64 hists__total_period(struct hists *hists)
 	return symbol_conf.filter_relative ? hists->stats.total_non_filtered_period :
 		hists->stats.total_period;
 }
+
+int parse_filter_percentage(const struct option *opt __maybe_unused,
+			    const char *arg, int unset __maybe_unused)
+{
+	if (!strcmp(arg, "relative"))
+		symbol_conf.filter_relative = true;
+	else if (!strcmp(arg, "absolute"))
+		symbol_conf.filter_relative = false;
+	else
+		return -1;
+
+	return 0;
+}
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 3191496bd3b7..a4ec336ae3fe 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -254,4 +254,9 @@ static inline int script_browse(const char *script_opt __maybe_unused)
 #endif
 
 unsigned int hists__sort_list_width(struct hists *hists);
+
+struct option;
+int parse_filter_percentage(const struct option *opt __maybe_unused,
+			    const char *arg, int unset __maybe_unused);
+
 #endif	/* __PERF_HIST_H */
-- 
1.7.11.7

[PATCH 6/9] perf diff: Add --percentage option

[Namhyung Kim]

The --percentage option is for controlling overhead percentage
displayed.  It can only receive either of "relative" or "absolute" and
affects -c delta output only.

For more information, please see previous commit same thing done to
"perf report".

Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-diff.txt | 21 ++++++++++++++++++---
 tools/perf/builtin-diff.c              | 30 ++++++++++++++++++++++--------
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/tools/perf/Documentation/perf-diff.txt b/tools/perf/Documentation/perf-diff.txt
index fdfceee0ffd0..fbfa1192923c 100644
--- a/tools/perf/Documentation/perf-diff.txt
+++ b/tools/perf/Documentation/perf-diff.txt
@@ -33,17 +33,20 @@ OPTIONS
 -d::
 --dsos=::
 	Only consider symbols in these dsos. CSV that understands
-	file://filename entries.
+	file://filename entries.  This option will affect the percentage
+	of the Baseline/Delta column.  See --percentage for more info.
 
 -C::
 --comms=::
 	Only consider symbols in these comms. CSV that understands
-	file://filename entries.
+	file://filename entries.  This option will affect the percentage
+	of the Baseline/Delta column.  See --percentage for more info.
 
 -S::
 --symbols=::
 	Only consider these symbols. CSV that understands
-	file://filename entries.
+	file://filename entries.  This option will affect the percentage
+	of the Baseline/Delta column.  See --percentage for more info.
 
 -s::
 --sort=::
@@ -89,6 +92,14 @@ OPTIONS
 --order::
        Specify compute sorting column number.
 
+--percentage::
+	Determine how to display the overhead percentage of filtered entries.
+	Filters can be applied by --comms, --dsos and/or --symbols options.
+
+	"relative" means it's relative to filtered entries only so that the
+	sum of shown entries will be always 100%.  "absolute" means it retains
+	the original value before and after the filter is applied.
+
 COMPARISON
 ----------
 The comparison is governed by the baseline file. The baseline perf.data
@@ -157,6 +168,10 @@ with:
   - period_percent being the % of the hist entry period value within
     single data file
 
+  - with filtering by -C, -d and/or -S, period_percent might be changed
+    relative to how entries are filtered.  Use --percentage=absolute to
+    prevent such fluctuation.
+
 ratio
 ~~~~~
 If specified the 'Ratio' column is displayed with value 'r' computed as:
diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 204fffe22532..c903fe13c173 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -220,7 +220,8 @@ static int setup_compute(const struct option *opt, const char *str,
 
 static double period_percent(struct hist_entry *he, u64 period)
 {
-	u64 total = he->hists->stats.total_period;
+	u64 total = hists__total_period(he->hists);
+
 	return (period * 100.0) / total;
 }
 
@@ -259,11 +260,18 @@ static s64 compute_wdiff(struct hist_entry *he, struct hist_entry *pair)
 static int formula_delta(struct hist_entry *he, struct hist_entry *pair,
 			 char *buf, size_t size)
 {
+	u64 he_total = he->hists->stats.total_period;
+	u64 pair_total = pair->hists->stats.total_period;
+
+	if (symbol_conf.filter_relative) {
+		he_total = he->hists->stats.total_non_filtered_period;
+		pair_total = pair->hists->stats.total_non_filtered_period;
+	}
 	return scnprintf(buf, size,
 			 "(%" PRIu64 " * 100 / %" PRIu64 ") - "
 			 "(%" PRIu64 " * 100 / %" PRIu64 ")",
-			  pair->stat.period, pair->hists->stats.total_period,
-			  he->stat.period, he->hists->stats.total_period);
+			 pair->stat.period, pair_total,
+			 he->stat.period, he_total);
 }
 
 static int formula_ratio(struct hist_entry *he, struct hist_entry *pair,
@@ -327,15 +335,16 @@ static int diff__process_sample_event(struct perf_tool *tool __maybe_unused,
 		return -1;
 	}
 
-	if (al.filtered)
-		return 0;
-
 	if (hists__add_entry(&evsel->hists, &al, sample->period,
 			     sample->weight, sample->transaction)) {
 		pr_warning("problem incrementing symbol period, skipping event\n");
 		return -1;
 	}
 
+	if (al.filtered == 0) {
+		evsel->hists.stats.total_non_filtered_period += sample->period;
+		evsel->hists.nr_non_filtered_entries++;
+	}
 	evsel->hists.stats.total_period += sample->period;
 	return 0;
 }
@@ -565,7 +574,9 @@ static void hists__compute_resort(struct hists *hists)
 	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_col_len(hists);
 
 	while (next != NULL) {
@@ -732,13 +743,16 @@ static const struct option options[] = {
 	OPT_STRING(0, "symfs", &symbol_conf.symfs, "directory",
 		    "Look for files with symbols relative to this directory"),
 	OPT_UINTEGER('o', "order", &sort_compute, "Specify compute sorting."),
+	OPT_CALLBACK(0, "percentage", NULL, "relative|absolute",
+		     "How to display percentage of filtered entries", parse_filter_percentage),
 	OPT_END()
 };
 
 static double baseline_percent(struct hist_entry *he)
 {
-	struct hists *hists = he->hists;
-	return 100.0 * he->stat.period / hists->stats.total_period;
+	u64 total = hists__total_period(he->hists);
+
+	return 100.0 * he->stat.period / total;
 }
 
 static int hpp__color_baseline(struct perf_hpp_fmt *fmt,
-- 
1.7.11.7

[PATCH 7/9] perf tools: Add hist.percentage config option

[Namhyung Kim]

Add hist.percentage option for setting default value of the
symbol_conf.filter_relative.  It affects the output of various perf
commands (like perf report, top and diff) only if filter(s) applied.

An user can write .perfconfig file like below to show absolute
percentage of filtered entries by default:

  $ cat ~/.perfconfig
  [hist]
  percentage = absolute

And it can be changed through command line:

  $ perf report --percentage relative

Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-diff.c | 2 ++
 tools/perf/util/config.c  | 4 ++++
 tools/perf/util/hist.c    | 8 ++++++++
 tools/perf/util/hist.h    | 1 +
 4 files changed, 15 insertions(+)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index c903fe13c173..6ef80f22c1e2 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -1134,6 +1134,8 @@ static int data_init(int argc, const char **argv)
 
 int cmd_diff(int argc, const char **argv, const char *prefix __maybe_unused)
 {
+	perf_config(perf_default_config, NULL);
+
 	sort_order = diff__default_sort_order;
 	argc = parse_options(argc, argv, options, diff_usage, 0);
 
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 3e0fdd369ccb..24519e14ac56 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -11,6 +11,7 @@
 #include "util.h"
 #include "cache.h"
 #include "exec_cmd.h"
+#include "util/hist.h"  /* perf_hist_config */
 
 #define MAXNAME (256)
 
@@ -355,6 +356,9 @@ int perf_default_config(const char *var, const char *value,
 	if (!prefixcmp(var, "core."))
 		return perf_default_core_config(var, value);
 
+	if (!prefixcmp(var, "hist."))
+		return perf_hist_config(var, value);
+
 	/* Add other config variables here. */
 	return 0;
 }
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 6e7eb53d4bcc..a0b32df4cd88 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -956,3 +956,11 @@ int parse_filter_percentage(const struct option *opt __maybe_unused,
 
 	return 0;
 }
+
+int perf_hist_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "hist.percentage"))
+		return parse_filter_percentage(NULL, value, 0);
+
+	return 0;
+}
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index a4ec336ae3fe..5a0343eb22e2 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -258,5 +258,6 @@ unsigned int hists__sort_list_width(struct hists *hists);
 struct option;
 int parse_filter_percentage(const struct option *opt __maybe_unused,
 			    const char *arg, int unset __maybe_unused);
+int perf_hist_config(const char *var, const char *value);
 
 #endif	/* __PERF_HIST_H */
-- 
1.7.11.7

[PATCH 8/9] perf ui/tui: Add 'F' hotkey to toggle percentage output

[Namhyung Kim]

Add 'F' hotkey to toggle relative and absolute percentage of filtered
entries.

Suggested-by: Jiri Olsa <jolsa@redhat.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 7ad11477a0f5..4d416984c59d 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1389,6 +1389,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	"C             Collapse all callchains\n"			\
 	"d             Zoom into current DSO\n"				\
 	"E             Expand all callchains\n"				\
+	"F             Toggle percentage of filtered entries\n"		\
 
 	/* help messages are sorted by lexical order of the hotkey */
 	const char report_help[] = HIST_BROWSER_HELP_COMMON
@@ -1494,6 +1495,9 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			if (env->arch)
 				tui__header_window(env);
 			continue;
+		case 'F':
+			symbol_conf.filter_relative ^= 1;
+			continue;
 		case K_F1:
 		case 'h':
 		case '?':
-- 
1.7.11.7

[PATCH 9/9] perf tools: Show absolute percentage by default

[Namhyung Kim]

Now perf report will show absolute percentage on filter entries by
default.

Suggested-by: Jiri Olsa <jolsa@redhat.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 233681e304dd..0ada68b3b096 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -33,7 +33,6 @@ struct symbol_conf symbol_conf = {
 	.try_vmlinux_path = true,
 	.annotate_src	  = true,
 	.demangle	  = true,
-	.filter_relative  = true,
 	.symfs            = "",
 };
 
-- 
1.7.11.7

Re: [PATCHSET 0/9] perf tools: Update on filtered entries' percentage output (v7)

[Andi Kleen]

On Mon, Mar 10, 2014 at 04:43:51PM +0900, Namhyung Kim wrote:
> Hello,
> 
> I added --percentage option to perf report to control display of
> percentage of filtered entries.
> 
>  usage: perf report [<options>]
> 
>         --percentage <relative|absolute>
>                           how to display percentage of filtered entries

Just as a high level comment. We have a lot of different display 
options now. It's hard to figure out what they all do for non experts.

It may be useful to define a number of high level layout combinations
that are useful for different situations and have a single
option to select everything together.

Perhaps --layout FOO and FOO a number of predefined layouts.

-Andi

Re: [PATCHSET 0/9] perf tools: Update on filtered entries' percentage output (v7)

[Davidlohr Bueso]

On Mon, 2014-03-10 at 23:08 +0100, Andi Kleen wrote:
> On Mon, Mar 10, 2014 at 04:43:51PM +0900, Namhyung Kim wrote:
> > Hello,
> > 
> > I added --percentage option to perf report to control display of
> > percentage of filtered entries.
> > 
> >  usage: perf report [<options>]
> > 
> >         --percentage <relative|absolute>
> >                           how to display percentage of filtered entries
> 
> Just as a high level comment. We have a lot of different display 
> options now. It's hard to figure out what they all do for non experts.
> 
> It may be useful to define a number of high level layout combinations
> that are useful for different situations and have a single
> option to select everything together.

Yes please, absolutely. The wiki page could use some love as well. I
imagine the manpage is updated but it's also nice to have some examples
of the newer features in perf.wiki.kernel.org.

Thanks,
Davidlohr

Re: [PATCHSET 0/9] perf tools: Update on filtered entries' percentage output (v7)

[Davidlohr Bueso]

On Mon, 2014-03-10 at 19:58 -0700, Davidlohr Bueso wrote:
> On Mon, 2014-03-10 at 23:08 +0100, Andi Kleen wrote:
> > On Mon, Mar 10, 2014 at 04:43:51PM +0900, Namhyung Kim wrote:
> > > Hello,
> > > 
> > > I added --percentage option to perf report to control display of
> > > percentage of filtered entries.
> > > 
> > >  usage: perf report [<options>]
> > > 
> > >         --percentage <relative|absolute>
> > >                           how to display percentage of filtered entries
> > 
> > Just as a high level comment. We have a lot of different display 
> > options now. It's hard to figure out what they all do for non experts.
> > 
> > It may be useful to define a number of high level layout combinations
> > that are useful for different situations and have a single
> > option to select everything together.
> 
> Yes please, absolutely. The wiki page could use some love as well. I
> imagine the manpage is updated but it's also nice to have some examples
> of the newer features in perf.wiki.kernel.org.

That reminds me, I should add an entry for perf-bench... lead by
example :S

Re: [PATCHSET 0/9] perf tools: Update on filtered entries' percentage output (v7)

[Namhyung Kim]

Hi Andi,

On Mon, 10 Mar 2014 23:08:19 +0100, Andi Kleen wrote:
> On Mon, Mar 10, 2014 at 04:43:51PM +0900, Namhyung Kim wrote:
>> Hello,
>> 
>> I added --percentage option to perf report to control display of
>> percentage of filtered entries.
>> 
>>  usage: perf report [<options>]
>> 
>>         --percentage <relative|absolute>
>>                           how to display percentage of filtered entries
>
> Just as a high level comment. We have a lot of different display 
> options now. It's hard to figure out what they all do for non experts.

Absolutely agreed.

>
> It may be useful to define a number of high level layout combinations
> that are useful for different situations and have a single
> option to select everything together.
>
> Perhaps --layout FOO and FOO a number of predefined layouts.

Yeah, that will be useful for me too :)

Btw, do you have concrete examples of the situations?  I'll also think
about it too, but it'd be wonderful if you guys share how you use the
perf tools for various purpose in detail.

Thanks,
Namhyung

Re: [PATCHSET 0/9] perf tools: Update on filtered entries' percentage output (v7)

[Namhyung Kim]

Hi Davidlohr,

On Mon, 10 Mar 2014 19:58:23 -0700, Davidlohr Bueso wrote:
> On Mon, 2014-03-10 at 23:08 +0100, Andi Kleen wrote:
>> On Mon, Mar 10, 2014 at 04:43:51PM +0900, Namhyung Kim wrote:
>> > Hello,
>> > 
>> > I added --percentage option to perf report to control display of
>> > percentage of filtered entries.
>> > 
>> >  usage: perf report [<options>]
>> > 
>> >         --percentage <relative|absolute>
>> >                           how to display percentage of filtered entries
>> 
>> Just as a high level comment. We have a lot of different display 
>> options now. It's hard to figure out what they all do for non experts.
>> 
>> It may be useful to define a number of high level layout combinations
>> that are useful for different situations and have a single
>> option to select everything together.
>
> Yes please, absolutely. The wiki page could use some love as well. I
> imagine the manpage is updated but it's also nice to have some examples
> of the newer features in perf.wiki.kernel.org.

Right, will try - as a side note, being a non-native speaker, I feel
more difficult to write a note/comment rather than a code. :)

Thanks,
Namhyung

Re: [PATCHSET 0/9] perf tools: Update on filtered entries' percentage output (v7)

[Namhyung Kim]

Ping!

On Mon, 10 Mar 2014 16:43:51 +0900, Namhyung Kim wrote:
> Hello,
>
> I added --percentage option to perf report to control display of
> percentage of filtered entries.
>
>  usage: perf report [<options>]
>
>         --percentage <relative|absolute>
>                           how to display percentage of filtered entries
>
> "relative" means it's relative to filtered entries only so that the
> sum of shown entries will be always 100%.  "absolute" means it retains
> original value before and after the filter applied.  In patch 9/9, I
> made the "absolute" as default since it makes more sense IMHO.
>     
>       $ perf report -s comm
>       # Overhead       Command
>       # ........  ............
>       #
>           74.19%           cc1
>            7.61%           gcc
>            6.11%            as
>            4.35%            sh
>            4.14%          make
>            1.13%        fixdep
>       ...
>     
>       $ perf report -s comm -c cc1,gcc --percentage absolute
>       # Overhead       Command
>       # ........  ............
>       #
>           74.19%           cc1
>            7.61%           gcc
>     
>       $ perf report -s comm -c cc1,gcc --percentage relative
>       # Overhead       Command
>       # ........  ............
>       #
>           90.69%           cc1
>            9.31%           gcc
>     
> Note that it has zero effect if no filter was applied.
>
>  * changes in v7:
>   - rename ->nr_filtered_* to ->nr_non_filtered_* (Arnaldo)
>   - remove an unneeded alignment change (Arnaldo)
>
>  * changes in v6:
>   - fix a bug in --stdio group report
>   - reuse __hpp__fmt() function in TUI/GTK
>   - add Acked-by from Jiri
>
>  * changes in v5:
>   - fix 0 samples in relative percent output (Jiri)
>   - factor hists__total_period function (Jiri)
>   - share config parsing code with option parser (Jiri)
>
>  * changes in v4:
>   - support perf top and perf diff also  (Jiri)
>   - add HIST_FILTER__HOST/GUEST  (Jiri)
>   - retain both of filtered and total stats  (Arnaldo)
>   - add 'F' hotkey on TUI  (Jiri)
>   - rename config variable to have "hist." prefix
>
>
> You can get this on the 'perf/percentage-v7' branch in my tree
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>
> Any comments are welcome, thanks
> Namhyung
>
>
> Namhyung Kim (9):
>   perf tools: Pass evsel to hpp->header/width functions explicitly
>   perf tools: Count periods of filtered entries separately
>   perf hists: Add support for showing relative percentage
>   perf report: Add --percentage option
>   perf top: Add --percentage option
>   perf diff: Add --percentage option
>   perf tools: Add hist.percentage config option
>   perf ui/tui: Add 'F' hotkey to toggle percentage output
>   perf tools: Show absolute percentage by default
>
>  tools/perf/Documentation/perf-diff.txt   | 21 ++++++++--
>  tools/perf/Documentation/perf-report.txt | 24 ++++++++---
>  tools/perf/Documentation/perf-top.txt    | 18 +++++++--
>  tools/perf/builtin-diff.c                | 39 ++++++++++++------
>  tools/perf/builtin-report.c              | 24 +++++++++--
>  tools/perf/builtin-top.c                 |  2 +
>  tools/perf/ui/browsers/hists.c           | 39 ++++++++++++++----
>  tools/perf/ui/gtk/hists.c                | 14 +++----
>  tools/perf/ui/hist.c                     | 32 +++++++--------
>  tools/perf/ui/stdio/hist.c               |  5 +--
>  tools/perf/util/config.c                 |  4 ++
>  tools/perf/util/event.c                  | 22 +++++------
>  tools/perf/util/hist.c                   | 68 ++++++++++++++++++++++----------
>  tools/perf/util/hist.h                   | 25 +++++++++++-
>  tools/perf/util/symbol.h                 |  5 ++-
>  15 files changed, 242 insertions(+), 100 deletions(-)

Re: [PATCH 2/9] perf tools: Count periods of filtered entries separately

[Arnaldo Carvalho de Melo]

Em Mon, Mar 10, 2014 at 04:43:53PM +0900, Namhyung Kim escreveu:
> @@ -749,9 +750,6 @@ int perf_event__preprocess_sample(const union perf_event *event,
>  	if (thread == NULL)
>  		return -1;
>  
> -	if (thread__is_filtered(thread))
> -		goto out_filtered;
> -

What was the intent of moving this test from here...

>  	dump_printf(" ... thread: %s:%d\n", thread__comm_str(thread), thread->tid);
>  	/*
>  	 * Have we already created the kernel maps for this machine?
> @@ -766,6 +764,10 @@ int perf_event__preprocess_sample(const union perf_event *event,
>  
>  	thread__find_addr_map(thread, machine, cpumode, MAP__FUNCTION,
>  			      sample->ip, al);
> +
> +	if (thread__is_filtered(thread))
> +		al->filtered |= (1 << HIST_FILTER__THREAD);
> +

... to here? At first I thought it was because thread__is_filtered()
would check something that thread__find_addr_map() was doing, but no,
its invariant, we can do it here or at the original site, so I'm keeping
it there, ok?

- Arnaldo

>  	dump_printf(" ...... dso: %s\n",
>  		    al->map ? al->map->dso->long_name :
>  			al->level == 'H' ? "[hypervisor]" : "<not found>");
> @@ -781,7 +783,7 @@ int perf_event__preprocess_sample(const union perf_event *event,
>  			       (dso->short_name != dso->long_name &&
>  				strlist__has_entry(symbol_conf.dso_list,
>  						   dso->long_name)))))
> -			goto out_filtered;
> +			al->filtered |= (1 << HIST_FILTER__DSO);
>  
>  		al->sym = map__find_symbol(al->map, al->addr,
>  					   machine->symbol_filter);
> @@ -790,11 +792,7 @@ int perf_event__preprocess_sample(const union perf_event *event,
>  	if (symbol_conf.sym_list &&
>  		(!al->sym || !strlist__has_entry(symbol_conf.sym_list,
>  						al->sym->name)))
> -		goto out_filtered;
> -
> -	return 0;
> +		al->filtered |= (1 << HIST_FILTER__SYMBOL);
>  
> -out_filtered:
> -	al->filtered = true;
>  	return 0;
>  }
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 0466efa71140..aa13aa9826d9 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -13,13 +13,6 @@ static bool hists__filter_entry_by_thread(struct hists *hists,
>  static bool hists__filter_entry_by_symbol(struct hists *hists,
>  					  struct hist_entry *he);
>  
> -enum hist_filter {
> -	HIST_FILTER__DSO,
> -	HIST_FILTER__THREAD,
> -	HIST_FILTER__PARENT,
> -	HIST_FILTER__SYMBOL,
> -};
> -
>  struct callchain_param	callchain_param = {
>  	.mode	= CHAIN_GRAPH_REL,
>  	.min_percent = 0.5,
> @@ -330,6 +323,7 @@ void hists__inc_nr_entries(struct hists *hists, struct hist_entry *h)
>  		hists__calc_col_len(hists, h);
>  		++hists->nr_entries;
>  		hists->stats.total_period += h->stat.period;
> +		hists->stats.total_non_filtered_period += h->stat.period;
>  	}
>  }
>  
> @@ -429,7 +423,7 @@ struct hist_entry *__hists__add_entry(struct hists *hists,
>  			.weight = weight,
>  		},
>  		.parent = sym_parent,
> -		.filtered = symbol__parent_filter(sym_parent),
> +		.filtered = symbol__parent_filter(sym_parent) | al->filtered,
>  		.hists	= hists,
>  		.branch_info = bi,
>  		.mem_info = mi,
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 0c76bf972736..a149f1adaae4 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -14,6 +14,15 @@ struct hist_entry;
>  struct addr_location;
>  struct symbol;
>  
> +enum hist_filter {
> +	HIST_FILTER__DSO,
> +	HIST_FILTER__THREAD,
> +	HIST_FILTER__PARENT,
> +	HIST_FILTER__SYMBOL,
> +	HIST_FILTER__GUEST,
> +	HIST_FILTER__HOST,
> +};
> +
>  /*
>   * The kernel collects the number of events it couldn't send in a stretch and
>   * when possible sends this number in a PERF_RECORD_LOST event. The number of
> @@ -28,6 +37,7 @@ struct symbol;
>   */
>  struct events_stats {
>  	u64 total_period;
> +	u64 total_non_filtered_period;
>  	u64 total_lost;
>  	u64 total_invalid_chains;
>  	u32 nr_events[PERF_RECORD_HEADER_MAX];
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 2553ae04b788..501e4e722e8e 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -186,7 +186,7 @@ struct addr_location {
>  	struct symbol *sym;
>  	u64	      addr;
>  	char	      level;
> -	bool	      filtered;
> +	u8	      filtered;
>  	u8	      cpumode;
>  	s32	      cpu;
>  };
> -- 
> 1.7.11.7

Re: [PATCH 2/9] perf tools: Count periods of filtered entries separately

[Namhyung Kim]

On Tue, Mar 18, 2014 at 5:08 AM, Arnaldo Carvalho de Melo
<acme@ghostprotocols.net> wrote:
> Em Mon, Mar 10, 2014 at 04:43:53PM +0900, Namhyung Kim escreveu:
>> @@ -749,9 +750,6 @@ int perf_event__preprocess_sample(const union perf_event *event,
>>       if (thread == NULL)
>>               return -1;
>>
>> -     if (thread__is_filtered(thread))
>> -             goto out_filtered;
>> -
>
> What was the intent of moving this test from here...
>
>>       dump_printf(" ... thread: %s:%d\n", thread__comm_str(thread), thread->tid);
>>       /*
>>        * Have we already created the kernel maps for this machine?
>> @@ -766,6 +764,10 @@ int perf_event__preprocess_sample(const union perf_event *event,
>>
>>       thread__find_addr_map(thread, machine, cpumode, MAP__FUNCTION,
>>                             sample->ip, al);
>> +
>> +     if (thread__is_filtered(thread))
>> +             al->filtered |= (1 << HIST_FILTER__THREAD);
>> +
>
> ... to here? At first I thought it was because thread__is_filtered()
> would check something that thread__find_addr_map() was doing, but no,
> its invariant, we can do it here or at the original site, so I'm keeping
> it there, ok?

It's because thread__find_addr_map() clears al->filtered, so filtering
with -d option won't work.  Maybe we can move initialization of the
al->filtered upto this function.

Thanks,
Namhyung

Re: [PATCH 2/9] perf tools: Count periods of filtered entries separately

[Namhyung Kim]

On Tue, Mar 18, 2014 at 1:19 PM, Namhyung Kim <namhyung@kernel.org> wrote:

> It's because thread__find_addr_map() clears al->filtered, so filtering
> with -d option won't work.  Maybe we can move initialization of the
> al->filtered upto this function.

Oh, I meant the -c/--comms option actually :)

Thanks,
Namhyung

[tip:perf/core] perf ui hists: Pass evsel to hpp->header/ width functions explicitly

[tip-bot for Namhyung Kim]

Commit-ID:  94a0793ddf7fa9890006a8dc203b985e7b120785
Gitweb:     http://git.kernel.org/tip/94a0793ddf7fa9890006a8dc203b985e7b120785
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Mon, 10 Mar 2014 16:43:52 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 14 Mar 2014 18:08:40 -0300

perf ui hists: Pass evsel to hpp->header/width functions explicitly

Those functions need evsel to investigate event group and it's passed
via hpp->ptr.  However as it can be missed easily so it's better to
pass it via an argument IMHO.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1394437440-11609-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-diff.c  |  7 ++++---
 tools/perf/ui/gtk/hists.c  |  3 +--
 tools/perf/ui/hist.c       | 24 ++++++++++--------------
 tools/perf/ui/stdio/hist.c |  5 ++---
 tools/perf/util/hist.h     |  6 ++++--
 5 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index a77e312..204fffe 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -952,8 +952,8 @@ static int hpp__entry_global(struct perf_hpp_fmt *_fmt, struct perf_hpp *hpp,
 				 dfmt->header_width, buf);
 }
 
-static int hpp__header(struct perf_hpp_fmt *fmt,
-		       struct perf_hpp *hpp)
+static int hpp__header(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
+		       struct perf_evsel *evsel __maybe_unused)
 {
 	struct diff_hpp_fmt *dfmt =
 		container_of(fmt, struct diff_hpp_fmt, fmt);
@@ -963,7 +963,8 @@ static int hpp__header(struct perf_hpp_fmt *fmt,
 }
 
 static int hpp__width(struct perf_hpp_fmt *fmt,
-		      struct perf_hpp *hpp __maybe_unused)
+		      struct perf_hpp *hpp __maybe_unused,
+		      struct perf_evsel *evsel __maybe_unused)
 {
 	struct diff_hpp_fmt *dfmt =
 		container_of(fmt, struct diff_hpp_fmt, fmt);
diff --git a/tools/perf/ui/gtk/hists.c b/tools/perf/ui/gtk/hists.c
index 7912dab..e395ef9 100644
--- a/tools/perf/ui/gtk/hists.c
+++ b/tools/perf/ui/gtk/hists.c
@@ -165,7 +165,6 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 	struct perf_hpp hpp = {
 		.buf		= s,
 		.size		= sizeof(s),
-		.ptr		= hists_to_evsel(hists),
 	};
 
 	nr_cols = 0;
@@ -192,7 +191,7 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 	col_idx = 0;
 
 	perf_hpp__for_each_format(fmt) {
-		fmt->header(fmt, &hpp);
+		fmt->header(fmt, &hpp, hists_to_evsel(hists));
 
 		gtk_tree_view_insert_column_with_attributes(GTK_TREE_VIEW(view),
 							    -1, ltrim(s),
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index ac39313..0f403b8 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -118,29 +118,27 @@ int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
 
 #define __HPP_HEADER_FN(_type, _str, _min_width, _unit_width) 		\
 static int hpp__header_##_type(struct perf_hpp_fmt *fmt __maybe_unused,	\
-			       struct perf_hpp *hpp)			\
+			       struct perf_hpp *hpp,			\
+			       struct perf_evsel *evsel)		\
 {									\
 	int len = _min_width;						\
 									\
-	if (symbol_conf.event_group) {					\
-		struct perf_evsel *evsel = hpp->ptr;			\
-									\
+	if (symbol_conf.event_group)					\
 		len = max(len, evsel->nr_members * _unit_width);	\
-	}								\
+									\
 	return scnprintf(hpp->buf, hpp->size, "%*s", len, _str);	\
 }
 
 #define __HPP_WIDTH_FN(_type, _min_width, _unit_width) 			\
 static int hpp__width_##_type(struct perf_hpp_fmt *fmt __maybe_unused,	\
-			      struct perf_hpp *hpp __maybe_unused)	\
+			      struct perf_hpp *hpp __maybe_unused,	\
+			      struct perf_evsel *evsel)			\
 {									\
 	int len = _min_width;						\
 									\
-	if (symbol_conf.event_group) {					\
-		struct perf_evsel *evsel = hpp->ptr;			\
-									\
+	if (symbol_conf.event_group)					\
 		len = max(len, evsel->nr_members * _unit_width);	\
-	}								\
+									\
 	return len;							\
 }
 
@@ -329,15 +327,13 @@ unsigned int hists__sort_list_width(struct hists *hists)
 	struct perf_hpp_fmt *fmt;
 	struct sort_entry *se;
 	int i = 0, ret = 0;
-	struct perf_hpp dummy_hpp = {
-		.ptr	= hists_to_evsel(hists),
-	};
+	struct perf_hpp dummy_hpp;
 
 	perf_hpp__for_each_format(fmt) {
 		if (i)
 			ret += 2;
 
-		ret += fmt->width(fmt, &dummy_hpp);
+		ret += fmt->width(fmt, &dummy_hpp, hists_to_evsel(hists));
 	}
 
 	list_for_each_entry(se, &hist_entry__sort_list, list)
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 9bad892..d59893e 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -379,7 +379,6 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 	struct perf_hpp dummy_hpp = {
 		.buf	= bf,
 		.size	= sizeof(bf),
-		.ptr	= hists_to_evsel(hists),
 	};
 	bool first = true;
 	size_t linesz;
@@ -398,7 +397,7 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 		else
 			first = false;
 
-		fmt->header(fmt, &dummy_hpp);
+		fmt->header(fmt, &dummy_hpp, hists_to_evsel(hists));
 		fprintf(fp, "%s", bf);
 	}
 
@@ -443,7 +442,7 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 		else
 			first = false;
 
-		width = fmt->width(fmt, &dummy_hpp);
+		width = fmt->width(fmt, &dummy_hpp, hists_to_evsel(hists));
 		for (i = 0; i < width; i++)
 			fprintf(fp, ".");
 	}
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 9e1cada..0c76bf9 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -132,8 +132,10 @@ struct perf_hpp {
 };
 
 struct perf_hpp_fmt {
-	int (*header)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp);
-	int (*width)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp);
+	int (*header)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
+		      struct perf_evsel *evsel);
+	int (*width)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
+		     struct perf_evsel *evsel);
 	int (*color)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 		     struct hist_entry *he);
 	int (*entry)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,

Re: [PATCH 2/9] perf tools: Count periods of filtered entries separately

[Arnaldo Carvalho de Melo]

Em Tue, Mar 18, 2014 at 01:19:07PM +0900, Namhyung Kim escreveu:
> On Tue, Mar 18, 2014 at 5:08 AM, Arnaldo Carvalho de Melo
> <acme@ghostprotocols.net> wrote:
> > Em Mon, Mar 10, 2014 at 04:43:53PM +0900, Namhyung Kim escreveu:
> >> @@ -749,9 +750,6 @@ int perf_event__preprocess_sample(const union perf_event *event,
> >>       if (thread == NULL)
> >>               return -1;
> >>
> >> -     if (thread__is_filtered(thread))
> >> -             goto out_filtered;
> >> -
> >
> > What was the intent of moving this test from here...
> >
> >>       dump_printf(" ... thread: %s:%d\n", thread__comm_str(thread), thread->tid);
> >>       /*
> >>        * Have we already created the kernel maps for this machine?
> >> @@ -766,6 +764,10 @@ int perf_event__preprocess_sample(const union perf_event *event,
> >>
> >>       thread__find_addr_map(thread, machine, cpumode, MAP__FUNCTION,
> >>                             sample->ip, al);
> >> +
> >> +     if (thread__is_filtered(thread))
> >> +             al->filtered |= (1 << HIST_FILTER__THREAD);
> >> +
> >
> > ... to here? At first I thought it was because thread__is_filtered()
> > would check something that thread__find_addr_map() was doing, but no,
> > its invariant, we can do it here or at the original site, so I'm keeping
> > it there, ok?
> 
> It's because thread__find_addr_map() clears al->filtered, so filtering
> with -d option won't work.  Maybe we can move initialization of the
> al->filtered upto this function.

So this is a separate patch with this explanation, I'll add it to the
series, thanks for the explanation!

- Arnaldo

Re: [PATCH 2/9] perf tools: Count periods of filtered entries separately

[Arnaldo Carvalho de Melo]

Em Tue, Mar 18, 2014 at 10:15:18AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Mar 18, 2014 at 01:19:07PM +0900, Namhyung Kim escreveu:
> > On Tue, Mar 18, 2014 at 5:08 AM, Arnaldo Carvalho de Melo
> > <acme@ghostprotocols.net> wrote:
> > > Em Mon, Mar 10, 2014 at 04:43:53PM +0900, Namhyung Kim escreveu:
> > >> @@ -749,9 +750,6 @@ int perf_event__preprocess_sample(const union perf_event *event,
> > >>       if (thread == NULL)
> > >>               return -1;
> > >>
> > >> -     if (thread__is_filtered(thread))
> > >> -             goto out_filtered;
> > >> -
> > >
> > > What was the intent of moving this test from here...
> > >
> > >>       dump_printf(" ... thread: %s:%d\n", thread__comm_str(thread), thread->tid);
> > >>       /*
> > >>        * Have we already created the kernel maps for this machine?
> > >> @@ -766,6 +764,10 @@ int perf_event__preprocess_sample(const union perf_event *event,
> > >>
> > >>       thread__find_addr_map(thread, machine, cpumode, MAP__FUNCTION,
> > >>                             sample->ip, al);
> > >> +
> > >> +     if (thread__is_filtered(thread))
> > >> +             al->filtered |= (1 << HIST_FILTER__THREAD);
> > >> +
> > >
> > > ... to here? At first I thought it was because thread__is_filtered()
> > > would check something that thread__find_addr_map() was doing, but no,
> > > its invariant, we can do it here or at the original site, so I'm keeping
> > > it there, ok?
> > 
> > It's because thread__find_addr_map() clears al->filtered, so filtering
> > with -d option won't work.  Maybe we can move initialization of the
> > al->filtered upto this function.
> 
> So this is a separate patch with this explanation, I'll add it to the
> series, thanks for the explanation!

Humm, it really needs to be folded into the patch that does all the
tests, as before we were just stopping the filters early and thus no
problem existed, its only now that we apply all the filters that we need
to be careful in knowing that thread__find_addr_map() is when
al->filtered gets initialized to zero, right?

- Arnaldo

Re: [PATCH 2/9] perf tools: Count periods of filtered entries separately

[Namhyung Kim]

On Tue, Mar 18, 2014 at 10:18 PM, Arnaldo Carvalho de Melo
<acme@ghostprotocols.net> wrote:
> Em Tue, Mar 18, 2014 at 10:15:18AM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Tue, Mar 18, 2014 at 01:19:07PM +0900, Namhyung Kim escreveu:
>> > On Tue, Mar 18, 2014 at 5:08 AM, Arnaldo Carvalho de Melo
>> > <acme@ghostprotocols.net> wrote:
>> > > Em Mon, Mar 10, 2014 at 04:43:53PM +0900, Namhyung Kim escreveu:
>> > >> @@ -749,9 +750,6 @@ int perf_event__preprocess_sample(const union perf_event *event,
>> > >>       if (thread == NULL)
>> > >>               return -1;
>> > >>
>> > >> -     if (thread__is_filtered(thread))
>> > >> -             goto out_filtered;
>> > >> -
>> > >
>> > > What was the intent of moving this test from here...
>> > >
>> > >>       dump_printf(" ... thread: %s:%d\n", thread__comm_str(thread), thread->tid);
>> > >>       /*
>> > >>        * Have we already created the kernel maps for this machine?
>> > >> @@ -766,6 +764,10 @@ int perf_event__preprocess_sample(const union perf_event *event,
>> > >>
>> > >>       thread__find_addr_map(thread, machine, cpumode, MAP__FUNCTION,
>> > >>                             sample->ip, al);
>> > >> +
>> > >> +     if (thread__is_filtered(thread))
>> > >> +             al->filtered |= (1 << HIST_FILTER__THREAD);
>> > >> +
>> > >
>> > > ... to here? At first I thought it was because thread__is_filtered()
>> > > would check something that thread__find_addr_map() was doing, but no,
>> > > its invariant, we can do it here or at the original site, so I'm keeping
>> > > it there, ok?
>> >
>> > It's because thread__find_addr_map() clears al->filtered, so filtering
>> > with -d option won't work.  Maybe we can move initialization of the
>> > al->filtered upto this function.
>>
>> So this is a separate patch with this explanation, I'll add it to the
>> series, thanks for the explanation!
>
> Humm, it really needs to be folded into the patch that does all the
> tests, as before we were just stopping the filters early and thus no
> problem existed, its only now that we apply all the filters that we need
> to be careful in knowing that thread__find_addr_map() is when
> al->filtered gets initialized to zero, right?

Right. :)

Thanks,
Namhyung

Re: [PATCH 3/9] perf hists: Add support for showing relative percentage

[Arnaldo Carvalho de Melo]

Em Mon, Mar 10, 2014 at 04:43:54PM +0900, Namhyung Kim escreveu:
> @@ -695,12 +695,17 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
>  	if (h->filtered)
>  		return;
>  
> -	++hists->nr_entries;
> -	if (h->ms.unfolded)
> +	hists->nr_entries++;
> +	hists->nr_non_filtered_entries++;

Why not keep existing practice? prefix or suffix generates the same
code, changing it from prefix to suffix increment just adds noise to the
patch :-\

Also, this is why I was asking about nr_entries and total_period being
invariant, looking at this function we can see it is _not_ invariant, as
when we apply/remove filters we touch them.

This is why I made the comment (in a private conversation) about having
a invariant_total_period invariant_nr_entries pair (I think I used some
other naming) while knowing that the existing variables nr_entries and
total_period are actually subjected to the filters being used.

I.e. to avoid confusion we need to make total_entries and nr_entries
never change when a filter is applied, touching only two new variables
for non_filtered total_period and nr_entries.

I'll try doing it if you don't do it first, but will first process some
more patches and submit what I already processed.

- Arnaldo

> +	if (h->ms.unfolded) {
>  		hists->nr_entries += h->nr_rows;
> +		hists->nr_non_filtered_entries += h->nr_rows;
> +	}
>  	h->row_offset = 0;
>  	hists->stats.total_period += h->stat.period;
> +	hists->stats.total_non_filtered_period += h->stat.period;
>  	hists->stats.nr_events[PERF_RECORD_SAMPLE] += h->stat.nr_events;
> +	hists->stats.nr_non_filtered_samples += h->stat.nr_events;
>  
>  	hists__calc_col_len(hists, h);
>  }
> @@ -723,7 +728,9 @@ void hists__filter_by_dso(struct hists *hists)
>  	struct rb_node *nd;
>  
>  	hists->nr_entries = hists->stats.total_period = 0;
> +	hists->nr_non_filtered_entries = hists->stats.total_non_filtered_period = 0;
>  	hists->stats.nr_events[PERF_RECORD_SAMPLE] = 0;
> +	hists->stats.nr_non_filtered_samples = 0;
>  	hists__reset_col_len(hists);
>  
>  	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
> @@ -756,7 +763,9 @@ void hists__filter_by_thread(struct hists *hists)
>  	struct rb_node *nd;
>  
>  	hists->nr_entries = hists->stats.total_period = 0;
> +	hists->nr_non_filtered_entries = hists->stats.total_non_filtered_period = 0;
>  	hists->stats.nr_events[PERF_RECORD_SAMPLE] = 0;
> +	hists->stats.nr_non_filtered_samples = 0;
>  	hists__reset_col_len(hists);
>  
>  	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
> @@ -787,7 +796,9 @@ void hists__filter_by_symbol(struct hists *hists)
>  	struct rb_node *nd;
>  
>  	hists->nr_entries = hists->stats.total_period = 0;
> +	hists->nr_non_filtered_entries = hists->stats.total_non_filtered_period = 0;
>  	hists->stats.nr_events[PERF_RECORD_SAMPLE] = 0;
> +	hists->stats.nr_non_filtered_samples = 0;
>  	hists__reset_col_len(hists);
>  
>  	for (nd = rb_first(&hists->entries); nd; nd = rb_next(nd)) {
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index a149f1adaae4..213551469f36 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -41,6 +41,7 @@ struct events_stats {
>  	u64 total_lost;
>  	u64 total_invalid_chains;
>  	u32 nr_events[PERF_RECORD_HEADER_MAX];
> +	u32 nr_non_filtered_samples;
>  	u32 nr_lost_warned;
>  	u32 nr_unknown_events;
>  	u32 nr_invalid_chains;
> @@ -84,6 +85,7 @@ struct hists {
>  	struct rb_root		entries;
>  	struct rb_root		entries_collapsed;
>  	u64			nr_entries;
> +	u64			nr_non_filtered_entries;
>  	const struct thread	*thread_filter;
>  	const struct dso	*dso_filter;
>  	const char		*uid_filter_str;
> -- 
> 1.7.11.7

Re: [PATCH 3/9] perf hists: Add support for showing relative percentage

[Namhyung Kim]

Hi Arnaldo,

On Tue, Mar 18, 2014 at 8:08 PM, Arnaldo Carvalho de Melo
<acme@ghostprotocols.net> wrote:
> Em Mon, Mar 10, 2014 at 04:43:54PM +0900, Namhyung Kim escreveu:
>> @@ -695,12 +695,17 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
>>       if (h->filtered)
>>               return;
>>
>> -     ++hists->nr_entries;
>> -     if (h->ms.unfolded)
>> +     hists->nr_entries++;
>> +     hists->nr_non_filtered_entries++;
>
> Why not keep existing practice? prefix or suffix generates the same
> code, changing it from prefix to suffix increment just adds noise to the
> patch :-\

Argh, sorry - I couldn't resist changing it.  Won't do that in the future. :-/

>
> Also, this is why I was asking about nr_entries and total_period being
> invariant, looking at this function we can see it is _not_ invariant, as
> when we apply/remove filters we touch them.
>
> This is why I made the comment (in a private conversation) about having
> a invariant_total_period invariant_nr_entries pair (I think I used some
> other naming) while knowing that the existing variables nr_entries and
> total_period are actually subjected to the filters being used.
>
> I.e. to avoid confusion we need to make total_entries and nr_entries
> never change when a filter is applied, touching only two new variables
> for non_filtered total_period and nr_entries.
>
> I'll try doing it if you don't do it first, but will first process some
> more patches and submit what I already processed.

Please see the next patch 4/9 ("perf report: Add --percentage option")
doing that.  What I did in this patch is just adding new
non_filtered_* fields and not changing existing behavior.  And in the
next patch, the total_period and nr_entries will become invariant.

Do you want to split the patch 4/9 or merge a part of it into this?

Thanks,
Namhyung

Re: [PATCH 3/9] perf hists: Add support for showing relative percentage

[Arnaldo Carvalho de Melo]

Em Wed, Mar 19, 2014 at 12:18:00AM +0000, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Tue, Mar 18, 2014 at 8:08 PM, Arnaldo Carvalho de Melo
> <acme@ghostprotocols.net> wrote:
> > Em Mon, Mar 10, 2014 at 04:43:54PM +0900, Namhyung Kim escreveu:
> >> @@ -695,12 +695,17 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
> >>       if (h->filtered)
> >>               return;
> >>
> >> -     ++hists->nr_entries;
> >> -     if (h->ms.unfolded)
> >> +     hists->nr_entries++;
> >> +     hists->nr_non_filtered_entries++;
> >
> > Why not keep existing practice? prefix or suffix generates the same
> > code, changing it from prefix to suffix increment just adds noise to the
> > patch :-\
> 
> Argh, sorry - I couldn't resist changing it.  Won't do that in the future. :-/

We should strive to be as minimalistic as possible. Reviewing is hard,
we shouldn't make it harder :-)

Cleanup/cosmetic patches, when deemed necessary should be done
separatetly and stated as such.
 
> > Also, this is why I was asking about nr_entries and total_period being
> > invariant, looking at this function we can see it is _not_ invariant, as
> > when we apply/remove filters we touch them.
> >
> > This is why I made the comment (in a private conversation) about having
> > a invariant_total_period invariant_nr_entries pair (I think I used some
> > other naming) while knowing that the existing variables nr_entries and
> > total_period are actually subjected to the filters being used.
> >
> > I.e. to avoid confusion we need to make total_entries and nr_entries
> > never change when a filter is applied, touching only two new variables
> > for non_filtered total_period and nr_entries.
> >
> > I'll try doing it if you don't do it first, but will first process some
> > more patches and submit what I already processed.
> 
> Please see the next patch 4/9 ("perf report: Add --percentage option")
> doing that.  What I did in this patch is just adding new
> non_filtered_* fields and not changing existing behavior.  And in the
> next patch, the total_period and nr_entries will become invariant.
> 
> Do you want to split the patch 4/9 or merge a part of it into this?

The point is that I don't look at all the patches (nor should I, I
think) to figure out what one wants to do after a series is applied.

So each patch should stand on its own, each one should be meaningful
without requiring reading what is coming next.

- Arnaldo