linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry
@ 2016-02-21 14:22 Namhyung Kim
  2016-02-21 14:22 ` [PATCH 2/5] perf tools: Fix segfault on dynamic entries Namhyung Kim
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Namhyung Kim @ 2016-02-21 14:22 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, David Ahern,
	Andi Kleen

The dynamic entry is created for each field in a tracepoint event.
Since they have no fixed hpp format index, it should skip when
perf_hpp__reset_width() is called.

This caused following assertion failure..

  $ perf record -e sched:sched_switch -a sleep 1

  $ perf report -s comm,next_pid --stdio
  perf: ui/hist.c:651: perf_hpp__reset_width:
    Assertion `!(fmt->idx >= PERF_HPP__MAX_INDEX)' failed.

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

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 1ba4117d9c2d..12223d791e9f 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -645,6 +645,9 @@ void perf_hpp__reset_width(struct perf_hpp_fmt *fmt, struct hists *hists)
 	if (perf_hpp__is_sort_entry(fmt))
 		return perf_hpp__reset_sort_width(fmt, hists);
 
+	if (perf_hpp__is_dynamic_entry(fmt))
+		return;
+
 	BUG_ON(fmt->idx >= PERF_HPP__MAX_INDEX);
 
 	switch (fmt->idx) {
-- 
2.7.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/5] perf tools: Fix segfault on dynamic entries
  2016-02-21 14:22 [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry Namhyung Kim
@ 2016-02-21 14:22 ` Namhyung Kim
  2016-02-21 17:37   ` Jiri Olsa
  2016-02-25  5:37   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2016-02-21 14:22 ` [PATCH 3/5] perf tools: Update srcline/file if needed Namhyung Kim
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 19+ messages in thread
From: Namhyung Kim @ 2016-02-21 14:22 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, David Ahern,
	Andi Kleen

The dynamic entry is created for each tracepoint event.  When it sets up
the sort key, it checks with existing keys using ->equal() callback.
But it missed to set the ->equal for dynamic entries.  The following
segfault was due to the missing ->equal() callback.

  (gdb) bt
  #0  0x0000000000140003 in ?? ()
  #1  0x0000000000537769 in fmt_equal (b=0x2106980, a=0x21067a0) at ui/hist.c:548
  #2  perf_hpp__setup_output_field (list=0x8c6d80 <perf_hpp_list>) at ui/hist.c:560
  #3  0x00000000004e927e in setup_sorting (evlist=<optimized out>) at util/sort.c:2642
  #4  0x000000000043cf50 in cmd_report (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>)
      at builtin-report.c:932
  #5  0x00000000004865a1 in run_builtin (p=p@entry=0x8bbce0 <commands+192>, argc=argc@entry=7,
      argv=argv@entry=0x7ffd24d56ce0) at perf.c:390
  #6  0x000000000042dc1f in handle_internal_command (argv=0x7ffd24d56ce0, argc=7) at perf.c:451
  #7  run_argv (argv=0x7ffd24d56a70, argcp=0x7ffd24d56a7c) at perf.c:495
  #8  main (argc=7, argv=0x7ffd24d56ce0) at perf.c:620

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

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index de715756f281..7daea71691df 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1835,6 +1835,20 @@ bool perf_hpp__is_dynamic_entry(struct perf_hpp_fmt *fmt)
 	return fmt->cmp == __sort__hde_cmp;
 }
 
+static bool __sort__hde_equal(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b)
+{
+	struct hpp_dynamic_entry *hde_a;
+	struct hpp_dynamic_entry *hde_b;
+
+	if (!perf_hpp__is_dynamic_entry(a) || !perf_hpp__is_dynamic_entry(b))
+		return false;
+
+	hde_a = container_of(a, struct hpp_dynamic_entry, hpp);
+	hde_b = container_of(b, struct hpp_dynamic_entry, hpp);
+
+	return hde_a->field == hde_b->field;
+}
+
 static void hde_free(struct perf_hpp_fmt *fmt)
 {
 	struct hpp_dynamic_entry *hde;
@@ -1867,6 +1881,7 @@ __alloc_dynamic_entry(struct perf_evsel *evsel, struct format_field *field)
 	hde->hpp.cmp = __sort__hde_cmp;
 	hde->hpp.collapse = __sort__hde_cmp;
 	hde->hpp.sort = __sort__hde_cmp;
+	hde->hpp.equal = __sort__hde_equal;
 	hde->hpp.free = hde_free;
 
 	INIT_LIST_HEAD(&hde->hpp.list);
-- 
2.7.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/5] perf tools: Update srcline/file if needed
  2016-02-21 14:22 [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry Namhyung Kim
  2016-02-21 14:22 ` [PATCH 2/5] perf tools: Fix segfault on dynamic entries Namhyung Kim
@ 2016-02-21 14:22 ` Namhyung Kim
  2016-02-21 17:39   ` Jiri Olsa
  2016-02-21 14:22 ` [PATCH 4/5] perf tools: Fix alignment on some sort keys Namhyung Kim
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2016-02-21 14:22 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, David Ahern,
	Andi Kleen

Normally the hist entry's srcline and/or srcfile is set during sorting.
However sometime it's possible to a hist entry's srcline is not set yet
after the sorting.  This is because the entry is so unique and other
sort keys already make it distinct.  Then the srcline/file sort didn't
have a chance to be called during the sorting.  In that case it has NULL
srcline/srcfile field and shows nothing.

Before:

  $ perf report -s comm,sym,srcline
  ...
  Overhead  Command       Symbol
  -----------------------------------------------------------------
    34.42%  swapper       [k] intel_idle          intel_idle.c:0
     2.44%  perf          [.] __poll_nocancel     (null)
     1.70%  gnome-shell   [k] fw_domains_get      (null)
     1.04%  Xorg          [k] sock_poll           (null)

After:

    34.42%  swapper       [k] intel_idle          intel_idle.c:0
     2.44%  perf          [.] __poll_nocancel     .:0
     1.70%  gnome-shell   [k] fw_domains_get      fw_domains_get+42
     1.04%  Xorg          [k] sock_poll           socket.c:0

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

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 7daea71691df..6808d73164b5 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -315,6 +315,16 @@ sort__srcline_cmp(struct hist_entry *left, struct hist_entry *right)
 static int hist_entry__srcline_snprintf(struct hist_entry *he, char *bf,
 					size_t size, unsigned int width)
 {
+	if (!he->srcline) {
+		if (!he->ms.map)
+			he->srcline = SRCLINE_UNKNOWN;
+		else {
+			struct map *map = he->ms.map;
+			he->srcline = get_srcline(map->dso,
+					   map__rip_2objdump(map, he->ip),
+						  he->ms.sym, true);
+		}
+	}
 	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcline);
 }
 
@@ -368,6 +378,12 @@ sort__srcfile_cmp(struct hist_entry *left, struct hist_entry *right)
 static int hist_entry__srcfile_snprintf(struct hist_entry *he, char *bf,
 					size_t size, unsigned int width)
 {
+	if (!he->srcfile) {
+		if (!he->ms.map)
+			he->srcfile = no_srcfile;
+		else
+			he->srcfile = get_srcfile(he);
+	}
 	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcfile);
 }
 
-- 
2.7.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/5] perf tools: Fix alignment on some sort keys
  2016-02-21 14:22 [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry Namhyung Kim
  2016-02-21 14:22 ` [PATCH 2/5] perf tools: Fix segfault on dynamic entries Namhyung Kim
  2016-02-21 14:22 ` [PATCH 3/5] perf tools: Update srcline/file if needed Namhyung Kim
@ 2016-02-21 14:22 ` Namhyung Kim
  2016-02-21 17:40   ` Jiri Olsa
  2016-02-21 14:22 ` [PATCH 5/5] perf tools: Fix column width setting on 'trace' sort key Namhyung Kim
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2016-02-21 14:22 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, David Ahern,
	Andi Kleen

The srcline, srcfile and trace sort keys can have long entries.  With
commit 89fee7094323 ("perf hists: Do column alignment on the format
iterator"), it now aligns output with hist_entry__snprintf_alignment().
So each (possibly long) sort entries don't need to do it themselves.

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

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 6808d73164b5..1d2b85c808d0 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -325,7 +325,7 @@ static int hist_entry__srcline_snprintf(struct hist_entry *he, char *bf,
 						  he->ms.sym, true);
 		}
 	}
-	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcline);
+	return repsep_snprintf(bf, size, "%-.*s", width, he->srcline);
 }
 
 struct sort_entry sort_srcline = {
@@ -384,7 +384,7 @@ static int hist_entry__srcfile_snprintf(struct hist_entry *he, char *bf,
 		else
 			he->srcfile = get_srcfile(he);
 	}
-	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcfile);
+	return repsep_snprintf(bf, size, "%-.*s", width, he->srcfile);
 }
 
 struct sort_entry sort_srcfile = {
@@ -514,11 +514,11 @@ static int hist_entry__trace_snprintf(struct hist_entry *he, char *bf,
 
 	evsel = hists_to_evsel(he->hists);
 	if (evsel->attr.type != PERF_TYPE_TRACEPOINT)
-		return scnprintf(bf, size, "%-*.*s", width, width, "N/A");
+		return scnprintf(bf, size, "%-.*s", width, "N/A");
 
 	if (he->trace_output == NULL)
 		he->trace_output = get_trace_output(he);
-	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->trace_output);
+	return repsep_snprintf(bf, size, "%-.*s", width, he->trace_output);
 }
 
 struct sort_entry sort_trace = {
-- 
2.7.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 5/5] perf tools: Fix column width setting on 'trace' sort key
  2016-02-21 14:22 [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry Namhyung Kim
                   ` (2 preceding siblings ...)
  2016-02-21 14:22 ` [PATCH 4/5] perf tools: Fix alignment on some sort keys Namhyung Kim
@ 2016-02-21 14:22 ` Namhyung Kim
  2016-02-21 17:41   ` Jiri Olsa
  2016-02-25  5:38   ` [tip:perf/core] " tip-bot for Namhyung Kim
  2016-02-21 17:37 ` [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry Jiri Olsa
  2016-02-25  5:38 ` [tip:perf/core] " tip-bot for Namhyung Kim
  5 siblings, 2 replies; 19+ messages in thread
From: Namhyung Kim @ 2016-02-21 14:22 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, David Ahern,
	Andi Kleen

It missed to update column length of the 'trace' sort key in the
hists__calc_col_len() so it might truncate the output.  It calculated
the column length in the ->cmp() callback originally but it doesn't
guarantee it's called always.

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

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 827c6cbcd05d..017eb5c42c37 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -179,6 +179,9 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 	if (h->transaction)
 		hists__new_col_len(hists, HISTC_TRANSACTION,
 				   hist_entry__transaction_len());
+
+	if (h->trace_output)
+		hists__new_col_len(hists, HISTC_TRACE, strlen(h->trace_output));
 }
 
 void hists__output_recalc_col_len(struct hists *hists, int max_rows)
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 1d2b85c808d0..ea05497cfee9 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -501,9 +501,6 @@ sort__trace_cmp(struct hist_entry *left, struct hist_entry *right)
 	if (right->trace_output == NULL)
 		right->trace_output = get_trace_output(right);
 
-	hists__new_col_len(left->hists, HISTC_TRACE, strlen(left->trace_output));
-	hists__new_col_len(right->hists, HISTC_TRACE, strlen(right->trace_output));
-
 	return strcmp(right->trace_output, left->trace_output);
 }
 
-- 
2.7.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry
  2016-02-21 14:22 [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry Namhyung Kim
                   ` (3 preceding siblings ...)
  2016-02-21 14:22 ` [PATCH 5/5] perf tools: Fix column width setting on 'trace' sort key Namhyung Kim
@ 2016-02-21 17:37 ` Jiri Olsa
  2016-02-22 15:08   ` Arnaldo Carvalho de Melo
  2016-02-25  5:38 ` [tip:perf/core] " tip-bot for Namhyung Kim
  5 siblings, 1 reply; 19+ messages in thread
From: Jiri Olsa @ 2016-02-21 17:37 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, Jiri Olsa,
	LKML, David Ahern, Andi Kleen

On Sun, Feb 21, 2016 at 11:22:34PM +0900, Namhyung Kim wrote:
> The dynamic entry is created for each field in a tracepoint event.
> Since they have no fixed hpp format index, it should skip when
> perf_hpp__reset_width() is called.
> 
> This caused following assertion failure..
> 
>   $ perf record -e sched:sched_switch -a sleep 1
> 
>   $ perf report -s comm,next_pid --stdio
>   perf: ui/hist.c:651: perf_hpp__reset_width:
>     Assertion `!(fmt->idx >= PERF_HPP__MAX_INDEX)' failed.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/ui/hist.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 1ba4117d9c2d..12223d791e9f 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -645,6 +645,9 @@ void perf_hpp__reset_width(struct perf_hpp_fmt *fmt, struct hists *hists)
>  	if (perf_hpp__is_sort_entry(fmt))
>  		return perf_hpp__reset_sort_width(fmt, hists);
>  
> +	if (perf_hpp__is_dynamic_entry(fmt))
> +		return;
> +
>  	BUG_ON(fmt->idx >= PERF_HPP__MAX_INDEX);
>  
>  	switch (fmt->idx) {
> -- 
> 2.7.1
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] perf tools: Fix segfault on dynamic entries
  2016-02-21 14:22 ` [PATCH 2/5] perf tools: Fix segfault on dynamic entries Namhyung Kim
@ 2016-02-21 17:37   ` Jiri Olsa
  2016-02-25  5:37   ` [tip:perf/core] " tip-bot for Namhyung Kim
  1 sibling, 0 replies; 19+ messages in thread
From: Jiri Olsa @ 2016-02-21 17:37 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, Jiri Olsa,
	LKML, David Ahern, Andi Kleen

On Sun, Feb 21, 2016 at 11:22:35PM +0900, Namhyung Kim wrote:
> The dynamic entry is created for each tracepoint event.  When it sets up
> the sort key, it checks with existing keys using ->equal() callback.
> But it missed to set the ->equal for dynamic entries.  The following
> segfault was due to the missing ->equal() callback.
> 
>   (gdb) bt
>   #0  0x0000000000140003 in ?? ()
>   #1  0x0000000000537769 in fmt_equal (b=0x2106980, a=0x21067a0) at ui/hist.c:548
>   #2  perf_hpp__setup_output_field (list=0x8c6d80 <perf_hpp_list>) at ui/hist.c:560
>   #3  0x00000000004e927e in setup_sorting (evlist=<optimized out>) at util/sort.c:2642
>   #4  0x000000000043cf50 in cmd_report (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>)
>       at builtin-report.c:932
>   #5  0x00000000004865a1 in run_builtin (p=p@entry=0x8bbce0 <commands+192>, argc=argc@entry=7,
>       argv=argv@entry=0x7ffd24d56ce0) at perf.c:390
>   #6  0x000000000042dc1f in handle_internal_command (argv=0x7ffd24d56ce0, argc=7) at perf.c:451
>   #7  run_argv (argv=0x7ffd24d56a70, argcp=0x7ffd24d56a7c) at perf.c:495
>   #8  main (argc=7, argv=0x7ffd24d56ce0) at perf.c:620
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/util/sort.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index de715756f281..7daea71691df 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1835,6 +1835,20 @@ bool perf_hpp__is_dynamic_entry(struct perf_hpp_fmt *fmt)
>  	return fmt->cmp == __sort__hde_cmp;
>  }
>  
> +static bool __sort__hde_equal(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b)
> +{
> +	struct hpp_dynamic_entry *hde_a;
> +	struct hpp_dynamic_entry *hde_b;
> +
> +	if (!perf_hpp__is_dynamic_entry(a) || !perf_hpp__is_dynamic_entry(b))
> +		return false;
> +
> +	hde_a = container_of(a, struct hpp_dynamic_entry, hpp);
> +	hde_b = container_of(b, struct hpp_dynamic_entry, hpp);
> +
> +	return hde_a->field == hde_b->field;
> +}
> +
>  static void hde_free(struct perf_hpp_fmt *fmt)
>  {
>  	struct hpp_dynamic_entry *hde;
> @@ -1867,6 +1881,7 @@ __alloc_dynamic_entry(struct perf_evsel *evsel, struct format_field *field)
>  	hde->hpp.cmp = __sort__hde_cmp;
>  	hde->hpp.collapse = __sort__hde_cmp;
>  	hde->hpp.sort = __sort__hde_cmp;
> +	hde->hpp.equal = __sort__hde_equal;
>  	hde->hpp.free = hde_free;
>  
>  	INIT_LIST_HEAD(&hde->hpp.list);
> -- 
> 2.7.1
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/5] perf tools: Update srcline/file if needed
  2016-02-21 14:22 ` [PATCH 3/5] perf tools: Update srcline/file if needed Namhyung Kim
@ 2016-02-21 17:39   ` Jiri Olsa
  2016-02-22  0:31     ` [PATCH v2 " Namhyung Kim
  0 siblings, 1 reply; 19+ messages in thread
From: Jiri Olsa @ 2016-02-21 17:39 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, Jiri Olsa,
	LKML, David Ahern, Andi Kleen

On Sun, Feb 21, 2016 at 11:22:36PM +0900, Namhyung Kim wrote:
> Normally the hist entry's srcline and/or srcfile is set during sorting.
> However sometime it's possible to a hist entry's srcline is not set yet
> after the sorting.  This is because the entry is so unique and other
> sort keys already make it distinct.  Then the srcline/file sort didn't
> have a chance to be called during the sorting.  In that case it has NULL
> srcline/srcfile field and shows nothing.
> 
> Before:
> 
>   $ perf report -s comm,sym,srcline
>   ...
>   Overhead  Command       Symbol
>   -----------------------------------------------------------------
>     34.42%  swapper       [k] intel_idle          intel_idle.c:0
>      2.44%  perf          [.] __poll_nocancel     (null)
>      1.70%  gnome-shell   [k] fw_domains_get      (null)
>      1.04%  Xorg          [k] sock_poll           (null)
> 
> After:
> 
>     34.42%  swapper       [k] intel_idle          intel_idle.c:0
>      2.44%  perf          [.] __poll_nocancel     .:0
>      1.70%  gnome-shell   [k] fw_domains_get      fw_domains_get+42
>      1.04%  Xorg          [k] sock_poll           socket.c:0
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/sort.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 7daea71691df..6808d73164b5 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -315,6 +315,16 @@ sort__srcline_cmp(struct hist_entry *left, struct hist_entry *right)
>  static int hist_entry__srcline_snprintf(struct hist_entry *he, char *bf,
>  					size_t size, unsigned int width)
>  {
> +	if (!he->srcline) {
> +		if (!he->ms.map)
> +			he->srcline = SRCLINE_UNKNOWN;
> +		else {
> +			struct map *map = he->ms.map;
> +			he->srcline = get_srcline(map->dso,
> +					   map__rip_2objdump(map, he->ip),
> +						  he->ms.sym, true);
> +		}
> +	}

could you put this into the function and use it
also within sort__srcline_cmp?

it's already 3 places duplicating the same code

thanks,
jirka

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/5] perf tools: Fix alignment on some sort keys
  2016-02-21 14:22 ` [PATCH 4/5] perf tools: Fix alignment on some sort keys Namhyung Kim
@ 2016-02-21 17:40   ` Jiri Olsa
  2016-02-22  0:32     ` [PATCH v2 " Namhyung Kim
  0 siblings, 1 reply; 19+ messages in thread
From: Jiri Olsa @ 2016-02-21 17:40 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, Jiri Olsa,
	LKML, David Ahern, Andi Kleen

On Sun, Feb 21, 2016 at 11:22:37PM +0900, Namhyung Kim wrote:
> The srcline, srcfile and trace sort keys can have long entries.  With
> commit 89fee7094323 ("perf hists: Do column alignment on the format
> iterator"), it now aligns output with hist_entry__snprintf_alignment().
> So each (possibly long) sort entries don't need to do it themselves.

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/sort.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 6808d73164b5..1d2b85c808d0 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -325,7 +325,7 @@ static int hist_entry__srcline_snprintf(struct hist_entry *he, char *bf,
>  						  he->ms.sym, true);
>  		}
>  	}
> -	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcline);
> +	return repsep_snprintf(bf, size, "%-.*s", width, he->srcline);
>  }
>  
>  struct sort_entry sort_srcline = {
> @@ -384,7 +384,7 @@ static int hist_entry__srcfile_snprintf(struct hist_entry *he, char *bf,
>  		else
>  			he->srcfile = get_srcfile(he);
>  	}
> -	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcfile);
> +	return repsep_snprintf(bf, size, "%-.*s", width, he->srcfile);
>  }
>  
>  struct sort_entry sort_srcfile = {
> @@ -514,11 +514,11 @@ static int hist_entry__trace_snprintf(struct hist_entry *he, char *bf,
>  
>  	evsel = hists_to_evsel(he->hists);
>  	if (evsel->attr.type != PERF_TYPE_TRACEPOINT)
> -		return scnprintf(bf, size, "%-*.*s", width, width, "N/A");
> +		return scnprintf(bf, size, "%-.*s", width, "N/A");
>  
>  	if (he->trace_output == NULL)
>  		he->trace_output = get_trace_output(he);
> -	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->trace_output);
> +	return repsep_snprintf(bf, size, "%-.*s", width, he->trace_output);
>  }
>  
>  struct sort_entry sort_trace = {
> -- 
> 2.7.1
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 5/5] perf tools: Fix column width setting on 'trace' sort key
  2016-02-21 14:22 ` [PATCH 5/5] perf tools: Fix column width setting on 'trace' sort key Namhyung Kim
@ 2016-02-21 17:41   ` Jiri Olsa
  2016-02-25  5:38   ` [tip:perf/core] " tip-bot for Namhyung Kim
  1 sibling, 0 replies; 19+ messages in thread
From: Jiri Olsa @ 2016-02-21 17:41 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, Jiri Olsa,
	LKML, David Ahern, Andi Kleen

On Sun, Feb 21, 2016 at 11:22:38PM +0900, Namhyung Kim wrote:
> It missed to update column length of the 'trace' sort key in the
> hists__calc_col_len() so it might truncate the output.  It calculated
> the column length in the ->cmp() callback originally but it doesn't
> guarantee it's called always.

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/hist.c | 3 +++
>  tools/perf/util/sort.c | 3 ---
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 827c6cbcd05d..017eb5c42c37 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -179,6 +179,9 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
>  	if (h->transaction)
>  		hists__new_col_len(hists, HISTC_TRANSACTION,
>  				   hist_entry__transaction_len());
> +
> +	if (h->trace_output)
> +		hists__new_col_len(hists, HISTC_TRACE, strlen(h->trace_output));
>  }
>  
>  void hists__output_recalc_col_len(struct hists *hists, int max_rows)
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 1d2b85c808d0..ea05497cfee9 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -501,9 +501,6 @@ sort__trace_cmp(struct hist_entry *left, struct hist_entry *right)
>  	if (right->trace_output == NULL)
>  		right->trace_output = get_trace_output(right);
>  
> -	hists__new_col_len(left->hists, HISTC_TRACE, strlen(left->trace_output));
> -	hists__new_col_len(right->hists, HISTC_TRACE, strlen(right->trace_output));
> -
>  	return strcmp(right->trace_output, left->trace_output);
>  }
>  
> -- 
> 2.7.1
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 3/5] perf tools: Update srcline/file if needed
  2016-02-21 17:39   ` Jiri Olsa
@ 2016-02-22  0:31     ` Namhyung Kim
  2016-02-22  6:49       ` Jiri Olsa
  2016-02-25  5:37       ` [tip:perf/core] " tip-bot for Namhyung Kim
  0 siblings, 2 replies; 19+ messages in thread
From: Namhyung Kim @ 2016-02-22  0:31 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, David Ahern,
	Andi Kleen

Normally the hist entry's srcline and/or srcfile is set during sorting.
However sometime it's possible to a hist entry's srcline is not set yet
after the sorting.  This is because the entry is so unique and other
sort keys already make it distinct.  Then the srcline/file sort didn't
have a chance to be called during the sorting.  In that case it has NULL
srcline/srcfile field and shows nothing.

Before:

  $ perf report -s comm,sym,srcline
  ...
  Overhead  Command       Symbol
  -----------------------------------------------------------------
    34.42%  swapper       [k] intel_idle          intel_idle.c:0
     2.44%  perf          [.] __poll_nocancel     (null)
     1.70%  gnome-shell   [k] fw_domains_get      (null)
     1.04%  Xorg          [k] sock_poll           (null)

After:

    34.42%  swapper       [k] intel_idle          intel_idle.c:0
     2.44%  perf          [.] __poll_nocancel     .:0
     1.70%  gnome-shell   [k] fw_domains_get      fw_domains_get+42
     1.04%  Xorg          [k] sock_poll           socket.c:0

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
factor out common code.

 tools/perf/util/sort.c | 64 ++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 33 deletions(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 7daea71691df..6f4605b5beb5 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -286,35 +286,34 @@ struct sort_entry sort_sym = {
 
 /* --sort srcline */
 
+static char *hist_entry__get_srcline(struct hist_entry *he)
+{
+	struct map *map = he->ms.map;
+
+	if (!map)
+		return SRCLINE_UNKNOWN;
+
+	return get_srcline(map->dso, map__rip_2objdump(map, he->ip),
+			   he->ms.sym, true);
+}
+
 static int64_t
 sort__srcline_cmp(struct hist_entry *left, struct hist_entry *right)
 {
-	if (!left->srcline) {
-		if (!left->ms.map)
-			left->srcline = SRCLINE_UNKNOWN;
-		else {
-			struct map *map = left->ms.map;
-			left->srcline = get_srcline(map->dso,
-					   map__rip_2objdump(map, left->ip),
-						    left->ms.sym, true);
-		}
-	}
-	if (!right->srcline) {
-		if (!right->ms.map)
-			right->srcline = SRCLINE_UNKNOWN;
-		else {
-			struct map *map = right->ms.map;
-			right->srcline = get_srcline(map->dso,
-					     map__rip_2objdump(map, right->ip),
-						     right->ms.sym, true);
-		}
-	}
+	if (!left->srcline)
+		left->srcline = hist_entry__get_srcline(left);
+	if (!right->srcline)
+		right->srcline = hist_entry__get_srcline(right);
+
 	return strcmp(right->srcline, left->srcline);
 }
 
 static int hist_entry__srcline_snprintf(struct hist_entry *he, char *bf,
 					size_t size, unsigned int width)
 {
+	if (!he->srcline)
+		he->srcline = hist_entry__get_srcline(he);
+
 	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcline);
 }
 
@@ -329,11 +328,14 @@ struct sort_entry sort_srcline = {
 
 static char no_srcfile[1];
 
-static char *get_srcfile(struct hist_entry *e)
+static char *hist_entry__get_srcfile(struct hist_entry *e)
 {
 	char *sf, *p;
 	struct map *map = e->ms.map;
 
+	if (!map)
+		return no_srcfile;
+
 	sf = __get_srcline(map->dso, map__rip_2objdump(map, e->ip),
 			 e->ms.sym, false, true);
 	if (!strcmp(sf, SRCLINE_UNKNOWN))
@@ -350,24 +352,20 @@ static char *get_srcfile(struct hist_entry *e)
 static int64_t
 sort__srcfile_cmp(struct hist_entry *left, struct hist_entry *right)
 {
-	if (!left->srcfile) {
-		if (!left->ms.map)
-			left->srcfile = no_srcfile;
-		else
-			left->srcfile = get_srcfile(left);
-	}
-	if (!right->srcfile) {
-		if (!right->ms.map)
-			right->srcfile = no_srcfile;
-		else
-			right->srcfile = get_srcfile(right);
-	}
+	if (!left->srcfile)
+		left->srcfile = hist_entry__get_srcfile(left);
+	if (!right->srcfile)
+		right->srcfile = hist_entry__get_srcfile(right);
+
 	return strcmp(right->srcfile, left->srcfile);
 }
 
 static int hist_entry__srcfile_snprintf(struct hist_entry *he, char *bf,
 					size_t size, unsigned int width)
 {
+	if (!he->srcfile)
+		he->srcfile = hist_entry__get_srcfile(he);
+
 	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcfile);
 }
 
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 4/5] perf tools: Fix alignment on some sort keys
  2016-02-21 17:40   ` Jiri Olsa
@ 2016-02-22  0:32     ` Namhyung Kim
  2016-02-25  5:38       ` [tip:perf/core] " tip-bot for Namhyung Kim
  0 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2016-02-22  0:32 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML, David Ahern,
	Andi Kleen

The srcline, srcfile and trace sort keys can have long entries.  With
commit 89fee7094323 ("perf hists: Do column alignment on the format
iterator"), it now aligns output with hist_entry__snprintf_alignment().
So each (possibly long) sort entries don't need to do it themselves.

Acked-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
rebased
 tools/perf/util/sort.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 6f4605b5beb5..a7d73e503b1b 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -314,7 +314,7 @@ static int hist_entry__srcline_snprintf(struct hist_entry *he, char *bf,
 	if (!he->srcline)
 		he->srcline = hist_entry__get_srcline(he);
 
-	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcline);
+	return repsep_snprintf(bf, size, "%-.*s", width, he->srcline);
 }
 
 struct sort_entry sort_srcline = {
@@ -366,7 +366,7 @@ static int hist_entry__srcfile_snprintf(struct hist_entry *he, char *bf,
 	if (!he->srcfile)
 		he->srcfile = hist_entry__get_srcfile(he);
 
-	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcfile);
+	return repsep_snprintf(bf, size, "%-.*s", width, he->srcfile);
 }
 
 struct sort_entry sort_srcfile = {
@@ -496,11 +496,11 @@ static int hist_entry__trace_snprintf(struct hist_entry *he, char *bf,
 
 	evsel = hists_to_evsel(he->hists);
 	if (evsel->attr.type != PERF_TYPE_TRACEPOINT)
-		return scnprintf(bf, size, "%-*.*s", width, width, "N/A");
+		return scnprintf(bf, size, "%-.*s", width, "N/A");
 
 	if (he->trace_output == NULL)
 		he->trace_output = get_trace_output(he);
-	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->trace_output);
+	return repsep_snprintf(bf, size, "%-.*s", width, he->trace_output);
 }
 
 struct sort_entry sort_trace = {
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/5] perf tools: Update srcline/file if needed
  2016-02-22  0:31     ` [PATCH v2 " Namhyung Kim
@ 2016-02-22  6:49       ` Jiri Olsa
  2016-02-25  5:37       ` [tip:perf/core] " tip-bot for Namhyung Kim
  1 sibling, 0 replies; 19+ messages in thread
From: Jiri Olsa @ 2016-02-22  6:49 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra, Jiri Olsa,
	LKML, David Ahern, Andi Kleen

On Mon, Feb 22, 2016 at 09:31:51AM +0900, Namhyung Kim wrote:
> Normally the hist entry's srcline and/or srcfile is set during sorting.
> However sometime it's possible to a hist entry's srcline is not set yet
> after the sorting.  This is because the entry is so unique and other
> sort keys already make it distinct.  Then the srcline/file sort didn't
> have a chance to be called during the sorting.  In that case it has NULL
> srcline/srcfile field and shows nothing.
> 
> Before:
> 
>   $ perf report -s comm,sym,srcline
>   ...
>   Overhead  Command       Symbol
>   -----------------------------------------------------------------
>     34.42%  swapper       [k] intel_idle          intel_idle.c:0
>      2.44%  perf          [.] __poll_nocancel     (null)
>      1.70%  gnome-shell   [k] fw_domains_get      (null)
>      1.04%  Xorg          [k] sock_poll           (null)
> 
> After:
> 
>     34.42%  swapper       [k] intel_idle          intel_idle.c:0
>      2.44%  perf          [.] __poll_nocancel     .:0
>      1.70%  gnome-shell   [k] fw_domains_get      fw_domains_get+42
>      1.04%  Xorg          [k] sock_poll           socket.c:0
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry
  2016-02-21 17:37 ` [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry Jiri Olsa
@ 2016-02-22 15:08   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 19+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-02-22 15:08 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Namhyung Kim, Ingo Molnar, Peter Zijlstra, Jiri Olsa, LKML,
	David Ahern, Andi Kleen

Em Sun, Feb 21, 2016 at 06:37:32PM +0100, Jiri Olsa escreveu:
> On Sun, Feb 21, 2016 at 11:22:34PM +0900, Namhyung Kim wrote:
> > The dynamic entry is created for each field in a tracepoint event.
> > Since they have no fixed hpp format index, it should skip when
> > perf_hpp__reset_width() is called.
> > 
> > This caused following assertion failure..
> > 
> >   $ perf record -e sched:sched_switch -a sleep 1
> > 
> >   $ perf report -s comm,next_pid --stdio
> >   perf: ui/hist.c:651: perf_hpp__reset_width:
> >     Assertion `!(fmt->idx >= PERF_HPP__MAX_INDEX)' failed.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Thanks, tested and applied the series,

- Arnaldo

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [tip:perf/core] perf tools: Fix segfault on dynamic entries
  2016-02-21 14:22 ` [PATCH 2/5] perf tools: Fix segfault on dynamic entries Namhyung Kim
  2016-02-21 17:37   ` Jiri Olsa
@ 2016-02-25  5:37   ` tip-bot for Namhyung Kim
  1 sibling, 0 replies; 19+ messages in thread
From: tip-bot for Namhyung Kim @ 2016-02-25  5:37 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: tglx, andi, peterz, linux-kernel, acme, jolsa, hpa, mingo,
	dsahern, namhyung

Commit-ID:  665aa75700edda07bd7f05acab86cef1a1a1ea66
Gitweb:     http://git.kernel.org/tip/665aa75700edda07bd7f05acab86cef1a1a1ea66
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sun, 21 Feb 2016 23:22:35 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 22 Feb 2016 12:01:09 -0300

perf tools: Fix segfault on dynamic entries

A dynamic entry is created for each tracepoint event.  When it sets up
the sort key, it checks with existing keys using ->equal() callback.
But it missed to set the ->equal for dynamic entries.  The following
segfault was due to the missing ->equal() callback.

  (gdb) bt
  #0  0x0000000000140003 in ?? ()
  #1  0x0000000000537769 in fmt_equal (b=0x2106980, a=0x21067a0) at ui/hist.c:548
  #2  perf_hpp__setup_output_field (list=0x8c6d80 <perf_hpp_list>) at ui/hist.c:560
  #3  0x00000000004e927e in setup_sorting (evlist=<optimized out>) at util/sort.c:2642
  #4  0x000000000043cf50 in cmd_report (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>)
      at builtin-report.c:932
  #5  0x00000000004865a1 in run_builtin (p=p@entry=0x8bbce0 <commands+192>, argc=argc@entry=7,
      argv=argv@entry=0x7ffd24d56ce0) at perf.c:390
  #6  0x000000000042dc1f in handle_internal_command (argv=0x7ffd24d56ce0, argc=7) at perf.c:451
  #7  run_argv (argv=0x7ffd24d56a70, argcp=0x7ffd24d56a7c) at perf.c:495
  #8  main (argc=7, argv=0x7ffd24d56ce0) at perf.c:620

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1456064558-13086-2-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/sort.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index de71575..7daea71 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1835,6 +1835,20 @@ bool perf_hpp__is_dynamic_entry(struct perf_hpp_fmt *fmt)
 	return fmt->cmp == __sort__hde_cmp;
 }
 
+static bool __sort__hde_equal(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b)
+{
+	struct hpp_dynamic_entry *hde_a;
+	struct hpp_dynamic_entry *hde_b;
+
+	if (!perf_hpp__is_dynamic_entry(a) || !perf_hpp__is_dynamic_entry(b))
+		return false;
+
+	hde_a = container_of(a, struct hpp_dynamic_entry, hpp);
+	hde_b = container_of(b, struct hpp_dynamic_entry, hpp);
+
+	return hde_a->field == hde_b->field;
+}
+
 static void hde_free(struct perf_hpp_fmt *fmt)
 {
 	struct hpp_dynamic_entry *hde;
@@ -1867,6 +1881,7 @@ __alloc_dynamic_entry(struct perf_evsel *evsel, struct format_field *field)
 	hde->hpp.cmp = __sort__hde_cmp;
 	hde->hpp.collapse = __sort__hde_cmp;
 	hde->hpp.sort = __sort__hde_cmp;
+	hde->hpp.equal = __sort__hde_equal;
 	hde->hpp.free = hde_free;
 
 	INIT_LIST_HEAD(&hde->hpp.list);

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [tip:perf/core] perf tools: Update srcline/file if needed
  2016-02-22  0:31     ` [PATCH v2 " Namhyung Kim
  2016-02-22  6:49       ` Jiri Olsa
@ 2016-02-25  5:37       ` tip-bot for Namhyung Kim
  1 sibling, 0 replies; 19+ messages in thread
From: tip-bot for Namhyung Kim @ 2016-02-25  5:37 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: dsahern, jolsa, tglx, acme, hpa, a.p.zijlstra, andi, linux-kernel,
	mingo, namhyung

Commit-ID:  cecaec635de3719ef56a9261c10cd8f2f74ebdb1
Gitweb:     http://git.kernel.org/tip/cecaec635de3719ef56a9261c10cd8f2f74ebdb1
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Mon, 22 Feb 2016 09:31:51 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 22 Feb 2016 12:04:34 -0300

perf tools: Update srcline/file if needed

Normally the hist entry's srcline and/or srcfile is set during sorting.
However sometime it's possible to a hist entry's srcline is not set yet
after the sorting.  This is because the entry is so unique and other
sort keys already make it distinct.  Then the srcline/file sort didn't
have a chance to be called during the sorting.  In that case it has NULL
srcline/srcfile field and shows nothing.

Before:

  $ perf report -s comm,sym,srcline
  ...
  Overhead  Command       Symbol
  -----------------------------------------------------------------
    34.42%  swapper       [k] intel_idle          intel_idle.c:0
     2.44%  perf          [.] __poll_nocancel     (null)
     1.70%  gnome-shell   [k] fw_domains_get      (null)
     1.04%  Xorg          [k] sock_poll           (null)

After:

    34.42%  swapper       [k] intel_idle          intel_idle.c:0
     2.44%  perf          [.] __poll_nocancel     .:0
     1.70%  gnome-shell   [k] fw_domains_get      fw_domains_get+42
     1.04%  Xorg          [k] sock_poll           socket.c:0

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1456101111-14400-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/sort.c | 64 ++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 33 deletions(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 7daea71..6f4605b 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -286,35 +286,34 @@ struct sort_entry sort_sym = {
 
 /* --sort srcline */
 
+static char *hist_entry__get_srcline(struct hist_entry *he)
+{
+	struct map *map = he->ms.map;
+
+	if (!map)
+		return SRCLINE_UNKNOWN;
+
+	return get_srcline(map->dso, map__rip_2objdump(map, he->ip),
+			   he->ms.sym, true);
+}
+
 static int64_t
 sort__srcline_cmp(struct hist_entry *left, struct hist_entry *right)
 {
-	if (!left->srcline) {
-		if (!left->ms.map)
-			left->srcline = SRCLINE_UNKNOWN;
-		else {
-			struct map *map = left->ms.map;
-			left->srcline = get_srcline(map->dso,
-					   map__rip_2objdump(map, left->ip),
-						    left->ms.sym, true);
-		}
-	}
-	if (!right->srcline) {
-		if (!right->ms.map)
-			right->srcline = SRCLINE_UNKNOWN;
-		else {
-			struct map *map = right->ms.map;
-			right->srcline = get_srcline(map->dso,
-					     map__rip_2objdump(map, right->ip),
-						     right->ms.sym, true);
-		}
-	}
+	if (!left->srcline)
+		left->srcline = hist_entry__get_srcline(left);
+	if (!right->srcline)
+		right->srcline = hist_entry__get_srcline(right);
+
 	return strcmp(right->srcline, left->srcline);
 }
 
 static int hist_entry__srcline_snprintf(struct hist_entry *he, char *bf,
 					size_t size, unsigned int width)
 {
+	if (!he->srcline)
+		he->srcline = hist_entry__get_srcline(he);
+
 	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcline);
 }
 
@@ -329,11 +328,14 @@ struct sort_entry sort_srcline = {
 
 static char no_srcfile[1];
 
-static char *get_srcfile(struct hist_entry *e)
+static char *hist_entry__get_srcfile(struct hist_entry *e)
 {
 	char *sf, *p;
 	struct map *map = e->ms.map;
 
+	if (!map)
+		return no_srcfile;
+
 	sf = __get_srcline(map->dso, map__rip_2objdump(map, e->ip),
 			 e->ms.sym, false, true);
 	if (!strcmp(sf, SRCLINE_UNKNOWN))
@@ -350,24 +352,20 @@ static char *get_srcfile(struct hist_entry *e)
 static int64_t
 sort__srcfile_cmp(struct hist_entry *left, struct hist_entry *right)
 {
-	if (!left->srcfile) {
-		if (!left->ms.map)
-			left->srcfile = no_srcfile;
-		else
-			left->srcfile = get_srcfile(left);
-	}
-	if (!right->srcfile) {
-		if (!right->ms.map)
-			right->srcfile = no_srcfile;
-		else
-			right->srcfile = get_srcfile(right);
-	}
+	if (!left->srcfile)
+		left->srcfile = hist_entry__get_srcfile(left);
+	if (!right->srcfile)
+		right->srcfile = hist_entry__get_srcfile(right);
+
 	return strcmp(right->srcfile, left->srcfile);
 }
 
 static int hist_entry__srcfile_snprintf(struct hist_entry *he, char *bf,
 					size_t size, unsigned int width)
 {
+	if (!he->srcfile)
+		he->srcfile = hist_entry__get_srcfile(he);
+
 	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcfile);
 }
 

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [tip:perf/core] perf tools: Fix alignment on some sort keys
  2016-02-22  0:32     ` [PATCH v2 " Namhyung Kim
@ 2016-02-25  5:38       ` tip-bot for Namhyung Kim
  0 siblings, 0 replies; 19+ messages in thread
From: tip-bot for Namhyung Kim @ 2016-02-25  5:38 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: a.p.zijlstra, acme, jolsa, tglx, linux-kernel, dsahern, andi,
	namhyung, mingo, hpa

Commit-ID:  2960ed6f8d6794dcb39ba48c3e515e5be18ee9e1
Gitweb:     http://git.kernel.org/tip/2960ed6f8d6794dcb39ba48c3e515e5be18ee9e1
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Mon, 22 Feb 2016 09:32:33 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 22 Feb 2016 12:05:55 -0300

perf tools: Fix alignment on some sort keys

The srcline, srcfile and trace sort keys can have long entries.  With
commit 89fee7094323 ("perf hists: Do column alignment on the format
iterator"), it now aligns output with hist_entry__snprintf_alignment().
So each (possibly long) sort entries don't need to do it themselves.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1456101153-14519-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/sort.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 6f4605b..a7d73e5 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -314,7 +314,7 @@ static int hist_entry__srcline_snprintf(struct hist_entry *he, char *bf,
 	if (!he->srcline)
 		he->srcline = hist_entry__get_srcline(he);
 
-	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcline);
+	return repsep_snprintf(bf, size, "%-.*s", width, he->srcline);
 }
 
 struct sort_entry sort_srcline = {
@@ -366,7 +366,7 @@ static int hist_entry__srcfile_snprintf(struct hist_entry *he, char *bf,
 	if (!he->srcfile)
 		he->srcfile = hist_entry__get_srcfile(he);
 
-	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->srcfile);
+	return repsep_snprintf(bf, size, "%-.*s", width, he->srcfile);
 }
 
 struct sort_entry sort_srcfile = {
@@ -496,11 +496,11 @@ static int hist_entry__trace_snprintf(struct hist_entry *he, char *bf,
 
 	evsel = hists_to_evsel(he->hists);
 	if (evsel->attr.type != PERF_TYPE_TRACEPOINT)
-		return scnprintf(bf, size, "%-*.*s", width, width, "N/A");
+		return scnprintf(bf, size, "%-.*s", width, "N/A");
 
 	if (he->trace_output == NULL)
 		he->trace_output = get_trace_output(he);
-	return repsep_snprintf(bf, size, "%-*.*s", width, width, he->trace_output);
+	return repsep_snprintf(bf, size, "%-.*s", width, he->trace_output);
 }
 
 struct sort_entry sort_trace = {

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [tip:perf/core] perf tools: Fix column width setting on 'trace' sort key
  2016-02-21 14:22 ` [PATCH 5/5] perf tools: Fix column width setting on 'trace' sort key Namhyung Kim
  2016-02-21 17:41   ` Jiri Olsa
@ 2016-02-25  5:38   ` tip-bot for Namhyung Kim
  1 sibling, 0 replies; 19+ messages in thread
From: tip-bot for Namhyung Kim @ 2016-02-25  5:38 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: andi, acme, tglx, namhyung, linux-kernel, dsahern, hpa, mingo,
	jolsa, peterz

Commit-ID:  0c0af78d472f96efe04daaaccede7522b2394b76
Gitweb:     http://git.kernel.org/tip/0c0af78d472f96efe04daaaccede7522b2394b76
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sun, 21 Feb 2016 23:22:38 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 22 Feb 2016 12:06:53 -0300

perf tools: Fix column width setting on 'trace' sort key

It missed to update column length of the 'trace' sort key in the
hists__calc_col_len() so it might truncate the output.  It calculated
the column length in the ->cmp() callback originally but it doesn't
guarantee it's called always.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1456064558-13086-5-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/hist.c | 3 +++
 tools/perf/util/sort.c | 3 ---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 827c6cb..017eb5c 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -179,6 +179,9 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 	if (h->transaction)
 		hists__new_col_len(hists, HISTC_TRANSACTION,
 				   hist_entry__transaction_len());
+
+	if (h->trace_output)
+		hists__new_col_len(hists, HISTC_TRACE, strlen(h->trace_output));
 }
 
 void hists__output_recalc_col_len(struct hists *hists, int max_rows)
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index a7d73e5..6d0f858 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -483,9 +483,6 @@ sort__trace_cmp(struct hist_entry *left, struct hist_entry *right)
 	if (right->trace_output == NULL)
 		right->trace_output = get_trace_output(right);
 
-	hists__new_col_len(left->hists, HISTC_TRACE, strlen(left->trace_output));
-	hists__new_col_len(right->hists, HISTC_TRACE, strlen(right->trace_output));
-
 	return strcmp(right->trace_output, left->trace_output);
 }
 

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [tip:perf/core] perf tools: Fix assertion failure on dynamic entry
  2016-02-21 14:22 [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry Namhyung Kim
                   ` (4 preceding siblings ...)
  2016-02-21 17:37 ` [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry Jiri Olsa
@ 2016-02-25  5:38 ` tip-bot for Namhyung Kim
  5 siblings, 0 replies; 19+ messages in thread
From: tip-bot for Namhyung Kim @ 2016-02-25  5:38 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: jolsa, tglx, linux-kernel, andi, mingo, namhyung, peterz, hpa,
	dsahern, acme

Commit-ID:  dd42baf1f64d7257258fa4f20064aee5160df369
Gitweb:     http://git.kernel.org/tip/dd42baf1f64d7257258fa4f20064aee5160df369
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Sun, 21 Feb 2016 23:22:34 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 22 Feb 2016 12:07:14 -0300

perf tools: Fix assertion failure on dynamic entry

The dynamic entry is created for each field in a tracepoint event.
Since they have no fixed hpp format index, it should skip when
perf_hpp__reset_width() is called.

This caused following assertion failure..

  $ perf record -e sched:sched_switch -a sleep 1

  $ perf report -s comm,next_pid --stdio
  perf: ui/hist.c:651: perf_hpp__reset_width:
    Assertion `!(fmt->idx >= PERF_HPP__MAX_INDEX)' failed.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1456064558-13086-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/hist.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 1ba4117..12223d7 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -645,6 +645,9 @@ void perf_hpp__reset_width(struct perf_hpp_fmt *fmt, struct hists *hists)
 	if (perf_hpp__is_sort_entry(fmt))
 		return perf_hpp__reset_sort_width(fmt, hists);
 
+	if (perf_hpp__is_dynamic_entry(fmt))
+		return;
+
 	BUG_ON(fmt->idx >= PERF_HPP__MAX_INDEX);
 
 	switch (fmt->idx) {

^ permalink raw reply related	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2016-02-25  6:31 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-21 14:22 [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry Namhyung Kim
2016-02-21 14:22 ` [PATCH 2/5] perf tools: Fix segfault on dynamic entries Namhyung Kim
2016-02-21 17:37   ` Jiri Olsa
2016-02-25  5:37   ` [tip:perf/core] " tip-bot for Namhyung Kim
2016-02-21 14:22 ` [PATCH 3/5] perf tools: Update srcline/file if needed Namhyung Kim
2016-02-21 17:39   ` Jiri Olsa
2016-02-22  0:31     ` [PATCH v2 " Namhyung Kim
2016-02-22  6:49       ` Jiri Olsa
2016-02-25  5:37       ` [tip:perf/core] " tip-bot for Namhyung Kim
2016-02-21 14:22 ` [PATCH 4/5] perf tools: Fix alignment on some sort keys Namhyung Kim
2016-02-21 17:40   ` Jiri Olsa
2016-02-22  0:32     ` [PATCH v2 " Namhyung Kim
2016-02-25  5:38       ` [tip:perf/core] " tip-bot for Namhyung Kim
2016-02-21 14:22 ` [PATCH 5/5] perf tools: Fix column width setting on 'trace' sort key Namhyung Kim
2016-02-21 17:41   ` Jiri Olsa
2016-02-25  5:38   ` [tip:perf/core] " tip-bot for Namhyung Kim
2016-02-21 17:37 ` [PATCH 1/5] perf tools: Fix assertion failure on dynamic entry Jiri Olsa
2016-02-22 15:08   ` Arnaldo Carvalho de Melo
2016-02-25  5:38 ` [tip:perf/core] " tip-bot for Namhyung Kim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).