[PATCHSET 00/10] perf tools: Cleanup for sort keys (v2)

[PATCHSET 00/10] perf tools: Cleanup for sort keys (v2)

[Namhyung Kim]

Hello,

This is a second version of my sort key cleanup series.  I Added
helper function for adding sort key and used unmap_ip() function for
printing symbol address as Jiri commented.  But didn't change name of
the sort keys to 'ip'.  If you really think I should change, I'm okay
to do it though.  I also cleaned up sort__has_sym and sort key eliding
code in this version.

You can get it from 'perf/sort-v4' branch on my tree at:

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


Any comments are welcome, thanks.
Namhyung


Namhyung Kim (10):
  perf sort: Factor out common code in sort_dimension__add()
  perf sort: Separate out memory-specific sort keys
  perf sort: Add 'addr' sort key
  perf sort: Add 'addr_to/from' sort key
  perf sort: Update documentation for sort keys
  perf hists: Move column length setting code
  perf sort: Cleanup sort__has_sym setting
  perf top: Use sort__has_sym
  perf hist browser: Use sort__has_sym
  perf sort: Consolidate sort_entry__setup_elide()

 tools/perf/Documentation/perf-diff.txt   |   2 +-
 tools/perf/Documentation/perf-report.txt |   5 +-
 tools/perf/Documentation/perf-top.txt    |   2 +-
 tools/perf/builtin-diff.c                |   6 +-
 tools/perf/builtin-report.c              |  28 +----
 tools/perf/builtin-top.c                 |  19 +--
 tools/perf/ui/browsers/hists.c           |   9 +-
 tools/perf/util/hist.c                   |  17 +--
 tools/perf/util/hist.h                   |   5 +-
 tools/perf/util/sort.c                   | 201 +++++++++++++++++++++++++------
 tools/perf/util/sort.h                   |  25 ++--
 tools/perf/util/top.h                    |   1 -
 12 files changed, 218 insertions(+), 102 deletions(-)

-- 
1.7.11.7

[PATCH 01/10] perf sort: Factor out common code in sort_dimension__add()

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Let's remove duplicate code.

Suggested-by: Jiri Olsa <jolsa@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Cc: Andi Kleen <andi@firstfloor.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.c | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index a6ddad41d57a..a997955085eb 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -895,6 +895,21 @@ static struct sort_dimension bstack_sort_dimensions[] = {
 
 #undef DIM
 
+static void __sort_dimension__add(struct sort_dimension *sd, enum sort_type idx)
+{
+	if (sd->taken)
+		return;
+
+	if (sd->entry->se_collapse)
+		sort__need_collapse = 1;
+
+	if (list_empty(&hist_entry__sort_list))
+		sort__first_dimension = idx;
+
+	list_add_tail(&sd->entry->list, &hist_entry__sort_list);
+	sd->taken = 1;
+}
+
 int sort_dimension__add(const char *tok)
 {
 	unsigned int i;
@@ -922,18 +937,7 @@ int sort_dimension__add(const char *tok)
 			sort__has_sym = 1;
 		}
 
-		if (sd->taken)
-			return 0;
-
-		if (sd->entry->se_collapse)
-			sort__need_collapse = 1;
-
-		if (list_empty(&hist_entry__sort_list))
-			sort__first_dimension = i;
-
-		list_add_tail(&sd->entry->list, &hist_entry__sort_list);
-		sd->taken = 1;
-
+		__sort_dimension__add(sd, i);
 		return 0;
 	}
 
@@ -949,18 +953,7 @@ int sort_dimension__add(const char *tok)
 		if (sd->entry == &sort_sym_from || sd->entry == &sort_sym_to)
 			sort__has_sym = 1;
 
-		if (sd->taken)
-			return 0;
-
-		if (sd->entry->se_collapse)
-			sort__need_collapse = 1;
-
-		if (list_empty(&hist_entry__sort_list))
-			sort__first_dimension = i + __SORT_BRANCH_STACK;
-
-		list_add_tail(&sd->entry->list, &hist_entry__sort_list);
-		sd->taken = 1;
-
+		__sort_dimension__add(sd, i + __SORT_BRANCH_STACK);
 		return 0;
 	}
 
-- 
1.7.11.7

[PATCH 02/10] perf sort: Separate out memory-specific sort keys

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Since they're used only for perf mem, separate out them to a different
dimension so that normal user cannot access them by any chance.

For global/local weights, I'm not entirely sure to place them into the
memory dimension.  But it's the only user at this time.

Cc: Andi Kleen <andi@firstfloor.org>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c |  2 ++
 tools/perf/util/sort.c      | 39 +++++++++++++++++++++++++++++++--------
 tools/perf/util/sort.h      | 19 +++++++++++--------
 3 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index c877982a64d3..669405c9b8a2 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -871,6 +871,8 @@ repeat:
 			fprintf(stderr, "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
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index a997955085eb..1dbf16949250 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -871,14 +871,6 @@ static struct sort_dimension common_sort_dimensions[] = {
 	DIM(SORT_PARENT, "parent", sort_parent),
 	DIM(SORT_CPU, "cpu", sort_cpu),
 	DIM(SORT_SRCLINE, "srcline", sort_srcline),
-	DIM(SORT_LOCAL_WEIGHT, "local_weight", sort_local_weight),
-	DIM(SORT_GLOBAL_WEIGHT, "weight", sort_global_weight),
-	DIM(SORT_MEM_DADDR_SYMBOL, "symbol_daddr", sort_mem_daddr_sym),
-	DIM(SORT_MEM_DADDR_DSO, "dso_daddr", sort_mem_daddr_dso),
-	DIM(SORT_MEM_LOCKED, "locked", sort_mem_locked),
-	DIM(SORT_MEM_TLB, "tlb", sort_mem_tlb),
-	DIM(SORT_MEM_LVL, "mem", sort_mem_lvl),
-	DIM(SORT_MEM_SNOOP, "snoop", sort_mem_snoop),
 };
 
 #undef DIM
@@ -895,6 +887,21 @@ static struct sort_dimension bstack_sort_dimensions[] = {
 
 #undef DIM
 
+#define DIM(d, n, func) [d - __SORT_MEMORY_MODE] = { .name = n, .entry = &(func) }
+
+static struct sort_dimension memory_sort_dimensions[] = {
+	DIM(SORT_LOCAL_WEIGHT, "local_weight", sort_local_weight),
+	DIM(SORT_GLOBAL_WEIGHT, "weight", sort_global_weight),
+	DIM(SORT_MEM_DADDR_SYMBOL, "symbol_daddr", sort_mem_daddr_sym),
+	DIM(SORT_MEM_DADDR_DSO, "dso_daddr", sort_mem_daddr_dso),
+	DIM(SORT_MEM_LOCKED, "locked", sort_mem_locked),
+	DIM(SORT_MEM_TLB, "tlb", sort_mem_tlb),
+	DIM(SORT_MEM_LVL, "mem", sort_mem_lvl),
+	DIM(SORT_MEM_SNOOP, "snoop", sort_mem_snoop),
+};
+
+#undef DIM
+
 static void __sort_dimension__add(struct sort_dimension *sd, enum sort_type idx)
 {
 	if (sd->taken)
@@ -957,6 +964,22 @@ int sort_dimension__add(const char *tok)
 		return 0;
 	}
 
+	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;
+
+		if (sort__mode != SORT_MODE__MEMORY)
+			return -EINVAL;
+
+		if (sd->entry == &sort_mem_daddr_sym)
+			sort__has_sym = 1;
+
+		__sort_dimension__add(sd, i + __SORT_MEMORY_MODE);
+		return 0;
+	}
+
 	return -ESRCH;
 }
 
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 39ff4b86ae84..0232d476da87 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -138,14 +138,6 @@ enum sort_type {
 	SORT_PARENT,
 	SORT_CPU,
 	SORT_SRCLINE,
-	SORT_LOCAL_WEIGHT,
-	SORT_GLOBAL_WEIGHT,
-	SORT_MEM_DADDR_SYMBOL,
-	SORT_MEM_DADDR_DSO,
-	SORT_MEM_LOCKED,
-	SORT_MEM_TLB,
-	SORT_MEM_LVL,
-	SORT_MEM_SNOOP,
 
 	/* branch stack specific sort keys */
 	__SORT_BRANCH_STACK,
@@ -154,6 +146,17 @@ enum sort_type {
 	SORT_SYM_FROM,
 	SORT_SYM_TO,
 	SORT_MISPREDICT,
+
+	/* memory mode specific sort keys */
+	__SORT_MEMORY_MODE,
+	SORT_LOCAL_WEIGHT = __SORT_MEMORY_MODE,
+	SORT_GLOBAL_WEIGHT,
+	SORT_MEM_DADDR_SYMBOL,
+	SORT_MEM_DADDR_DSO,
+	SORT_MEM_LOCKED,
+	SORT_MEM_TLB,
+	SORT_MEM_LVL,
+	SORT_MEM_SNOOP,
 };
 
 /*
-- 
1.7.11.7

[PATCH 03/10] perf sort: Add 'addr' sort key

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

New addr sort key provides a way to sort the entries by the symbol
addresses.  It can be helpful to figure out symbol resolution problem
when a dso cannot do it properly as well as finding hotpath in a dso
and/or a function.

Suggested-by: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=55561
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c |  2 ++
 tools/perf/util/hist.h |  3 ++-
 tools/perf/util/sort.c | 29 +++++++++++++++++++++++++++++
 tools/perf/util/sort.h |  1 +
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 72b4eec820c3..c098d6ebab1f 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -52,6 +52,8 @@ void hists__reset_col_len(struct hists *hists)
 
 	for (col = 0; col < HISTC_NR_COLS; ++col)
 		hists__set_col_len(hists, col, 0);
+
+	hists__set_col_len(hists, HISTC_ADDR, BITS_PER_LONG / 4 + 2);
 }
 
 static void hists__set_unres_dso_col_len(struct hists *hists, int dso)
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 14c2fe20aa62..9599f805828f 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -43,12 +43,13 @@ enum hist_column {
 	HISTC_COMM,
 	HISTC_PARENT,
 	HISTC_CPU,
+	HISTC_SRCLINE,
+	HISTC_ADDR,
 	HISTC_MISPREDICT,
 	HISTC_SYMBOL_FROM,
 	HISTC_SYMBOL_TO,
 	HISTC_DSO_FROM,
 	HISTC_DSO_TO,
-	HISTC_SRCLINE,
 	HISTC_LOCAL_WEIGHT,
 	HISTC_GLOBAL_WEIGHT,
 	HISTC_MEM_DADDR_SYMBOL,
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 1dbf16949250..5640a95b3575 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -342,6 +342,34 @@ struct sort_entry sort_cpu = {
 	.se_width_idx	= HISTC_CPU,
 };
 
+/* --sort addr */
+
+static int64_t
+sort__addr_cmp(struct hist_entry *left, struct hist_entry *right)
+{
+	return right->ip - left->ip;
+}
+
+static int hist_entry__addr_snprintf(struct hist_entry *self, char *bf,
+				     size_t size, unsigned int width)
+{
+	struct map *map = self->ms.map;
+	u64 addr = self->ip;
+
+	if (map)
+		addr = map->unmap_ip(map, self->ip);
+
+	return repsep_snprintf(bf, size, "%#*llu", width, addr);
+}
+
+struct sort_entry sort_addr = {
+	.se_header      = "Address",
+	.se_cmp	        = sort__addr_cmp,
+	.se_snprintf    = hist_entry__addr_snprintf,
+	.se_width_idx	= HISTC_ADDR,
+};
+
+
 /* sort keys for branch stacks */
 
 static int64_t
@@ -871,6 +899,7 @@ static struct sort_dimension common_sort_dimensions[] = {
 	DIM(SORT_PARENT, "parent", sort_parent),
 	DIM(SORT_CPU, "cpu", sort_cpu),
 	DIM(SORT_SRCLINE, "srcline", sort_srcline),
+	DIM(SORT_ADDR, "addr", sort_addr),
 };
 
 #undef DIM
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 0232d476da87..0815e344f38c 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -138,6 +138,7 @@ enum sort_type {
 	SORT_PARENT,
 	SORT_CPU,
 	SORT_SRCLINE,
+	SORT_ADDR,
 
 	/* branch stack specific sort keys */
 	__SORT_BRANCH_STACK,
-- 
1.7.11.7

[PATCH 04/10] perf sort: Add 'addr_to/from' sort key

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

New addr_{to,from} sort keys provide a way to sort the entries by the
source/target symbol addresses.

Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c |  2 ++
 tools/perf/util/hist.h |  2 ++
 tools/perf/util/sort.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/sort.h |  2 ++
 4 files changed, 56 insertions(+)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index c098d6ebab1f..1fb1535940f8 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -54,6 +54,8 @@ void hists__reset_col_len(struct hists *hists)
 		hists__set_col_len(hists, col, 0);
 
 	hists__set_col_len(hists, HISTC_ADDR, BITS_PER_LONG / 4 + 2);
+	hists__set_col_len(hists, HISTC_ADDR_FROM, BITS_PER_LONG / 4 + 2);
+	hists__set_col_len(hists, HISTC_ADDR_TO, BITS_PER_LONG / 4 + 2);
 }
 
 static void hists__set_unres_dso_col_len(struct hists *hists, int dso)
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 9599f805828f..2640fcc566e9 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -50,6 +50,8 @@ enum hist_column {
 	HISTC_SYMBOL_TO,
 	HISTC_DSO_FROM,
 	HISTC_DSO_TO,
+	HISTC_ADDR_FROM,
+	HISTC_ADDR_TO,
 	HISTC_LOCAL_WEIGHT,
 	HISTC_GLOBAL_WEIGHT,
 	HISTC_MEM_DADDR_SYMBOL,
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 5640a95b3575..7f66c822d8bd 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -442,6 +442,40 @@ static int hist_entry__sym_to_snprintf(struct hist_entry *self, char *bf,
 
 }
 
+static int64_t
+sort__addr_from_cmp(struct hist_entry *left, struct hist_entry *right)
+{
+	struct addr_map_symbol *from_l = &left->branch_info->from;
+	struct addr_map_symbol *from_r = &right->branch_info->from;
+
+	return from_r->addr - from_l->addr;
+}
+
+static int hist_entry__addr_from_snprintf(struct hist_entry *self, char *bf,
+					  size_t size, unsigned int width)
+{
+	struct addr_map_symbol *from = &self->branch_info->from;
+	return repsep_snprintf(bf, size, "%#*"PRIx64, width,
+					(uint64_t)from->addr);
+}
+
+static int64_t
+sort__addr_to_cmp(struct hist_entry *left, struct hist_entry *right)
+{
+	struct addr_map_symbol *to_l = &left->branch_info->to;
+	struct addr_map_symbol *to_r = &right->branch_info->to;
+
+	return to_r->addr - to_l->addr;
+}
+
+static int hist_entry__addr_to_snprintf(struct hist_entry *self, char *bf,
+					size_t size, unsigned int width)
+{
+	struct addr_map_symbol *to = &self->branch_info->to;
+	return repsep_snprintf(bf, size, "%#*"PRIx64, width,
+					(uint64_t)to->addr);
+}
+
 struct sort_entry sort_dso_from = {
 	.se_header	= "Source Shared Object",
 	.se_cmp		= sort__dso_from_cmp,
@@ -470,6 +504,20 @@ struct sort_entry sort_sym_to = {
 	.se_width_idx	= HISTC_SYMBOL_TO,
 };
 
+struct sort_entry sort_addr_from = {
+	.se_header	= "Source Address",
+	.se_cmp		= sort__addr_from_cmp,
+	.se_snprintf	= hist_entry__addr_from_snprintf,
+	.se_width_idx	= HISTC_ADDR_FROM,
+};
+
+struct sort_entry sort_addr_to = {
+	.se_header	= "Target Address",
+	.se_cmp		= sort__addr_to_cmp,
+	.se_snprintf	= hist_entry__addr_to_snprintf,
+	.se_width_idx	= HISTC_ADDR_TO,
+};
+
 static int64_t
 sort__mispredict_cmp(struct hist_entry *left, struct hist_entry *right)
 {
@@ -911,6 +959,8 @@ static struct sort_dimension bstack_sort_dimensions[] = {
 	DIM(SORT_DSO_TO, "dso_to", sort_dso_to),
 	DIM(SORT_SYM_FROM, "symbol_from", sort_sym_from),
 	DIM(SORT_SYM_TO, "symbol_to", sort_sym_to),
+	DIM(SORT_ADDR_FROM, "addr_from", sort_addr_from),
+	DIM(SORT_ADDR_TO, "addr_to", sort_addr_to),
 	DIM(SORT_MISPREDICT, "mispredict", sort_mispredict),
 };
 
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 0815e344f38c..1f945a3b2e5b 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -146,6 +146,8 @@ enum sort_type {
 	SORT_DSO_TO,
 	SORT_SYM_FROM,
 	SORT_SYM_TO,
+	SORT_ADDR_FROM,
+	SORT_ADDR_TO,
 	SORT_MISPREDICT,
 
 	/* memory mode specific sort keys */
-- 
1.7.11.7

[PATCH 05/10] perf sort: Update documentation for sort keys

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Update and add missing description of new sort keys.

Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-diff.txt   | 2 +-
 tools/perf/Documentation/perf-report.txt | 5 ++++-
 tools/perf/Documentation/perf-top.txt    | 2 +-
 tools/perf/builtin-diff.c                | 2 +-
 tools/perf/builtin-report.c              | 6 +++---
 tools/perf/builtin-top.c                 | 3 ++-
 6 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/tools/perf/Documentation/perf-diff.txt b/tools/perf/Documentation/perf-diff.txt
index 5b3123d5721f..f74ec064db0e 100644
--- a/tools/perf/Documentation/perf-diff.txt
+++ b/tools/perf/Documentation/perf-diff.txt
@@ -47,7 +47,7 @@ OPTIONS
 
 -s::
 --sort=::
-	Sort by key(s): pid, comm, dso, symbol.
+	Sort by key(s): pid, comm, dso, symbol, parent, cpu, srcline, addr.
 
 -t::
 --field-separator=::
diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index 7d5f4f38aa52..54acc51995fc 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -59,7 +59,7 @@ OPTIONS
 --sort=::
 	Sort histogram entries by given key(s) - multiple keys can be specified
 	in CSV format.  Following sort keys are available:
-	pid, comm, dso, symbol, parent, cpu, srcline, weight, local_weight.
+	pid, comm, dso, symbol, parent, cpu, srcline, addr.
 
 	Each key has following meaning:
 
@@ -72,6 +72,7 @@ OPTIONS
 	- cpu: cpu number the task ran at the time of sample
 	- srcline: filename and line number executed at the time of sample.  The
 	DWARF debuggin info must be provided.
+	- addr: address of function executed at the time of sample
 
 	By default, comm, dso and symbol keys are used.
 	(i.e. --sort comm,dso,symbol)
@@ -84,6 +85,8 @@ OPTIONS
 	- dso_to: name of library or module branched to
 	- symbol_from: name of function branched from
 	- symbol_to: name of function branched to
+	- addr_from: address of function branched from
+	- addr_to: address of function branched to
 	- mispredict: "N" for predicted branch, "Y" for mispredicted branch
 
 	And default sort keys are changed to comm, dso_from, symbol_from, dso_to
diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
index 9f1a2fe54757..2b45e799c4be 100644
--- a/tools/perf/Documentation/perf-top.txt
+++ b/tools/perf/Documentation/perf-top.txt
@@ -112,7 +112,7 @@ Default is to monitor all CPUS.
 
 -s::
 --sort::
-	Sort by key(s): pid, comm, dso, symbol, parent, srcline, weight, local_weight.
+	Sort by key(s): pid, comm, dso, symbol, parent, srcline, cpu, addr.
 
 -n::
 --show-nr-samples::
diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 2d0462d89a97..03b56c542bb6 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -542,7 +542,7 @@ static const struct option options[] = {
 	OPT_STRING('S', "symbols", &symbol_conf.sym_list_str, "symbol[,symbol...]",
 		   "only consider these symbols"),
 	OPT_STRING('s', "sort", &sort_order, "key[,key2...]",
-		   "sort by key(s): pid, comm, dso, symbol, parent"),
+		   "sort by key(s): pid, comm, dso, symbol, parent, cpu, srcline, addr"),
 	OPT_STRING('t', "field-separator", &symbol_conf.field_sep, "separator",
 		   "separator for columns, no spaces will be added between "
 		   "columns '.' is reserved."),
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 669405c9b8a2..c95fd92fbd44 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -756,9 +756,9 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		    "Use the stdio interface"),
 	OPT_STRING('s', "sort", &sort_order, "key[,key2...]",
 		   "sort by key(s): pid, comm, dso, symbol, parent, cpu, srcline,"
-		   " dso_to, dso_from, symbol_to, symbol_from, mispredict,"
-		   " weight, local_weight, mem, symbol_daddr, dso_daddr, tlb, "
-		   "snoop, locked"),
+		   " addr, dso_to, dso_from, symbol_to, symbol_from, addr_to,"
+		   " addr_from, mispredict, weight, local_weight, mem,"
+		   " symbol_daddr, dso_daddr, tlb, snoop, locked"),
 	OPT_BOOLEAN(0, "showcpuutilization", &symbol_conf.show_cpu_utilization,
 		    "Show sample percentage for different cpu modes"),
 	OPT_STRING('p', "parent", &parent_pattern, "regex",
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 67bdb9f14ad6..9aae3f518f39 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1089,7 +1089,8 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_INCR('v', "verbose", &verbose,
 		    "be more verbose (show counter open errors, etc)"),
 	OPT_STRING('s', "sort", &sort_order, "key[,key2...]",
-		   "sort by key(s): pid, comm, dso, symbol, parent, weight, local_weight"),
+		   "sort by key(s): pid, comm, dso, symbol, parent, cpu,"
+		   " srcline, addr"),
 	OPT_BOOLEAN('n', "show-nr-samples", &symbol_conf.show_nr_samples,
 		    "Show a column with the number of samples"),
 	OPT_CALLBACK_DEFAULT('G', "call-graph", &top.record_opts,
-- 
1.7.11.7

[PATCH 06/10] perf hists: Move column length setting code

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

They are set to constant length so no need to update every time.

Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 1fb1535940f8..e144aefc76e6 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -56,6 +56,12 @@ void hists__reset_col_len(struct hists *hists)
 	hists__set_col_len(hists, HISTC_ADDR, BITS_PER_LONG / 4 + 2);
 	hists__set_col_len(hists, HISTC_ADDR_FROM, BITS_PER_LONG / 4 + 2);
 	hists__set_col_len(hists, HISTC_ADDR_TO, BITS_PER_LONG / 4 + 2);
+	hists__set_col_len(hists, HISTC_LOCAL_WEIGHT, 12);
+	hists__set_col_len(hists, HISTC_GLOBAL_WEIGHT, 12);
+	hists__set_col_len(hists, HISTC_MEM_LOCKED, 6);
+	hists__set_col_len(hists, HISTC_MEM_TLB, 22);
+	hists__set_col_len(hists, HISTC_MEM_SNOOP, 12);
+	hists__set_col_len(hists, HISTC_MEM_LVL, 21 + 3);
 }
 
 static void hists__set_unres_dso_col_len(struct hists *hists, int dso)
@@ -156,13 +162,6 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 		hists__new_col_len(hists, HISTC_MEM_DADDR_SYMBOL, symlen);
 		hists__set_unres_dso_col_len(hists, HISTC_MEM_DADDR_DSO);
 	}
-
-	hists__new_col_len(hists, HISTC_MEM_LOCKED, 6);
-	hists__new_col_len(hists, HISTC_MEM_TLB, 22);
-	hists__new_col_len(hists, HISTC_MEM_SNOOP, 12);
-	hists__new_col_len(hists, HISTC_MEM_LVL, 21 + 3);
-	hists__new_col_len(hists, HISTC_LOCAL_WEIGHT, 12);
-	hists__new_col_len(hists, HISTC_GLOBAL_WEIGHT, 12);
 }
 
 void hists__output_recalc_col_len(struct hists *hists, int max_rows)
-- 
1.7.11.7

[PATCH 07/10] perf sort: Cleanup sort__has_sym setting

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

The sort__has_sym variable is set only if a symbol-related sort key
was added.  Since branch stack and memory sort dimensions are
separated, it doesn't need to be checked from common dimension.

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

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 7f66c822d8bd..213831133e08 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1016,10 +1016,7 @@ int sort_dimension__add(const char *tok)
 				return -EINVAL;
 			}
 			sort__has_parent = 1;
-		} else if (sd->entry == &sort_sym ||
-			   sd->entry == &sort_sym_from ||
-			   sd->entry == &sort_sym_to ||
-			   sd->entry == &sort_mem_daddr_sym) {
+		} else if (sd->entry == &sort_sym) {
 			sort__has_sym = 1;
 		}
 
-- 
1.7.11.7

[PATCH 08/10] perf top: Use sort__has_sym

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

perf top had a similar variable sort_has_symbols for the same purpose.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-top.c | 12 +++---------
 tools/perf/util/top.h    |  1 -
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 9aae3f518f39..4aa504baaf0b 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -794,7 +794,7 @@ static void perf_event__process_sample(struct perf_tool *tool,
 				return;
 		}
 
-		if (top->sort_has_symbols)
+		if (sort__has_sym)
 			perf_top__record_precise_ip(top, he, evsel->idx, ip);
 	}
 
@@ -912,9 +912,9 @@ out_err:
 	return -1;
 }
 
-static int perf_top__setup_sample_type(struct perf_top *top)
+static int perf_top__setup_sample_type(struct perf_top *top __maybe_unused)
 {
-	if (!top->sort_has_symbols) {
+	if (!sort__has_sym) {
 		if (symbol_conf.use_callchain) {
 			ui__error("Selected -g but \"sym\" not present in --sort/-s.");
 			return -EINVAL;
@@ -1205,12 +1205,6 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", stdout);
 	sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list, "symbol", stdout);
 
-	/*
-	 * Avoid annotation data structures overhead when symbols aren't on the
-	 * sort list.
-	 */
-	top.sort_has_symbols = sort_sym.list.next != NULL;
-
 	get_term_dimensions(&top.winsize);
 	if (top.print_entries == 0) {
 		struct sigaction act = {
diff --git a/tools/perf/util/top.h b/tools/perf/util/top.h
index 7ebf357dc9e1..f0a862539ba9 100644
--- a/tools/perf/util/top.h
+++ b/tools/perf/util/top.h
@@ -26,7 +26,6 @@ struct perf_top {
 	int		   print_entries, count_filter, delay_secs;
 	bool		   hide_kernel_symbols, hide_user_symbols, zero;
 	bool		   use_tui, use_stdio;
-	bool		   sort_has_symbols;
 	bool		   kptr_restrict_warned;
 	bool		   vmlinux_warned;
 	bool		   dump_symtab;
-- 
1.7.11.7

[PATCH 09/10] perf hist browser: Use sort__has_sym

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

The TUI hist browser had a similar variable has_symbols for the same
purpose.  Let's get rid of the duplication.

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

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index cad8e37f05d9..a4268cab1921 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -25,7 +25,6 @@ struct hist_browser {
 	struct map_symbol   *selection;
 	int		     print_seq;
 	bool		     show_dso;
-	bool		     has_symbols;
 };
 
 extern void hist_browser__init_hpp(void);
@@ -1155,10 +1154,6 @@ static struct hist_browser *hist_browser__new(struct hists *hists)
 		browser->b.refresh = hist_browser__refresh;
 		browser->b.seek = ui_browser__hists_seek;
 		browser->b.use_navkeypressed = true;
-		if (sort__mode == SORT_MODE__BRANCH)
-			browser->has_symbols = sort_sym_from.list.next != NULL;
-		else
-			browser->has_symbols = sort_sym.list.next != NULL;
 	}
 
 	return browser;
@@ -1386,7 +1381,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			 */
 			goto out_free_stack;
 		case 'a':
-			if (!browser->has_symbols) {
+			if (!sort__has_sym) {
 				ui_browser__warning(&browser->b, delay_secs * 2,
 			"Annotation is only available for symbolic views, "
 			"include \"sym*\" in --sort to use it.");
@@ -1485,7 +1480,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 			continue;
 		}
 
-		if (!browser->has_symbols)
+		if (!sort__has_sym)
 			goto add_exit_option;
 
 		if (sort__mode == SORT_MODE__BRANCH) {
-- 
1.7.11.7

[PATCH 10/10] perf sort: Consolidate sort_entry__setup_elide()

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

The same code was duplicate to places, factor them out to common
sort__setup_elide().

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-diff.c   |  4 +---
 tools/perf/builtin-report.c | 20 +-------------------
 tools/perf/builtin-top.c    |  4 +---
 tools/perf/util/sort.c      | 45 +++++++++++++++++++++++++++++++++++++++++++--
 tools/perf/util/sort.h      |  3 +--
 5 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 03b56c542bb6..316bf13e59c7 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -611,9 +611,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	setup_pager();
 
-	sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso", NULL);
-	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", NULL);
-	sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list, "symbol", NULL);
+	sort__setup_elide(NULL);
 
 	return __cmd_diff();
 }
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index c95fd92fbd44..bff244fa4b5d 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -937,25 +937,7 @@ repeat:
 		report.symbol_filter_str = argv[0];
 	}
 
-	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", stdout);
-
-	if (sort__mode == SORT_MODE__BRANCH) {
-		sort_entry__setup_elide(&sort_dso_from, symbol_conf.dso_from_list, "dso_from", stdout);
-		sort_entry__setup_elide(&sort_dso_to, symbol_conf.dso_to_list, "dso_to", stdout);
-		sort_entry__setup_elide(&sort_sym_from, symbol_conf.sym_from_list, "sym_from", stdout);
-		sort_entry__setup_elide(&sort_sym_to, symbol_conf.sym_to_list, "sym_to", stdout);
-	} else {
-		if (report.mem_mode) {
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "symbol_daddr", stdout);
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso_daddr", stdout);
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "mem", stdout);
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "local_weight", stdout);
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "tlb", stdout);
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "snoop", stdout);
-		}
-		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso", stdout);
-		sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list, "symbol", stdout);
-	}
+	sort__setup_elide(stdout);
 
 	ret = __cmd_report(&report);
 	if (ret == K_SWITCH_INPUT_DATA) {
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 4aa504baaf0b..fe4acf568483 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1201,9 +1201,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (symbol__init() < 0)
 		return -1;
 
-	sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso", stdout);
-	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", stdout);
-	sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list, "symbol", stdout);
+	sort__setup_elide(stdout);
 
 	get_term_dimensions(&top.winsize);
 	if (top.print_entries == 0) {
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 213831133e08..86ae94d8782e 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1,5 +1,6 @@
 #include "sort.h"
 #include "hist.h"
+#include "symbol.h"
 
 regex_t		parent_regex;
 const char	default_parent_pattern[] = "^sys_|^do_page_fault";
@@ -1085,8 +1086,9 @@ int setup_sorting(void)
 	return ret;
 }
 
-void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
-			     const char *list_name, FILE *fp)
+static void sort_entry__setup_elide(struct sort_entry *self,
+				    struct strlist *list,
+				    const char *list_name, FILE *fp)
 {
 	if (list && strlist__nr_entries(list) == 1) {
 		if (fp != NULL)
@@ -1095,3 +1097,42 @@ void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
 		self->elide = true;
 	}
 }
+
+void sort__setup_elide(FILE *output)
+{
+	sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+				"dso", output);
+	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list,
+				"comm", output);
+	sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list,
+				"symbol", output);
+
+	if (sort__mode == SORT_MODE__BRANCH) {
+		sort_entry__setup_elide(&sort_dso_from,
+					symbol_conf.dso_from_list,
+					"dso_from", output);
+		sort_entry__setup_elide(&sort_dso_to,
+					symbol_conf.dso_to_list,
+					"dso_to", output);
+		sort_entry__setup_elide(&sort_sym_from,
+					symbol_conf.sym_from_list,
+					"sym_from", output);
+		sort_entry__setup_elide(&sort_sym_to,
+					symbol_conf.sym_to_list,
+					"sym_to", output);
+	} else if (sort__mode == SORT_MODE__MEMORY) {
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"symbol_daddr", output);
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"dso_daddr", output);
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"mem", output);
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"local_weight", output);
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"tlb", output);
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"snoop", output);
+	}
+
+}
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 1f945a3b2e5b..c80aac4ae3a2 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -184,7 +184,6 @@ extern struct list_head hist_entry__sort_list;
 
 int setup_sorting(void);
 extern int sort_dimension__add(const char *);
-void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
-			     const char *list_name, FILE *fp);
+void sort__setup_elide(FILE *fp);
 
 #endif	/* __PERF_SORT_H */
-- 
1.7.11.7

Re: [PATCH 03/10] perf sort: Add 'addr' sort key

[Arnaldo Carvalho de Melo]

What I expected was that the result was this:

perf report --sort addr | grep -v ^# | sort -k2 -n | less

And in hexadecimal, can you fix this?

- Arnaldo

Em Wed, Apr 03, 2013 at 09:26:12PM +0900, Namhyung Kim escreveu:
>  static void hists__set_unres_dso_col_len(struct hists *hists, int dso)
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 14c2fe20aa62..9599f805828f 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -43,12 +43,13 @@ enum hist_column {
>  	HISTC_COMM,
>  	HISTC_PARENT,
>  	HISTC_CPU,
> +	HISTC_SRCLINE,

Why move SRCLINE?

> +	HISTC_ADDR,
>  	HISTC_MISPREDICT,
>  	HISTC_SYMBOL_FROM,
>  	HISTC_SYMBOL_TO,
>  	HISTC_DSO_FROM,
>  	HISTC_DSO_TO,
> -	HISTC_SRCLINE,
>  	HISTC_LOCAL_WEIGHT,
>  	HISTC_GLOBAL_WEIGHT,
>  	HISTC_MEM_DADDR_SYMBOL,
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 1dbf16949250..5640a95b3575 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -342,6 +342,34 @@ struct sort_entry sort_cpu = {
>  	.se_width_idx	= HISTC_CPU,
>  };
>  
> +/* --sort addr */
> +
> +static int64_t
> +sort__addr_cmp(struct hist_entry *left, struct hist_entry *right)
> +{
> +	return right->ip - left->ip;
> +}
> +
> +static int hist_entry__addr_snprintf(struct hist_entry *self, char *bf,
> +				     size_t size, unsigned int width)
> +{
> +	struct map *map = self->ms.map;
> +	u64 addr = self->ip;
> +
> +	if (map)
> +		addr = map->unmap_ip(map, self->ip);
> +
> +	return repsep_snprintf(bf, size, "%#*llu", width, addr);
> +}
> +
> +struct sort_entry sort_addr = {
> +	.se_header      = "Address",
> +	.se_cmp	        = sort__addr_cmp,
> +	.se_snprintf    = hist_entry__addr_snprintf,
> +	.se_width_idx	= HISTC_ADDR,
> +};
> +
> +
>  /* sort keys for branch stacks */
>  
>  static int64_t
> @@ -871,6 +899,7 @@ static struct sort_dimension common_sort_dimensions[] = {
>  	DIM(SORT_PARENT, "parent", sort_parent),
>  	DIM(SORT_CPU, "cpu", sort_cpu),
>  	DIM(SORT_SRCLINE, "srcline", sort_srcline),
> +	DIM(SORT_ADDR, "addr", sort_addr),
>  };
>  
>  #undef DIM
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 0232d476da87..0815e344f38c 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -138,6 +138,7 @@ enum sort_type {
>  	SORT_PARENT,
>  	SORT_CPU,
>  	SORT_SRCLINE,
> +	SORT_ADDR,
>  
>  	/* branch stack specific sort keys */
>  	__SORT_BRANCH_STACK,
> -- 
> 1.7.11.7

Re: [PATCH 09/10] perf hist browser: Use sort__has_sym

[Arnaldo Carvalho de Melo]

Em Wed, Apr 03, 2013 at 09:26:18PM +0900, Namhyung Kim escreveu:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> The TUI hist browser had a similar variable has_symbols for the same
> purpose.  Let's get rid of the duplication.

I'm ok with that, if it involves removing sort__has_sym, that is a
global variable, making it impossible to use different sort orders in
the same session, if we ever want to do that :-)

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/ui/browsers/hists.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index cad8e37f05d9..a4268cab1921 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -25,7 +25,6 @@ struct hist_browser {
>  	struct map_symbol   *selection;
>  	int		     print_seq;
>  	bool		     show_dso;
> -	bool		     has_symbols;
>  };
>  
>  extern void hist_browser__init_hpp(void);
> @@ -1155,10 +1154,6 @@ static struct hist_browser *hist_browser__new(struct hists *hists)
>  		browser->b.refresh = hist_browser__refresh;
>  		browser->b.seek = ui_browser__hists_seek;
>  		browser->b.use_navkeypressed = true;
> -		if (sort__mode == SORT_MODE__BRANCH)
> -			browser->has_symbols = sort_sym_from.list.next != NULL;
> -		else
> -			browser->has_symbols = sort_sym.list.next != NULL;
>  	}
>  
>  	return browser;
> @@ -1386,7 +1381,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  			 */
>  			goto out_free_stack;
>  		case 'a':
> -			if (!browser->has_symbols) {
> +			if (!sort__has_sym) {
>  				ui_browser__warning(&browser->b, delay_secs * 2,
>  			"Annotation is only available for symbolic views, "
>  			"include \"sym*\" in --sort to use it.");
> @@ -1485,7 +1480,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  			continue;
>  		}
>  
> -		if (!browser->has_symbols)
> +		if (!sort__has_sym)
>  			goto add_exit_option;
>  
>  		if (sort__mode == SORT_MODE__BRANCH) {
> -- 
> 1.7.11.7

Re: [PATCH 10/10] perf sort: Consolidate sort_entry__setup_elide()

[Arnaldo Carvalho de Melo]

Em Wed, Apr 03, 2013 at 09:26:19PM +0900, Namhyung Kim escreveu:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> The same code was duplicate to places, factor them out to common
> sort__setup_elide().

Looks ok, applying after fixing up fuzzes due to this being at the end
of the patchseries. Things like this that are clear cleanups are best
positioned in the start of the patch series.

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-diff.c   |  4 +---
>  tools/perf/builtin-report.c | 20 +-------------------
>  tools/perf/builtin-top.c    |  4 +---
>  tools/perf/util/sort.c      | 45 +++++++++++++++++++++++++++++++++++++++++++--
>  tools/perf/util/sort.h      |  3 +--
>  5 files changed, 47 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index 03b56c542bb6..316bf13e59c7 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -611,9 +611,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix __maybe_unused)
>  
>  	setup_pager();
>  
> -	sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso", NULL);
> -	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", NULL);
> -	sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list, "symbol", NULL);
> +	sort__setup_elide(NULL);
>  
>  	return __cmd_diff();
>  }
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index c95fd92fbd44..bff244fa4b5d 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -937,25 +937,7 @@ repeat:
>  		report.symbol_filter_str = argv[0];
>  	}
>  
> -	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", stdout);
> -
> -	if (sort__mode == SORT_MODE__BRANCH) {
> -		sort_entry__setup_elide(&sort_dso_from, symbol_conf.dso_from_list, "dso_from", stdout);
> -		sort_entry__setup_elide(&sort_dso_to, symbol_conf.dso_to_list, "dso_to", stdout);
> -		sort_entry__setup_elide(&sort_sym_from, symbol_conf.sym_from_list, "sym_from", stdout);
> -		sort_entry__setup_elide(&sort_sym_to, symbol_conf.sym_to_list, "sym_to", stdout);
> -	} else {
> -		if (report.mem_mode) {
> -			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "symbol_daddr", stdout);
> -			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso_daddr", stdout);
> -			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "mem", stdout);
> -			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "local_weight", stdout);
> -			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "tlb", stdout);
> -			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "snoop", stdout);
> -		}
> -		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso", stdout);
> -		sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list, "symbol", stdout);
> -	}
> +	sort__setup_elide(stdout);
>  
>  	ret = __cmd_report(&report);
>  	if (ret == K_SWITCH_INPUT_DATA) {
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 4aa504baaf0b..fe4acf568483 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1201,9 +1201,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (symbol__init() < 0)
>  		return -1;
>  
> -	sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso", stdout);
> -	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", stdout);
> -	sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list, "symbol", stdout);
> +	sort__setup_elide(stdout);
>  
>  	get_term_dimensions(&top.winsize);
>  	if (top.print_entries == 0) {
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 213831133e08..86ae94d8782e 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1,5 +1,6 @@
>  #include "sort.h"
>  #include "hist.h"
> +#include "symbol.h"
>  
>  regex_t		parent_regex;
>  const char	default_parent_pattern[] = "^sys_|^do_page_fault";
> @@ -1085,8 +1086,9 @@ int setup_sorting(void)
>  	return ret;
>  }
>  
> -void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
> -			     const char *list_name, FILE *fp)
> +static void sort_entry__setup_elide(struct sort_entry *self,
> +				    struct strlist *list,
> +				    const char *list_name, FILE *fp)
>  {
>  	if (list && strlist__nr_entries(list) == 1) {
>  		if (fp != NULL)
> @@ -1095,3 +1097,42 @@ void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
>  		self->elide = true;
>  	}
>  }
> +
> +void sort__setup_elide(FILE *output)
> +{
> +	sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
> +				"dso", output);
> +	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list,
> +				"comm", output);
> +	sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list,
> +				"symbol", output);
> +
> +	if (sort__mode == SORT_MODE__BRANCH) {
> +		sort_entry__setup_elide(&sort_dso_from,
> +					symbol_conf.dso_from_list,
> +					"dso_from", output);
> +		sort_entry__setup_elide(&sort_dso_to,
> +					symbol_conf.dso_to_list,
> +					"dso_to", output);
> +		sort_entry__setup_elide(&sort_sym_from,
> +					symbol_conf.sym_from_list,
> +					"sym_from", output);
> +		sort_entry__setup_elide(&sort_sym_to,
> +					symbol_conf.sym_to_list,
> +					"sym_to", output);
> +	} else if (sort__mode == SORT_MODE__MEMORY) {
> +		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
> +					"symbol_daddr", output);
> +		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
> +					"dso_daddr", output);
> +		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
> +					"mem", output);
> +		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
> +					"local_weight", output);
> +		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
> +					"tlb", output);
> +		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
> +					"snoop", output);
> +	}
> +
> +}
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 1f945a3b2e5b..c80aac4ae3a2 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -184,7 +184,6 @@ extern struct list_head hist_entry__sort_list;
>  
>  int setup_sorting(void);
>  extern int sort_dimension__add(const char *);
> -void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
> -			     const char *list_name, FILE *fp);
> +void sort__setup_elide(FILE *fp);
>  
>  #endif	/* __PERF_SORT_H */
> -- 
> 1.7.11.7

Re: [PATCH 03/10] perf sort: Add 'addr' sort key

[Namhyung Kim]

Hi Arnaldo,

On Wed, 3 Apr 2013 14:06:10 -0300, Arnaldo Carvalho de Melo wrote:
> What I expected was that the result was this:
>
> perf report --sort addr | grep -v ^# | sort -k2 -n | less
>
> And in hexadecimal, can you fix this?

Oops, it was a mistake in the last minute change, sorry. :(

>
> Em Wed, Apr 03, 2013 at 09:26:12PM +0900, Namhyung Kim escreveu:
>>  static void hists__set_unres_dso_col_len(struct hists *hists, int dso)
>> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
>> index 14c2fe20aa62..9599f805828f 100644
>> --- a/tools/perf/util/hist.h
>> +++ b/tools/perf/util/hist.h
>> @@ -43,12 +43,13 @@ enum hist_column {
>>  	HISTC_COMM,
>>  	HISTC_PARENT,
>>  	HISTC_CPU,
>> +	HISTC_SRCLINE,
>
> Why move SRCLINE?

Because it's in common dimension.  I'd like to separate it to give a
consistent view.

Thanks,
Namhyung

>
>> +	HISTC_ADDR,
>>  	HISTC_MISPREDICT,
>>  	HISTC_SYMBOL_FROM,
>>  	HISTC_SYMBOL_TO,
>>  	HISTC_DSO_FROM,
>>  	HISTC_DSO_TO,
>> -	HISTC_SRCLINE,
>>  	HISTC_LOCAL_WEIGHT,
>>  	HISTC_GLOBAL_WEIGHT,
>>  	HISTC_MEM_DADDR_SYMBOL,
>> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
>> index 1dbf16949250..5640a95b3575 100644
>> --- a/tools/perf/util/sort.c
>> +++ b/tools/perf/util/sort.c
>> @@ -342,6 +342,34 @@ struct sort_entry sort_cpu = {
>>  	.se_width_idx	= HISTC_CPU,
>>  };
>>  
>> +/* --sort addr */
>> +
>> +static int64_t
>> +sort__addr_cmp(struct hist_entry *left, struct hist_entry *right)
>> +{
>> +	return right->ip - left->ip;
>> +}
>> +
>> +static int hist_entry__addr_snprintf(struct hist_entry *self, char *bf,
>> +				     size_t size, unsigned int width)
>> +{
>> +	struct map *map = self->ms.map;
>> +	u64 addr = self->ip;
>> +
>> +	if (map)
>> +		addr = map->unmap_ip(map, self->ip);
>> +
>> +	return repsep_snprintf(bf, size, "%#*llu", width, addr);
>> +}
>> +
>> +struct sort_entry sort_addr = {
>> +	.se_header      = "Address",
>> +	.se_cmp	        = sort__addr_cmp,
>> +	.se_snprintf    = hist_entry__addr_snprintf,
>> +	.se_width_idx	= HISTC_ADDR,
>> +};
>> +
>> +
>>  /* sort keys for branch stacks */
>>  
>>  static int64_t
>> @@ -871,6 +899,7 @@ static struct sort_dimension common_sort_dimensions[] = {
>>  	DIM(SORT_PARENT, "parent", sort_parent),
>>  	DIM(SORT_CPU, "cpu", sort_cpu),
>>  	DIM(SORT_SRCLINE, "srcline", sort_srcline),
>> +	DIM(SORT_ADDR, "addr", sort_addr),
>>  };
>>  
>>  #undef DIM
>> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
>> index 0232d476da87..0815e344f38c 100644
>> --- a/tools/perf/util/sort.h
>> +++ b/tools/perf/util/sort.h
>> @@ -138,6 +138,7 @@ enum sort_type {
>>  	SORT_PARENT,
>>  	SORT_CPU,
>>  	SORT_SRCLINE,
>> +	SORT_ADDR,
>>  
>>  	/* branch stack specific sort keys */
>>  	__SORT_BRANCH_STACK,
>> -- 
>> 1.7.11.7

Re: [PATCH 09/10] perf hist browser: Use sort__has_sym

[Namhyung Kim]

On Wed, 3 Apr 2013 14:07:54 -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 03, 2013 at 09:26:18PM +0900, Namhyung Kim escreveu:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> The TUI hist browser had a similar variable has_symbols for the same
>> purpose.  Let's get rid of the duplication.
>
> I'm ok with that, if it involves removing sort__has_sym, that is a
> global variable, making it impossible to use different sort orders in
> the same session, if we ever want to do that :-)

So you want to keep current code for a potential future change?

Anyway it seems current logic would fail if only "symbol_to" sort key
was used without "symbol_from" in branch mode.

Thanks,
Namhyung

>  
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/perf/ui/browsers/hists.c | 9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>> 
>> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
>> index cad8e37f05d9..a4268cab1921 100644
>> --- a/tools/perf/ui/browsers/hists.c
>> +++ b/tools/perf/ui/browsers/hists.c
>> @@ -25,7 +25,6 @@ struct hist_browser {
>>  	struct map_symbol   *selection;
>>  	int		     print_seq;
>>  	bool		     show_dso;
>> -	bool		     has_symbols;
>>  };
>>  
>>  extern void hist_browser__init_hpp(void);
>> @@ -1155,10 +1154,6 @@ static struct hist_browser *hist_browser__new(struct hists *hists)
>>  		browser->b.refresh = hist_browser__refresh;
>>  		browser->b.seek = ui_browser__hists_seek;
>>  		browser->b.use_navkeypressed = true;
>> -		if (sort__mode == SORT_MODE__BRANCH)
>> -			browser->has_symbols = sort_sym_from.list.next != NULL;
>> -		else
>> -			browser->has_symbols = sort_sym.list.next != NULL;
>>  	}
>>  
>>  	return browser;
>> @@ -1386,7 +1381,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>>  			 */
>>  			goto out_free_stack;
>>  		case 'a':
>> -			if (!browser->has_symbols) {
>> +			if (!sort__has_sym) {
>>  				ui_browser__warning(&browser->b, delay_secs * 2,
>>  			"Annotation is only available for symbolic views, "
>>  			"include \"sym*\" in --sort to use it.");
>> @@ -1485,7 +1480,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>>  			continue;
>>  		}
>>  
>> -		if (!browser->has_symbols)
>> +		if (!sort__has_sym)
>>  			goto add_exit_option;
>>  
>>  		if (sort__mode == SORT_MODE__BRANCH) {
>> -- 
>> 1.7.11.7

Re: [PATCH 10/10] perf sort: Consolidate sort_entry__setup_elide()

[Namhyung Kim]

On Wed, 3 Apr 2013 14:09:44 -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 03, 2013 at 09:26:19PM +0900, Namhyung Kim escreveu:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> The same code was duplicate to places, factor them out to common
>> sort__setup_elide().
>
> Looks ok, applying after fixing up fuzzes due to this being at the end
> of the patchseries. Things like this that are clear cleanups are best
> positioned in the start of the patch series.

Will do that next time!

Thanks,
Namhyung

[tip:perf/core] perf sort: Factor out common code in sort_dimension__add()

[tip-bot for Namhyung Kim]

Commit-ID:  2f532d09fa3a7eaf7cf1c23de9767eab8c8c0e7e
Gitweb:     http://git.kernel.org/tip/2f532d09fa3a7eaf7cf1c23de9767eab8c8c0e7e
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Wed, 3 Apr 2013 21:26:10 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 28 May 2013 16:23:53 +0300

perf sort: Factor out common code in sort_dimension__add()

Let's remove duplicate code.

Suggested-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1364991979-3008-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/sort.c | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index a6ddad4..a997955 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -895,6 +895,21 @@ static struct sort_dimension bstack_sort_dimensions[] = {
 
 #undef DIM
 
+static void __sort_dimension__add(struct sort_dimension *sd, enum sort_type idx)
+{
+	if (sd->taken)
+		return;
+
+	if (sd->entry->se_collapse)
+		sort__need_collapse = 1;
+
+	if (list_empty(&hist_entry__sort_list))
+		sort__first_dimension = idx;
+
+	list_add_tail(&sd->entry->list, &hist_entry__sort_list);
+	sd->taken = 1;
+}
+
 int sort_dimension__add(const char *tok)
 {
 	unsigned int i;
@@ -922,18 +937,7 @@ int sort_dimension__add(const char *tok)
 			sort__has_sym = 1;
 		}
 
-		if (sd->taken)
-			return 0;
-
-		if (sd->entry->se_collapse)
-			sort__need_collapse = 1;
-
-		if (list_empty(&hist_entry__sort_list))
-			sort__first_dimension = i;
-
-		list_add_tail(&sd->entry->list, &hist_entry__sort_list);
-		sd->taken = 1;
-
+		__sort_dimension__add(sd, i);
 		return 0;
 	}
 
@@ -949,18 +953,7 @@ int sort_dimension__add(const char *tok)
 		if (sd->entry == &sort_sym_from || sd->entry == &sort_sym_to)
 			sort__has_sym = 1;
 
-		if (sd->taken)
-			return 0;
-
-		if (sd->entry->se_collapse)
-			sort__need_collapse = 1;
-
-		if (list_empty(&hist_entry__sort_list))
-			sort__first_dimension = i + __SORT_BRANCH_STACK;
-
-		list_add_tail(&sd->entry->list, &hist_entry__sort_list);
-		sd->taken = 1;
-
+		__sort_dimension__add(sd, i + __SORT_BRANCH_STACK);
 		return 0;
 	}
 

[tip:perf/core] perf sort: Separate out memory-specific sort keys

[tip-bot for Namhyung Kim]

Commit-ID:  afab87b91f3f331d55664172dad8e476e6ffca9d
Gitweb:     http://git.kernel.org/tip/afab87b91f3f331d55664172dad8e476e6ffca9d
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Wed, 3 Apr 2013 21:26:11 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 28 May 2013 16:23:54 +0300

perf sort: Separate out memory-specific sort keys

Since they're used only for perf mem, separate out them to a different
dimension so that normal user cannot access them by any chance.

For global/local weights, I'm not entirely sure to place them into the
memory dimension.  But it's the only user at this time.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1364991979-3008-3-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-report.c |  2 ++
 tools/perf/util/sort.c      | 39 +++++++++++++++++++++++++++++++--------
 tools/perf/util/sort.h      | 19 +++++++++++--------
 3 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index c877982..669405c 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -871,6 +871,8 @@ repeat:
 			fprintf(stderr, "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
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index a997955..1dbf169 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -871,14 +871,6 @@ static struct sort_dimension common_sort_dimensions[] = {
 	DIM(SORT_PARENT, "parent", sort_parent),
 	DIM(SORT_CPU, "cpu", sort_cpu),
 	DIM(SORT_SRCLINE, "srcline", sort_srcline),
-	DIM(SORT_LOCAL_WEIGHT, "local_weight", sort_local_weight),
-	DIM(SORT_GLOBAL_WEIGHT, "weight", sort_global_weight),
-	DIM(SORT_MEM_DADDR_SYMBOL, "symbol_daddr", sort_mem_daddr_sym),
-	DIM(SORT_MEM_DADDR_DSO, "dso_daddr", sort_mem_daddr_dso),
-	DIM(SORT_MEM_LOCKED, "locked", sort_mem_locked),
-	DIM(SORT_MEM_TLB, "tlb", sort_mem_tlb),
-	DIM(SORT_MEM_LVL, "mem", sort_mem_lvl),
-	DIM(SORT_MEM_SNOOP, "snoop", sort_mem_snoop),
 };
 
 #undef DIM
@@ -895,6 +887,21 @@ static struct sort_dimension bstack_sort_dimensions[] = {
 
 #undef DIM
 
+#define DIM(d, n, func) [d - __SORT_MEMORY_MODE] = { .name = n, .entry = &(func) }
+
+static struct sort_dimension memory_sort_dimensions[] = {
+	DIM(SORT_LOCAL_WEIGHT, "local_weight", sort_local_weight),
+	DIM(SORT_GLOBAL_WEIGHT, "weight", sort_global_weight),
+	DIM(SORT_MEM_DADDR_SYMBOL, "symbol_daddr", sort_mem_daddr_sym),
+	DIM(SORT_MEM_DADDR_DSO, "dso_daddr", sort_mem_daddr_dso),
+	DIM(SORT_MEM_LOCKED, "locked", sort_mem_locked),
+	DIM(SORT_MEM_TLB, "tlb", sort_mem_tlb),
+	DIM(SORT_MEM_LVL, "mem", sort_mem_lvl),
+	DIM(SORT_MEM_SNOOP, "snoop", sort_mem_snoop),
+};
+
+#undef DIM
+
 static void __sort_dimension__add(struct sort_dimension *sd, enum sort_type idx)
 {
 	if (sd->taken)
@@ -957,6 +964,22 @@ int sort_dimension__add(const char *tok)
 		return 0;
 	}
 
+	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;
+
+		if (sort__mode != SORT_MODE__MEMORY)
+			return -EINVAL;
+
+		if (sd->entry == &sort_mem_daddr_sym)
+			sort__has_sym = 1;
+
+		__sort_dimension__add(sd, i + __SORT_MEMORY_MODE);
+		return 0;
+	}
+
 	return -ESRCH;
 }
 
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 39ff4b8..0232d47 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -138,14 +138,6 @@ enum sort_type {
 	SORT_PARENT,
 	SORT_CPU,
 	SORT_SRCLINE,
-	SORT_LOCAL_WEIGHT,
-	SORT_GLOBAL_WEIGHT,
-	SORT_MEM_DADDR_SYMBOL,
-	SORT_MEM_DADDR_DSO,
-	SORT_MEM_LOCKED,
-	SORT_MEM_TLB,
-	SORT_MEM_LVL,
-	SORT_MEM_SNOOP,
 
 	/* branch stack specific sort keys */
 	__SORT_BRANCH_STACK,
@@ -154,6 +146,17 @@ enum sort_type {
 	SORT_SYM_FROM,
 	SORT_SYM_TO,
 	SORT_MISPREDICT,
+
+	/* memory mode specific sort keys */
+	__SORT_MEMORY_MODE,
+	SORT_LOCAL_WEIGHT = __SORT_MEMORY_MODE,
+	SORT_GLOBAL_WEIGHT,
+	SORT_MEM_DADDR_SYMBOL,
+	SORT_MEM_DADDR_DSO,
+	SORT_MEM_LOCKED,
+	SORT_MEM_TLB,
+	SORT_MEM_LVL,
+	SORT_MEM_SNOOP,
 };
 
 /*

[tip:perf/core] perf sort: Consolidate sort_entry__setup_elide()

[tip-bot for Namhyung Kim]

Commit-ID:  08e71542fd0f4a0e30b4e3794329d63ae891e0c0
Gitweb:     http://git.kernel.org/tip/08e71542fd0f4a0e30b4e3794329d63ae891e0c0
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Wed, 3 Apr 2013 21:26:19 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 28 May 2013 16:23:54 +0300

perf sort: Consolidate sort_entry__setup_elide()

The same code was duplicate to places, factor them out to common
sort__setup_elide().

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1364991979-3008-11-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-diff.c   |  4 +---
 tools/perf/builtin-report.c | 20 +-------------------
 tools/perf/builtin-top.c    |  4 +---
 tools/perf/util/sort.c      | 45 +++++++++++++++++++++++++++++++++++++++++++--
 tools/perf/util/sort.h      |  3 +--
 5 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 2d0462d..cabbea5 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -611,9 +611,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	setup_pager();
 
-	sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso", NULL);
-	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", NULL);
-	sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list, "symbol", NULL);
+	sort__setup_elide(NULL);
 
 	return __cmd_diff();
 }
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 669405c..d45bf9b 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -937,25 +937,7 @@ repeat:
 		report.symbol_filter_str = argv[0];
 	}
 
-	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", stdout);
-
-	if (sort__mode == SORT_MODE__BRANCH) {
-		sort_entry__setup_elide(&sort_dso_from, symbol_conf.dso_from_list, "dso_from", stdout);
-		sort_entry__setup_elide(&sort_dso_to, symbol_conf.dso_to_list, "dso_to", stdout);
-		sort_entry__setup_elide(&sort_sym_from, symbol_conf.sym_from_list, "sym_from", stdout);
-		sort_entry__setup_elide(&sort_sym_to, symbol_conf.sym_to_list, "sym_to", stdout);
-	} else {
-		if (report.mem_mode) {
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "symbol_daddr", stdout);
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso_daddr", stdout);
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "mem", stdout);
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "local_weight", stdout);
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "tlb", stdout);
-			sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "snoop", stdout);
-		}
-		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso", stdout);
-		sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list, "symbol", stdout);
-	}
+	sort__setup_elide(stdout);
 
 	ret = __cmd_report(&report);
 	if (ret == K_SWITCH_INPUT_DATA) {
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 67bdb9f..2eb272d 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1200,9 +1200,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (symbol__init() < 0)
 		return -1;
 
-	sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list, "dso", stdout);
-	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", stdout);
-	sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list, "symbol", stdout);
+	sort__setup_elide(stdout);
 
 	/*
 	 * Avoid annotation data structures overhead when symbols aren't on the
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 1dbf169..701ab1d 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1,5 +1,6 @@
 #include "sort.h"
 #include "hist.h"
+#include "symbol.h"
 
 regex_t		parent_regex;
 const char	default_parent_pattern[] = "^sys_|^do_page_fault";
@@ -1009,8 +1010,9 @@ int setup_sorting(void)
 	return ret;
 }
 
-void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
-			     const char *list_name, FILE *fp)
+static void sort_entry__setup_elide(struct sort_entry *self,
+				    struct strlist *list,
+				    const char *list_name, FILE *fp)
 {
 	if (list && strlist__nr_entries(list) == 1) {
 		if (fp != NULL)
@@ -1019,3 +1021,42 @@ void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
 		self->elide = true;
 	}
 }
+
+void sort__setup_elide(FILE *output)
+{
+	sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+				"dso", output);
+	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list,
+				"comm", output);
+	sort_entry__setup_elide(&sort_sym, symbol_conf.sym_list,
+				"symbol", output);
+
+	if (sort__mode == SORT_MODE__BRANCH) {
+		sort_entry__setup_elide(&sort_dso_from,
+					symbol_conf.dso_from_list,
+					"dso_from", output);
+		sort_entry__setup_elide(&sort_dso_to,
+					symbol_conf.dso_to_list,
+					"dso_to", output);
+		sort_entry__setup_elide(&sort_sym_from,
+					symbol_conf.sym_from_list,
+					"sym_from", output);
+		sort_entry__setup_elide(&sort_sym_to,
+					symbol_conf.sym_to_list,
+					"sym_to", output);
+	} else if (sort__mode == SORT_MODE__MEMORY) {
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"symbol_daddr", output);
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"dso_daddr", output);
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"mem", output);
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"local_weight", output);
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"tlb", output);
+		sort_entry__setup_elide(&sort_dso, symbol_conf.dso_list,
+					"snoop", output);
+	}
+
+}
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 0232d47..51f1b5a 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -181,7 +181,6 @@ extern struct list_head hist_entry__sort_list;
 
 int setup_sorting(void);
 extern int sort_dimension__add(const char *);
-void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
-			     const char *list_name, FILE *fp);
+void sort__setup_elide(FILE *fp);
 
 #endif	/* __PERF_SORT_H */

Re: [tip:perf/core] perf sort: Separate out memory-specific sort keys

[Andi Kleen]

On Fri, May 31, 2013 at 04:20:20AM -0700, tip-bot for Namhyung Kim wrote:
> perf sort: Separate out memory-specific sort keys
> 
> Since they're used only for perf mem, separate out them to a different
> dimension so that normal user cannot access them by any chance.
> 
> For global/local weights, I'm not entirely sure to place them into the
> memory dimension.  But it's the only user at this time.

So I was finally able to test with this patch, but 
I found it completely breaks TSX weight abort profiling.

It uses weight (global/local), but it's not 
running in memory mode.

I'll send a patch to move weight back into
the common keys.

-Andi

Re: [tip:perf/core] perf sort: Separate out memory-specific sort keys

[Namhyung Kim]

Hi Andi,

2013-06-25 AM 9:30, Andi Kleen wrote:
> On Fri, May 31, 2013 at 04:20:20AM -0700, tip-bot for Namhyung Kim wrote:
>> perf sort: Separate out memory-specific sort keys
>>
>> Since they're used only for perf mem, separate out them to a different
>> dimension so that normal user cannot access them by any chance.
>>
>> For global/local weights, I'm not entirely sure to place them into the
>> memory dimension.  But it's the only user at this time.
>
> So I was finally able to test with this patch, but
> I found it completely breaks TSX weight abort profiling.
>
> It uses weight (global/local), but it's not
> running in memory mode.
>
> I'll send a patch to move weight back into
> the common keys.

I'm not sure it should move to the common keys as normal perf session 
won't have those.  I guess you need to set up a couple of TSX-specific 
sort keys like perf mem, if so what about moving the two weights to your 
table?

Thanks,
Namhyung

Re: [tip:perf/core] perf sort: Separate out memory-specific sort keys

[Andi Kleen]

> I'm not sure it should move to the common keys as normal perf
> session won't have those.  

Why not? If I enable weight sampling i get weights perfectly
fine in any session using the right PEBS events.

> I guess you need to set up a couple of
> TSX-specific sort keys like perf mem, if so what about moving the
> two weights to your table?

There's no TSX table and no separate perf TSX session.
It's just some additional attributes.

-Andi

Re: [tip:perf/core] perf sort: Separate out memory-specific sort keys

[Namhyung Kim]

2013-06-25 AM 10:02, Andi Kleen wrote:
>> I'm not sure it should move to the common keys as normal perf
>> session won't have those.
>
> Why not? If I enable weight sampling i get weights perfectly
> fine in any session using the right PEBS events.
>
>> I guess you need to set up a couple of
>> TSX-specific sort keys like perf mem, if so what about moving the
>> two weights to your table?
>
> There's no TSX table and no separate perf TSX session.
> It's just some additional attributes.

Okay then, I have no objection for moving them. :)

Thanks,
Namhyung