[PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Namhyung Kim]

Hello,

This is a patchset implementing -F/--fields option to setup output
field/column as Ingo requested.  It depends on my --percentage
patchset [1].

The -F option can receive any sort keys that -s option recognize, plus
following fields (name can be changed):

  overhead, overhead_sys, overhead_us, sample, period

The overhead_guest_sys and overhead_guest_us might be avaiable when
you profile guest machines.

Output will be sorted by in order of fields and sort keys passed by -s
option will be added to the output field list automatically.  If you
want to change the order of sorting you can give -s option in addition
to -F option.  To support old behavior, it'll also prepend 'overhead'
field to the sort keys unless you give -F option explicitly.


  $ perf report -s dso,sym
  ...
  # Overhead  Shared Object                      Symbol
  # ........  .............  ..........................
  #
      13.75%  ld-2.17.so     [.] strcmp                
      10.00%  abc            [.] a                     
      10.00%  abc            [.] b                     
      10.00%  abc            [.] c                     
       8.75%  abc            [.] main                  
       7.50%  libc-2.17.so   [.] _setjmp               
       6.25%  abc            [.] _init                 
       6.25%  abc            [.] frame_dummy           
       5.00%  abc            [.] __libc_csu_init       
       5.00%  ld-2.17.so     [.] _dl_name_match_p      
       3.75%  libc-2.17.so   [.] __new_exitfn          
       2.50%  libc-2.17.so   [.] __cxa_atexit          
       1.25%  ld-2.17.so     [.] _dl_check_map_versions
       1.25%  ld-2.17.so     [.] _dl_setup_hash        
       1.25%  ld-2.17.so     [.] _dl_sysdep_start      
       1.25%  ld-2.17.so     [.] brk                   
       1.25%  ld-2.17.so     [.] calloc@plt            
       1.25%  ld-2.17.so     [.] dl_main               
       1.25%  ld-2.17.so     [.] match_symbol          
       1.25%  ld-2.17.so     [.] sbrk                  
       1.25%  ld-2.17.so     [.] strlen                


  $ perf report -F sym,sample,overhead
  ...
  #                     Symbol       Samples  Overhead
  # ..........................  ............  ........
  #
    [.] __cxa_atexit                       2     2.50%
    [.] __libc_csu_init                    4     5.00%
    [.] __new_exitfn                       3     3.75%
    [.] _dl_check_map_versions             1     1.25%
    [.] _dl_name_match_p                   4     5.00%
    [.] _dl_setup_hash                     1     1.25%
    [.] _dl_sysdep_start                   1     1.25%
    [.] _init                              5     6.25%
    [.] _setjmp                            6     7.50%
    [.] a                                  8    10.00%
    [.] b                                  8    10.00%
    [.] brk                                1     1.25%
    [.] c                                  8    10.00%
    [.] calloc@plt                         1     1.25%
    [.] dl_main                            1     1.25%
    [.] frame_dummy                        5     6.25%
    [.] main                               7     8.75%
    [.] match_symbol                       1     1.25%
    [.] sbrk                               1     1.25%
    [.] strcmp                            11    13.75%
    [.] strlen                             1     1.25%


  $ perf report -F sym,sample -s overhead
  ...
  #                     Symbol       Samples  Overhead
  # ..........................  ............  ........
  #
    [.] strcmp                            11    13.75%
    [.] a                                  8    10.00%
    [.] b                                  8    10.00%
    [.] c                                  8    10.00%
    [.] main                               7     8.75%
    [.] _setjmp                            6     7.50%
    [.] _init                              5     6.25%
    [.] frame_dummy                        5     6.25%
    [.] __libc_csu_init                    4     5.00%
    [.] _dl_name_match_p                   4     5.00%
    [.] __new_exitfn                       3     3.75%
    [.] __cxa_atexit                       2     2.50%
    [.] _dl_check_map_versions             1     1.25%
    [.] _dl_setup_hash                     1     1.25%
    [.] _dl_sysdep_start                   1     1.25%
    [.] brk                                1     1.25%
    [.] calloc@plt                         1     1.25%
    [.] dl_main                            1     1.25%
    [.] match_symbol                       1     1.25%
    [.] sbrk                               1     1.25%
    [.] strlen                             1     1.25%


 * changes in v4:
  - fix a tui navigation bug
  - fix a bug in output change of perf diff
  - move call to perf_hpp__init() out of setup_browser()
  - fix alignment of some output fields on stdio

 * changes in v3:
  - rename to --fields option for consistency  (David)
  - prevent to add same keys multiple times
  - change dso sorting to show unknown dsos last
  - fix minor bugs

 * changes in v2:
  - add a cleanup patch using ui__has_annotation()
  - cleanup default sort order managment
  - support perf top also
  - handle elided sort entries properly
  - add Acked-by's from Ingo


I pushed the patch series on the 'perf/field-v4' branch in my tree

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


Any comments are welcome, please test!

Thanks,
Namhyung


[1] https://lkml.org/lkml/2014/4/10/397

Namhyung Kim (17):
  perf tools: Add ->cmp(), ->collapse() and ->sort() to perf_hpp_fmt
  perf tools: Convert sort entries to hpp formats
  perf tools: Use hpp formats to sort hist entries
  perf tools: Support event grouping in hpp ->sort()
  perf tools: Use hpp formats to sort final output
  perf tools: Consolidate output field handling to hpp format routines
  perf ui: Get rid of callback from __hpp__fmt()
  perf tools: Allow hpp fields to be sort keys
  perf tools: Consolidate management of default sort orders
  perf tools: Call perf_hpp__init() before setting up GUI browsers
  perf report: Add -F option to specify output fields
  perf tools: Add ->sort() member to struct sort_entry
  perf report/tui: Fix a bug when --fields/sort is given
  perf top: Add --fields option to specify output fields
  perf diff: Add missing setup_output_field()
  perf tools: Skip elided sort entries
  perf hists: Reset width of output fields with header length

 tools/perf/Documentation/perf-report.txt |  10 +
 tools/perf/Documentation/perf-top.txt    |   9 +
 tools/perf/builtin-diff.c                |   3 +
 tools/perf/builtin-report.c              |  31 +--
 tools/perf/builtin-top.c                 |  12 +-
 tools/perf/ui/browsers/hists.c           |  76 +++----
 tools/perf/ui/gtk/hists.c                |  41 +---
 tools/perf/ui/hist.c                     | 189 ++++++++++++++--
 tools/perf/ui/setup.c                    |   2 -
 tools/perf/ui/stdio/hist.c               |  54 ++---
 tools/perf/util/hist.c                   |  83 ++-----
 tools/perf/util/hist.h                   |  20 +-
 tools/perf/util/sort.c                   | 364 ++++++++++++++++++++++++++++++-
 tools/perf/util/sort.h                   |   4 +
 14 files changed, 654 insertions(+), 244 deletions(-)

-- 
1.9.2

[PATCH 01/17] perf tools: Add ->cmp(), ->collapse() and ->sort() to perf_hpp_fmt

[Namhyung Kim]

Those function pointers will be used to sort report output based on
the selected fields.  This is a preparation of later change.

Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/hist.c   | 39 +++++++++++++++++++++++++++++++++++----
 tools/perf/util/hist.h |  3 +++
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 0912805c08f4..d4a4f2e7eb43 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -192,6 +192,14 @@ static int hpp__entry_##_type(struct perf_hpp_fmt *_fmt __maybe_unused,		\
 			  hpp_entry_scnprintf, true);				\
 }
 
+#define __HPP_SORT_FN(_type, _field)						\
+static int64_t hpp__sort_##_type(struct hist_entry *a, struct hist_entry *b)	\
+{										\
+	s64 __a = he_get_##_field(a);						\
+	s64 __b = he_get_##_field(b);						\
+	return __a - __b;							\
+}
+
 #define __HPP_ENTRY_RAW_FN(_type, _field)					\
 static u64 he_get_raw_##_field(struct hist_entry *he)				\
 {										\
@@ -206,16 +214,27 @@ static int hpp__entry_##_type(struct perf_hpp_fmt *_fmt __maybe_unused,		\
 			  hpp_entry_scnprintf, false);				\
 }
 
+#define __HPP_SORT_RAW_FN(_type, _field)					\
+static int64_t hpp__sort_##_type(struct hist_entry *a, struct hist_entry *b)	\
+{										\
+	s64 __a = he_get_raw_##_field(a);					\
+	s64 __b = he_get_raw_##_field(b);					\
+	return __a - __b;							\
+}
+
+
 #define HPP_PERCENT_FNS(_type, _str, _field, _min_width, _unit_width)	\
 __HPP_HEADER_FN(_type, _str, _min_width, _unit_width)			\
 __HPP_WIDTH_FN(_type, _min_width, _unit_width)				\
 __HPP_COLOR_PERCENT_FN(_type, _field)					\
-__HPP_ENTRY_PERCENT_FN(_type, _field)
+__HPP_ENTRY_PERCENT_FN(_type, _field)					\
+__HPP_SORT_FN(_type, _field)
 
 #define HPP_RAW_FNS(_type, _str, _field, _min_width, _unit_width)	\
 __HPP_HEADER_FN(_type, _str, _min_width, _unit_width)			\
 __HPP_WIDTH_FN(_type, _min_width, _unit_width)				\
-__HPP_ENTRY_RAW_FN(_type, _field)
+__HPP_ENTRY_RAW_FN(_type, _field)					\
+__HPP_SORT_RAW_FN(_type, _field)
 
 
 HPP_PERCENT_FNS(overhead, "Overhead", period, 8, 8)
@@ -227,19 +246,31 @@ HPP_PERCENT_FNS(overhead_guest_us, "guest usr", period_guest_us, 9, 8)
 HPP_RAW_FNS(samples, "Samples", nr_events, 12, 12)
 HPP_RAW_FNS(period, "Period", period, 12, 12)
 
+static int64_t hpp__nop_cmp(struct hist_entry *a __maybe_unused,
+			    struct hist_entry *b __maybe_unused)
+{
+	return 0;
+}
+
 #define HPP__COLOR_PRINT_FNS(_name)			\
 	{						\
 		.header	= hpp__header_ ## _name,	\
 		.width	= hpp__width_ ## _name,		\
 		.color	= hpp__color_ ## _name,		\
-		.entry	= hpp__entry_ ## _name		\
+		.entry	= hpp__entry_ ## _name,		\
+		.cmp	= hpp__nop_cmp,			\
+		.collapse = hpp__nop_cmp,		\
+		.sort	= hpp__sort_ ## _name,		\
 	}
 
 #define HPP__PRINT_FNS(_name)				\
 	{						\
 		.header	= hpp__header_ ## _name,	\
 		.width	= hpp__width_ ## _name,		\
-		.entry	= hpp__entry_ ## _name		\
+		.entry	= hpp__entry_ ## _name,		\
+		.cmp	= hpp__nop_cmp,			\
+		.collapse = hpp__nop_cmp,		\
+		.sort	= hpp__sort_ ## _name,		\
 	}
 
 struct perf_hpp_fmt perf_hpp__format[] = {
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 5a0343eb22e2..9ef3b3a75f03 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -153,6 +153,9 @@ struct perf_hpp_fmt {
 		     struct hist_entry *he);
 	int (*entry)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 		     struct hist_entry *he);
+	int64_t (*cmp)(struct hist_entry *a, struct hist_entry *b);
+	int64_t (*collapse)(struct hist_entry *a, struct hist_entry *b);
+	int64_t (*sort)(struct hist_entry *a, struct hist_entry *b);
 
 	struct list_head list;
 };
-- 
1.9.2

[PATCH 02/17] perf tools: Convert sort entries to hpp formats

[Namhyung Kim]

This is a preparation of consolidating management of output field and
sort keys.

Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/hist.c   |  6 ++++
 tools/perf/util/hist.h |  6 ++++
 tools/perf/util/sort.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 88 insertions(+), 4 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index d4a4f2e7eb43..a6eea666b443 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -284,6 +284,7 @@ struct perf_hpp_fmt perf_hpp__format[] = {
 };
 
 LIST_HEAD(perf_hpp__list);
+LIST_HEAD(perf_hpp__sort_list);
 
 
 #undef HPP__COLOR_PRINT_FNS
@@ -325,6 +326,11 @@ void perf_hpp__column_register(struct perf_hpp_fmt *format)
 	list_add_tail(&format->list, &perf_hpp__list);
 }
 
+void perf_hpp__register_sort_field(struct perf_hpp_fmt *format)
+{
+	list_add_tail(&format->sort_list, &perf_hpp__sort_list);
+}
+
 void perf_hpp__column_enable(unsigned col)
 {
 	BUG_ON(col >= PERF_HPP__MAX_INDEX);
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 9ef3b3a75f03..0f8d46803c92 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -158,13 +158,18 @@ struct perf_hpp_fmt {
 	int64_t (*sort)(struct hist_entry *a, struct hist_entry *b);
 
 	struct list_head list;
+	struct list_head sort_list;
 };
 
 extern struct list_head perf_hpp__list;
+extern struct list_head perf_hpp__sort_list;
 
 #define perf_hpp__for_each_format(format) \
 	list_for_each_entry(format, &perf_hpp__list, list)
 
+#define perf_hpp__for_each_sort_list(format) \
+	list_for_each_entry(format, &perf_hpp__sort_list, sort_list)
+
 extern struct perf_hpp_fmt perf_hpp__format[];
 
 enum {
@@ -183,6 +188,7 @@ enum {
 void perf_hpp__init(void);
 void perf_hpp__column_register(struct perf_hpp_fmt *format);
 void perf_hpp__column_enable(unsigned col);
+void perf_hpp__register_sort_field(struct perf_hpp_fmt *format);
 
 typedef u64 (*hpp_field_fn)(struct hist_entry *he);
 typedef int (*hpp_callback_fn)(struct perf_hpp *hpp, bool front);
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 635cd8f8b22e..b2829f947053 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -2,6 +2,7 @@
 #include "hist.h"
 #include "comm.h"
 #include "symbol.h"
+#include "evsel.h"
 
 regex_t		parent_regex;
 const char	default_parent_pattern[] = "^sys_|^do_page_fault";
@@ -1027,10 +1028,80 @@ static struct sort_dimension memory_sort_dimensions[] = {
 
 #undef DIM
 
-static void __sort_dimension__add(struct sort_dimension *sd, enum sort_type idx)
+struct hpp_sort_entry {
+	struct perf_hpp_fmt hpp;
+	struct sort_entry *se;
+};
+
+static int __sort__hpp_header(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
+			      struct perf_evsel *evsel)
+{
+	struct hpp_sort_entry *hse;
+	size_t len;
+
+	hse = container_of(fmt, struct hpp_sort_entry, hpp);
+	len = hists__col_len(&evsel->hists, hse->se->se_width_idx);
+
+	return scnprintf(hpp->buf, hpp->size, "%*s", len, hse->se->se_header);
+}
+
+static int __sort__hpp_width(struct perf_hpp_fmt *fmt,
+			     struct perf_hpp *hpp __maybe_unused,
+			     struct perf_evsel *evsel)
+{
+	struct hpp_sort_entry *hse;
+
+	hse = container_of(fmt, struct hpp_sort_entry, hpp);
+
+	return hists__col_len(&evsel->hists, hse->se->se_width_idx);
+}
+
+static int __sort__hpp_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
+			     struct hist_entry *he)
+{
+	struct hpp_sort_entry *hse;
+	size_t len;
+
+	hse = container_of(fmt, struct hpp_sort_entry, hpp);
+	len = hists__col_len(he->hists, hse->se->se_width_idx);
+
+	return hse->se->se_snprintf(he, hpp->buf, hpp->size, len);
+}
+
+static int __sort_dimension__add_hpp(struct sort_dimension *sd)
+{
+	struct hpp_sort_entry *hse;
+
+	hse = malloc(sizeof(*hse));
+	if (hse == NULL) {
+		pr_err("Memory allocation failed\n");
+		return -1;
+	}
+
+	hse->se = sd->entry;
+	hse->hpp.header = __sort__hpp_header;
+	hse->hpp.width = __sort__hpp_width;
+	hse->hpp.entry = __sort__hpp_entry;
+	hse->hpp.color = NULL;
+
+	hse->hpp.cmp = sd->entry->se_cmp;
+	hse->hpp.collapse = sd->entry->se_collapse ? : sd->entry->se_cmp;
+	hse->hpp.sort = hse->hpp.collapse;
+
+	INIT_LIST_HEAD(&hse->hpp.list);
+	INIT_LIST_HEAD(&hse->hpp.sort_list);
+
+	perf_hpp__register_sort_field(&hse->hpp);
+	return 0;
+}
+
+static int __sort_dimension__add(struct sort_dimension *sd, enum sort_type idx)
 {
 	if (sd->taken)
-		return;
+		return 0;
+
+	if (__sort_dimension__add_hpp(sd) < 0)
+		return -1;
 
 	if (sd->entry->se_collapse)
 		sort__need_collapse = 1;
@@ -1040,6 +1111,8 @@ static void __sort_dimension__add(struct sort_dimension *sd, enum sort_type idx)
 
 	list_add_tail(&sd->entry->list, &hist_entry__sort_list);
 	sd->taken = 1;
+
+	return 0;
 }
 
 int sort_dimension__add(const char *tok)
@@ -1068,8 +1141,7 @@ int sort_dimension__add(const char *tok)
 			sort__has_dso = 1;
 		}
 
-		__sort_dimension__add(sd, i);
-		return 0;
+		return __sort_dimension__add(sd, i);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(bstack_sort_dimensions); i++) {
-- 
1.9.2

[PATCH 03/17] perf tools: Use hpp formats to sort hist entries

[Namhyung Kim]

It wrapped sort entries to hpp functions, so using the hpp sort list
to sort entries.

Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 5a892477aa50..fd9576e16bbe 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -437,11 +437,11 @@ struct hist_entry *__hists__add_entry(struct hists *hists,
 int64_t
 hist_entry__cmp(struct hist_entry *left, struct hist_entry *right)
 {
-	struct sort_entry *se;
+	struct perf_hpp_fmt *fmt;
 	int64_t cmp = 0;
 
-	list_for_each_entry(se, &hist_entry__sort_list, list) {
-		cmp = se->se_cmp(left, right);
+	perf_hpp__for_each_sort_list(fmt) {
+		cmp = fmt->cmp(left, right);
 		if (cmp)
 			break;
 	}
@@ -452,15 +452,11 @@ hist_entry__cmp(struct hist_entry *left, struct hist_entry *right)
 int64_t
 hist_entry__collapse(struct hist_entry *left, struct hist_entry *right)
 {
-	struct sort_entry *se;
+	struct perf_hpp_fmt *fmt;
 	int64_t cmp = 0;
 
-	list_for_each_entry(se, &hist_entry__sort_list, list) {
-		int64_t (*f)(struct hist_entry *, struct hist_entry *);
-
-		f = se->se_collapse ?: se->se_cmp;
-
-		cmp = f(left, right);
+	perf_hpp__for_each_sort_list(fmt) {
+		cmp = fmt->collapse(left, right);
 		if (cmp)
 			break;
 	}
-- 
1.9.2

[PATCH 04/17] perf tools: Support event grouping in hpp ->sort()

[Namhyung Kim]

Move logic of hist_entry__sort_on_period to __hpp__sort() in order to
support event group report.

Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/hist.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index a6eea666b443..c65a7fd744c6 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -116,6 +116,62 @@ int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
 	return ret;
 }
 
+static int field_cmp(u64 field_a, u64 field_b)
+{
+	if (field_a > field_b)
+		return 1;
+	if (field_a < field_b)
+		return -1;
+	return 0;
+}
+
+static int __hpp__sort(struct hist_entry *a, struct hist_entry *b,
+		       hpp_field_fn get_field)
+{
+	s64 ret;
+	int i, nr_members;
+	struct perf_evsel *evsel;
+	struct hist_entry *pair;
+	u64 *fields_a, *fields_b;
+
+	ret = field_cmp(get_field(a), get_field(b));
+	if (ret || !symbol_conf.event_group)
+		return ret;
+
+	evsel = hists_to_evsel(a->hists);
+	if (!perf_evsel__is_group_event(evsel))
+		return ret;
+
+	nr_members = evsel->nr_members;
+	fields_a = calloc(sizeof(*fields_a), nr_members);
+	fields_b = calloc(sizeof(*fields_b), nr_members);
+
+	if (!fields_a || !fields_b)
+		goto out;
+
+	list_for_each_entry(pair, &a->pairs.head, pairs.node) {
+		evsel = hists_to_evsel(pair->hists);
+		fields_a[perf_evsel__group_idx(evsel)] = get_field(pair);
+	}
+
+	list_for_each_entry(pair, &b->pairs.head, pairs.node) {
+		evsel = hists_to_evsel(pair->hists);
+		fields_b[perf_evsel__group_idx(evsel)] = get_field(pair);
+	}
+
+	for (i = 1; i < nr_members; i++) {
+		ret = fields_a[i] - fields_b[i];
+		if (ret)
+			break;
+	}
+
+out:
+	free(fields_a);
+	free(fields_b);
+
+	return ret;
+}
+
 #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,			\
@@ -195,9 +251,7 @@ static int hpp__entry_##_type(struct perf_hpp_fmt *_fmt __maybe_unused,		\
 #define __HPP_SORT_FN(_type, _field)						\
 static int64_t hpp__sort_##_type(struct hist_entry *a, struct hist_entry *b)	\
 {										\
-	s64 __a = he_get_##_field(a);						\
-	s64 __b = he_get_##_field(b);						\
-	return __a - __b;							\
+	return __hpp__sort(a, b, he_get_##_field);				\
 }
 
 #define __HPP_ENTRY_RAW_FN(_type, _field)					\
@@ -217,9 +271,7 @@ static int hpp__entry_##_type(struct perf_hpp_fmt *_fmt __maybe_unused,		\
 #define __HPP_SORT_RAW_FN(_type, _field)					\
 static int64_t hpp__sort_##_type(struct hist_entry *a, struct hist_entry *b)	\
 {										\
-	s64 __a = he_get_raw_##_field(a);					\
-	s64 __b = he_get_raw_##_field(b);					\
-	return __a - __b;							\
+	return __hpp__sort(a, b, he_get_raw_##_field);				\
 }
 
 
-- 
1.9.2

[PATCH 05/17] perf tools: Use hpp formats to sort final output

[Namhyung Kim]

Convert output sorting function to use ->sort hpp functions.

Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 62 +++++++-------------------------------------------
 1 file changed, 8 insertions(+), 54 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index fd9576e16bbe..a73aea306647 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -569,64 +569,18 @@ void hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
 	}
 }
 
-/*
- * reverse the map, sort on period.
- */
-
-static int period_cmp(u64 period_a, u64 period_b)
+static int hist_entry__sort(struct hist_entry *a, struct hist_entry *b)
 {
-	if (period_a > period_b)
-		return 1;
-	if (period_a < period_b)
-		return -1;
-	return 0;
-}
-
-static int hist_entry__sort_on_period(struct hist_entry *a,
-				      struct hist_entry *b)
-{
-	int ret;
-	int i, nr_members;
-	struct perf_evsel *evsel;
-	struct hist_entry *pair;
-	u64 *periods_a, *periods_b;
-
-	ret = period_cmp(a->stat.period, b->stat.period);
-	if (ret || !symbol_conf.event_group)
-		return ret;
-
-	evsel = hists_to_evsel(a->hists);
-	nr_members = evsel->nr_members;
-	if (nr_members <= 1)
-		return ret;
-
-	periods_a = zalloc(sizeof(periods_a) * nr_members);
-	periods_b = zalloc(sizeof(periods_b) * nr_members);
-
-	if (!periods_a || !periods_b)
-		goto out;
-
-	list_for_each_entry(pair, &a->pairs.head, pairs.node) {
-		evsel = hists_to_evsel(pair->hists);
-		periods_a[perf_evsel__group_idx(evsel)] = pair->stat.period;
-	}
-
-	list_for_each_entry(pair, &b->pairs.head, pairs.node) {
-		evsel = hists_to_evsel(pair->hists);
-		periods_b[perf_evsel__group_idx(evsel)] = pair->stat.period;
-	}
+	struct perf_hpp_fmt *fmt;
+	int64_t cmp = 0;
 
-	for (i = 1; i < nr_members; i++) {
-		ret = period_cmp(periods_a[i], periods_b[i]);
-		if (ret)
+	perf_hpp__for_each_format(fmt) {
+		cmp = fmt->sort(a, b);
+		if (cmp)
 			break;
 	}
 
-out:
-	free(periods_a);
-	free(periods_b);
-
-	return ret;
+	return cmp;
 }
 
 static void __hists__insert_output_entry(struct rb_root *entries,
@@ -645,7 +599,7 @@ static void __hists__insert_output_entry(struct rb_root *entries,
 		parent = *p;
 		iter = rb_entry(parent, struct hist_entry, rb_node);
 
-		if (hist_entry__sort_on_period(he, iter) > 0)
+		if (hist_entry__sort(he, iter) > 0)
 			p = &(*p)->rb_left;
 		else
 			p = &(*p)->rb_right;
-- 
1.9.2

[PATCH 06/17] perf tools: Consolidate output field handling to hpp format routines

[Namhyung Kim]

Until now the hpp and sort functions do similar jobs different ways.
Since the sort functions converted/wrapped to hpp formats it can do
the job in a uniform way.

The perf_hpp__sort_list has a list of hpp formats to sort entries and
the perf_hpp__list has a list of hpp formats to print output result.

To have a backward compatiblity, it automatically adds 'overhead'
field in front of sort list.  And then all of fields in sort list
added to the output list (if it's not already there).

Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c |  4 ++--
 tools/perf/ui/gtk/hists.c      | 31 -------------------------
 tools/perf/ui/hist.c           | 28 +++++++++++++++++++++++
 tools/perf/ui/stdio/hist.c     | 52 +++++++++++++-----------------------------
 tools/perf/util/hist.c         |  2 +-
 5 files changed, 47 insertions(+), 70 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 4d416984c59d..c4616e9f309a 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -731,8 +731,8 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 		if (!browser->b.navkeypressed)
 			width += 1;
 
-		hist_entry__sort_snprintf(entry, s, sizeof(s), browser->hists);
-		slsmg_write_nstring(s, width);
+		slsmg_write_nstring("", width);
+
 		++row;
 		++printed;
 	} else
diff --git a/tools/perf/ui/gtk/hists.c b/tools/perf/ui/gtk/hists.c
index 91f10f3f6dd1..d5c336e1bb14 100644
--- a/tools/perf/ui/gtk/hists.c
+++ b/tools/perf/ui/gtk/hists.c
@@ -153,7 +153,6 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 	struct perf_hpp_fmt *fmt;
 	GType col_types[MAX_COLUMNS];
 	GtkCellRenderer *renderer;
-	struct sort_entry *se;
 	GtkTreeStore *store;
 	struct rb_node *nd;
 	GtkWidget *view;
@@ -172,16 +171,6 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 	perf_hpp__for_each_format(fmt)
 		col_types[nr_cols++] = G_TYPE_STRING;
 
-	list_for_each_entry(se, &hist_entry__sort_list, list) {
-		if (se->elide)
-			continue;
-
-		if (se == &sort_sym)
-			sym_col = nr_cols;
-
-		col_types[nr_cols++] = G_TYPE_STRING;
-	}
-
 	store = gtk_tree_store_newv(nr_cols, col_types);
 
 	view = gtk_tree_view_new();
@@ -199,16 +188,6 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 							    col_idx++, NULL);
 	}
 
-	list_for_each_entry(se, &hist_entry__sort_list, list) {
-		if (se->elide)
-			continue;
-
-		gtk_tree_view_insert_column_with_attributes(GTK_TREE_VIEW(view),
-							    -1, se->se_header,
-							    renderer, "text",
-							    col_idx++, NULL);
-	}
-
 	for (col_idx = 0; col_idx < nr_cols; col_idx++) {
 		GtkTreeViewColumn *column;
 
@@ -253,16 +232,6 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 			gtk_tree_store_set(store, &iter, col_idx++, s, -1);
 		}
 
-		list_for_each_entry(se, &hist_entry__sort_list, list) {
-			if (se->elide)
-				continue;
-
-			se->se_snprintf(h, s, ARRAY_SIZE(s),
-					hists__col_len(hists, se->se_width_idx));
-
-			gtk_tree_store_set(store, &iter, col_idx++, s, -1);
-		}
-
 		if (symbol_conf.use_callchain && sort__has_sym) {
 			if (callchain_param.mode == CHAIN_GRAPH_REL)
 				total = h->stat.period;
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index c65a7fd744c6..32d2dfd794d9 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -352,8 +352,18 @@ LIST_HEAD(perf_hpp__sort_list);
 #undef __HPP_ENTRY_RAW_FN
 
 
+void perf_hpp__setup_output_field(void);
+
 void perf_hpp__init(void)
 {
+	struct list_head *list;
+	int i;
+
+	for (i = 0; i < PERF_HPP__MAX_INDEX; i++) {
+		INIT_LIST_HEAD(&perf_hpp__format[i].list);
+		INIT_LIST_HEAD(&perf_hpp__format[i].sort_list);
+	}
+
 	perf_hpp__column_enable(PERF_HPP__OVERHEAD);
 
 	if (symbol_conf.show_cpu_utilization) {
@@ -371,6 +381,13 @@ void perf_hpp__init(void)
 
 	if (symbol_conf.show_total_period)
 		perf_hpp__column_enable(PERF_HPP__PERIOD);
+
+	/* prepend overhead field for backward compatiblity.  */
+	list = &perf_hpp__format[PERF_HPP__OVERHEAD].sort_list;
+	if (list_empty(list))
+		list_add(list, &perf_hpp__sort_list);
+
+	perf_hpp__setup_output_field();
 }
 
 void perf_hpp__column_register(struct perf_hpp_fmt *format)
@@ -389,6 +406,17 @@ void perf_hpp__column_enable(unsigned col)
 	perf_hpp__column_register(&perf_hpp__format[col]);
 }
 
+void perf_hpp__setup_output_field(void)
+{
+	struct perf_hpp_fmt *fmt;
+
+	/* append sort keys to output field */
+	perf_hpp__for_each_sort_list(fmt) {
+		if (list_empty(&fmt->list))
+			perf_hpp__column_register(fmt);
+	}
+}
+
 int hist_entry__sort_snprintf(struct hist_entry *he, char *s, size_t size,
 			      struct hists *hists)
 {
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index d59893edf031..6c4ea9c89220 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -353,8 +353,7 @@ static int hist_entry__fprintf(struct hist_entry *he, size_t size,
 	if (size == 0 || size > bfsz)
 		size = hpp.size = bfsz;
 
-	ret = hist_entry__period_snprintf(&hpp, he);
-	hist_entry__sort_snprintf(he, bf + ret, size - ret, hists);
+	hist_entry__period_snprintf(&hpp, he);
 
 	ret = fprintf(fp, "%s\n", bf);
 
@@ -386,28 +385,9 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 
 	init_rem_hits();
 
-	if (!show_header)
-		goto print_entries;
-
-	fprintf(fp, "# ");
-
-	perf_hpp__for_each_format(fmt) {
-		if (!first)
-			fprintf(fp, "%s", sep ?: "  ");
-		else
-			first = false;
-
-		fmt->header(fmt, &dummy_hpp, hists_to_evsel(hists));
-		fprintf(fp, "%s", bf);
-	}
-
 	list_for_each_entry(se, &hist_entry__sort_list, list) {
 		if (se->elide)
 			continue;
-		if (sep) {
-			fprintf(fp, "%c%s", *sep, se->se_header);
-			continue;
-		}
 		width = strlen(se->se_header);
 		if (symbol_conf.col_width_list_str) {
 			if (col_width) {
@@ -420,7 +400,21 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 		}
 		if (!hists__new_col_len(hists, se->se_width_idx, width))
 			width = hists__col_len(hists, se->se_width_idx);
-		fprintf(fp, "  %*s", width, se->se_header);
+	}
+
+	if (!show_header)
+		goto print_entries;
+
+	fprintf(fp, "# ");
+
+	perf_hpp__for_each_format(fmt) {
+		if (!first)
+			fprintf(fp, "%s", sep ?: "  ");
+		else
+			first = false;
+
+		fmt->header(fmt, &dummy_hpp, hists_to_evsel(hists));
+		fprintf(fp, "%s", bf);
 	}
 
 	fprintf(fp, "\n");
@@ -447,20 +441,6 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 			fprintf(fp, ".");
 	}
 
-	list_for_each_entry(se, &hist_entry__sort_list, list) {
-		unsigned int i;
-
-		if (se->elide)
-			continue;
-
-		fprintf(fp, "  ");
-		width = hists__col_len(hists, se->se_width_idx);
-		if (width == 0)
-			width = strlen(se->se_header);
-		for (i = 0; i < width; i++)
-			fprintf(fp, ".");
-	}
-
 	fprintf(fp, "\n");
 	if (max_rows && ++nr_rows >= max_rows)
 		goto out;
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index a73aea306647..017b4896a9a0 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -574,7 +574,7 @@ static int hist_entry__sort(struct hist_entry *a, struct hist_entry *b)
 	struct perf_hpp_fmt *fmt;
 	int64_t cmp = 0;
 
-	perf_hpp__for_each_format(fmt) {
+	perf_hpp__for_each_sort_list(fmt) {
 		cmp = fmt->sort(a, b);
 		if (cmp)
 			break;
-- 
1.9.2

[PATCH 07/17] perf ui: Get rid of callback from __hpp__fmt()

[Namhyung Kim]

The callback was used by TUI for determining color of folded sign
using percent of first field/column.  But it cannot be used anymore
since it now support dynamic reording of output field.

So move the logic to the hist_browser__show_entry().

Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 62 ++++++++++++++++--------------------------
 tools/perf/ui/gtk/hists.c      |  2 +-
 tools/perf/ui/hist.c           | 28 ++++++-------------
 tools/perf/util/hist.h         |  4 +--
 4 files changed, 34 insertions(+), 62 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index c4616e9f309a..415bdeefd532 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -587,35 +587,6 @@ struct hpp_arg {
 	bool current_entry;
 };
 
-static int __hpp__overhead_callback(struct perf_hpp *hpp, bool front)
-{
-	struct hpp_arg *arg = hpp->ptr;
-
-	if (arg->current_entry && arg->b->navkeypressed)
-		ui_browser__set_color(arg->b, HE_COLORSET_SELECTED);
-	else
-		ui_browser__set_color(arg->b, HE_COLORSET_NORMAL);
-
-	if (front) {
-		if (!symbol_conf.use_callchain)
-			return 0;
-
-		slsmg_printf("%c ", arg->folded_sign);
-		return 2;
-	}
-
-	return 0;
-}
-
-static int __hpp__color_callback(struct perf_hpp *hpp, bool front __maybe_unused)
-{
-	struct hpp_arg *arg = hpp->ptr;
-
-	if (!arg->current_entry || !arg->b->navkeypressed)
-		ui_browser__set_color(arg->b, HE_COLORSET_NORMAL);
-	return 0;
-}
-
 static int __hpp__slsmg_color_printf(struct perf_hpp *hpp, const char *fmt, ...)
 {
 	struct hpp_arg *arg = hpp->ptr;
@@ -636,7 +607,7 @@ static int __hpp__slsmg_color_printf(struct perf_hpp *hpp, const char *fmt, ...)
 	return ret;
 }
 
-#define __HPP_COLOR_PERCENT_FN(_type, _field, _cb)			\
+#define __HPP_COLOR_PERCENT_FN(_type, _field)				\
 static u64 __hpp_get_##_field(struct hist_entry *he)			\
 {									\
 	return he->stat._field;						\
@@ -647,15 +618,15 @@ hist_browser__hpp_color_##_type(struct perf_hpp_fmt *fmt __maybe_unused,\
 				struct perf_hpp *hpp,			\
 				struct hist_entry *he)			\
 {									\
-	return __hpp__fmt(hpp, he, __hpp_get_##_field, _cb, " %6.2f%%",	\
+	return __hpp__fmt(hpp, he, __hpp_get_##_field, " %6.2f%%",	\
 			  __hpp__slsmg_color_printf, true);		\
 }
 
-__HPP_COLOR_PERCENT_FN(overhead, period, __hpp__overhead_callback)
-__HPP_COLOR_PERCENT_FN(overhead_sys, period_sys, __hpp__color_callback)
-__HPP_COLOR_PERCENT_FN(overhead_us, period_us, __hpp__color_callback)
-__HPP_COLOR_PERCENT_FN(overhead_guest_sys, period_guest_sys, __hpp__color_callback)
-__HPP_COLOR_PERCENT_FN(overhead_guest_us, period_guest_us, __hpp__color_callback)
+__HPP_COLOR_PERCENT_FN(overhead, period)
+__HPP_COLOR_PERCENT_FN(overhead_sys, period_sys)
+__HPP_COLOR_PERCENT_FN(overhead_us, period_us)
+__HPP_COLOR_PERCENT_FN(overhead_guest_sys, period_guest_sys)
+__HPP_COLOR_PERCENT_FN(overhead_guest_us, period_guest_us)
 
 #undef __HPP_COLOR_PERCENT_FN
 
@@ -700,7 +671,7 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 
 	if (row_offset == 0) {
 		struct hpp_arg arg = {
-			.b 		= &browser->b,
+			.b		= &browser->b,
 			.folded_sign	= folded_sign,
 			.current_entry	= current_entry,
 		};
@@ -713,11 +684,24 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 		ui_browser__gotorc(&browser->b, row, 0);
 
 		perf_hpp__for_each_format(fmt) {
-			if (!first) {
+			if (current_entry && browser->b.navkeypressed) {
+				ui_browser__set_color(&browser->b,
+						      HE_COLORSET_SELECTED);
+			} else {
+				ui_browser__set_color(&browser->b,
+						      HE_COLORSET_NORMAL);
+			}
+
+			if (first) {
+				if (symbol_conf.use_callchain) {
+					slsmg_printf("%c ", folded_sign);
+					width -= 2;
+				}
+				first = false;
+			} else {
 				slsmg_printf("  ");
 				width -= 2;
 			}
-			first = false;
 
 			if (fmt->color) {
 				width -= fmt->color(fmt, &hpp, entry);
diff --git a/tools/perf/ui/gtk/hists.c b/tools/perf/ui/gtk/hists.c
index d5c336e1bb14..2237245bfac0 100644
--- a/tools/perf/ui/gtk/hists.c
+++ b/tools/perf/ui/gtk/hists.c
@@ -43,7 +43,7 @@ static int perf_gtk__hpp_color_##_type(struct perf_hpp_fmt *fmt __maybe_unused,
 				       struct perf_hpp *hpp,			\
 				       struct hist_entry *he)			\
 {										\
-	return __hpp__fmt(hpp, he, he_get_##_field, NULL, " %6.2f%%",		\
+	return __hpp__fmt(hpp, he, he_get_##_field, " %6.2f%%",			\
 			  __percent_color_snprintf, true);			\
 }
 
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 32d2dfd794d9..400dad8c41e4 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -16,20 +16,15 @@
 })
 
 int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
-	       hpp_field_fn get_field, hpp_callback_fn callback,
-	       const char *fmt, hpp_snprint_fn print_fn, bool fmt_percent)
+	       hpp_field_fn get_field, const char *fmt,
+	       hpp_snprint_fn print_fn, bool fmt_percent)
 {
-	int ret = 0;
+	int ret;
 	struct hists *hists = he->hists;
 	struct perf_evsel *evsel = hists_to_evsel(hists);
 	char *buf = hpp->buf;
 	size_t size = hpp->size;
 
-	if (callback) {
-		ret = callback(hpp, true);
-		advance_hpp(hpp, ret);
-	}
-
 	if (fmt_percent) {
 		double percent = 0.0;
 		u64 total = hists__total_period(hists);
@@ -37,9 +32,9 @@ int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
 		if (total)
 			percent = 100.0 * get_field(he) / total;
 
-		ret += hpp__call_print_fn(hpp, print_fn, fmt, percent);
+		ret = hpp__call_print_fn(hpp, print_fn, fmt, percent);
 	} else
-		ret += hpp__call_print_fn(hpp, print_fn, fmt, get_field(he));
+		ret = hpp__call_print_fn(hpp, print_fn, fmt, get_field(he));
 
 	if (perf_evsel__is_group_event(evsel)) {
 		int prev_idx, idx_delta;
@@ -99,13 +94,6 @@ int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
 		}
 	}
 
-	if (callback) {
-		int __ret = callback(hpp, false);
-
-		advance_hpp(hpp, __ret);
-		ret += __ret;
-	}
-
 	/*
 	 * Restore original buf and size as it's where caller expects
 	 * the result will be saved.
@@ -235,7 +223,7 @@ static u64 he_get_##_field(struct hist_entry *he)				\
 static int hpp__color_##_type(struct perf_hpp_fmt *fmt __maybe_unused,		\
 			      struct perf_hpp *hpp, struct hist_entry *he) 	\
 {										\
-	return __hpp__fmt(hpp, he, he_get_##_field, NULL, " %6.2f%%",		\
+	return __hpp__fmt(hpp, he, he_get_##_field, " %6.2f%%",			\
 			  hpp_color_scnprintf, true);				\
 }
 
@@ -244,7 +232,7 @@ static int hpp__entry_##_type(struct perf_hpp_fmt *_fmt __maybe_unused,		\
 			      struct perf_hpp *hpp, struct hist_entry *he) 	\
 {										\
 	const char *fmt = symbol_conf.field_sep ? " %.2f" : " %6.2f%%";		\
-	return __hpp__fmt(hpp, he, he_get_##_field, NULL, fmt,			\
+	return __hpp__fmt(hpp, he, he_get_##_field, fmt,			\
 			  hpp_entry_scnprintf, true);				\
 }
 
@@ -264,7 +252,7 @@ static int hpp__entry_##_type(struct perf_hpp_fmt *_fmt __maybe_unused,		\
 			      struct perf_hpp *hpp, struct hist_entry *he) 	\
 {										\
 	const char *fmt = symbol_conf.field_sep ? " %"PRIu64 : " %11"PRIu64;	\
-	return __hpp__fmt(hpp, he, he_get_raw_##_field, NULL, fmt,		\
+	return __hpp__fmt(hpp, he, he_get_raw_##_field, fmt,			\
 			  hpp_entry_scnprintf, false);				\
 }
 
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 0f8d46803c92..4f70fb490a63 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -195,8 +195,8 @@ typedef int (*hpp_callback_fn)(struct perf_hpp *hpp, bool front);
 typedef int (*hpp_snprint_fn)(struct perf_hpp *hpp, const char *fmt, ...);
 
 int __hpp__fmt(struct perf_hpp *hpp, struct hist_entry *he,
-	       hpp_field_fn get_field, hpp_callback_fn callback,
-	       const char *fmt, hpp_snprint_fn print_fn, bool fmt_percent);
+	       hpp_field_fn get_field, const char *fmt,
+	       hpp_snprint_fn print_fn, bool fmt_percent);
 
 static inline void advance_hpp(struct perf_hpp *hpp, int inc)
 {
-- 
1.9.2

[PATCH 08/17] perf tools: Allow hpp fields to be sort keys

[Namhyung Kim]

Add overhead{,_sys,_us,_guest_sys,_guest_us}, sample and period sort
keys so that they can be selected with --sort/-s option.

  $ perf report -s period,comm --stdio
  ...
  # Overhead        Period          Command
  # ........  ............  ...............
  #
      47.06%           152          swapper
      13.93%            45  qemu-system-arm
      12.38%            40         synergys
       3.72%            12          firefox
       2.48%             8            xchat

Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/hist.c   |  9 +++++++--
 tools/perf/util/sort.c | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 400dad8c41e4..f3e96463550b 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -348,8 +348,13 @@ void perf_hpp__init(void)
 	int i;
 
 	for (i = 0; i < PERF_HPP__MAX_INDEX; i++) {
-		INIT_LIST_HEAD(&perf_hpp__format[i].list);
-		INIT_LIST_HEAD(&perf_hpp__format[i].sort_list);
+		struct perf_hpp_fmt *fmt = &perf_hpp__format[i];
+
+		INIT_LIST_HEAD(&fmt->list);
+
+		/* sort_list may be linked by setup_sorting() */
+		if (fmt->sort_list.next == NULL)
+			INIT_LIST_HEAD(&fmt->sort_list);
 	}
 
 	perf_hpp__column_enable(PERF_HPP__OVERHEAD);
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index b2829f947053..916652af8304 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1028,6 +1028,26 @@ static struct sort_dimension memory_sort_dimensions[] = {
 
 #undef DIM
 
+struct hpp_dimension {
+	const char		*name;
+	struct perf_hpp_fmt	*fmt;
+	int			taken;
+};
+
+#define DIM(d, n) { .name = n, .fmt = &perf_hpp__format[d], }
+
+static struct hpp_dimension hpp_sort_dimensions[] = {
+	DIM(PERF_HPP__OVERHEAD, "overhead"),
+	DIM(PERF_HPP__OVERHEAD_SYS, "overhead_sys"),
+	DIM(PERF_HPP__OVERHEAD_US, "overhead_us"),
+	DIM(PERF_HPP__OVERHEAD_GUEST_SYS, "overhead_guest_sys"),
+	DIM(PERF_HPP__OVERHEAD_GUEST_US, "overhead_guest_us"),
+	DIM(PERF_HPP__SAMPLES, "sample"),
+	DIM(PERF_HPP__PERIOD, "period"),
+};
+
+#undef DIM
+
 struct hpp_sort_entry {
 	struct perf_hpp_fmt hpp;
 	struct sort_entry *se;
@@ -1115,6 +1135,16 @@ static int __sort_dimension__add(struct sort_dimension *sd, enum sort_type idx)
 	return 0;
 }
 
+static int __hpp_dimension__add(struct hpp_dimension *hd)
+{
+	if (!hd->taken) {
+		hd->taken = 1;
+
+		perf_hpp__register_sort_field(hd->fmt);
+	}
+	return 0;
+}
+
 int sort_dimension__add(const char *tok)
 {
 	unsigned int i;
@@ -1144,6 +1174,15 @@ int sort_dimension__add(const char *tok)
 		return __sort_dimension__add(sd, i);
 	}
 
+	for (i = 0; i < ARRAY_SIZE(hpp_sort_dimensions); i++) {
+		struct hpp_dimension *hd = &hpp_sort_dimensions[i];
+
+		if (strncasecmp(tok, hd->name, strlen(tok)))
+			continue;
+
+		return __hpp_dimension__add(hd);
+	}
+
 	for (i = 0; i < ARRAY_SIZE(bstack_sort_dimensions); i++) {
 		struct sort_dimension *sd = &bstack_sort_dimensions[i];
 
-- 
1.9.2

[PATCH 09/17] perf tools: Consolidate management of default sort orders

[Namhyung Kim]

The perf uses different default sort orders for different use-cases,
and this was scattered throughout the code.  Add get_default_sort_
order() function to handle this and change initial value of sort_order
to NULL to distinguish it from user-given one.

Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c | 18 ------------------
 tools/perf/builtin-top.c    |  3 +--
 tools/perf/util/sort.c      | 25 +++++++++++++++++++++++--
 tools/perf/util/sort.h      |  1 +
 4 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 76e2bb6cf571..26de5c14369b 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -809,30 +809,12 @@ repeat:
 	if (branch_mode == -1 && has_br_stack)
 		sort__mode = SORT_MODE__BRANCH;
 
-	/* sort__mode could be NORMAL if --no-branch-stack */
-	if (sort__mode == SORT_MODE__BRANCH) {
-		/*
-		 * if no sort_order is provided, then specify
-		 * branch-mode specific order
-		 */
-		if (sort_order == default_sort_order)
-			sort_order = "comm,dso_from,symbol_from,"
-				     "dso_to,symbol_to";
-
-	}
 	if (report.mem_mode) {
 		if (sort__mode == SORT_MODE__BRANCH) {
 			pr_err("branch and mem mode incompatible\n");
 			goto error;
 		}
 		sort__mode = SORT_MODE__MEMORY;
-
-		/*
-		 * if no sort_order is provided, then specify
-		 * branch-mode specific order
-		 */
-		if (sort_order == default_sort_order)
-			sort_order = "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked";
 	}
 
 	if (setup_sorting() < 0) {
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 37d30460bada..bb2aa6645a7e 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1137,8 +1137,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (argc)
 		usage_with_options(top_usage, options);
 
-	if (sort_order == default_sort_order)
-		sort_order = "dso,symbol";
+	sort__mode = SORT_MODE__TOP;
 
 	if (setup_sorting() < 0) {
 		parse_options_usage(top_usage, options, "s", 1);
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 916652af8304..2f83965ab2c0 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -8,7 +8,10 @@ regex_t		parent_regex;
 const char	default_parent_pattern[] = "^sys_|^do_page_fault";
 const char	*parent_pattern = default_parent_pattern;
 const char	default_sort_order[] = "comm,dso,symbol";
-const char	*sort_order = default_sort_order;
+const char	default_branch_sort_order[] = "comm,dso_from,symbol_from,dso_to,symbol_to";
+const char	default_mem_sort_order[] = "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked";
+const char	default_top_sort_order[] = "dso,symbol";
+const char	*sort_order;
 regex_t		ignore_callees_regex;
 int		have_ignore_callees = 0;
 int		sort__need_collapse = 0;
@@ -1218,11 +1221,29 @@ int sort_dimension__add(const char *tok)
 	return -ESRCH;
 }
 
+static const char *get_default_sort_order(void)
+{
+	const char *default_sort_orders[] = {
+		default_sort_order,
+		default_branch_sort_order,
+		default_mem_sort_order,
+		default_top_sort_order,
+	};
+
+	BUG_ON(sort__mode > ARRAY_SIZE(default_sort_orders));
+
+	return default_sort_orders[sort__mode];
+}
+
 int setup_sorting(void)
 {
-	char *tmp, *tok, *str = strdup(sort_order);
+	char *tmp, *tok, *str;
 	int ret = 0;
 
+	if (sort_order == NULL)
+		sort_order = get_default_sort_order();
+
+	str = strdup(sort_order);
 	if (str == NULL) {
 		error("Not enough memory to setup sort keys");
 		return -ENOMEM;
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 43e5ff42a609..35b53cc56feb 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -133,6 +133,7 @@ enum sort_mode {
 	SORT_MODE__NORMAL,
 	SORT_MODE__BRANCH,
 	SORT_MODE__MEMORY,
+	SORT_MODE__TOP,
 };
 
 enum sort_type {
-- 
1.9.2

[PATCH 10/17] perf tools: Call perf_hpp__init() before setting up GUI browsers

[Namhyung Kim]

So that it can be set properly prior to set up output fields.  That
makes easy to handle/warn errors during the setup since it doesn't
need to be bothered with the GUI.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c    | 6 +++---
 tools/perf/builtin-top.c       | 2 ++
 tools/perf/ui/browsers/hists.c | 2 --
 tools/perf/ui/gtk/hists.c      | 2 --
 tools/perf/ui/setup.c          | 2 --
 5 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 26de5c14369b..ca9f3742f887 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -827,16 +827,16 @@ repeat:
 			goto error;
 	}
 
+	perf_hpp__init();
+
 	/* Force tty output for header output. */
 	if (report.header || report.header_only)
 		use_browser = 0;
 
 	if (strcmp(input_name, "-") != 0)
 		setup_browser(true);
-	else {
+	else
 		use_browser = 0;
-		perf_hpp__init();
-	}
 
 	if (report.header || report.header_only) {
 		perf_session__fprintf_info(session, stdout,
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index bb2aa6645a7e..9309629394dd 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1147,6 +1147,8 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	/* display thread wants entries to be collapsed in a different tree */
 	sort__need_collapse = 1;
 
+	perf_hpp__init();
+
 	if (top.use_stdio)
 		use_browser = 0;
 	else if (top.use_tui)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 415bdeefd532..5c85240aec18 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -632,8 +632,6 @@ __HPP_COLOR_PERCENT_FN(overhead_guest_us, period_guest_us)
 
 void hist_browser__init_hpp(void)
 {
-	perf_hpp__init();
-
 	perf_hpp__format[PERF_HPP__OVERHEAD].color =
 				hist_browser__hpp_color_overhead;
 	perf_hpp__format[PERF_HPP__OVERHEAD_SYS].color =
diff --git a/tools/perf/ui/gtk/hists.c b/tools/perf/ui/gtk/hists.c
index 2237245bfac0..fd52669018ee 100644
--- a/tools/perf/ui/gtk/hists.c
+++ b/tools/perf/ui/gtk/hists.c
@@ -58,8 +58,6 @@ __HPP_COLOR_PERCENT_FN(overhead_guest_us, period_guest_us)
 
 void perf_gtk__init_hpp(void)
 {
-	perf_hpp__init();
-
 	perf_hpp__format[PERF_HPP__OVERHEAD].color =
 				perf_gtk__hpp_color_overhead;
 	perf_hpp__format[PERF_HPP__OVERHEAD_SYS].color =
diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
index 5df5140a9f29..ba51fa8a1176 100644
--- a/tools/perf/ui/setup.c
+++ b/tools/perf/ui/setup.c
@@ -86,8 +86,6 @@ void setup_browser(bool fallback_to_pager)
 		use_browser = 0;
 		if (fallback_to_pager)
 			setup_pager();
-
-		perf_hpp__init();
 		break;
 	}
 }
-- 
1.9.2

[PATCH 11/17] perf report: Add -F option to specify output fields

[Namhyung Kim]

The -F/--fields option is to allow user setup output field in any
order.  It can recieve any sort keys and following (hpp) fields:

  overhead, overhead_sys, overhead_us, sample and period

If guest profiling is enabled, overhead_guest_{sys,us} will be
available too.

The output fields also affect sort order unless you give -s/--sort
option.  And any keys specified on -s option, will also be added to
the output field list automatically.

  $ perf report -F sym,sample,overhead
  ...
  #                     Symbol       Samples  Overhead
  # ..........................  ............  ........
  #
    [.] __cxa_atexit                       2     2.50%
    [.] __libc_csu_init                    4     5.00%
    [.] __new_exitfn                       3     3.75%
    [.] _dl_check_map_versions             1     1.25%
    [.] _dl_name_match_p                   4     5.00%
    [.] _dl_setup_hash                     1     1.25%
    [.] _dl_sysdep_start                   1     1.25%
    [.] _init                              5     6.25%
    [.] _setjmp                            6     7.50%
    [.] a                                  8    10.00%
    [.] b                                  8    10.00%
    [.] brk                                1     1.25%
    [.] c                                  8    10.00%

Note that, the example output above is captured after applying next
patch which fixes sort/comparing behavior.

Requested-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-report.txt |  10 ++
 tools/perf/builtin-report.c              |   7 ++
 tools/perf/ui/hist.c                     |  43 ++++++--
 tools/perf/util/hist.h                   |   5 +
 tools/perf/util/sort.c                   | 180 ++++++++++++++++++++++++++++++-
 tools/perf/util/sort.h                   |   2 +
 6 files changed, 237 insertions(+), 10 deletions(-)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index 09af66298564..8adbadf34b37 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -98,6 +98,16 @@ OPTIONS
 	And default sort keys are changed to comm, dso_from, symbol_from, dso_to
 	and symbol_to, see '--branch-stack'.
 
+-F::
+--fields=::
+	Specify output field - multiple keys can be specified in CSV format.
+	Following fields are available:
+	overhead, overhead_sys, overhead_us, sample and period.
+	Also it can contain any sort key(s).
+
+	By default, every sort keys not specified in -F will be appended
+	automatically.
+
 -p::
 --parent=<regex>::
         A regex filter to identify parent. The parent is a caller of this
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index ca9f3742f887..e7f6501cc811 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -705,6 +705,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		   " dso_to, dso_from, symbol_to, symbol_from, mispredict,"
 		   " weight, local_weight, mem, symbol_daddr, dso_daddr, tlb, "
 		   "snoop, locked, abort, in_tx, transaction"),
+	OPT_STRING('F', "fields", &field_order, "key[,keys...]",
+		   "output field(s): overhead, period, sample plus all of sort keys"),
 	OPT_BOOLEAN(0, "showcpuutilization", &symbol_conf.show_cpu_utilization,
 		    "Show sample percentage for different cpu modes"),
 	OPT_STRING('p', "parent", &parent_pattern, "regex",
@@ -829,6 +831,11 @@ repeat:
 
 	perf_hpp__init();
 
+	if (setup_output_field() < 0) {
+		parse_options_usage(report_usage, options, "F", 1);
+		goto error;
+	}
+
 	/* Force tty output for header output. */
 	if (report.header || report.header_only)
 		use_browser = 0;
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index f3e96463550b..fad84211b359 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -340,8 +340,6 @@ LIST_HEAD(perf_hpp__sort_list);
 #undef __HPP_ENTRY_RAW_FN
 
 
-void perf_hpp__setup_output_field(void);
-
 void perf_hpp__init(void)
 {
 	struct list_head *list;
@@ -357,6 +355,12 @@ void perf_hpp__init(void)
 			INIT_LIST_HEAD(&fmt->sort_list);
 	}
 
+	/*
+	 * If user specified field order, no need to setup default fields.
+	 */
+	if (field_order)
+		return;
+
 	perf_hpp__column_enable(PERF_HPP__OVERHEAD);
 
 	if (symbol_conf.show_cpu_utilization) {
@@ -379,8 +383,6 @@ void perf_hpp__init(void)
 	list = &perf_hpp__format[PERF_HPP__OVERHEAD].sort_list;
 	if (list_empty(list))
 		list_add(list, &perf_hpp__sort_list);
-
-	perf_hpp__setup_output_field();
 }
 
 void perf_hpp__column_register(struct perf_hpp_fmt *format)
@@ -405,8 +407,37 @@ void perf_hpp__setup_output_field(void)
 
 	/* append sort keys to output field */
 	perf_hpp__for_each_sort_list(fmt) {
-		if (list_empty(&fmt->list))
-			perf_hpp__column_register(fmt);
+		if (!list_empty(&fmt->list))
+			continue;
+
+		/*
+		 * sort entry fields are dynamically created,
+		 * so they can share a same sort key even though
+		 * the list is empty.
+		 */
+		if (perf_hpp__is_sort_entry(fmt)) {
+			struct perf_hpp_fmt *pos;
+
+			perf_hpp__for_each_format(pos) {
+				if (perf_hpp__same_sort_entry(pos, fmt))
+					goto next;
+			}
+		}
+
+		perf_hpp__column_register(fmt);
+next:
+		continue;
+	}
+}
+
+void perf_hpp__append_sort_keys(void)
+{
+	struct perf_hpp_fmt *fmt;
+
+	/* append output fields to sort keys */
+	perf_hpp__for_each_format(fmt) {
+		if (list_empty(&fmt->sort_list))
+			perf_hpp__register_sort_field(fmt);
 	}
 }
 
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 4f70fb490a63..4c5c2837a27d 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -189,6 +189,11 @@ void perf_hpp__init(void);
 void perf_hpp__column_register(struct perf_hpp_fmt *format);
 void perf_hpp__column_enable(unsigned col);
 void perf_hpp__register_sort_field(struct perf_hpp_fmt *format);
+void perf_hpp__setup_output_field(void);
+void perf_hpp__append_sort_keys(void);
+
+bool perf_hpp__is_sort_entry(struct perf_hpp_fmt *format);
+bool perf_hpp__same_sort_entry(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b);
 
 typedef u64 (*hpp_field_fn)(struct hist_entry *he);
 typedef int (*hpp_callback_fn)(struct perf_hpp *hpp, bool front);
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 2f83965ab2c0..639dd49f2884 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -12,6 +12,7 @@ const char	default_branch_sort_order[] = "comm,dso_from,symbol_from,dso_to,symbo
 const char	default_mem_sort_order[] = "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked";
 const char	default_top_sort_order[] = "dso,symbol";
 const char	*sort_order;
+const char	*field_order;
 regex_t		ignore_callees_regex;
 int		have_ignore_callees = 0;
 int		sort__need_collapse = 0;
@@ -1056,6 +1057,20 @@ struct hpp_sort_entry {
 	struct sort_entry *se;
 };
 
+bool perf_hpp__same_sort_entry(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b)
+{
+	struct hpp_sort_entry *hse_a;
+	struct hpp_sort_entry *hse_b;
+
+	if (!perf_hpp__is_sort_entry(a) || !perf_hpp__is_sort_entry(b))
+		return false;
+
+	hse_a = container_of(a, struct hpp_sort_entry, hpp);
+	hse_b = container_of(b, struct hpp_sort_entry, hpp);
+
+	return hse_a->se == hse_b->se;
+}
+
 static int __sort__hpp_header(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 			      struct perf_evsel *evsel)
 {
@@ -1091,14 +1106,15 @@ static int __sort__hpp_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 	return hse->se->se_snprintf(he, hpp->buf, hpp->size, len);
 }
 
-static int __sort_dimension__add_hpp(struct sort_dimension *sd)
+static struct hpp_sort_entry *
+__sort_dimension__alloc_hpp(struct sort_dimension *sd)
 {
 	struct hpp_sort_entry *hse;
 
 	hse = malloc(sizeof(*hse));
 	if (hse == NULL) {
 		pr_err("Memory allocation failed\n");
-		return -1;
+		return NULL;
 	}
 
 	hse->se = sd->entry;
@@ -1114,16 +1130,42 @@ static int __sort_dimension__add_hpp(struct sort_dimension *sd)
 	INIT_LIST_HEAD(&hse->hpp.list);
 	INIT_LIST_HEAD(&hse->hpp.sort_list);
 
+	return hse;
+}
+
+bool perf_hpp__is_sort_entry(struct perf_hpp_fmt *format)
+{
+	return format->header == __sort__hpp_header;
+}
+
+static int __sort_dimension__add_hpp_sort(struct sort_dimension *sd)
+{
+	struct hpp_sort_entry *hse = __sort_dimension__alloc_hpp(sd);
+
+	if (hse == NULL)
+		return -1;
+
 	perf_hpp__register_sort_field(&hse->hpp);
 	return 0;
 }
 
+static int __sort_dimension__add_hpp_output(struct sort_dimension *sd)
+{
+	struct hpp_sort_entry *hse = __sort_dimension__alloc_hpp(sd);
+
+	if (hse == NULL)
+		return -1;
+
+	perf_hpp__column_register(&hse->hpp);
+	return 0;
+}
+
 static int __sort_dimension__add(struct sort_dimension *sd, enum sort_type idx)
 {
 	if (sd->taken)
 		return 0;
 
-	if (__sort_dimension__add_hpp(sd) < 0)
+	if (__sort_dimension__add_hpp_sort(sd) < 0)
 		return -1;
 
 	if (sd->entry->se_collapse)
@@ -1148,6 +1190,28 @@ static int __hpp_dimension__add(struct hpp_dimension *hd)
 	return 0;
 }
 
+static int __sort_dimension__add_output(struct sort_dimension *sd)
+{
+	if (sd->taken)
+		return 0;
+
+	if (__sort_dimension__add_hpp_output(sd) < 0)
+		return -1;
+
+	sd->taken = 1;
+	return 0;
+}
+
+static int __hpp_dimension__add_output(struct hpp_dimension *hd)
+{
+	if (!hd->taken) {
+		hd->taken = 1;
+
+		perf_hpp__column_register(hd->fmt);
+	}
+	return 0;
+}
+
 int sort_dimension__add(const char *tok)
 {
 	unsigned int i;
@@ -1240,8 +1304,17 @@ int setup_sorting(void)
 	char *tmp, *tok, *str;
 	int ret = 0;
 
-	if (sort_order == NULL)
+	if (sort_order == NULL) {
+		if (field_order) {
+			/*
+			 * If user specified field order but no sort order,
+			 * we'll honor it and not add default sort orders.
+			 */
+			return 0;
+		}
+
 		sort_order = get_default_sort_order();
+	}
 
 	str = strdup(sort_order);
 	if (str == NULL) {
@@ -1328,3 +1401,102 @@ void sort__setup_elide(FILE *output)
 	list_for_each_entry(se, &hist_entry__sort_list, list)
 		se->elide = false;
 }
+
+static int output_field_add(char *tok)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(common_sort_dimensions); i++) {
+		struct sort_dimension *sd = &common_sort_dimensions[i];
+
+		if (strncasecmp(tok, sd->name, strlen(tok)))
+			continue;
+
+		return __sort_dimension__add_output(sd);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(hpp_sort_dimensions); i++) {
+		struct hpp_dimension *hd = &hpp_sort_dimensions[i];
+
+		if (strncasecmp(tok, hd->name, strlen(tok)))
+			continue;
+
+		return __hpp_dimension__add_output(hd);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(bstack_sort_dimensions); i++) {
+		struct sort_dimension *sd = &bstack_sort_dimensions[i];
+
+		if (strncasecmp(tok, sd->name, strlen(tok)))
+			continue;
+
+		return __sort_dimension__add_output(sd);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(memory_sort_dimensions); i++) {
+		struct sort_dimension *sd = &memory_sort_dimensions[i];
+
+		if (strncasecmp(tok, sd->name, strlen(tok)))
+			continue;
+
+		return __sort_dimension__add_output(sd);
+	}
+
+	return -ESRCH;
+}
+
+static void reset_dimensions(void)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(common_sort_dimensions); i++)
+		common_sort_dimensions[i].taken = 0;
+
+	for (i = 0; i < ARRAY_SIZE(hpp_sort_dimensions); i++)
+		hpp_sort_dimensions[i].taken = 0;
+
+	for (i = 0; i < ARRAY_SIZE(bstack_sort_dimensions); i++)
+		bstack_sort_dimensions[i].taken = 0;
+
+	for (i = 0; i < ARRAY_SIZE(memory_sort_dimensions); i++)
+		memory_sort_dimensions[i].taken = 0;
+}
+
+int setup_output_field(void)
+{
+	char *tmp, *tok, *str;
+	int ret = 0;
+
+	if (field_order == NULL)
+		goto out;
+
+	reset_dimensions();
+
+	str = strdup(field_order);
+	if (str == NULL) {
+		error("Not enough memory to setup output fields");
+		return -ENOMEM;
+	}
+
+	for (tok = strtok_r(str, ", ", &tmp);
+			tok; tok = strtok_r(NULL, ", ", &tmp)) {
+		ret = output_field_add(tok);
+		if (ret == -EINVAL) {
+			error("Invalid --fields key: `%s'", tok);
+			break;
+		} else if (ret == -ESRCH) {
+			error("Unknown --fields key: `%s'", tok);
+			break;
+		}
+	}
+
+	free(str);
+
+out:
+	/* copy sort keys to output fields */
+	perf_hpp__setup_output_field();
+	/* and then copy output fields to sort keys */
+	perf_hpp__append_sort_keys();
+
+	return ret;
+}
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 35b53cc56feb..02706c9766d6 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -25,6 +25,7 @@
 
 extern regex_t parent_regex;
 extern const char *sort_order;
+extern const char *field_order;
 extern const char default_parent_pattern[];
 extern const char *parent_pattern;
 extern const char default_sort_order[];
@@ -190,6 +191,7 @@ extern struct sort_entry sort_thread;
 extern struct list_head hist_entry__sort_list;
 
 int setup_sorting(void);
+int setup_output_field(void);
 extern int sort_dimension__add(const char *);
 void sort__setup_elide(FILE *fp);
 
-- 
1.9.2

[PATCH 12/17] perf tools: Add ->sort() member to struct sort_entry

[Namhyung Kim]

Currently, what the sort_entry does is just identifying hist entries
so that they can be grouped properly.  However, with -F option
support, it indeed needs to sort entries appropriately to be shown to
users.  So add ->sort() member to do it.

Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.c | 27 ++++++++++++++++++++++-----
 tools/perf/util/sort.h |  1 +
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 639dd49f2884..1e7b80e517d5 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -98,6 +98,12 @@ sort__comm_collapse(struct hist_entry *left, struct hist_entry *right)
 	return comm__str(right->comm) - comm__str(left->comm);
 }
 
+static int64_t
+sort__comm_sort(struct hist_entry *left, struct hist_entry *right)
+{
+	return strcmp(comm__str(right->comm), comm__str(left->comm));
+}
+
 static int hist_entry__comm_snprintf(struct hist_entry *he, char *bf,
 				     size_t size, unsigned int width)
 {
@@ -108,6 +114,7 @@ struct sort_entry sort_comm = {
 	.se_header	= "Command",
 	.se_cmp		= sort__comm_cmp,
 	.se_collapse	= sort__comm_collapse,
+	.se_sort	= sort__comm_sort,
 	.se_snprintf	= hist_entry__comm_snprintf,
 	.se_width_idx	= HISTC_COMM,
 };
@@ -121,7 +128,7 @@ static int64_t _sort__dso_cmp(struct map *map_l, struct map *map_r)
 	const char *dso_name_l, *dso_name_r;
 
 	if (!dso_l || !dso_r)
-		return cmp_null(dso_l, dso_r);
+		return cmp_null(dso_r, dso_l);
 
 	if (verbose) {
 		dso_name_l = dso_l->long_name;
@@ -137,7 +144,7 @@ static int64_t _sort__dso_cmp(struct map *map_l, struct map *map_r)
 static int64_t
 sort__dso_cmp(struct hist_entry *left, struct hist_entry *right)
 {
-	return _sort__dso_cmp(left->ms.map, right->ms.map);
+	return _sort__dso_cmp(right->ms.map, left->ms.map);
 }
 
 static int _hist_entry__dso_snprintf(struct map *map, char *bf,
@@ -209,6 +216,15 @@ sort__sym_cmp(struct hist_entry *left, struct hist_entry *right)
 	return _sort__sym_cmp(left->ms.sym, right->ms.sym);
 }
 
+static int64_t
+sort__sym_sort(struct hist_entry *left, struct hist_entry *right)
+{
+	if (!left->ms.sym || !right->ms.sym)
+		return cmp_null(left->ms.sym, right->ms.sym);
+
+	return strcmp(right->ms.sym->name, left->ms.sym->name);
+}
+
 static int _hist_entry__sym_snprintf(struct map *map, struct symbol *sym,
 				     u64 ip, char level, char *bf, size_t size,
 				     unsigned int width)
@@ -255,6 +271,7 @@ static int hist_entry__sym_snprintf(struct hist_entry *he, char *bf,
 struct sort_entry sort_sym = {
 	.se_header	= "Symbol",
 	.se_cmp		= sort__sym_cmp,
+	.se_sort	= sort__sym_sort,
 	.se_snprintf	= hist_entry__sym_snprintf,
 	.se_width_idx	= HISTC_SYMBOL,
 };
@@ -282,7 +299,7 @@ sort__srcline_cmp(struct hist_entry *left, struct hist_entry *right)
 					    map__rip_2objdump(map, right->ip));
 		}
 	}
-	return strcmp(left->srcline, right->srcline);
+	return strcmp(right->srcline, left->srcline);
 }
 
 static int hist_entry__srcline_snprintf(struct hist_entry *he, char *bf,
@@ -310,7 +327,7 @@ sort__parent_cmp(struct hist_entry *left, struct hist_entry *right)
 	if (!sym_l || !sym_r)
 		return cmp_null(sym_l, sym_r);
 
-	return strcmp(sym_l->name, sym_r->name);
+	return strcmp(sym_r->name, sym_l->name);
 }
 
 static int hist_entry__parent_snprintf(struct hist_entry *he, char *bf,
@@ -1125,7 +1142,7 @@ __sort_dimension__alloc_hpp(struct sort_dimension *sd)
 
 	hse->hpp.cmp = sd->entry->se_cmp;
 	hse->hpp.collapse = sd->entry->se_collapse ? : sd->entry->se_cmp;
-	hse->hpp.sort = hse->hpp.collapse;
+	hse->hpp.sort = sd->entry->se_sort ? : hse->hpp.collapse;
 
 	INIT_LIST_HEAD(&hse->hpp.list);
 	INIT_LIST_HEAD(&hse->hpp.sort_list);
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 02706c9766d6..cd679a56c81d 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -181,6 +181,7 @@ struct sort_entry {
 
 	int64_t (*se_cmp)(struct hist_entry *, struct hist_entry *);
 	int64_t (*se_collapse)(struct hist_entry *, struct hist_entry *);
+	int64_t	(*se_sort)(struct hist_entry *, struct hist_entry *);
 	int	(*se_snprintf)(struct hist_entry *he, char *bf, size_t size,
 			       unsigned int width);
 	u8	se_width_idx;
-- 
1.9.2

[PATCH 13/17] perf report/tui: Fix a bug when --fields/sort is given

[Namhyung Kim]

The hists__filter_entries() function is called when down arrow key is
pressed for navigating through the entries in TUI.  It has a check for
filtering out entries that have very small overhead (under min_pcnt).

However it just assumed the entries are sorted by the overhead so when
it saw such a small overheaded entry, it just stopped navigating as an
optimization.  But it's not true anymore due to new --fields and
--sort optoin behavior and this case users cannot go down to a next
entry if ther's an entry with small overhead in-between.

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

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 5c85240aec18..2771b7aa84dd 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -783,10 +783,7 @@ static struct rb_node *hists__filter_entries(struct rb_node *nd,
 		if (total)
 			percent = h->stat.period * 100.0 / total;
 
-		if (percent < min_pcnt)
-			return NULL;
-
-		if (!h->filtered)
+		if (!h->filtered && percent >= min_pcnt)
 			return nd;
 
 		nd = rb_next(nd);
-- 
1.9.2

[PATCH 14/17] perf top: Add --fields option to specify output fields

[Namhyung Kim]

The --fields option is to allow user setup output field in any order.
It can recieve any sort keys and following (hpp) fields:

  overhead, overhead_sys, overhead_us, sample and period

If guest profiling is enabled, overhead_guest_{sys,us} will be
available too.

More more information, please see previous patch "perf report:
Add -F option to specify output fields"

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-top.txt | 9 +++++++++
 tools/perf/builtin-top.c              | 7 +++++++
 2 files changed, 16 insertions(+)

diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
index 64ed79c43639..feac28017419 100644
--- a/tools/perf/Documentation/perf-top.txt
+++ b/tools/perf/Documentation/perf-top.txt
@@ -115,6 +115,15 @@ Default is to monitor all CPUS.
 	Sort by key(s): pid, comm, dso, symbol, parent, srcline, weight,
 	local_weight, abort, in_tx, transaction
 
+--fields=::
+	Specify output field - multiple keys can be specified in CSV format.
+	Following fields are available:
+	overhead, overhead_sys, overhead_us, sample and period.
+	Also it can contain any sort key(s).
+
+	By default, every sort keys not specified in --field will be appended
+	automatically.
+
 -n::
 --show-nr-samples::
 	Show a column with the number of samples.
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 9309629394dd..7d133dff5e15 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1085,6 +1085,8 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_STRING('s', "sort", &sort_order, "key[,key2...]",
 		   "sort by key(s): pid, comm, dso, symbol, parent, weight, local_weight,"
 		   " abort, in_tx, transaction"),
+	OPT_STRING(0, "fields", &field_order, "key[,keys...]",
+		   "output field(s): overhead, period, sample plus all of sort keys"),
 	OPT_BOOLEAN('n', "show-nr-samples", &symbol_conf.show_nr_samples,
 		    "Show a column with the number of samples"),
 	OPT_CALLBACK_NOOPT('g', NULL, &top.record_opts,
@@ -1149,6 +1151,11 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	perf_hpp__init();
 
+	if (setup_output_field() < 0) {
+		parse_options_usage(top_usage, options, "fields", 0);
+		goto out_delete_evlist;
+	}
+
 	if (top.use_stdio)
 		use_browser = 0;
 	else if (top.use_tui)
-- 
1.9.2

[PATCH 15/17] perf diff: Add missing setup_output_field()

[Namhyung Kim]

The perf diff command itself doesn't make use of the --fields option,
it still needs to call the function since the output only work with
that way.

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

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 6ef80f22c1e2..7f8352f10ff3 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -1150,6 +1150,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (setup_sorting() < 0)
 		usage_with_options(diff_usage, options);
 
+	if (setup_output_field() < 0)
+		usage_with_options(diff_usage, options);
+
 	setup_pager();
 
 	sort__setup_elide(NULL);
-- 
1.9.2

[PATCH 16/17] perf tools: Skip elided sort entries

[Namhyung Kim]

When it converted sort entries to hpp formats, it missed se->elide
handling, so add it for compatibility.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c |  3 +++
 tools/perf/ui/gtk/hists.c      |  6 ++++++
 tools/perf/ui/stdio/hist.c     |  9 +++++++++
 tools/perf/util/hist.c         |  9 +++++++++
 tools/perf/util/hist.h         |  1 +
 tools/perf/util/sort.c         | 11 +++++++++++
 6 files changed, 39 insertions(+)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 2771b7aa84dd..8895b4717015 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -682,6 +682,9 @@ static int hist_browser__show_entry(struct hist_browser *browser,
 		ui_browser__gotorc(&browser->b, row, 0);
 
 		perf_hpp__for_each_format(fmt) {
+			if (perf_hpp__should_skip(fmt))
+				continue;
+
 			if (current_entry && browser->b.navkeypressed) {
 				ui_browser__set_color(&browser->b,
 						      HE_COLORSET_SELECTED);
diff --git a/tools/perf/ui/gtk/hists.c b/tools/perf/ui/gtk/hists.c
index fd52669018ee..9d90683914d4 100644
--- a/tools/perf/ui/gtk/hists.c
+++ b/tools/perf/ui/gtk/hists.c
@@ -178,6 +178,9 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 	col_idx = 0;
 
 	perf_hpp__for_each_format(fmt) {
+		if (perf_hpp__should_skip(fmt))
+			continue;
+
 		fmt->header(fmt, &hpp, hists_to_evsel(hists));
 
 		gtk_tree_view_insert_column_with_attributes(GTK_TREE_VIEW(view),
@@ -222,6 +225,9 @@ static void perf_gtk__show_hists(GtkWidget *window, struct hists *hists,
 		col_idx = 0;
 
 		perf_hpp__for_each_format(fmt) {
+			if (perf_hpp__should_skip(fmt))
+				continue;
+
 			if (fmt->color)
 				fmt->color(fmt, &hpp, h);
 			else
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 6c4ea9c89220..5ecd433bee26 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -319,6 +319,9 @@ static int hist_entry__period_snprintf(struct perf_hpp *hpp,
 		return 0;
 
 	perf_hpp__for_each_format(fmt) {
+		if (perf_hpp__should_skip(fmt))
+			continue;
+
 		/*
 		 * If there's no field_sep, we still need
 		 * to display initial '  '.
@@ -408,6 +411,9 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 	fprintf(fp, "# ");
 
 	perf_hpp__for_each_format(fmt) {
+		if (perf_hpp__should_skip(fmt))
+			continue;
+
 		if (!first)
 			fprintf(fp, "%s", sep ?: "  ");
 		else
@@ -431,6 +437,9 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 	perf_hpp__for_each_format(fmt) {
 		unsigned int i;
 
+		if (perf_hpp__should_skip(fmt))
+			continue;
+
 		if (!first)
 			fprintf(fp, "%s", sep ?: "  ");
 		else
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 017b4896a9a0..7f47ba4d3f77 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -441,6 +441,9 @@ hist_entry__cmp(struct hist_entry *left, struct hist_entry *right)
 	int64_t cmp = 0;
 
 	perf_hpp__for_each_sort_list(fmt) {
+		if (perf_hpp__should_skip(fmt))
+			continue;
+
 		cmp = fmt->cmp(left, right);
 		if (cmp)
 			break;
@@ -456,6 +459,9 @@ hist_entry__collapse(struct hist_entry *left, struct hist_entry *right)
 	int64_t cmp = 0;
 
 	perf_hpp__for_each_sort_list(fmt) {
+		if (perf_hpp__should_skip(fmt))
+			continue;
+
 		cmp = fmt->collapse(left, right);
 		if (cmp)
 			break;
@@ -575,6 +581,9 @@ static int hist_entry__sort(struct hist_entry *a, struct hist_entry *b)
 	int64_t cmp = 0;
 
 	perf_hpp__for_each_sort_list(fmt) {
+		if (perf_hpp__should_skip(fmt))
+			continue;
+
 		cmp = fmt->sort(a, b);
 		if (cmp)
 			break;
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 4c5c2837a27d..826ad4b354a4 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -194,6 +194,7 @@ void perf_hpp__append_sort_keys(void);
 
 bool perf_hpp__is_sort_entry(struct perf_hpp_fmt *format);
 bool perf_hpp__same_sort_entry(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b);
+bool perf_hpp__should_skip(struct perf_hpp_fmt *format);
 
 typedef u64 (*hpp_field_fn)(struct hist_entry *he);
 typedef int (*hpp_callback_fn)(struct perf_hpp *hpp, bool front);
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 1e7b80e517d5..9dc33df4f9a6 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1355,6 +1355,17 @@ int setup_sorting(void)
 	return ret;
 }
 
+bool perf_hpp__should_skip(struct perf_hpp_fmt *format)
+{
+	if (perf_hpp__is_sort_entry(format)) {
+		struct hpp_sort_entry *hse;
+
+		hse = container_of(format, struct hpp_sort_entry, hpp);
+		return hse->se->elide;
+	}
+	return false;
+}
+
 static void sort_entry__setup_elide(struct sort_entry *se,
 				    struct strlist *list,
 				    const char *list_name, FILE *fp)
-- 
1.9.2

[PATCH 17/17] perf hists: Reset width of output fields with header length

[Namhyung Kim]

Some fields missed to set default column length so it broke align in
--stdio output.  Add perf_hpp__reset_width() to set it to a sane
default value.

Note that this change will ignore -w/--column-widths option for now.

Before:
  $ perf report -F cpu,comm,overhead --stdio
  ...
  # CPU          Command  Overhead
  #   ...............  ........
  #
    0          firefox     2.65%
    0      kworker/0:0     1.45%
    0          swapper     5.52%
    0         synergys     0.92%
    1          firefox     4.54%

After:
  # CPU          Command  Overhead
  # ...  ...............  ........
  #
      0          firefox     2.65%
      0      kworker/0:0     1.45%
      0          swapper     5.52%
      0         synergys     0.92%
      1          firefox     4.54%

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/stdio/hist.c | 21 +++------------------
 tools/perf/util/hist.h     |  1 +
 tools/perf/util/sort.c     | 12 ++++++++++++
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 5ecd433bee26..a1b133fed607 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -370,12 +370,10 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 		      int max_cols, float min_pcnt, FILE *fp)
 {
 	struct perf_hpp_fmt *fmt;
-	struct sort_entry *se;
 	struct rb_node *nd;
 	size_t ret = 0;
 	unsigned int width;
 	const char *sep = symbol_conf.field_sep;
-	const char *col_width = symbol_conf.col_width_list_str;
 	int nr_rows = 0;
 	char bf[96];
 	struct perf_hpp dummy_hpp = {
@@ -388,22 +386,9 @@ size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
 
 	init_rem_hits();
 
-	list_for_each_entry(se, &hist_entry__sort_list, list) {
-		if (se->elide)
-			continue;
-		width = strlen(se->se_header);
-		if (symbol_conf.col_width_list_str) {
-			if (col_width) {
-				hists__set_col_len(hists, se->se_width_idx,
-						   atoi(col_width));
-				col_width = strchr(col_width, ',');
-				if (col_width)
-					++col_width;
-			}
-		}
-		if (!hists__new_col_len(hists, se->se_width_idx, width))
-			width = hists__col_len(hists, se->se_width_idx);
-	}
+
+	perf_hpp__for_each_format(fmt)
+		perf_hpp__reset_width(fmt, hists);
 
 	if (!show_header)
 		goto print_entries;
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 826ad4b354a4..ab47e122341b 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -195,6 +195,7 @@ void perf_hpp__append_sort_keys(void);
 bool perf_hpp__is_sort_entry(struct perf_hpp_fmt *format);
 bool perf_hpp__same_sort_entry(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b);
 bool perf_hpp__should_skip(struct perf_hpp_fmt *format);
+void perf_hpp__reset_width(struct perf_hpp_fmt *fmt, struct hists *hists);
 
 typedef u64 (*hpp_field_fn)(struct hist_entry *he);
 typedef int (*hpp_callback_fn)(struct perf_hpp *hpp, bool front);
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 9dc33df4f9a6..d4502db36cba 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1088,6 +1088,18 @@ bool perf_hpp__same_sort_entry(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b)
 	return hse_a->se == hse_b->se;
 }
 
+void perf_hpp__reset_width(struct perf_hpp_fmt *fmt, struct hists *hists)
+{
+	struct hpp_sort_entry *hse;
+
+	if (!perf_hpp__is_sort_entry(fmt))
+		return;
+
+	hse = container_of(fmt, struct hpp_sort_entry, hpp);
+	hists__new_col_len(hists, hse->se->se_width_idx,
+			   strlen(hse->se->se_header));
+}
+
 static int __sort__hpp_header(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 			      struct perf_evsel *evsel)
 {
-- 
1.9.2

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Don Zickus]

On Wed, Apr 16, 2014 at 12:05:37PM +0900, Namhyung Kim wrote:
> Hello,
> 
> This is a patchset implementing -F/--fields option to setup output
> field/column as Ingo requested.  It depends on my --percentage
> patchset [1].
> 
> The -F option can receive any sort keys that -s option recognize, plus
> following fields (name can be changed):
> 
>   overhead, overhead_sys, overhead_us, sample, period
> 
> The overhead_guest_sys and overhead_guest_us might be avaiable when
> you profile guest machines.
> 
> Output will be sorted by in order of fields and sort keys passed by -s
> option will be added to the output field list automatically.  If you
> want to change the order of sorting you can give -s option in addition
> to -F option.  To support old behavior, it'll also prepend 'overhead'
> field to the sort keys unless you give -F option explicitly.

So I am struggling a little bit to get this working correctly.  I had it
in my head that I could sort internally with -s and re-sort the output
based on -F, but it doesn't seem to be working that way.

For example with

./perf mem record -a grep -r foo /* > /dev/null
./perf mem report -F overhead,symbol_daddr,pid -s symbol_daddr,pid --stdio 

I was thinking I could sort everything based on the symbol_daddr and pid.
Then re-sort the output to display the highest 'symbol_daddr,pid' pair.
But it didn't seem to work that way.  Instead it seems like I get the
original sort just displayed in the -F format.

Did I misunderstand what the -F can do or I am using it wrong?

Cheers,
Don



> 
> 
>   $ perf report -s dso,sym
>   ...
>   # Overhead  Shared Object                      Symbol
>   # ........  .............  ..........................
>   #
>       13.75%  ld-2.17.so     [.] strcmp                
>       10.00%  abc            [.] a                     
>       10.00%  abc            [.] b                     
>       10.00%  abc            [.] c                     
>        8.75%  abc            [.] main                  
>        7.50%  libc-2.17.so   [.] _setjmp               
>        6.25%  abc            [.] _init                 
>        6.25%  abc            [.] frame_dummy           
>        5.00%  abc            [.] __libc_csu_init       
>        5.00%  ld-2.17.so     [.] _dl_name_match_p      
>        3.75%  libc-2.17.so   [.] __new_exitfn          
>        2.50%  libc-2.17.so   [.] __cxa_atexit          
>        1.25%  ld-2.17.so     [.] _dl_check_map_versions
>        1.25%  ld-2.17.so     [.] _dl_setup_hash        
>        1.25%  ld-2.17.so     [.] _dl_sysdep_start      
>        1.25%  ld-2.17.so     [.] brk                   
>        1.25%  ld-2.17.so     [.] calloc@plt            
>        1.25%  ld-2.17.so     [.] dl_main               
>        1.25%  ld-2.17.so     [.] match_symbol          
>        1.25%  ld-2.17.so     [.] sbrk                  
>        1.25%  ld-2.17.so     [.] strlen                
> 
> 
>   $ perf report -F sym,sample,overhead
>   ...
>   #                     Symbol       Samples  Overhead
>   # ..........................  ............  ........
>   #
>     [.] __cxa_atexit                       2     2.50%
>     [.] __libc_csu_init                    4     5.00%
>     [.] __new_exitfn                       3     3.75%
>     [.] _dl_check_map_versions             1     1.25%
>     [.] _dl_name_match_p                   4     5.00%
>     [.] _dl_setup_hash                     1     1.25%
>     [.] _dl_sysdep_start                   1     1.25%
>     [.] _init                              5     6.25%
>     [.] _setjmp                            6     7.50%
>     [.] a                                  8    10.00%
>     [.] b                                  8    10.00%
>     [.] brk                                1     1.25%
>     [.] c                                  8    10.00%
>     [.] calloc@plt                         1     1.25%
>     [.] dl_main                            1     1.25%
>     [.] frame_dummy                        5     6.25%
>     [.] main                               7     8.75%
>     [.] match_symbol                       1     1.25%
>     [.] sbrk                               1     1.25%
>     [.] strcmp                            11    13.75%
>     [.] strlen                             1     1.25%
> 
> 
>   $ perf report -F sym,sample -s overhead
>   ...
>   #                     Symbol       Samples  Overhead
>   # ..........................  ............  ........
>   #
>     [.] strcmp                            11    13.75%
>     [.] a                                  8    10.00%
>     [.] b                                  8    10.00%
>     [.] c                                  8    10.00%
>     [.] main                               7     8.75%
>     [.] _setjmp                            6     7.50%
>     [.] _init                              5     6.25%
>     [.] frame_dummy                        5     6.25%
>     [.] __libc_csu_init                    4     5.00%
>     [.] _dl_name_match_p                   4     5.00%
>     [.] __new_exitfn                       3     3.75%
>     [.] __cxa_atexit                       2     2.50%
>     [.] _dl_check_map_versions             1     1.25%
>     [.] _dl_setup_hash                     1     1.25%
>     [.] _dl_sysdep_start                   1     1.25%
>     [.] brk                                1     1.25%
>     [.] calloc@plt                         1     1.25%
>     [.] dl_main                            1     1.25%
>     [.] match_symbol                       1     1.25%
>     [.] sbrk                               1     1.25%
>     [.] strlen                             1     1.25%
> 
> 
>  * changes in v4:
>   - fix a tui navigation bug
>   - fix a bug in output change of perf diff
>   - move call to perf_hpp__init() out of setup_browser()
>   - fix alignment of some output fields on stdio
> 
>  * changes in v3:
>   - rename to --fields option for consistency  (David)
>   - prevent to add same keys multiple times
>   - change dso sorting to show unknown dsos last
>   - fix minor bugs
> 
>  * changes in v2:
>   - add a cleanup patch using ui__has_annotation()
>   - cleanup default sort order managment
>   - support perf top also
>   - handle elided sort entries properly
>   - add Acked-by's from Ingo
> 
> 
> I pushed the patch series on the 'perf/field-v4' branch in my tree
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> 
> 
> Any comments are welcome, please test!
> 
> Thanks,
> Namhyung
> 
> 
> [1] https://lkml.org/lkml/2014/4/10/397
> 
> Namhyung Kim (17):
>   perf tools: Add ->cmp(), ->collapse() and ->sort() to perf_hpp_fmt
>   perf tools: Convert sort entries to hpp formats
>   perf tools: Use hpp formats to sort hist entries
>   perf tools: Support event grouping in hpp ->sort()
>   perf tools: Use hpp formats to sort final output
>   perf tools: Consolidate output field handling to hpp format routines
>   perf ui: Get rid of callback from __hpp__fmt()
>   perf tools: Allow hpp fields to be sort keys
>   perf tools: Consolidate management of default sort orders
>   perf tools: Call perf_hpp__init() before setting up GUI browsers
>   perf report: Add -F option to specify output fields
>   perf tools: Add ->sort() member to struct sort_entry
>   perf report/tui: Fix a bug when --fields/sort is given
>   perf top: Add --fields option to specify output fields
>   perf diff: Add missing setup_output_field()
>   perf tools: Skip elided sort entries
>   perf hists: Reset width of output fields with header length
> 
>  tools/perf/Documentation/perf-report.txt |  10 +
>  tools/perf/Documentation/perf-top.txt    |   9 +
>  tools/perf/builtin-diff.c                |   3 +
>  tools/perf/builtin-report.c              |  31 +--
>  tools/perf/builtin-top.c                 |  12 +-
>  tools/perf/ui/browsers/hists.c           |  76 +++----
>  tools/perf/ui/gtk/hists.c                |  41 +---
>  tools/perf/ui/hist.c                     | 189 ++++++++++++++--
>  tools/perf/ui/setup.c                    |   2 -
>  tools/perf/ui/stdio/hist.c               |  54 ++---
>  tools/perf/util/hist.c                   |  83 ++-----
>  tools/perf/util/hist.h                   |  20 +-
>  tools/perf/util/sort.c                   | 364 ++++++++++++++++++++++++++++++-
>  tools/perf/util/sort.h                   |   4 +
>  14 files changed, 654 insertions(+), 244 deletions(-)
> 
> -- 
> 1.9.2
> 

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Namhyung Kim]

Hi Don,

On Tue, 22 Apr 2014 17:16:47 -0400, Don Zickus wrote:
> On Wed, Apr 16, 2014 at 12:05:37PM +0900, Namhyung Kim wrote:
>> Hello,
>> 
>> This is a patchset implementing -F/--fields option to setup output
>> field/column as Ingo requested.  It depends on my --percentage
>> patchset [1].
>> 
>> The -F option can receive any sort keys that -s option recognize, plus
>> following fields (name can be changed):
>> 
>>   overhead, overhead_sys, overhead_us, sample, period
>> 
>> The overhead_guest_sys and overhead_guest_us might be avaiable when
>> you profile guest machines.
>> 
>> Output will be sorted by in order of fields and sort keys passed by -s
>> option will be added to the output field list automatically.  If you
>> want to change the order of sorting you can give -s option in addition
>> to -F option.  To support old behavior, it'll also prepend 'overhead'
>> field to the sort keys unless you give -F option explicitly.
>
> So I am struggling a little bit to get this working correctly.  I had it
> in my head that I could sort internally with -s and re-sort the output
> based on -F, but it doesn't seem to be working that way.

Hmm.. probably it's me miss something on perf mem side..  I don't have
an access to a machine to test it now.

>
> For example with
>
> ./perf mem record -a grep -r foo /* > /dev/null
> ./perf mem report -F overhead,symbol_daddr,pid -s symbol_daddr,pid --stdio 
>
> I was thinking I could sort everything based on the symbol_daddr and pid.
> Then re-sort the output to display the highest 'symbol_daddr,pid' pair.
> But it didn't seem to work that way.  Instead it seems like I get the
> original sort just displayed in the -F format.

Could you please show me the output of your example?

Thanks,
Namhyung

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Don Zickus]

On Wed, Apr 23, 2014 at 03:15:35PM +0900, Namhyung Kim wrote:
> Hi Don,
> 
> On Tue, 22 Apr 2014 17:16:47 -0400, Don Zickus wrote:
> > On Wed, Apr 16, 2014 at 12:05:37PM +0900, Namhyung Kim wrote:
> >> Hello,
> >> 
> >> This is a patchset implementing -F/--fields option to setup output
> >> field/column as Ingo requested.  It depends on my --percentage
> >> patchset [1].
> >> 
> >> The -F option can receive any sort keys that -s option recognize, plus
> >> following fields (name can be changed):
> >> 
> >>   overhead, overhead_sys, overhead_us, sample, period
> >> 
> >> The overhead_guest_sys and overhead_guest_us might be avaiable when
> >> you profile guest machines.
> >> 
> >> Output will be sorted by in order of fields and sort keys passed by -s
> >> option will be added to the output field list automatically.  If you
> >> want to change the order of sorting you can give -s option in addition
> >> to -F option.  To support old behavior, it'll also prepend 'overhead'
> >> field to the sort keys unless you give -F option explicitly.
> >
> > So I am struggling a little bit to get this working correctly.  I had it
> > in my head that I could sort internally with -s and re-sort the output
> > based on -F, but it doesn't seem to be working that way.
> 
> Hmm.. probably it's me miss something on perf mem side..  I don't have
> an access to a machine to test it now.
> 
> >
> > For example with
> >
> > ./perf mem record -a grep -r foo /* > /dev/null
> > ./perf mem report -F overhead,symbol_daddr,pid -s symbol_daddr,pid --stdio 
> >
> > I was thinking I could sort everything based on the symbol_daddr and pid.
> > Then re-sort the output to display the highest 'symbol_daddr,pid' pair.
> > But it didn't seem to work that way.  Instead it seems like I get the
> > original sort just displayed in the -F format.
> 
> Could you please show me the output of your example?


# To display the perf.data header info, please use --header/--header-only
# options.
#
# Samples: 96K of event 'cpu/mem-loads/pp'
# Total weight : 1102938
# Sort order   : symbol_daddr,pid
#
# Overhead Data Symbol          Command:  Pid
# ........ ......................................................................
#
     0.00%  [k] 0xffff8807a8c1cf80 grep:116437
     0.00%  [k] 0xffff8807a8c8cee0 grep:116437
     0.00%  [k] 0xffff8807a8dceea0 grep:116437
     0.01%  [k] 0xffff8807a9298dc0 grep:116437
     0.01%  [k] 0xffff8807a934be40 grep:116437
     0.00%  [k] 0xffff8807a9416ec0 grep:116437
     0.02%  [k] 0xffff8807a9735700 grep:116437
     0.00%  [k] 0xffff8807a98e9460 grep:116437
     0.02%  [k] 0xffff8807a9afc890 grep:116437
     0.00%  [k] 0xffff8807aa64feb0 grep:116437
     0.02%  [k] 0xffff8807aa6b0030 grep:116437

Cheers,
Don

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Namhyung Kim]

Hi Don,

2014-04-23 (수), 08:58 -0400, Don Zickus:
> On Wed, Apr 23, 2014 at 03:15:35PM +0900, Namhyung Kim wrote:
> > On Tue, 22 Apr 2014 17:16:47 -0400, Don Zickus wrote:
> > > ./perf mem record -a grep -r foo /* > /dev/null
> > > ./perf mem report -F overhead,symbol_daddr,pid -s symbol_daddr,pid --stdio 
> > >
> > > I was thinking I could sort everything based on the symbol_daddr and pid.
> > > Then re-sort the output to display the highest 'symbol_daddr,pid' pair.
> > > But it didn't seem to work that way.  Instead it seems like I get the
> > > original sort just displayed in the -F format.
> > 
> > Could you please show me the output of your example?
> 
> 
> # To display the perf.data header info, please use --header/--header-only
> # options.
> #
> # Samples: 96K of event 'cpu/mem-loads/pp'
> # Total weight : 1102938
> # Sort order   : symbol_daddr,pid
> #
> # Overhead Data Symbol          Command:  Pid
> # ........ ......................................................................
> #
>      0.00%  [k] 0xffff8807a8c1cf80 grep:116437
>      0.00%  [k] 0xffff8807a8c8cee0 grep:116437
>      0.00%  [k] 0xffff8807a8dceea0 grep:116437
>      0.01%  [k] 0xffff8807a9298dc0 grep:116437
>      0.01%  [k] 0xffff8807a934be40 grep:116437
>      0.00%  [k] 0xffff8807a9416ec0 grep:116437
>      0.02%  [k] 0xffff8807a9735700 grep:116437
>      0.00%  [k] 0xffff8807a98e9460 grep:116437
>      0.02%  [k] 0xffff8807a9afc890 grep:116437
>      0.00%  [k] 0xffff8807aa64feb0 grep:116437
>      0.02%  [k] 0xffff8807aa6b0030 grep:116437

Hmm.. it seems that it's exactly sorted by the data symbol addresses, so
I don't see any problem here.  What did you expect?  If you want to see
those symbol_daddr,pid pair to be sorted by overhead, you can use the
one of -F or -s option only.

IOW, "perf mem report -F overhead,symbol_daddr,pid" and "perf mem report
-s symbol_daddr,pid" should show same result.  FYI but "perf mem report
-F overhead,symbol_daddr,pid -s symbol_addr,pid" will be different.

It's due to a compatibility reason.  If -s option is used without -F
option, it'll prepend 'overhead' sort key to preserve old behavior (adds
the fields to output but sort by overhead only).

Thanks,
Namhyung

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Don Zickus]

On Thu, Apr 24, 2014 at 10:41:39PM +0900, Namhyung Kim wrote:
> Hi Don,
> 
> 2014-04-23 (수), 08:58 -0400, Don Zickus:
> > On Wed, Apr 23, 2014 at 03:15:35PM +0900, Namhyung Kim wrote:
> > > On Tue, 22 Apr 2014 17:16:47 -0400, Don Zickus wrote:
> > > > ./perf mem record -a grep -r foo /* > /dev/null
> > > > ./perf mem report -F overhead,symbol_daddr,pid -s symbol_daddr,pid --stdio 
> > > >
> > > > I was thinking I could sort everything based on the symbol_daddr and pid.
> > > > Then re-sort the output to display the highest 'symbol_daddr,pid' pair.
> > > > But it didn't seem to work that way.  Instead it seems like I get the
> > > > original sort just displayed in the -F format.
> > > 
> > > Could you please show me the output of your example?
> > 
> > 
> > # To display the perf.data header info, please use --header/--header-only
> > # options.
> > #
> > # Samples: 96K of event 'cpu/mem-loads/pp'
> > # Total weight : 1102938
> > # Sort order   : symbol_daddr,pid
> > #
> > # Overhead Data Symbol          Command:  Pid
> > # ........ ......................................................................
> > #
> >      0.00%  [k] 0xffff8807a8c1cf80 grep:116437
> >      0.00%  [k] 0xffff8807a8c8cee0 grep:116437
> >      0.00%  [k] 0xffff8807a8dceea0 grep:116437
> >      0.01%  [k] 0xffff8807a9298dc0 grep:116437
> >      0.01%  [k] 0xffff8807a934be40 grep:116437
> >      0.00%  [k] 0xffff8807a9416ec0 grep:116437
> >      0.02%  [k] 0xffff8807a9735700 grep:116437
> >      0.00%  [k] 0xffff8807a98e9460 grep:116437
> >      0.02%  [k] 0xffff8807a9afc890 grep:116437
> >      0.00%  [k] 0xffff8807aa64feb0 grep:116437
> >      0.02%  [k] 0xffff8807aa6b0030 grep:116437
> 
> Hmm.. it seems that it's exactly sorted by the data symbol addresses, so
> I don't see any problem here.  What did you expect?  If you want to see
> those symbol_daddr,pid pair to be sorted by overhead, you can use the
> one of -F or -s option only.

Good question.  I guess I was hoping to see things sorted by overhead, but
as you said removing all the -F options gives me that.  I have been
distracted with other fires this week, I lost focus at what I was trying
to accomplish.

Let me figure that out again and try to come up with a more clear email
explaining what I was looking for (for myself at least :-) ).

Sorry for the distraction.

Cheers,
Don

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Namhyung Kim]

Hi Don,

On Thu, 24 Apr 2014 17:00:15 -0400, Don Zickus wrote:
> On Thu, Apr 24, 2014 at 10:41:39PM +0900, Namhyung Kim wrote:
>> Hmm.. it seems that it's exactly sorted by the data symbol addresses, so
>> I don't see any problem here.  What did you expect?  If you want to see
>> those symbol_daddr,pid pair to be sorted by overhead, you can use the
>> one of -F or -s option only.
>
> Good question.  I guess I was hoping to see things sorted by overhead, but
> as you said removing all the -F options gives me that.  I have been
> distracted with other fires this week, I lost focus at what I was trying
> to accomplish.
>
> Let me figure that out again and try to come up with a more clear email
> explaining what I was looking for (for myself at least :-) ).
>
> Sorry for the distraction.

No need to sorry and thank you for testing and feedbacks :)

Thanks,
Namhyung

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Don Zickus]

On Thu, Apr 24, 2014 at 05:00:15PM -0400, Don Zickus wrote:
> On Thu, Apr 24, 2014 at 10:41:39PM +0900, Namhyung Kim wrote:
> > Hi Don,
> > 
> > 2014-04-23 (수), 08:58 -0400, Don Zickus:
> > > On Wed, Apr 23, 2014 at 03:15:35PM +0900, Namhyung Kim wrote:
> > > > On Tue, 22 Apr 2014 17:16:47 -0400, Don Zickus wrote:
> > > > > ./perf mem record -a grep -r foo /* > /dev/null
> > > > > ./perf mem report -F overhead,symbol_daddr,pid -s symbol_daddr,pid --stdio 
> > > > >
> > > > > I was thinking I could sort everything based on the symbol_daddr and pid.
> > > > > Then re-sort the output to display the highest 'symbol_daddr,pid' pair.
> > > > > But it didn't seem to work that way.  Instead it seems like I get the
> > > > > original sort just displayed in the -F format.
> > > > 
> > > > Could you please show me the output of your example?
> > > 
> > > 
> > > # To display the perf.data header info, please use --header/--header-only
> > > # options.
> > > #
> > > # Samples: 96K of event 'cpu/mem-loads/pp'
> > > # Total weight : 1102938
> > > # Sort order   : symbol_daddr,pid
> > > #
> > > # Overhead Data Symbol          Command:  Pid
> > > # ........ ......................................................................
> > > #
> > >      0.00%  [k] 0xffff8807a8c1cf80 grep:116437
> > >      0.00%  [k] 0xffff8807a8c8cee0 grep:116437
> > >      0.00%  [k] 0xffff8807a8dceea0 grep:116437
> > >      0.01%  [k] 0xffff8807a9298dc0 grep:116437
> > >      0.01%  [k] 0xffff8807a934be40 grep:116437
> > >      0.00%  [k] 0xffff8807a9416ec0 grep:116437
> > >      0.02%  [k] 0xffff8807a9735700 grep:116437
> > >      0.00%  [k] 0xffff8807a98e9460 grep:116437
> > >      0.02%  [k] 0xffff8807a9afc890 grep:116437
> > >      0.00%  [k] 0xffff8807aa64feb0 grep:116437
> > >      0.02%  [k] 0xffff8807aa6b0030 grep:116437
> > 
> > Hmm.. it seems that it's exactly sorted by the data symbol addresses, so
> > I don't see any problem here.  What did you expect?  If you want to see
> > those symbol_daddr,pid pair to be sorted by overhead, you can use the
> > one of -F or -s option only.
> 
> Good question.  I guess I was hoping to see things sorted by overhead, but
> as you said removing all the -F options gives me that.  I have been
> distracted with other fires this week, I lost focus at what I was trying
> to accomplish.
> 
> Let me figure that out again and try to come up with a more clear email
> explaining what I was looking for (for myself at least :-) ).

Ok.  I think I figured out what I need.  This might be quite long..


Our orignal concept for the c2c tool was to sort hist entries into
cachelines, filter in only the HITMs and stores and re-sort based on
cachelines with the most weight.

So using today's perf with a new search called 'cacheline' to achieve
this (copy-n-pasted):

----
#define CACHE_LINESIZE       64
#define CLINE_OFFSET_MSK     (CACHE_LINESIZE - 1)
#define CLADRS(a)            ((a) & ~(CLINE_OFFSET_MSK))
#define CLOFFSET(a)          (int)((a) &  (CLINE_OFFSET_MSK))

static int64_t
sort__cacheline_cmp(struct hist_entry *left, struct hist_entry *right)
{
       u64 l, r;
       struct map *l_map, *r_map;

       if (!left->mem_info)  return -1;
       if (!right->mem_info) return 1;

       /* group event types together */
       if (left->cpumode > right->cpumode) return -1;
       if (left->cpumode < right->cpumode) return 1;

       l_map = left->mem_info->daddr.map;
       r_map = right->mem_info->daddr.map;

       /* properly sort NULL maps to help combine them */
       if (!l_map && !r_map)
               goto addr;

       if (!l_map) return -1;
       if (!r_map) return 1;

       if (l_map->maj > r_map->maj) return -1;
       if (l_map->maj < r_map->maj) return 1;

       if (l_map->min > r_map->min) return -1;
       if (l_map->min < r_map->min) return 1;

       if (l_map->ino > r_map->ino) return -1;
       if (l_map->ino < r_map->ino) return 1;

       if (l_map->ino_generation > r_map->ino_generation) return -1;
       if (l_map->ino_generation < r_map->ino_generation) return 1;

       /*
        * Addresses with no major/minor numbers are assumed to be
        * anonymous in userspace.  Sort those on pid then address.
        *
        * The kernel and non-zero major/minor mapped areas are
        * assumed to be unity mapped.  Sort those on address.
        */

       if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
           !l_map->maj && !l_map->min && !l_map->ino &&
           !l_map->ino_generation) {
               /* userspace anonymous */

               if (left->thread->pid_ > right->thread->pid_) return -1;
               if (left->thread->pid_ < right->thread->pid_) return 1;
       }

addr:
       /* al_addr does all the right addr - start + offset calculations */
       l = CLADRS(left->mem_info->daddr.al_addr);
       r = CLADRS(right->mem_info->daddr.al_addr);

       if (l > r) return -1;
       if (l < r) return 1;

       return 0;
}

----

I can get the following 'perf mem report' outputs

I used a special program called hitm_test3 which purposely generates
HITMs either locally or remotely based on cpu input.  It does this by
having processA grab lockX from cacheline1, release lockY from cacheline2,
then processB grabs lockY from cacheline2 and releases lockX from
cacheline1 (IOW ping pong two locks across two cachelines), found here

http://people.redhat.com/dzickus/hitm_test/

[ perf mem record -a hitm_test -s1,19 -c1000000 -t]

(where -s is the cpus to bind to, -c is loop count, -t disables internal
perf tracking)

(using 'perf mem' to auto generate correct record/report options for
cachelines)
(the hitm counts should be higher, but sampling is a crapshoot.  Using
ld_lat=30 would probably filter most of the L1 hits)

Table 1: normal perf
#perf mem report --stdio -s cacheline,pid


# Overhead       Samples  Cacheline                       Command:  Pid
# ........  ............  .......................  ....................
#
    47.61%         42257  [.] 0x0000000000000080      hitm_test3:146344
    46.14%         42596  [.] 0000000000000000        hitm_test3:146343
     2.16%          2074  [.] 0x0000000000003340      hitm_test3:146344
     1.88%          1796  [.] 0x0000000000003340      hitm_test3:146343
     0.20%           140  [.] 0x00007ffff291ce00      hitm_test3:146344
     0.18%           126  [.] 0x00007ffff291ce00      hitm_test3:146343
     0.10%             1  [k] 0xffff88042f071500         swapper:     0
     0.07%             1  [k] 0xffff88042ef747c0     watchdog/11:    62
...

Ok, now I know the hottest cachelines. Not to bad.  However, in order to
determine cacheline contention, it would be nice to know the offsets into
the cacheline to see if there is contention or not. Unfortunately, the way
the sorting works here, all the hist_entry data was combined into each
cacheline, so I lose my granularity...

I can do:

Table 2: normal perf
#perf mem report --stdio -s cacheline,pid,dso_daddr,mem


# Overhead       Samples  Cacheline                       Command:  Pid
# Data Object             Memory access
# ........  ............  .......................  ....................
# ..............................  ........................
#
    45.24%         42581  [.] 0000000000000000        hitm_test3:146343 SYSV00000000 (deleted)          L1 hit
    44.43%         42231  [.] 0x0000000000000080      hitm_test3:146344 SYSV00000000 (deleted)          L1 hit
     2.19%            13  [.] 0x0000000000000080      hitm_test3:146344 SYSV00000000 (deleted)          Local RAM hit
     2.16%          2074  [.] 0x0000000000003340      hitm_test3:146344 hitm_test3                      L1 hit
     1.88%          1796  [.] 0x0000000000003340      hitm_test3:146343 hitm_test3                      L1 hit
     1.00%            13  [.] 0x0000000000000080      hitm_test3:146344 SYSV00000000 (deleted)          Remote Cache (1 hop) hit
     0.91%            15  [.] 0000000000000000        hitm_test3:146343 SYSV00000000 (deleted)          Remote Cache (1 hop) hit
     0.20%           140  [.] 0x00007ffff291ce00      hitm_test3:146344 [stack]                         L1 hit
     0.18%           126  [.] 0x00007ffff291ce00      hitm_test3:146343 [stack]                         L1 hit

Now I have some granularity (though the program keeps hitting the same
offset in the cacheline) and some different levels of memory operations.
Seems like a step forward.  However, the cacheline is broken up a little
bit (see 0x0000000000000080 is split up three ways).

I can now see where the cache contention is but I don't know how prevalent
it is (what percentage of the cacheline is under contention).  No need to
waste time with cachelines that have little or no contention.

Hmm, what if I used the -F option to group all the cachelines and their
offsets together.

Table 3: perf with -F
#perf mem report --stdio -s cacheline,pid,dso_daddr,mem  -i don.data -F cacheline,pid,dso_daddr,mem,overhead,sample|grep 0000000000000
  [k] 0000000000000000           swapper:     0  [kernel.kallsyms] Uncached hit                 0.00%             1
  [k] 0000000000000000            kipmi0:  1500  [kernel.kallsyms] Uncached hit                 0.02%             1
  [.] 0000000000000000        hitm_test3:146343  SYSV00000000 (deleted) L1 hit                      45.24%         42581
  [.] 0000000000000000        hitm_test3:146343  SYSV00000000 (deleted) Remote Cache (1 hop) hit     0.91%            15
  [.] 0x0000000000000080      hitm_test3:146344  SYSV00000000 (deleted) L1 hit                      44.43%         42231
  [.] 0x0000000000000080      hitm_test3:146344  SYSV00000000 (deleted) Local RAM hit                2.19%            13
  [.] 0x0000000000000080      hitm_test3:146344  SYSV00000000 (deleted) Remote Cache (1 hop) hit     1.00%            13

Now I have the ability to see the whole cacheline easily and can probably
roughly calculate the contention in my head.  Of course there was some
pre-determined knowledge that was needed to get this info (like which
cacheline is interesting from Table 1).

Of course, our c2c tool was trying to make the output more readable and
more obvious such that the user didn't have to know what to look for.

Internally our tool sorts similar to Table2, but then resorts onto a new
rbtree with a struct c2c_hit based on the hottest cachelines.  Based on
this new rbtree we can print our analysis easily.

This new rbtree is slightly different than the -F output in that we
'group' cacheline entries together and re-sort that group.  The -F option
just resorts the sorted hist_entry and has no concept of grouping.




We would prefer to have a 'group' sorting concept as we believe that is
the easiest way to organize the data.  But I don't know if that can be
incorporated into the 'perf' tool itself, or just keep that concept local
to our flavor of the perf subcommand.

I am hoping this semi-concocted example gives a better picture of the
problem I am trying to wrestle with.

Help? Thoughts?

Cheers,
Don

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Namhyung Kim]

Hi Don,

On Mon, 28 Apr 2014 15:46:42 -0400, Don Zickus wrote:
> On Thu, Apr 24, 2014 at 05:00:15PM -0400, Don Zickus wrote:
>> On Thu, Apr 24, 2014 at 10:41:39PM +0900, Namhyung Kim wrote:
>> > Hi Don,
>> > 
>> > 2014-04-23 (수), 08:58 -0400, Don Zickus:
>> > > On Wed, Apr 23, 2014 at 03:15:35PM +0900, Namhyung Kim wrote:
>> > > > On Tue, 22 Apr 2014 17:16:47 -0400, Don Zickus wrote:
>> > > > > ./perf mem record -a grep -r foo /* > /dev/null
>> > > > > ./perf mem report -F overhead,symbol_daddr,pid -s symbol_daddr,pid --stdio 
>> > > > >
>> > > > > I was thinking I could sort everything based on the symbol_daddr and pid.
>> > > > > Then re-sort the output to display the highest 'symbol_daddr,pid' pair.
>> > > > > But it didn't seem to work that way.  Instead it seems like I get the
>> > > > > original sort just displayed in the -F format.
>> > > > 
>> > > > Could you please show me the output of your example?
>> > > 
>> > > 
>> > > # To display the perf.data header info, please use --header/--header-only
>> > > # options.
>> > > #
>> > > # Samples: 96K of event 'cpu/mem-loads/pp'
>> > > # Total weight : 1102938
>> > > # Sort order   : symbol_daddr,pid
>> > > #
>> > > # Overhead Data Symbol          Command:  Pid
>> > > # ........ ......................................................................
>> > > #
>> > >      0.00%  [k] 0xffff8807a8c1cf80 grep:116437
>> > >      0.00%  [k] 0xffff8807a8c8cee0 grep:116437
>> > >      0.00%  [k] 0xffff8807a8dceea0 grep:116437
>> > >      0.01%  [k] 0xffff8807a9298dc0 grep:116437
>> > >      0.01%  [k] 0xffff8807a934be40 grep:116437
>> > >      0.00%  [k] 0xffff8807a9416ec0 grep:116437
>> > >      0.02%  [k] 0xffff8807a9735700 grep:116437
>> > >      0.00%  [k] 0xffff8807a98e9460 grep:116437
>> > >      0.02%  [k] 0xffff8807a9afc890 grep:116437
>> > >      0.00%  [k] 0xffff8807aa64feb0 grep:116437
>> > >      0.02%  [k] 0xffff8807aa6b0030 grep:116437
>> > 
>> > Hmm.. it seems that it's exactly sorted by the data symbol addresses, so
>> > I don't see any problem here.  What did you expect?  If you want to see
>> > those symbol_daddr,pid pair to be sorted by overhead, you can use the
>> > one of -F or -s option only.
>> 
>> Good question.  I guess I was hoping to see things sorted by overhead, but
>> as you said removing all the -F options gives me that.  I have been
>> distracted with other fires this week, I lost focus at what I was trying
>> to accomplish.
>> 
>> Let me figure that out again and try to come up with a more clear email
>> explaining what I was looking for (for myself at least :-) ).
>
> Ok.  I think I figured out what I need.  This might be quite long..

Great. :)

>
>
> Our orignal concept for the c2c tool was to sort hist entries into
> cachelines, filter in only the HITMs and stores and re-sort based on
> cachelines with the most weight.
>
> So using today's perf with a new search called 'cacheline' to achieve
> this (copy-n-pasted):

Maybe 'd'cacheline is a more appropriate name IMHO.

>
> ----
> #define CACHE_LINESIZE       64
> #define CLINE_OFFSET_MSK     (CACHE_LINESIZE - 1)
> #define CLADRS(a)            ((a) & ~(CLINE_OFFSET_MSK))
> #define CLOFFSET(a)          (int)((a) &  (CLINE_OFFSET_MSK))
>
> static int64_t
> sort__cacheline_cmp(struct hist_entry *left, struct hist_entry *right)
> {
>        u64 l, r;
>        struct map *l_map, *r_map;
>
>        if (!left->mem_info)  return -1;
>        if (!right->mem_info) return 1;
>
>        /* group event types together */
>        if (left->cpumode > right->cpumode) return -1;
>        if (left->cpumode < right->cpumode) return 1;
>
>        l_map = left->mem_info->daddr.map;
>        r_map = right->mem_info->daddr.map;
>
>        /* properly sort NULL maps to help combine them */
>        if (!l_map && !r_map)
>                goto addr;
>
>        if (!l_map) return -1;
>        if (!r_map) return 1;
>
>        if (l_map->maj > r_map->maj) return -1;
>        if (l_map->maj < r_map->maj) return 1;
>
>        if (l_map->min > r_map->min) return -1;
>        if (l_map->min < r_map->min) return 1;
>
>        if (l_map->ino > r_map->ino) return -1;
>        if (l_map->ino < r_map->ino) return 1;
>
>        if (l_map->ino_generation > r_map->ino_generation) return -1;
>        if (l_map->ino_generation < r_map->ino_generation) return 1;
>
>        /*
>         * Addresses with no major/minor numbers are assumed to be
>         * anonymous in userspace.  Sort those on pid then address.
>         *
>         * The kernel and non-zero major/minor mapped areas are
>         * assumed to be unity mapped.  Sort those on address.
>         */
>
>        if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
>            !l_map->maj && !l_map->min && !l_map->ino &&
>            !l_map->ino_generation) {
>                /* userspace anonymous */
>
>                if (left->thread->pid_ > right->thread->pid_) return -1;
>                if (left->thread->pid_ < right->thread->pid_) return 1;

Isn't it necessary to check whether the address is in a same map in case
of anon pages?  I mean the daddr.al_addr is a map-relative offset so it
might have same value for different maps.


>        }
>
> addr:
>        /* al_addr does all the right addr - start + offset calculations */
>        l = CLADRS(left->mem_info->daddr.al_addr);
>        r = CLADRS(right->mem_info->daddr.al_addr);
>
>        if (l > r) return -1;
>        if (l < r) return 1;
>
>        return 0;
> }
>
> ----
>
> I can get the following 'perf mem report' outputs
>
> I used a special program called hitm_test3 which purposely generates
> HITMs either locally or remotely based on cpu input.  It does this by
> having processA grab lockX from cacheline1, release lockY from cacheline2,
> then processB grabs lockY from cacheline2 and releases lockX from
> cacheline1 (IOW ping pong two locks across two cachelines), found here
>
> http://people.redhat.com/dzickus/hitm_test/
>
> [ perf mem record -a hitm_test -s1,19 -c1000000 -t]
>
> (where -s is the cpus to bind to, -c is loop count, -t disables internal
> perf tracking)
>
> (using 'perf mem' to auto generate correct record/report options for
> cachelines)
> (the hitm counts should be higher, but sampling is a crapshoot.  Using
> ld_lat=30 would probably filter most of the L1 hits)
>
> Table 1: normal perf
> #perf mem report --stdio -s cacheline,pid
>
>
> # Overhead       Samples  Cacheline                       Command:  Pid
> # ........  ............  .......................  ....................
> #
>     47.61%         42257  [.] 0x0000000000000080      hitm_test3:146344
>     46.14%         42596  [.] 0000000000000000        hitm_test3:146343
>      2.16%          2074  [.] 0x0000000000003340      hitm_test3:146344
>      1.88%          1796  [.] 0x0000000000003340      hitm_test3:146343
>      0.20%           140  [.] 0x00007ffff291ce00      hitm_test3:146344
>      0.18%           126  [.] 0x00007ffff291ce00      hitm_test3:146343
>      0.10%             1  [k] 0xffff88042f071500         swapper:     0
>      0.07%             1  [k] 0xffff88042ef747c0     watchdog/11:    62
> ...
>
> Ok, now I know the hottest cachelines. Not to bad.  However, in order to
> determine cacheline contention, it would be nice to know the offsets into
> the cacheline to see if there is contention or not. Unfortunately, the way
> the sorting works here, all the hist_entry data was combined into each
> cacheline, so I lose my granularity...
>
> I can do:
>
> Table 2: normal perf
> #perf mem report --stdio -s cacheline,pid,dso_daddr,mem
>
>
> # Overhead       Samples  Cacheline                       Command:  Pid
> # Data Object             Memory access
> # ........  ............  .......................  ....................
> # ..............................  ........................
> #
>     45.24%         42581  [.] 0000000000000000        hitm_test3:146343 SYSV00000000 (deleted)          L1 hit
>     44.43%         42231  [.] 0x0000000000000080      hitm_test3:146344 SYSV00000000 (deleted)          L1 hit
>      2.19%            13  [.] 0x0000000000000080      hitm_test3:146344 SYSV00000000 (deleted)          Local RAM hit
>      2.16%          2074  [.] 0x0000000000003340      hitm_test3:146344 hitm_test3                      L1 hit
>      1.88%          1796  [.] 0x0000000000003340      hitm_test3:146343 hitm_test3                      L1 hit
>      1.00%            13  [.] 0x0000000000000080      hitm_test3:146344 SYSV00000000 (deleted)          Remote Cache (1 hop) hit
>      0.91%            15  [.] 0000000000000000        hitm_test3:146343 SYSV00000000 (deleted)          Remote Cache (1 hop) hit
>      0.20%           140  [.] 0x00007ffff291ce00      hitm_test3:146344 [stack]                         L1 hit
>      0.18%           126  [.] 0x00007ffff291ce00      hitm_test3:146343 [stack]                         L1 hit
>
> Now I have some granularity (though the program keeps hitting the same
> offset in the cacheline) and some different levels of memory operations.
> Seems like a step forward.  However, the cacheline is broken up a little
> bit (see 0x0000000000000080 is split up three ways).
>
> I can now see where the cache contention is but I don't know how prevalent
> it is (what percentage of the cacheline is under contention).  No need to
> waste time with cachelines that have little or no contention.
>
> Hmm, what if I used the -F option to group all the cachelines and their
> offsets together.
>
> Table 3: perf with -F
> #perf mem report --stdio -s cacheline,pid,dso_daddr,mem  -i don.data -F cacheline,pid,dso_daddr,mem,overhead,sample|grep 0000000000000
>   [k] 0000000000000000           swapper:     0  [kernel.kallsyms] Uncached hit                 0.00%             1
>   [k] 0000000000000000            kipmi0:  1500  [kernel.kallsyms] Uncached hit                 0.02%             1
>   [.] 0000000000000000        hitm_test3:146343  SYSV00000000 (deleted) L1 hit                      45.24%         42581
>   [.] 0000000000000000        hitm_test3:146343  SYSV00000000 (deleted) Remote Cache (1 hop) hit     0.91%            15
>   [.] 0x0000000000000080      hitm_test3:146344  SYSV00000000 (deleted) L1 hit                      44.43%         42231
>   [.] 0x0000000000000080      hitm_test3:146344  SYSV00000000 (deleted) Local RAM hit                2.19%            13
>   [.] 0x0000000000000080      hitm_test3:146344  SYSV00000000 (deleted) Remote Cache (1 hop) hit     1.00%            13
>
> Now I have the ability to see the whole cacheline easily and can probably
> roughly calculate the contention in my head.  Of course there was some
> pre-determined knowledge that was needed to get this info (like which
> cacheline is interesting from Table 1).
>
> Of course, our c2c tool was trying to make the output more readable and
> more obvious such that the user didn't have to know what to look for.
>
> Internally our tool sorts similar to Table2, but then resorts onto a new
> rbtree with a struct c2c_hit based on the hottest cachelines.  Based on
> this new rbtree we can print our analysis easily.
>
> This new rbtree is slightly different than the -F output in that we
> 'group' cacheline entries together and re-sort that group.  The -F option
> just resorts the sorted hist_entry and has no concept of grouping.
>
>
>
>
> We would prefer to have a 'group' sorting concept as we believe that is
> the easiest way to organize the data.  But I don't know if that can be
> incorporated into the 'perf' tool itself, or just keep that concept local
> to our flavor of the perf subcommand.
>
> I am hoping this semi-concocted example gives a better picture of the
> problem I am trying to wrestle with.

Yep, I understand your problem.

And I think it's good for having the group sorting concept in perf tools
for general use.  But it has a conflict with the proposed change of -F
option when non-sort keys are used for the -s or -F.  So it needs more
thinking..

Unfortunately I'll be busy by the end of next week.  So I'll be able to
discuss and work on it later.

Thanks,
Namhyung

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Don Zickus]

On Tue, Apr 29, 2014 at 10:13:35AM +0900, Namhyung Kim wrote:
> > Our orignal concept for the c2c tool was to sort hist entries into
> > cachelines, filter in only the HITMs and stores and re-sort based on
> > cachelines with the most weight.
> >
> > So using today's perf with a new search called 'cacheline' to achieve
> > this (copy-n-pasted):
> 
> Maybe 'd'cacheline is a more appropriate name IMHO.

Fair enough :-)

> 
> >
> > ----
> > #define CACHE_LINESIZE       64
> > #define CLINE_OFFSET_MSK     (CACHE_LINESIZE - 1)
> > #define CLADRS(a)            ((a) & ~(CLINE_OFFSET_MSK))
> > #define CLOFFSET(a)          (int)((a) &  (CLINE_OFFSET_MSK))
> >
> > static int64_t
> > sort__cacheline_cmp(struct hist_entry *left, struct hist_entry *right)
> > {
> >        u64 l, r;
> >        struct map *l_map, *r_map;
> >
> >        if (!left->mem_info)  return -1;
> >        if (!right->mem_info) return 1;
> >
> >        /* group event types together */
> >        if (left->cpumode > right->cpumode) return -1;
> >        if (left->cpumode < right->cpumode) return 1;
> >
> >        l_map = left->mem_info->daddr.map;
> >        r_map = right->mem_info->daddr.map;
> >
> >        /* properly sort NULL maps to help combine them */
> >        if (!l_map && !r_map)
> >                goto addr;
> >
> >        if (!l_map) return -1;
> >        if (!r_map) return 1;
> >
> >        if (l_map->maj > r_map->maj) return -1;
> >        if (l_map->maj < r_map->maj) return 1;
> >
> >        if (l_map->min > r_map->min) return -1;
> >        if (l_map->min < r_map->min) return 1;
> >
> >        if (l_map->ino > r_map->ino) return -1;
> >        if (l_map->ino < r_map->ino) return 1;
> >
> >        if (l_map->ino_generation > r_map->ino_generation) return -1;
> >        if (l_map->ino_generation < r_map->ino_generation) return 1;
> >
> >        /*
> >         * Addresses with no major/minor numbers are assumed to be
> >         * anonymous in userspace.  Sort those on pid then address.
> >         *
> >         * The kernel and non-zero major/minor mapped areas are
> >         * assumed to be unity mapped.  Sort those on address.
> >         */
> >
> >        if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
> >            !l_map->maj && !l_map->min && !l_map->ino &&
> >            !l_map->ino_generation) {
> >                /* userspace anonymous */
> >
> >                if (left->thread->pid_ > right->thread->pid_) return -1;
> >                if (left->thread->pid_ < right->thread->pid_) return 1;
> 
> Isn't it necessary to check whether the address is in a same map in case
> of anon pages?  I mean the daddr.al_addr is a map-relative offset so it
> might have same value for different maps.

That's why I sort on pids here.  Because the anon address might have the
same value for different maps.  The thought was to group all the pid
addresses together to keep things seperated.

Do you see a different way to solve the problem?  I am not sure al_addr
vs. addr will make much difference here.

> 
> 
> >        }
> >
> > addr:
> >        /* al_addr does all the right addr - start + offset calculations */
> >        l = CLADRS(left->mem_info->daddr.al_addr);
> >        r = CLADRS(right->mem_info->daddr.al_addr);
> >
> >        if (l > r) return -1;
> >        if (l < r) return 1;
> >
> >        return 0;
> > }
> >
> > ----
> >
> > I can get the following 'perf mem report' outputs
> >
> > I used a special program called hitm_test3 which purposely generates
> > HITMs either locally or remotely based on cpu input.  It does this by
> > having processA grab lockX from cacheline1, release lockY from cacheline2,
> > then processB grabs lockY from cacheline2 and releases lockX from
> > cacheline1 (IOW ping pong two locks across two cachelines), found here
> >
> > http://people.redhat.com/dzickus/hitm_test/
> >
> > [ perf mem record -a hitm_test -s1,19 -c1000000 -t]
> >
> > (where -s is the cpus to bind to, -c is loop count, -t disables internal
> > perf tracking)
> >
> > (using 'perf mem' to auto generate correct record/report options for
> > cachelines)
> > (the hitm counts should be higher, but sampling is a crapshoot.  Using
> > ld_lat=30 would probably filter most of the L1 hits)
> >
> > Table 1: normal perf
> > #perf mem report --stdio -s cacheline,pid
> >
> >
> > # Overhead       Samples  Cacheline                       Command:  Pid
> > # ........  ............  .......................  ....................
> > #
> >     47.61%         42257  [.] 0x0000000000000080      hitm_test3:146344
> >     46.14%         42596  [.] 0000000000000000        hitm_test3:146343
> >      2.16%          2074  [.] 0x0000000000003340      hitm_test3:146344
> >      1.88%          1796  [.] 0x0000000000003340      hitm_test3:146343
> >      0.20%           140  [.] 0x00007ffff291ce00      hitm_test3:146344
> >      0.18%           126  [.] 0x00007ffff291ce00      hitm_test3:146343
> >      0.10%             1  [k] 0xffff88042f071500         swapper:     0
> >      0.07%             1  [k] 0xffff88042ef747c0     watchdog/11:    62
> > ...
> >
> > Ok, now I know the hottest cachelines. Not to bad.  However, in order to
> > determine cacheline contention, it would be nice to know the offsets into
> > the cacheline to see if there is contention or not. Unfortunately, the way
> > the sorting works here, all the hist_entry data was combined into each
> > cacheline, so I lose my granularity...
> >
> > I can do:
> >
> > Table 2: normal perf
> > #perf mem report --stdio -s cacheline,pid,dso_daddr,mem
> >
> >
> > # Overhead       Samples  Cacheline                       Command:  Pid
> > # Data Object             Memory access
> > # ........  ............  .......................  ....................
> > # ..............................  ........................
> > #
> >     45.24%         42581  [.] 0000000000000000        hitm_test3:146343 SYSV00000000 (deleted)          L1 hit
> >     44.43%         42231  [.] 0x0000000000000080      hitm_test3:146344 SYSV00000000 (deleted)          L1 hit
> >      2.19%            13  [.] 0x0000000000000080      hitm_test3:146344 SYSV00000000 (deleted)          Local RAM hit
> >      2.16%          2074  [.] 0x0000000000003340      hitm_test3:146344 hitm_test3                      L1 hit
> >      1.88%          1796  [.] 0x0000000000003340      hitm_test3:146343 hitm_test3                      L1 hit
> >      1.00%            13  [.] 0x0000000000000080      hitm_test3:146344 SYSV00000000 (deleted)          Remote Cache (1 hop) hit
> >      0.91%            15  [.] 0000000000000000        hitm_test3:146343 SYSV00000000 (deleted)          Remote Cache (1 hop) hit
> >      0.20%           140  [.] 0x00007ffff291ce00      hitm_test3:146344 [stack]                         L1 hit
> >      0.18%           126  [.] 0x00007ffff291ce00      hitm_test3:146343 [stack]                         L1 hit
> >
> > Now I have some granularity (though the program keeps hitting the same
> > offset in the cacheline) and some different levels of memory operations.
> > Seems like a step forward.  However, the cacheline is broken up a little
> > bit (see 0x0000000000000080 is split up three ways).
> >
> > I can now see where the cache contention is but I don't know how prevalent
> > it is (what percentage of the cacheline is under contention).  No need to
> > waste time with cachelines that have little or no contention.
> >
> > Hmm, what if I used the -F option to group all the cachelines and their
> > offsets together.
> >
> > Table 3: perf with -F
> > #perf mem report --stdio -s cacheline,pid,dso_daddr,mem  -i don.data -F cacheline,pid,dso_daddr,mem,overhead,sample|grep 0000000000000
> >   [k] 0000000000000000           swapper:     0  [kernel.kallsyms] Uncached hit                 0.00%             1
> >   [k] 0000000000000000            kipmi0:  1500  [kernel.kallsyms] Uncached hit                 0.02%             1
> >   [.] 0000000000000000        hitm_test3:146343  SYSV00000000 (deleted) L1 hit                      45.24%         42581
> >   [.] 0000000000000000        hitm_test3:146343  SYSV00000000 (deleted) Remote Cache (1 hop) hit     0.91%            15
> >   [.] 0x0000000000000080      hitm_test3:146344  SYSV00000000 (deleted) L1 hit                      44.43%         42231
> >   [.] 0x0000000000000080      hitm_test3:146344  SYSV00000000 (deleted) Local RAM hit                2.19%            13
> >   [.] 0x0000000000000080      hitm_test3:146344  SYSV00000000 (deleted) Remote Cache (1 hop) hit     1.00%            13
> >
> > Now I have the ability to see the whole cacheline easily and can probably
> > roughly calculate the contention in my head.  Of course there was some
> > pre-determined knowledge that was needed to get this info (like which
> > cacheline is interesting from Table 1).
> >
> > Of course, our c2c tool was trying to make the output more readable and
> > more obvious such that the user didn't have to know what to look for.
> >
> > Internally our tool sorts similar to Table2, but then resorts onto a new
> > rbtree with a struct c2c_hit based on the hottest cachelines.  Based on
> > this new rbtree we can print our analysis easily.
> >
> > This new rbtree is slightly different than the -F output in that we
> > 'group' cacheline entries together and re-sort that group.  The -F option
> > just resorts the sorted hist_entry and has no concept of grouping.
> >
> >
> >
> >
> > We would prefer to have a 'group' sorting concept as we believe that is
> > the easiest way to organize the data.  But I don't know if that can be
> > incorporated into the 'perf' tool itself, or just keep that concept local
> > to our flavor of the perf subcommand.
> >
> > I am hoping this semi-concocted example gives a better picture of the
> > problem I am trying to wrestle with.
> 
> Yep, I understand your problem.
> 
> And I think it's good for having the group sorting concept in perf tools
> for general use.  But it has a conflict with the proposed change of -F
> option when non-sort keys are used for the -s or -F.  So it needs more
> thinking..
> 
> Unfortunately I'll be busy by the end of next week.  So I'll be able to
> discuss and work on it later.

Ok.  I am glad you are able to understand my problem.  It will be nice to
have others looking at a solution too. :-)

Cheers,
Don

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Namhyung Kim]

Hi Don,

On Tue, 29 Apr 2014 13:27:35 -0400, Don Zickus wrote:
> On Tue, Apr 29, 2014 at 10:13:35AM +0900, Namhyung Kim wrote:
>> >        /*
>> >         * Addresses with no major/minor numbers are assumed to be
>> >         * anonymous in userspace.  Sort those on pid then address.
>> >         *
>> >         * The kernel and non-zero major/minor mapped areas are
>> >         * assumed to be unity mapped.  Sort those on address.
>> >         */
>> >
>> >        if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
>> >            !l_map->maj && !l_map->min && !l_map->ino &&
>> >            !l_map->ino_generation) {
>> >                /* userspace anonymous */
>> >
>> >                if (left->thread->pid_ > right->thread->pid_) return -1;
>> >                if (left->thread->pid_ < right->thread->pid_) return 1;
>> 
>> Isn't it necessary to check whether the address is in a same map in case
>> of anon pages?  I mean the daddr.al_addr is a map-relative offset so it
>> might have same value for different maps.
>
> That's why I sort on pids here.  Because the anon address might have the
> same value for different maps.  The thought was to group all the pid
> addresses together to keep things seperated.
>
> Do you see a different way to solve the problem?  I am not sure al_addr
> vs. addr will make much difference here.

I'm not saying to get rid of the pid check, I'm saying that it might
need to add another check for maps (i.e. start address) as there might
be many maps in a single address space.

Thanks,
Namhyung

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Don Zickus]

On Wed, Apr 30, 2014 at 08:38:10AM +0900, Namhyung Kim wrote:
> Hi Don,
> 
> On Tue, 29 Apr 2014 13:27:35 -0400, Don Zickus wrote:
> > On Tue, Apr 29, 2014 at 10:13:35AM +0900, Namhyung Kim wrote:
> >> >        /*
> >> >         * Addresses with no major/minor numbers are assumed to be
> >> >         * anonymous in userspace.  Sort those on pid then address.
> >> >         *
> >> >         * The kernel and non-zero major/minor mapped areas are
> >> >         * assumed to be unity mapped.  Sort those on address.
> >> >         */
> >> >
> >> >        if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
> >> >            !l_map->maj && !l_map->min && !l_map->ino &&
> >> >            !l_map->ino_generation) {
> >> >                /* userspace anonymous */
> >> >
> >> >                if (left->thread->pid_ > right->thread->pid_) return -1;
> >> >                if (left->thread->pid_ < right->thread->pid_) return 1;
> >> 
> >> Isn't it necessary to check whether the address is in a same map in case
> >> of anon pages?  I mean the daddr.al_addr is a map-relative offset so it
> >> might have same value for different maps.
> >
> > That's why I sort on pids here.  Because the anon address might have the
> > same value for different maps.  The thought was to group all the pid
> > addresses together to keep things seperated.
> >
> > Do you see a different way to solve the problem?  I am not sure al_addr
> > vs. addr will make much difference here.
> 
> I'm not saying to get rid of the pid check, I'm saying that it might
> need to add another check for maps (i.e. start address) as there might
> be many maps in a single address space.

Hmm, I guess I would need to see an example.  While I agree there might be
many maps in a single address space (/proc/<pid>/maps demonstrates that),
I understood them to map to a unique location (ie no overlap) unless they
are shared.

I am willing to believe I missed scenario when sorting, I just can't think
of it (so I wouldn't know how to fix it).  That's why I was looking for an
example to make it more obvious to me.  Sorry for being slow..

Cheers,
Don

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Jiri Olsa]

On Wed, Apr 16, 2014 at 12:05:37PM +0900, Namhyung Kim wrote:

SNIP

> 
>  * changes in v4:
>   - fix a tui navigation bug
>   - fix a bug in output change of perf diff
>   - move call to perf_hpp__init() out of setup_browser()
>   - fix alignment of some output fields on stdio
> 
>  * changes in v3:
>   - rename to --fields option for consistency  (David)
>   - prevent to add same keys multiple times
>   - change dso sorting to show unknown dsos last
>   - fix minor bugs
> 
>  * changes in v2:
>   - add a cleanup patch using ui__has_annotation()
>   - cleanup default sort order managment
>   - support perf top also
>   - handle elided sort entries properly
>   - add Acked-by's from Ingo
> 
> 
> I pushed the patch series on the 'perf/field-v4' branch in my tree
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

hi,
I tried to merge this with my latest perf/core and got
some conflicts. Looks like it's missing the latest hist
code changes.. could you please rebase?

thanks,
jirka

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Namhyung Kim]

Hi Don,

On Wed, 30 Apr 2014 09:35:55 -0400, Don Zickus wrote:
> On Wed, Apr 30, 2014 at 08:38:10AM +0900, Namhyung Kim wrote:
>> Hi Don,
>> 
>> On Tue, 29 Apr 2014 13:27:35 -0400, Don Zickus wrote:
>> > On Tue, Apr 29, 2014 at 10:13:35AM +0900, Namhyung Kim wrote:
>> >> >        /*
>> >> >         * Addresses with no major/minor numbers are assumed to be
>> >> >         * anonymous in userspace.  Sort those on pid then address.
>> >> >         *
>> >> >         * The kernel and non-zero major/minor mapped areas are
>> >> >         * assumed to be unity mapped.  Sort those on address.
>> >> >         */
>> >> >
>> >> >        if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
>> >> >            !l_map->maj && !l_map->min && !l_map->ino &&
>> >> >            !l_map->ino_generation) {
>> >> >                /* userspace anonymous */
>> >> >
>> >> >                if (left->thread->pid_ > right->thread->pid_) return -1;
>> >> >                if (left->thread->pid_ < right->thread->pid_) return 1;
>> >> 
>> >> Isn't it necessary to check whether the address is in a same map in case
>> >> of anon pages?  I mean the daddr.al_addr is a map-relative offset so it
>> >> might have same value for different maps.
>> >
>> > That's why I sort on pids here.  Because the anon address might have the
>> > same value for different maps.  The thought was to group all the pid
>> > addresses together to keep things seperated.
>> >
>> > Do you see a different way to solve the problem?  I am not sure al_addr
>> > vs. addr will make much difference here.
>> 
>> I'm not saying to get rid of the pid check, I'm saying that it might
>> need to add another check for maps (i.e. start address) as there might
>> be many maps in a single address space.
>
> Hmm, I guess I would need to see an example.  While I agree there might be
> many maps in a single address space (/proc/<pid>/maps demonstrates that),
> I understood them to map to a unique location (ie no overlap) unless they
> are shared.
>
> I am willing to believe I missed scenario when sorting, I just can't think
> of it (so I wouldn't know how to fix it).  That's why I was looking for an
> example to make it more obvious to me.  Sorry for being slow..

I'm also sorry for being late.  Looking at the code, it seems to use
identity__map_ip() for anon maps so my concern is bogus.  Please just
forget about it and keep going.  Sorry for interrupting your work..

Thanks,
Namhyung

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Namhyung Kim]

Hi Jiri,

On Sun, 4 May 2014 19:53:34 +0200, Jiri Olsa wrote:
> On Wed, Apr 16, 2014 at 12:05:37PM +0900, Namhyung Kim wrote:
>
> SNIP
>
>> 
>>  * changes in v4:
>>   - fix a tui navigation bug
>>   - fix a bug in output change of perf diff
>>   - move call to perf_hpp__init() out of setup_browser()
>>   - fix alignment of some output fields on stdio
>> 
>>  * changes in v3:
>>   - rename to --fields option for consistency  (David)
>>   - prevent to add same keys multiple times
>>   - change dso sorting to show unknown dsos last
>>   - fix minor bugs
>> 
>>  * changes in v2:
>>   - add a cleanup patch using ui__has_annotation()
>>   - cleanup default sort order managment
>>   - support perf top also
>>   - handle elided sort entries properly
>>   - add Acked-by's from Ingo
>> 
>> 
>> I pushed the patch series on the 'perf/field-v4' branch in my tree
>> 
>>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>
> hi,
> I tried to merge this with my latest perf/core and got
> some conflicts. Looks like it's missing the latest hist
> code changes.. could you please rebase?

I can do it maybe next week.  But I need to think about the
grouping/hierarchy concept that Don suggested beforehand.

Thanks,
Namhyung

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Don Zickus]

On Wed, May 07, 2014 at 12:05:58PM +0900, Namhyung Kim wrote:
> Hi Don,
> 
> On Wed, 30 Apr 2014 09:35:55 -0400, Don Zickus wrote:
> > On Wed, Apr 30, 2014 at 08:38:10AM +0900, Namhyung Kim wrote:
> >> Hi Don,
> >> 
> >> On Tue, 29 Apr 2014 13:27:35 -0400, Don Zickus wrote:
> >> > On Tue, Apr 29, 2014 at 10:13:35AM +0900, Namhyung Kim wrote:
> >> >> >        /*
> >> >> >         * Addresses with no major/minor numbers are assumed to be
> >> >> >         * anonymous in userspace.  Sort those on pid then address.
> >> >> >         *
> >> >> >         * The kernel and non-zero major/minor mapped areas are
> >> >> >         * assumed to be unity mapped.  Sort those on address.
> >> >> >         */
> >> >> >
> >> >> >        if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
> >> >> >            !l_map->maj && !l_map->min && !l_map->ino &&
> >> >> >            !l_map->ino_generation) {
> >> >> >                /* userspace anonymous */
> >> >> >
> >> >> >                if (left->thread->pid_ > right->thread->pid_) return -1;
> >> >> >                if (left->thread->pid_ < right->thread->pid_) return 1;
> >> >> 
> >> >> Isn't it necessary to check whether the address is in a same map in case
> >> >> of anon pages?  I mean the daddr.al_addr is a map-relative offset so it
> >> >> might have same value for different maps.
> >> >
> >> > That's why I sort on pids here.  Because the anon address might have the
> >> > same value for different maps.  The thought was to group all the pid
> >> > addresses together to keep things seperated.
> >> >
> >> > Do you see a different way to solve the problem?  I am not sure al_addr
> >> > vs. addr will make much difference here.
> >> 
> >> I'm not saying to get rid of the pid check, I'm saying that it might
> >> need to add another check for maps (i.e. start address) as there might
> >> be many maps in a single address space.
> >
> > Hmm, I guess I would need to see an example.  While I agree there might be
> > many maps in a single address space (/proc/<pid>/maps demonstrates that),
> > I understood them to map to a unique location (ie no overlap) unless they
> > are shared.
> >
> > I am willing to believe I missed scenario when sorting, I just can't think
> > of it (so I wouldn't know how to fix it).  That's why I was looking for an
> > example to make it more obvious to me.  Sorry for being slow..
> 
> I'm also sorry for being late.  Looking at the code, it seems to use
> identity__map_ip() for anon maps so my concern is bogus.  Please just
> forget about it and keep going.  Sorry for interrupting your work..

No worries.  Always good to have an extra pair of eyes.

I am a little stuck about where to go.  I think it might make sense to
submit this as a standalone patch (despite the possible group limitation
we are discussing).  Thoughts?

Cheers,
Don

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Namhyung Kim]

Hi Don,

On Wed, 7 May 2014 11:22:19 -0400, Don Zickus wrote:
> On Wed, May 07, 2014 at 12:05:58PM +0900, Namhyung Kim wrote:
>> Hi Don,
>> 
>> On Wed, 30 Apr 2014 09:35:55 -0400, Don Zickus wrote:
>> > On Wed, Apr 30, 2014 at 08:38:10AM +0900, Namhyung Kim wrote:
>> >> Hi Don,
>> >> 
>> >> On Tue, 29 Apr 2014 13:27:35 -0400, Don Zickus wrote:
>> >> > On Tue, Apr 29, 2014 at 10:13:35AM +0900, Namhyung Kim wrote:
>> >> >> >        /*
>> >> >> >         * Addresses with no major/minor numbers are assumed to be
>> >> >> >         * anonymous in userspace.  Sort those on pid then address.
>> >> >> >         *
>> >> >> >         * The kernel and non-zero major/minor mapped areas are
>> >> >> >         * assumed to be unity mapped.  Sort those on address.
>> >> >> >         */
>> >> >> >
>> >> >> >        if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
>> >> >> >            !l_map->maj && !l_map->min && !l_map->ino &&
>> >> >> >            !l_map->ino_generation) {
>> >> >> >                /* userspace anonymous */
>> >> >> >
>> >> >> >                if (left->thread->pid_ > right->thread->pid_) return -1;
>> >> >> >                if (left->thread->pid_ < right->thread->pid_) return 1;
>> >> >> 
>> >> >> Isn't it necessary to check whether the address is in a same map in case
>> >> >> of anon pages?  I mean the daddr.al_addr is a map-relative offset so it
>> >> >> might have same value for different maps.
>> >> >
>> >> > That's why I sort on pids here.  Because the anon address might have the
>> >> > same value for different maps.  The thought was to group all the pid
>> >> > addresses together to keep things seperated.
>> >> >
>> >> > Do you see a different way to solve the problem?  I am not sure al_addr
>> >> > vs. addr will make much difference here.
>> >> 
>> >> I'm not saying to get rid of the pid check, I'm saying that it might
>> >> need to add another check for maps (i.e. start address) as there might
>> >> be many maps in a single address space.
>> >
>> > Hmm, I guess I would need to see an example.  While I agree there might be
>> > many maps in a single address space (/proc/<pid>/maps demonstrates that),
>> > I understood them to map to a unique location (ie no overlap) unless they
>> > are shared.
>> >
>> > I am willing to believe I missed scenario when sorting, I just can't think
>> > of it (so I wouldn't know how to fix it).  That's why I was looking for an
>> > example to make it more obvious to me.  Sorry for being slow..
>> 
>> I'm also sorry for being late.  Looking at the code, it seems to use
>> identity__map_ip() for anon maps so my concern is bogus.  Please just
>> forget about it and keep going.  Sorry for interrupting your work..
>
> No worries.  Always good to have an extra pair of eyes.
>
> I am a little stuck about where to go.  I think it might make sense to
> submit this as a standalone patch (despite the possible group limitation
> we are discussing).  Thoughts?

You meant to submit the 'dcacheline' sort key patch?  I'm okay with it.

Things are getting more confusing when thinking about complex
scenarios..  so I'd rather finish this -F option patchset and start to
think about how to integrate group sorting with it.

Thanks,
Namhyung

Re: [PATCHSET 00/17] perf report: Add -F option for specifying output fields (v4)

[Don Zickus]

On Fri, May 09, 2014 at 03:11:28PM +0900, Namhyung Kim wrote:
> >> > I am willing to believe I missed scenario when sorting, I just can't think
> >> > of it (so I wouldn't know how to fix it).  That's why I was looking for an
> >> > example to make it more obvious to me.  Sorry for being slow..
> >> 
> >> I'm also sorry for being late.  Looking at the code, it seems to use
> >> identity__map_ip() for anon maps so my concern is bogus.  Please just
> >> forget about it and keep going.  Sorry for interrupting your work..
> >
> > No worries.  Always good to have an extra pair of eyes.
> >
> > I am a little stuck about where to go.  I think it might make sense to
> > submit this as a standalone patch (despite the possible group limitation
> > we are discussing).  Thoughts?
> 
> You meant to submit the 'dcacheline' sort key patch?  I'm okay with it.

Ok.  I'll try to put together a patchset next week for that piece.

> 
> Things are getting more confusing when thinking about complex
> scenarios..  so I'd rather finish this -F option patchset and start to
> think about how to integrate group sorting with it.

Hehe.  I understand.  That's fine.

Cheers,
Don