All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys
@ 2013-04-01 11:35 Namhyung Kim
  2013-04-01 11:35 ` [PATCH 1/9] perf hists: Fix an invalid memory free on he->branch_info Namhyung Kim
                   ` (9 more replies)
  0 siblings, 10 replies; 21+ messages in thread
From: Namhyung Kim @ 2013-04-01 11:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
	Stephane Eranian, Andi Kleen, Jiri Olsa, David Ahern

Hi,

This is a couple of bugfix, cleanup and enhancement for sort keys.

When I rebased previous --sort addr* patches, I found that separating
sort mode might be helpful to users so I did the cleanup.  But during
the test I, in turn, found a bug in freeing the branch info of a hist
entry.  And the mem info patch is only compile-tested - it'd be great
if Stephane could take a look and help to test.

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

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


Any comments are welcome, thanks.
Namhyung


Namhyung Kim (9):
  perf hists: Fix an invalid memory free on he->branch_info
  perf hists: Free unused mem info of a matched hist entry
  perf report: Fix alignment of symbol column when -v is given
  perf sort: Introduce sort__mode variable
  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

 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              |  40 ++++++----
 tools/perf/builtin-top.c                 |   3 +-
 tools/perf/ui/browsers/hists.c           |   4 +-
 tools/perf/util/hist.c                   |  63 ++++++++++-----
 tools/perf/util/hist.h                   |   5 +-
 tools/perf/util/sort.c                   | 129 ++++++++++++++++++++++++++++---
 tools/perf/util/sort.h                   |  30 ++++---
 11 files changed, 223 insertions(+), 62 deletions(-)

-- 
1.7.11.7


^ permalink raw reply	[flat|nested] 21+ messages in thread
* [PATCH 1/9] perf hists: Fix an invalid memory free on he->branch_info
  2013-04-01 11:35 [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Namhyung Kim
@ 2013-04-01 11:35 ` Namhyung Kim
  2013-05-31 11:13   ` [tip:perf/core] perf hists: Fix an invalid memory free on he-> branch_info tip-bot for Namhyung Kim
  2013-04-01 11:35 ` [PATCH 2/9] perf hists: Free unused mem info of a matched hist entry Namhyung Kim
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 21+ messages in thread
From: Namhyung Kim @ 2013-04-01 11:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
	Stephane Eranian, Andi Kleen, Jiri Olsa, David Ahern

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

The branch info was allocated for the whole stack and passed matching
hist entry for each level during processing samples.  Thus when a hist
entry tries to free its branch info like in hists__collapse_insert_entry
it'll face following error.

  *** glibc detected *** perf: munmap_chunk(): invalid pointer: 0x00000000014e9d20 ***
  ======= Backtrace: =========
  /lib64/libc.so.6[0x387d47ae16]
  perf[0x4923bd]
  perf(cmd_report+0xd68)[0x432a08]
  perf[0x41a663]
  perf(main+0x58f)[0x419eaf]
  /lib64/libc.so.6(__libc_start_main+0xf5)[0x387d421735]
  perf[0x419f95]

Fix it by allocating and copying branch info for each new hist entry.

Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c |  9 ++++++---
 tools/perf/util/hist.c      | 14 ++++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index bd0ca81eeaca..d9f2de3e81fe 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -187,6 +187,9 @@ static int perf_report__add_branch_hist_entry(struct perf_tool *tool,
 	for (i = 0; i < sample->branch_stack->nr; i++) {
 		if (rep->hide_unresolved && !(bi[i].from.sym && bi[i].to.sym))
 			continue;
+
+		err = -ENOMEM;
+
 		/*
 		 * The report shows the percentage of total branches captured
 		 * and not events sampled. Thus we use a pseudo period of 1.
@@ -195,7 +198,6 @@ static int perf_report__add_branch_hist_entry(struct perf_tool *tool,
 				&bi[i], 1, 1);
 		if (he) {
 			struct annotation *notes;
-			err = -ENOMEM;
 			bx = he->branch_info;
 			if (bx->from.sym && use_browser == 1 && sort__has_sym) {
 				notes = symbol__annotation(bx->from.sym);
@@ -226,11 +228,12 @@ static int perf_report__add_branch_hist_entry(struct perf_tool *tool,
 			}
 			evsel->hists.stats.total_period += 1;
 			hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE);
-			err = 0;
 		} else
-			return -ENOMEM;
+			goto out;
 	}
+	err = 0;
 out:
+	free(bi);
 	return err;
 }
 
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 6b32721f829a..9438d576459d 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -292,6 +292,20 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template)
 			he->ms.map->referenced = true;
 
 		if (he->branch_info) {
+			/*
+			 * This branch info is (a part of) allocated from
+			 * machine__resolve_bstack() and will be freed after
+			 * adding new entries.  So we need to save a copy.
+			 */
+			he->branch_info = malloc(sizeof(*he->branch_info));
+			if (he->branch_info == NULL) {
+				free(he);
+				return NULL;
+			}
+
+			memcpy(he->branch_info, template->branch_info,
+			       sizeof(*he->branch_info));
+
 			if (he->branch_info->from.map)
 				he->branch_info->from.map->referenced = true;
 			if (he->branch_info->to.map)
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 2/9] perf hists: Free unused mem info of a matched hist entry
  2013-04-01 11:35 [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Namhyung Kim
  2013-04-01 11:35 ` [PATCH 1/9] perf hists: Fix an invalid memory free on he->branch_info Namhyung Kim
@ 2013-04-01 11:35 ` Namhyung Kim
  2013-05-31 11:15   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2013-04-01 11:35 ` [PATCH 3/9] perf report: Fix alignment of symbol column when -v is given Namhyung Kim
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 21+ messages in thread
From: Namhyung Kim @ 2013-04-01 11:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
	Stephane Eranian, Andi Kleen, Jiri Olsa, David Ahern

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

The mem info is shared between matched entries so one should be freed.

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

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 9438d576459d..514fc0470e38 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -374,6 +374,12 @@ static struct hist_entry *add_hist_entry(struct hists *hists,
 		if (!cmp) {
 			he_stat__add_period(&he->stat, period, weight);
 
+			/*
+			 * This mem info was allocated from machine__resolve_mem
+			 * and will not be used anymore.
+			 */
+			free(entry->mem_info);
+
 			/* If the map of an existing hist_entry has
 			 * become out-of-date due to an exec() or
 			 * similar, update it.  Otherwise we will
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 3/9] perf report: Fix alignment of symbol column when -v is given
  2013-04-01 11:35 [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Namhyung Kim
  2013-04-01 11:35 ` [PATCH 1/9] perf hists: Fix an invalid memory free on he->branch_info Namhyung Kim
  2013-04-01 11:35 ` [PATCH 2/9] perf hists: Free unused mem info of a matched hist entry Namhyung Kim
@ 2013-04-01 11:35 ` Namhyung Kim
  2013-05-31 11:16   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2013-04-01 11:35 ` [PATCH 4/9] perf sort: Introduce sort__mode variable Namhyung Kim
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 21+ messages in thread
From: Namhyung Kim @ 2013-04-01 11:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
	Stephane Eranian, Andi Kleen, Jiri Olsa, David Ahern

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

When -v option is given, the symbol sort key prints its address also
but it wasn't properly aligned since hists__calc_col_len() misses the
additional part.  Also it missed 2 spaces for 0x prefix when printing.

  $ perf report --stdio -v -s sym
  # Samples: 133  of event 'cycles'
  # Event count (approx.): 50536717
  #
  # Overhead                          Symbol
  # ........  ..............................
  #
      12.20%  0xffffffff81384c50 v [k] intel_idle
       7.62%  0xffffffff8170976a v [k] ftrace_caller
       7.02%  0x2d986d         B [.] 0x00000000002d986d

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 26 +++++++++++++++-----------
 tools/perf/util/sort.c |  2 +-
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 514fc0470e38..72b4eec820c3 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -70,9 +70,17 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 	int symlen;
 	u16 len;
 
-	if (h->ms.sym)
-		hists__new_col_len(hists, HISTC_SYMBOL, h->ms.sym->namelen + 4);
-	else {
+	/*
+	 * +4 accounts for '[x] ' priv level info
+	 * +2 accounts for 0x prefix on raw addresses
+	 * +3 accounts for ' y ' symtab origin info
+	 */
+	if (h->ms.sym) {
+		symlen = h->ms.sym->namelen + 4;
+		if (verbose)
+			symlen += BITS_PER_LONG / 4 + 2 + 3;
+		hists__new_col_len(hists, HISTC_SYMBOL, symlen);
+	} else {
 		symlen = unresolved_col_width + 4 + 2;
 		hists__new_col_len(hists, HISTC_SYMBOL, symlen);
 		hists__set_unres_dso_col_len(hists, HISTC_DSO);
@@ -91,12 +99,10 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 		hists__new_col_len(hists, HISTC_PARENT, h->parent->namelen);
 
 	if (h->branch_info) {
-		/*
-		 * +4 accounts for '[x] ' priv level info
-		 * +2 account of 0x prefix on raw addresses
-		 */
 		if (h->branch_info->from.sym) {
 			symlen = (int)h->branch_info->from.sym->namelen + 4;
+			if (verbose)
+				symlen += BITS_PER_LONG / 4 + 2 + 3;
 			hists__new_col_len(hists, HISTC_SYMBOL_FROM, symlen);
 
 			symlen = dso__name_len(h->branch_info->from.map->dso);
@@ -109,6 +115,8 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 
 		if (h->branch_info->to.sym) {
 			symlen = (int)h->branch_info->to.sym->namelen + 4;
+			if (verbose)
+				symlen += BITS_PER_LONG / 4 + 2 + 3;
 			hists__new_col_len(hists, HISTC_SYMBOL_TO, symlen);
 
 			symlen = dso__name_len(h->branch_info->to.map->dso);
@@ -121,10 +129,6 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 	}
 
 	if (h->mem_info) {
-		/*
-		 * +4 accounts for '[x] ' priv level info
-		 * +2 account of 0x prefix on raw addresses
-		 */
 		if (h->mem_info->daddr.sym) {
 			symlen = (int)h->mem_info->daddr.sym->namelen + 4
 			       + unresolved_col_width + 2;
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 5f52d492590c..16d5e38befe5 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -194,7 +194,7 @@ static int _hist_entry__sym_snprintf(struct map *map, struct symbol *sym,
 	if (verbose) {
 		char o = map ? dso__symtab_origin(map->dso) : '!';
 		ret += repsep_snprintf(bf, size, "%-#*llx %c ",
-				       BITS_PER_LONG / 4, ip, o);
+				       BITS_PER_LONG / 4 + 2, ip, o);
 	}
 
 	ret += repsep_snprintf(bf + ret, size - ret, "[%c] ", level);
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 4/9] perf sort: Introduce sort__mode variable
  2013-04-01 11:35 [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Namhyung Kim
                   ` (2 preceding siblings ...)
  2013-04-01 11:35 ` [PATCH 3/9] perf report: Fix alignment of symbol column when -v is given Namhyung Kim
@ 2013-04-01 11:35 ` Namhyung Kim
  2013-05-31 11:17   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2013-04-01 11:35 ` [PATCH 5/9] perf sort: Separate out memory-specific sort keys Namhyung Kim
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 21+ messages in thread
From: Namhyung Kim @ 2013-04-01 11:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
	Stephane Eranian, Andi Kleen, Jiri Olsa, David Ahern

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

It's used for determining current sort mode which can be one of
NORMAL, BRANCH and new MEMORY.

Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c    | 23 +++++++++++++----------
 tools/perf/ui/browsers/hists.c |  4 ++--
 tools/perf/util/sort.c         |  4 ++--
 tools/perf/util/sort.h         |  8 +++++++-
 4 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index d9f2de3e81fe..c877982a64d3 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -311,7 +311,7 @@ static int process_sample_event(struct perf_tool *tool,
 	if (rep->cpu_list && !test_bit(sample->cpu, rep->cpu_bitmap))
 		return 0;
 
-	if (sort__branch_mode == 1) {
+	if (sort__mode == SORT_MODE__BRANCH) {
 		if (perf_report__add_branch_hist_entry(tool, &al, sample,
 						       evsel, machine)) {
 			pr_debug("problem adding lbr entry, skipping event\n");
@@ -387,7 +387,7 @@ static int perf_report__setup_sample_type(struct perf_report *rep)
 			}
 	}
 
-	if (sort__branch_mode == 1) {
+	if (sort__mode == SORT_MODE__BRANCH) {
 		if (!self->fd_pipe &&
 		    !(sample_type & PERF_SAMPLE_BRANCH_STACK)) {
 			ui__error("Selected -b but no branch data. "
@@ -694,7 +694,9 @@ static int
 parse_branch_mode(const struct option *opt __maybe_unused,
 		  const char *str __maybe_unused, int unset)
 {
-	sort__branch_mode = !unset;
+	int *branch_mode = opt->value;
+
+	*branch_mode = !unset;
 	return 0;
 }
 
@@ -703,6 +705,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	struct perf_session *session;
 	struct stat st;
 	bool has_br_stack = false;
+	int branch_mode = -1;
 	int ret = -1;
 	char callchain_default_opt[] = "fractal,0.5,callee";
 	const char * const report_usage[] = {
@@ -799,7 +802,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		    "Show a column with the sum of periods"),
 	OPT_BOOLEAN(0, "group", &symbol_conf.event_group,
 		    "Show event group information together"),
-	OPT_CALLBACK_NOOPT('b', "branch-stack", &sort__branch_mode, "",
+	OPT_CALLBACK_NOOPT('b', "branch-stack", &branch_mode, "",
 		    "use branch records for histogram filling", parse_branch_mode),
 	OPT_STRING(0, "objdump", &objdump_path, "path",
 		   "objdump binary to use for disassembly and annotations"),
@@ -849,11 +852,11 @@ repeat:
 	has_br_stack = perf_header__has_feat(&session->header,
 					     HEADER_BRANCH_STACK);
 
-	if (sort__branch_mode == -1 && has_br_stack)
-		sort__branch_mode = 1;
+	if (branch_mode == -1 && has_br_stack)
+		sort__mode = SORT_MODE__BRANCH;
 
-	/* sort__branch_mode could be 0 if --no-branch-stack */
-	if (sort__branch_mode == 1) {
+	/* 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
@@ -864,7 +867,7 @@ repeat:
 
 	}
 	if (report.mem_mode) {
-		if (sort__branch_mode == 1) {
+		if (sort__mode == SORT_MODE__BRANCH) {
 			fprintf(stderr, "branch and mem mode incompatible\n");
 			goto error;
 		}
@@ -934,7 +937,7 @@ repeat:
 
 	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", stdout);
 
-	if (sort__branch_mode == 1) {
+	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);
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 0cada654d387..cea7ef969cbc 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1155,7 +1155,7 @@ 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__branch_mode == 1)
+		if (sort__mode == SORT_MODE__BRANCH)
 			browser->has_symbols = sort_sym_from.list.next != NULL;
 		else
 			browser->has_symbols = sort_sym.list.next != NULL;
@@ -1487,7 +1487,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		if (!browser->has_symbols)
 			goto add_exit_option;
 
-		if (sort__branch_mode == 1) {
+		if (sort__mode == SORT_MODE__BRANCH) {
 			bi = browser->he_selection->branch_info;
 			if (browser->selection != NULL &&
 			    bi &&
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 16d5e38befe5..a6ddad41d57a 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -9,7 +9,7 @@ const char	*sort_order = default_sort_order;
 int		sort__need_collapse = 0;
 int		sort__has_parent = 0;
 int		sort__has_sym = 0;
-int		sort__branch_mode = -1; /* -1 = means not set */
+enum sort_mode	sort__mode = SORT_MODE__NORMAL;
 
 enum sort_type	sort__first_dimension;
 
@@ -943,7 +943,7 @@ int sort_dimension__add(const char *tok)
 		if (strncasecmp(tok, sd->name, strlen(tok)))
 			continue;
 
-		if (sort__branch_mode != 1)
+		if (sort__mode != SORT_MODE__BRANCH)
 			return -EINVAL;
 
 		if (sd->entry == &sort_sym_from || sd->entry == &sort_sym_to)
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index f24bdf64238c..39ff4b86ae84 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -32,7 +32,7 @@ extern const char default_sort_order[];
 extern int sort__need_collapse;
 extern int sort__has_parent;
 extern int sort__has_sym;
-extern int sort__branch_mode;
+extern enum sort_mode sort__mode;
 extern struct sort_entry sort_comm;
 extern struct sort_entry sort_dso;
 extern struct sort_entry sort_sym;
@@ -123,6 +123,12 @@ static inline void hist_entry__add_pair(struct hist_entry *he,
 	list_add_tail(&he->pairs.head, &pair->pairs.node);
 }
 
+enum sort_mode {
+	SORT_MODE__NORMAL,
+	SORT_MODE__BRANCH,
+	SORT_MODE__MEMORY,
+};
+
 enum sort_type {
 	/* common sort keys */
 	SORT_PID,
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 5/9] perf sort: Separate out memory-specific sort keys
  2013-04-01 11:35 [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Namhyung Kim
                   ` (3 preceding siblings ...)
  2013-04-01 11:35 ` [PATCH 4/9] perf sort: Introduce sort__mode variable Namhyung Kim
@ 2013-04-01 11:35 ` Namhyung Kim
  2013-04-01 20:29   ` Jiri Olsa
  2013-04-01 11:35 ` [PATCH 6/9] perf sort: Add 'addr' sort key Namhyung Kim
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 21+ messages in thread
From: Namhyung Kim @ 2013-04-01 11:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
	Stephane Eranian, Andi Kleen, Jiri Olsa, David Ahern

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      | 50 +++++++++++++++++++++++++++++++++++++--------
 tools/perf/util/sort.h      | 19 +++++++++--------
 3 files changed, 55 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 a6ddad41d57a..c19bf213cc86 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
+
 int sort_dimension__add(const char *tok)
 {
 	unsigned int i;
@@ -964,6 +971,33 @@ 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;
+
+		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_MEMORY_MODE;
+
+		list_add_tail(&sd->entry->list, &hist_entry__sort_list);
+		sd->taken = 1;
+
+		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


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 6/9] perf sort: Add 'addr' sort key
  2013-04-01 11:35 [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Namhyung Kim
                   ` (4 preceding siblings ...)
  2013-04-01 11:35 ` [PATCH 5/9] perf sort: Separate out memory-specific sort keys Namhyung Kim
@ 2013-04-01 11:35 ` Namhyung Kim
  2013-04-01 20:40   ` Jiri Olsa
  2013-04-01 11:35 ` [PATCH 7/9] perf sort: Add 'addr_to/from' " Namhyung Kim
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 21+ messages in thread
From: Namhyung Kim @ 2013-04-01 11:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
	Stephane Eranian, Andi Kleen, Jiri Olsa, David Ahern

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 | 23 +++++++++++++++++++++++
 tools/perf/util/sort.h |  1 +
 4 files changed, 28 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 c19bf213cc86..fecde9347cbd 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -342,6 +342,28 @@ 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)
+{
+	return repsep_snprintf(bf, size, "%#*"PRIx64, width, (uint64_t)self->ip);
+}
+
+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 +893,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


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 7/9] perf sort: Add 'addr_to/from' sort key
  2013-04-01 11:35 [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Namhyung Kim
                   ` (5 preceding siblings ...)
  2013-04-01 11:35 ` [PATCH 6/9] perf sort: Add 'addr' sort key Namhyung Kim
@ 2013-04-01 11:35 ` Namhyung Kim
  2013-04-01 11:35 ` [PATCH 8/9] perf sort: Update documentation for sort keys Namhyung Kim
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2013-04-01 11:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
	Stephane Eranian, Andi Kleen, Jiri Olsa, David Ahern

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 fecde9347cbd..19f360b6e698 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -436,6 +436,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,
@@ -464,6 +498,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)
 {
@@ -905,6 +953,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


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 8/9] perf sort: Update documentation for sort keys
  2013-04-01 11:35 [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Namhyung Kim
                   ` (6 preceding siblings ...)
  2013-04-01 11:35 ` [PATCH 7/9] perf sort: Add 'addr_to/from' " Namhyung Kim
@ 2013-04-01 11:35 ` Namhyung Kim
  2013-04-01 11:35 ` [PATCH 9/9] perf hists: Move column length setting code Namhyung Kim
  2013-04-02 15:05 ` [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Arnaldo Carvalho de Melo
  9 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2013-04-01 11:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
	Stephane Eranian, Andi Kleen, Jiri Olsa, David Ahern

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


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 9/9] perf hists: Move column length setting code
  2013-04-01 11:35 [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Namhyung Kim
                   ` (7 preceding siblings ...)
  2013-04-01 11:35 ` [PATCH 8/9] perf sort: Update documentation for sort keys Namhyung Kim
@ 2013-04-01 11:35 ` Namhyung Kim
  2013-04-02 15:05 ` [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Arnaldo Carvalho de Melo
  9 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2013-04-01 11:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
	Stephane Eranian, Andi Kleen, Jiri Olsa, David Ahern

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


^ permalink raw reply related	[flat|nested] 21+ messages in thread
* Re: [PATCH 5/9] perf sort: Separate out memory-specific sort keys
  2013-04-01 11:35 ` [PATCH 5/9] perf sort: Separate out memory-specific sort keys Namhyung Kim
@ 2013-04-01 20:29   ` Jiri Olsa
  2013-04-02  1:56     ` Namhyung Kim
  0 siblings, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2013-04-01 20:29 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, Namhyung Kim, LKML, Stephane Eranian, Andi Kleen,
	David Ahern

On Mon, Apr 01, 2013 at 08:35:21PM +0900, Namhyung Kim wrote:

SNIP

> +
>  int sort_dimension__add(const char *tok)
>  {
>  	unsigned int i;
> @@ -964,6 +971,33 @@ 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;
> +
> +		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_MEMORY_MODE;
> +
> +		list_add_tail(&sd->entry->list, &hist_entry__sort_list);
> +		sd->taken = 1;
> +
> +		return 0;

nice idea, I wonder some of the above code be put into function
and used also in previous loop, no big deal though

jirka

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 6/9] perf sort: Add 'addr' sort key
  2013-04-01 11:35 ` [PATCH 6/9] perf sort: Add 'addr' sort key Namhyung Kim
@ 2013-04-01 20:40   ` Jiri Olsa
  2013-04-02  2:15     ` Namhyung Kim
  0 siblings, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2013-04-01 20:40 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, Namhyung Kim, LKML, Stephane Eranian, Andi Kleen,
	David Ahern

On Mon, Apr 01, 2013 at 08:35:22PM +0900, Namhyung Kim wrote:
> 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.

maybe it's just the recent mem profiling patches, but wouldn't
it be better to use 'ip' instead of 'addr'?

also it's following code getting the data:
...
        if (sample_type & PERF_SAMPLE_IP)
                data->ip = perf_instruction_pointer(regs);
...

same for the "perf sort: Add 'addr_to/from' sort key" patch

jirka

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 5/9] perf sort: Separate out memory-specific sort keys
  2013-04-01 20:29   ` Jiri Olsa
@ 2013-04-02  1:56     ` Namhyung Kim
  0 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2013-04-02  1:56 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, Namhyung Kim, LKML, Stephane Eranian, Andi Kleen,
	David Ahern

On Mon, 1 Apr 2013 22:29:37 +0200, Jiri Olsa wrote:
> On Mon, Apr 01, 2013 at 08:35:21PM +0900, Namhyung Kim wrote:
>
> SNIP
>
>> +
>>  int sort_dimension__add(const char *tok)
>>  {
>>  	unsigned int i;
>> @@ -964,6 +971,33 @@ 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;
>> +
>> +		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_MEMORY_MODE;
>> +
>> +		list_add_tail(&sd->entry->list, &hist_entry__sort_list);
>> +		sd->taken = 1;
>> +
>> +		return 0;
>
> nice idea, I wonder some of the above code be put into function
> and used also in previous loop, no big deal though

Hmm.. okay.  I'll try. :)

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 6/9] perf sort: Add 'addr' sort key
  2013-04-01 20:40   ` Jiri Olsa
@ 2013-04-02  2:15     ` Namhyung Kim
  2013-04-02  8:40       ` Jiri Olsa
  0 siblings, 1 reply; 21+ messages in thread
From: Namhyung Kim @ 2013-04-02  2:15 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, Namhyung Kim, LKML, Stephane Eranian, Andi Kleen,
	David Ahern

On Mon, 1 Apr 2013 22:40:23 +0200, Jiri Olsa wrote:
> On Mon, Apr 01, 2013 at 08:35:22PM +0900, Namhyung Kim wrote:
>> 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.
>
> maybe it's just the recent mem profiling patches, but wouldn't
> it be better to use 'ip' instead of 'addr'?
>
> also it's following code getting the data:
> ...
>         if (sample_type & PERF_SAMPLE_IP)
>                 data->ip = perf_instruction_pointer(regs);
> ...
>
> same for the "perf sort: Add 'addr_to/from' sort key" patch

I'm not sure I understand what you mean exactly.

I used hist_entry->ip but it was set by al->addr which was converted
from the original sample ip to a relative ip by map->map_ip().

I can change it to use ->unmap_ip() before printing.  Is that your
concern?

And it seems that addr in branch info is original ip, Stephane?

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 6/9] perf sort: Add 'addr' sort key
  2013-04-02  2:15     ` Namhyung Kim
@ 2013-04-02  8:40       ` Jiri Olsa
  2013-04-03  9:10         ` Namhyung Kim
  0 siblings, 1 reply; 21+ messages in thread
From: Jiri Olsa @ 2013-04-02  8:40 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, Namhyung Kim, LKML, Stephane Eranian, Andi Kleen,
	David Ahern

On Tue, Apr 02, 2013 at 11:15:23AM +0900, Namhyung Kim wrote:
> On Mon, 1 Apr 2013 22:40:23 +0200, Jiri Olsa wrote:
> > On Mon, Apr 01, 2013 at 08:35:22PM +0900, Namhyung Kim wrote:
> >> 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.
> >
> > maybe it's just the recent mem profiling patches, but wouldn't
> > it be better to use 'ip' instead of 'addr'?
> >
> > also it's following code getting the data:
> > ...
> >         if (sample_type & PERF_SAMPLE_IP)
> >                 data->ip = perf_instruction_pointer(regs);
> > ...
> >
> > same for the "perf sort: Add 'addr_to/from' sort key" patch
> 
> I'm not sure I understand what you mean exactly.
> 
> I used hist_entry->ip but it was set by al->addr which was converted
> from the original sample ip to a relative ip by map->map_ip().
> 
> I can change it to use ->unmap_ip() before printing.  Is that your
> concern?

what I meant was the 'addr' name itself for -s option, like use:
  '-s ip' instead of '-s addr'

I'm fine with the implementation

jirka

> 
> And it seems that addr in branch info is original ip, Stephane?
> 
> Thanks,
> Namhyung

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys
  2013-04-01 11:35 [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Namhyung Kim
                   ` (8 preceding siblings ...)
  2013-04-01 11:35 ` [PATCH 9/9] perf hists: Move column length setting code Namhyung Kim
@ 2013-04-02 15:05 ` Arnaldo Carvalho de Melo
  9 siblings, 0 replies; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2013-04-02 15:05 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Namhyung Kim, LKML,
	Stephane Eranian, Andi Kleen, Jiri Olsa, David Ahern

Em Mon, Apr 01, 2013 at 08:35:16PM +0900, Namhyung Kim escreveu:
> This is a couple of bugfix, cleanup and enhancement for sort keys.

Applied 1-4.

- Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 6/9] perf sort: Add 'addr' sort key
  2013-04-02  8:40       ` Jiri Olsa
@ 2013-04-03  9:10         ` Namhyung Kim
  0 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2013-04-03  9:10 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Paul Mackerras,
	Ingo Molnar, Namhyung Kim, LKML, Stephane Eranian, Andi Kleen,
	David Ahern

Hi Jiri,

On Tue, 2 Apr 2013 10:40:14 +0200, Jiri Olsa wrote:
> On Tue, Apr 02, 2013 at 11:15:23AM +0900, Namhyung Kim wrote:
>> On Mon, 1 Apr 2013 22:40:23 +0200, Jiri Olsa wrote:
>> > On Mon, Apr 01, 2013 at 08:35:22PM +0900, Namhyung Kim wrote:
>> >> 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.
>> >
>> > maybe it's just the recent mem profiling patches, but wouldn't
>> > it be better to use 'ip' instead of 'addr'?
>> >
>> > also it's following code getting the data:
>> > ...
>> >         if (sample_type & PERF_SAMPLE_IP)
>> >                 data->ip = perf_instruction_pointer(regs);
>> > ...
>> >
>> > same for the "perf sort: Add 'addr_to/from' sort key" patch
>> 
>> I'm not sure I understand what you mean exactly.
>> 
>> I used hist_entry->ip but it was set by al->addr which was converted
>> from the original sample ip to a relative ip by map->map_ip().
>> 
>> I can change it to use ->unmap_ip() before printing.  Is that your
>> concern?
>
> what I meant was the 'addr' name itself for -s option, like use:
>   '-s ip' instead of '-s addr'

Well, I don't know what's better.  But the 'addr' looks more natural to
me and it's requested originally.  And the 'ip' can have multiple
meaning. :)

Thanks,
Namhyung


^ permalink raw reply	[flat|nested] 21+ messages in thread
* [tip:perf/core] perf hists: Fix an invalid memory free on he-> branch_info
  2013-04-01 11:35 ` [PATCH 1/9] perf hists: Fix an invalid memory free on he->branch_info Namhyung Kim
@ 2013-05-31 11:13   ` tip-bot for Namhyung Kim
  0 siblings, 0 replies; 21+ messages in thread
From: tip-bot for Namhyung Kim @ 2013-05-31 11:13 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, linux-kernel, eranian, paulus, hpa, mingo, andi,
	a.p.zijlstra, namhyung.kim, namhyung, jolsa, dsahern, tglx

Commit-ID:  26353a61b977e57b58dd3555bc0422fea46c5ad6
Gitweb:     http://git.kernel.org/tip/26353a61b977e57b58dd3555bc0422fea46c5ad6
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Mon, 1 Apr 2013 20:35:17 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 28 May 2013 16:23:52 +0300

perf hists: Fix an invalid memory free on he->branch_info

The branch info was allocated for the whole stack and passed matching
hist entry for each level during processing samples.  Thus when a hist
entry tries to free its branch info like in hists__collapse_insert_entry
it'll face following error.

  *** glibc detected *** perf: munmap_chunk(): invalid pointer: 0x00000000014e9d20 ***
  ======= Backtrace: =========
  /lib64/libc.so.6[0x387d47ae16]
  perf[0x4923bd]
  perf(cmd_report+0xd68)[0x432a08]
  perf[0x41a663]
  perf(main+0x58f)[0x419eaf]
  /lib64/libc.so.6(__libc_start_main+0xf5)[0x387d421735]
  perf[0x419f95]

Fix it by allocating and copying branch info for each new hist entry.

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/1364816125-12212-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-report.c |  9 ++++++---
 tools/perf/util/hist.c      | 14 ++++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index bd0ca81..d9f2de3 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -187,6 +187,9 @@ static int perf_report__add_branch_hist_entry(struct perf_tool *tool,
 	for (i = 0; i < sample->branch_stack->nr; i++) {
 		if (rep->hide_unresolved && !(bi[i].from.sym && bi[i].to.sym))
 			continue;
+
+		err = -ENOMEM;
+
 		/*
 		 * The report shows the percentage of total branches captured
 		 * and not events sampled. Thus we use a pseudo period of 1.
@@ -195,7 +198,6 @@ static int perf_report__add_branch_hist_entry(struct perf_tool *tool,
 				&bi[i], 1, 1);
 		if (he) {
 			struct annotation *notes;
-			err = -ENOMEM;
 			bx = he->branch_info;
 			if (bx->from.sym && use_browser == 1 && sort__has_sym) {
 				notes = symbol__annotation(bx->from.sym);
@@ -226,11 +228,12 @@ static int perf_report__add_branch_hist_entry(struct perf_tool *tool,
 			}
 			evsel->hists.stats.total_period += 1;
 			hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE);
-			err = 0;
 		} else
-			return -ENOMEM;
+			goto out;
 	}
+	err = 0;
 out:
+	free(bi);
 	return err;
 }
 
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 6b32721..9438d57 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -292,6 +292,20 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template)
 			he->ms.map->referenced = true;
 
 		if (he->branch_info) {
+			/*
+			 * This branch info is (a part of) allocated from
+			 * machine__resolve_bstack() and will be freed after
+			 * adding new entries.  So we need to save a copy.
+			 */
+			he->branch_info = malloc(sizeof(*he->branch_info));
+			if (he->branch_info == NULL) {
+				free(he);
+				return NULL;
+			}
+
+			memcpy(he->branch_info, template->branch_info,
+			       sizeof(*he->branch_info));
+
 			if (he->branch_info->from.map)
 				he->branch_info->from.map->referenced = true;
 			if (he->branch_info->to.map)

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [tip:perf/core] perf hists: Free unused mem info of a matched hist entry
  2013-04-01 11:35 ` [PATCH 2/9] perf hists: Free unused mem info of a matched hist entry Namhyung Kim
@ 2013-05-31 11:15   ` tip-bot for Namhyung Kim
  0 siblings, 0 replies; 21+ messages in thread
From: tip-bot for Namhyung Kim @ 2013-05-31 11:15 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, linux-kernel, eranian, paulus, hpa, mingo, andi,
	a.p.zijlstra, namhyung.kim, namhyung, jolsa, dsahern, tglx

Commit-ID:  ceb2acbc2c1387c8785b3c98b482f5a2b89447c3
Gitweb:     http://git.kernel.org/tip/ceb2acbc2c1387c8785b3c98b482f5a2b89447c3
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Mon, 1 Apr 2013 20:35:18 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 28 May 2013 16:23:52 +0300

perf hists: Free unused mem info of a matched hist entry

The mem info is shared between matched entries so one should be freed.

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/1364816125-12212-3-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 9438d57..514fc04 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -374,6 +374,12 @@ static struct hist_entry *add_hist_entry(struct hists *hists,
 		if (!cmp) {
 			he_stat__add_period(&he->stat, period, weight);
 
+			/*
+			 * This mem info was allocated from machine__resolve_mem
+			 * and will not be used anymore.
+			 */
+			free(entry->mem_info);
+
 			/* If the map of an existing hist_entry has
 			 * become out-of-date due to an exec() or
 			 * similar, update it.  Otherwise we will

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [tip:perf/core] perf report: Fix alignment of symbol column when -v is given
  2013-04-01 11:35 ` [PATCH 3/9] perf report: Fix alignment of symbol column when -v is given Namhyung Kim
@ 2013-05-31 11:16   ` tip-bot for Namhyung Kim
  0 siblings, 0 replies; 21+ messages in thread
From: tip-bot for Namhyung Kim @ 2013-05-31 11:16 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, linux-kernel, eranian, paulus, hpa, mingo, andi,
	a.p.zijlstra, namhyung.kim, namhyung, jolsa, dsahern, tglx

Commit-ID:  ded19d57a621e92a27a05972949ad3230f84d0b0
Gitweb:     http://git.kernel.org/tip/ded19d57a621e92a27a05972949ad3230f84d0b0
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Mon, 1 Apr 2013 20:35:19 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 28 May 2013 16:23:53 +0300

perf report: Fix alignment of symbol column when -v is given

When -v option is given, the symbol sort key prints its address also but
it wasn't properly aligned since hists__calc_col_len() misses the
additional part.  Also it missed 2 spaces for 0x prefix when printing.

  $ perf report --stdio -v -s sym
  # Samples: 133  of event 'cycles'
  # Event count (approx.): 50536717
  #
  # Overhead                          Symbol
  # ........  ..............................
  #
      12.20%  0xffffffff81384c50 v [k] intel_idle
       7.62%  0xffffffff8170976a v [k] ftrace_caller
       7.02%  0x2d986d         B [.] 0x00000000002d986d

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/1364816125-12212-4-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 26 +++++++++++++++-----------
 tools/perf/util/sort.c |  2 +-
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 514fc04..72b4eec 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -70,9 +70,17 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 	int symlen;
 	u16 len;
 
-	if (h->ms.sym)
-		hists__new_col_len(hists, HISTC_SYMBOL, h->ms.sym->namelen + 4);
-	else {
+	/*
+	 * +4 accounts for '[x] ' priv level info
+	 * +2 accounts for 0x prefix on raw addresses
+	 * +3 accounts for ' y ' symtab origin info
+	 */
+	if (h->ms.sym) {
+		symlen = h->ms.sym->namelen + 4;
+		if (verbose)
+			symlen += BITS_PER_LONG / 4 + 2 + 3;
+		hists__new_col_len(hists, HISTC_SYMBOL, symlen);
+	} else {
 		symlen = unresolved_col_width + 4 + 2;
 		hists__new_col_len(hists, HISTC_SYMBOL, symlen);
 		hists__set_unres_dso_col_len(hists, HISTC_DSO);
@@ -91,12 +99,10 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 		hists__new_col_len(hists, HISTC_PARENT, h->parent->namelen);
 
 	if (h->branch_info) {
-		/*
-		 * +4 accounts for '[x] ' priv level info
-		 * +2 account of 0x prefix on raw addresses
-		 */
 		if (h->branch_info->from.sym) {
 			symlen = (int)h->branch_info->from.sym->namelen + 4;
+			if (verbose)
+				symlen += BITS_PER_LONG / 4 + 2 + 3;
 			hists__new_col_len(hists, HISTC_SYMBOL_FROM, symlen);
 
 			symlen = dso__name_len(h->branch_info->from.map->dso);
@@ -109,6 +115,8 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 
 		if (h->branch_info->to.sym) {
 			symlen = (int)h->branch_info->to.sym->namelen + 4;
+			if (verbose)
+				symlen += BITS_PER_LONG / 4 + 2 + 3;
 			hists__new_col_len(hists, HISTC_SYMBOL_TO, symlen);
 
 			symlen = dso__name_len(h->branch_info->to.map->dso);
@@ -121,10 +129,6 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 	}
 
 	if (h->mem_info) {
-		/*
-		 * +4 accounts for '[x] ' priv level info
-		 * +2 account of 0x prefix on raw addresses
-		 */
 		if (h->mem_info->daddr.sym) {
 			symlen = (int)h->mem_info->daddr.sym->namelen + 4
 			       + unresolved_col_width + 2;
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 5f52d49..16d5e38 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -194,7 +194,7 @@ static int _hist_entry__sym_snprintf(struct map *map, struct symbol *sym,
 	if (verbose) {
 		char o = map ? dso__symtab_origin(map->dso) : '!';
 		ret += repsep_snprintf(bf, size, "%-#*llx %c ",
-				       BITS_PER_LONG / 4, ip, o);
+				       BITS_PER_LONG / 4 + 2, ip, o);
 	}
 
 	ret += repsep_snprintf(bf + ret, size - ret, "[%c] ", level);

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [tip:perf/core] perf sort: Introduce sort__mode variable
  2013-04-01 11:35 ` [PATCH 4/9] perf sort: Introduce sort__mode variable Namhyung Kim
@ 2013-05-31 11:17   ` tip-bot for Namhyung Kim
  0 siblings, 0 replies; 21+ messages in thread
From: tip-bot for Namhyung Kim @ 2013-05-31 11:17 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, linux-kernel, eranian, paulus, hpa, mingo, andi,
	a.p.zijlstra, namhyung.kim, namhyung, jolsa, dsahern, tglx

Commit-ID:  55369fc179b0572d0b4a06a9be1d2779b3ac22e0
Gitweb:     http://git.kernel.org/tip/55369fc179b0572d0b4a06a9be1d2779b3ac22e0
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Mon, 1 Apr 2013 20:35:20 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 28 May 2013 16:23:53 +0300

perf sort: Introduce sort__mode variable

It's used for determining current sort mode which can be one of
NORMAL, BRANCH and new MEMORY.

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/1364816125-12212-5-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-report.c    | 23 +++++++++++++----------
 tools/perf/ui/browsers/hists.c |  4 ++--
 tools/perf/util/sort.c         |  4 ++--
 tools/perf/util/sort.h         |  8 +++++++-
 4 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index d9f2de3..c877982 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -311,7 +311,7 @@ static int process_sample_event(struct perf_tool *tool,
 	if (rep->cpu_list && !test_bit(sample->cpu, rep->cpu_bitmap))
 		return 0;
 
-	if (sort__branch_mode == 1) {
+	if (sort__mode == SORT_MODE__BRANCH) {
 		if (perf_report__add_branch_hist_entry(tool, &al, sample,
 						       evsel, machine)) {
 			pr_debug("problem adding lbr entry, skipping event\n");
@@ -387,7 +387,7 @@ static int perf_report__setup_sample_type(struct perf_report *rep)
 			}
 	}
 
-	if (sort__branch_mode == 1) {
+	if (sort__mode == SORT_MODE__BRANCH) {
 		if (!self->fd_pipe &&
 		    !(sample_type & PERF_SAMPLE_BRANCH_STACK)) {
 			ui__error("Selected -b but no branch data. "
@@ -694,7 +694,9 @@ static int
 parse_branch_mode(const struct option *opt __maybe_unused,
 		  const char *str __maybe_unused, int unset)
 {
-	sort__branch_mode = !unset;
+	int *branch_mode = opt->value;
+
+	*branch_mode = !unset;
 	return 0;
 }
 
@@ -703,6 +705,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	struct perf_session *session;
 	struct stat st;
 	bool has_br_stack = false;
+	int branch_mode = -1;
 	int ret = -1;
 	char callchain_default_opt[] = "fractal,0.5,callee";
 	const char * const report_usage[] = {
@@ -799,7 +802,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		    "Show a column with the sum of periods"),
 	OPT_BOOLEAN(0, "group", &symbol_conf.event_group,
 		    "Show event group information together"),
-	OPT_CALLBACK_NOOPT('b', "branch-stack", &sort__branch_mode, "",
+	OPT_CALLBACK_NOOPT('b', "branch-stack", &branch_mode, "",
 		    "use branch records for histogram filling", parse_branch_mode),
 	OPT_STRING(0, "objdump", &objdump_path, "path",
 		   "objdump binary to use for disassembly and annotations"),
@@ -849,11 +852,11 @@ repeat:
 	has_br_stack = perf_header__has_feat(&session->header,
 					     HEADER_BRANCH_STACK);
 
-	if (sort__branch_mode == -1 && has_br_stack)
-		sort__branch_mode = 1;
+	if (branch_mode == -1 && has_br_stack)
+		sort__mode = SORT_MODE__BRANCH;
 
-	/* sort__branch_mode could be 0 if --no-branch-stack */
-	if (sort__branch_mode == 1) {
+	/* 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
@@ -864,7 +867,7 @@ repeat:
 
 	}
 	if (report.mem_mode) {
-		if (sort__branch_mode == 1) {
+		if (sort__mode == SORT_MODE__BRANCH) {
 			fprintf(stderr, "branch and mem mode incompatible\n");
 			goto error;
 		}
@@ -934,7 +937,7 @@ repeat:
 
 	sort_entry__setup_elide(&sort_comm, symbol_conf.comm_list, "comm", stdout);
 
-	if (sort__branch_mode == 1) {
+	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);
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index d88a2d0..cad8e37 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1155,7 +1155,7 @@ 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__branch_mode == 1)
+		if (sort__mode == SORT_MODE__BRANCH)
 			browser->has_symbols = sort_sym_from.list.next != NULL;
 		else
 			browser->has_symbols = sort_sym.list.next != NULL;
@@ -1488,7 +1488,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		if (!browser->has_symbols)
 			goto add_exit_option;
 
-		if (sort__branch_mode == 1) {
+		if (sort__mode == SORT_MODE__BRANCH) {
 			bi = browser->he_selection->branch_info;
 			if (browser->selection != NULL &&
 			    bi &&
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 16d5e38..a6ddad4 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -9,7 +9,7 @@ const char	*sort_order = default_sort_order;
 int		sort__need_collapse = 0;
 int		sort__has_parent = 0;
 int		sort__has_sym = 0;
-int		sort__branch_mode = -1; /* -1 = means not set */
+enum sort_mode	sort__mode = SORT_MODE__NORMAL;
 
 enum sort_type	sort__first_dimension;
 
@@ -943,7 +943,7 @@ int sort_dimension__add(const char *tok)
 		if (strncasecmp(tok, sd->name, strlen(tok)))
 			continue;
 
-		if (sort__branch_mode != 1)
+		if (sort__mode != SORT_MODE__BRANCH)
 			return -EINVAL;
 
 		if (sd->entry == &sort_sym_from || sd->entry == &sort_sym_to)
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index f24bdf6..39ff4b8 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -32,7 +32,7 @@ extern const char default_sort_order[];
 extern int sort__need_collapse;
 extern int sort__has_parent;
 extern int sort__has_sym;
-extern int sort__branch_mode;
+extern enum sort_mode sort__mode;
 extern struct sort_entry sort_comm;
 extern struct sort_entry sort_dso;
 extern struct sort_entry sort_sym;
@@ -123,6 +123,12 @@ static inline void hist_entry__add_pair(struct hist_entry *he,
 	list_add_tail(&he->pairs.head, &pair->pairs.node);
 }
 
+enum sort_mode {
+	SORT_MODE__NORMAL,
+	SORT_MODE__BRANCH,
+	SORT_MODE__MEMORY,
+};
+
 enum sort_type {
 	/* common sort keys */
 	SORT_PID,

^ permalink raw reply related	[flat|nested] 21+ messages in thread
end of thread, other threads:[~2013-05-31 11:18 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-01 11:35 [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Namhyung Kim
2013-04-01 11:35 ` [PATCH 1/9] perf hists: Fix an invalid memory free on he->branch_info Namhyung Kim
2013-05-31 11:13   ` [tip:perf/core] perf hists: Fix an invalid memory free on he-> branch_info tip-bot for Namhyung Kim
2013-04-01 11:35 ` [PATCH 2/9] perf hists: Free unused mem info of a matched hist entry Namhyung Kim
2013-05-31 11:15   ` [tip:perf/core] " tip-bot for Namhyung Kim
2013-04-01 11:35 ` [PATCH 3/9] perf report: Fix alignment of symbol column when -v is given Namhyung Kim
2013-05-31 11:16   ` [tip:perf/core] " tip-bot for Namhyung Kim
2013-04-01 11:35 ` [PATCH 4/9] perf sort: Introduce sort__mode variable Namhyung Kim
2013-05-31 11:17   ` [tip:perf/core] " tip-bot for Namhyung Kim
2013-04-01 11:35 ` [PATCH 5/9] perf sort: Separate out memory-specific sort keys Namhyung Kim
2013-04-01 20:29   ` Jiri Olsa
2013-04-02  1:56     ` Namhyung Kim
2013-04-01 11:35 ` [PATCH 6/9] perf sort: Add 'addr' sort key Namhyung Kim
2013-04-01 20:40   ` Jiri Olsa
2013-04-02  2:15     ` Namhyung Kim
2013-04-02  8:40       ` Jiri Olsa
2013-04-03  9:10         ` Namhyung Kim
2013-04-01 11:35 ` [PATCH 7/9] perf sort: Add 'addr_to/from' " Namhyung Kim
2013-04-01 11:35 ` [PATCH 8/9] perf sort: Update documentation for sort keys Namhyung Kim
2013-04-01 11:35 ` [PATCH 9/9] perf hists: Move column length setting code Namhyung Kim
2013-04-02 15:05 ` [PATCHSET 0/9] perf tools: Bug fix and cleanup for sort keys Arnaldo Carvalho de Melo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.