[PATCH 1/3] perf top: Fix freeze on --call-graph flat/folded

[PATCH 1/3] perf top: Fix freeze on --call-graph flat/folded

[Namhyung Kim]

The callchain rbtree is rebuilt periodically, so it needs to
reinitialize the root everytime.  Otherwise it can be stuck in the
rbtree insertion with stale pointers.

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

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index fc3b1e0d09ee..564377d2bebf 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -290,6 +290,7 @@ static void
 sort_chain_flat(struct rb_root *rb_root, struct callchain_root *root,
 		u64 min_hit, struct callchain_param *param __maybe_unused)
 {
+	*rb_root = RB_ROOT;
 	__sort_chain_flat(rb_root, &root->node, min_hit);
 }
 
-- 
2.6.2

[PATCH 2/3] perf callchain: Stop resolving callchains after invalid address

[Namhyung Kim]

Unwinding optimized binaries using frame pointer gives garbage.  Check
callchain address and stop if it's under vm.mmap_min_addr sysctl value.

Before:
  $ perf report --stdio --no-children -g callee
  ...

   1.37%  perf    [kernel.vmlinux]    [k] smp_call_function_single
               |
               ---smp_call_function_single
                  _perf_event_enable
                  perf_event_for_each_child
                  perf_ioctl
                  do_vfs_ioctl
                  sys_ioctl
                  entry_SYSCALL_64_fastpath
                  __GI___ioctl
                  0
                  0
                  0x1c5aa70
                  0x1c5b910
                  0x1c5aa70
                  0x1c5b910
                  0x1c5aa70
                  0x1c5b910
                  0x1c5aa70
                  0x1c5b910
                  0x1c5aa70
                  0x1c5b910
		  ...

After:
  $ perf report --stdio --no-children -g callee
  ...

   1.37%  perf    [kernel.vmlinux]    [k] smp_call_function_single
               |
               ---smp_call_function_single
                  _perf_event_enable
                  perf_event_for_each_child
                  perf_ioctl
                  do_vfs_ioctl
                  sys_ioctl
                  entry_SYSCALL_64_fastpath
                  __GI___ioctl

  $ perf report --stdio --no-children -g caller
  ...

   1.37%  perf    [kernel.vmlinux]    [k] smp_call_function_single
               |
               ---__GI__ioctl
                  entry_SYSCALL_64_fastpath
                  sys_ioctl
                  do_vfs_ioctl
                  perf_ioctl
                  perf_event_for_each_child
                  _perf_event_enable
                  smp_call_function_single

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/perf.c         |  6 ++++++
 tools/perf/util/machine.c | 13 +++++++++++++
 tools/perf/util/util.c    |  1 +
 tools/perf/util/util.h    |  1 +
 4 files changed, 21 insertions(+)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 4bee53c3f796..0075a6d38e3a 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -18,6 +18,7 @@
 #include "util/bpf-loader.h"
 #include "util/debug.h"
 #include <api/fs/tracing_path.h>
+#include <api/fs/fs.h>
 #include <pthread.h>
 
 const char perf_usage_string[] =
@@ -528,11 +529,16 @@ int main(int argc, const char **argv)
 {
 	const char *cmd;
 	char sbuf[STRERR_BUFSIZE];
+	int min_addr;
 
 	/* The page_size is placed in util object. */
 	page_size = sysconf(_SC_PAGE_SIZE);
 	cacheline_size = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
 
+	if (sysctl__read_int("vm/mmap_min_addr", &min_addr) < 0)
+		min_addr = page_size;
+	mmap_min_addr = min_addr;
+
 	cmd = perf_extract_argv0_path(argv[0]);
 	if (!cmd)
 		cmd = "perf-help";
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 7f5071a4d9aa..0a2f35e0d737 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1860,6 +1860,19 @@ static int thread__resolve_callchain_sample(struct thread *thread,
 #endif
 		ip = chain->ips[j];
 
+		/*
+		 * Callchain value under mmap_min_addr means it's broken
+		 * or the end of callchain.  Stop.
+		 */
+		if (ip < mmap_min_addr) {
+			if (callchain_param.order == ORDER_CALLEE)
+				break;
+
+			/* ignore current callchains for CALLER order */
+			callchain_cursor_reset(&callchain_cursor);
+			continue;
+		}
+
 		err = add_callchain_ip(thread, parent, root_al, &cpumode, ip);
 
 		if (err)
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 75759aebc7b8..8e198acd9fa6 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -30,6 +30,7 @@ struct callchain_param	callchain_param = {
  */
 unsigned int page_size;
 int cacheline_size;
+unsigned long mmap_min_addr;
 
 bool test_attr__enabled;
 
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index dcc659017976..506d4dbb58be 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -281,6 +281,7 @@ void sighandler_dump_stack(int sig);
 
 extern unsigned int page_size;
 extern int cacheline_size;
+extern unsigned long mmap_min_addr;
 
 void get_term_dimensions(struct winsize *ws);
 void set_term_quiet_input(struct termios *old);
-- 
2.6.2

Re: [PATCH 2/3] perf callchain: Stop resolving callchains after invalid address

[Ingo Molnar]

* Namhyung Kim <namhyung@kernel.org> wrote:

> Unwinding optimized binaries using frame pointer gives garbage.  Check
> callchain address and stop if it's under vm.mmap_min_addr sysctl value.
> 
> Before:
>   $ perf report --stdio --no-children -g callee
>   ...
> 
>    1.37%  perf    [kernel.vmlinux]    [k] smp_call_function_single
>                |
>                ---smp_call_function_single
>                   _perf_event_enable
>                   perf_event_for_each_child
>                   perf_ioctl
>                   do_vfs_ioctl
>                   sys_ioctl
>                   entry_SYSCALL_64_fastpath
>                   __GI___ioctl
>                   0
>                   0
>                   0x1c5aa70
>                   0x1c5b910
>                   0x1c5aa70
>                   0x1c5b910
>                   0x1c5aa70
>                   0x1c5b910
>                   0x1c5aa70
>                   0x1c5b910
>                   0x1c5aa70
>                   0x1c5b910
> 		  ...
> 
> After:
>   $ perf report --stdio --no-children -g callee
>   ...
> 
>    1.37%  perf    [kernel.vmlinux]    [k] smp_call_function_single
>                |
>                ---smp_call_function_single
>                   _perf_event_enable
>                   perf_event_for_each_child
>                   perf_ioctl
>                   do_vfs_ioctl
>                   sys_ioctl
>                   entry_SYSCALL_64_fastpath
>                   __GI___ioctl

In addition to that, would it make sense to terminate the callchain with an 
indicator that we found something anomalous? Such an extra line:

                    ...

would not be intrusive, but would tell the informed reader that it's not a normal 
ending of the call chain.

This assumes that we can tell apart 'normal end of call chain' from 'seems to end 
with garbage poiner' cases - can do we that?

Thanks,

	Ingo

Re: [PATCH 2/3] perf callchain: Stop resolving callchains after invalid address

[Namhyung Kim]

Hi Ingo,

On Thu, Nov 26, 2015 at 08:43:35AM +0100, Ingo Molnar wrote:
> 
> * Namhyung Kim <namhyung@kernel.org> wrote:
> 
> > Unwinding optimized binaries using frame pointer gives garbage.  Check
> > callchain address and stop if it's under vm.mmap_min_addr sysctl value.
> > 
> > Before:
> >   $ perf report --stdio --no-children -g callee
> >   ...
> > 
> >    1.37%  perf    [kernel.vmlinux]    [k] smp_call_function_single
> >                |
> >                ---smp_call_function_single
> >                   _perf_event_enable
> >                   perf_event_for_each_child
> >                   perf_ioctl
> >                   do_vfs_ioctl
> >                   sys_ioctl
> >                   entry_SYSCALL_64_fastpath
> >                   __GI___ioctl
> >                   0
> >                   0
> >                   0x1c5aa70
> >                   0x1c5b910
> >                   0x1c5aa70
> >                   0x1c5b910
> >                   0x1c5aa70
> >                   0x1c5b910
> >                   0x1c5aa70
> >                   0x1c5b910
> >                   0x1c5aa70
> >                   0x1c5b910
> > 		  ...
> > 
> > After:
> >   $ perf report --stdio --no-children -g callee
> >   ...
> > 
> >    1.37%  perf    [kernel.vmlinux]    [k] smp_call_function_single
> >                |
> >                ---smp_call_function_single
> >                   _perf_event_enable
> >                   perf_event_for_each_child
> >                   perf_ioctl
> >                   do_vfs_ioctl
> >                   sys_ioctl
> >                   entry_SYSCALL_64_fastpath
> >                   __GI___ioctl
> 
> In addition to that, would it make sense to terminate the callchain with an 
> indicator that we found something anomalous? Such an extra line:
> 
>                     ...
> 
> would not be intrusive, but would tell the informed reader that it's not a normal 
> ending of the call chain.
> 
> This assumes that we can tell apart 'normal end of call chain' from 'seems to end 
> with garbage poiner' cases - can do we that?

In case of fp unwind, I'm not sure we can determine whether it's
normal end or not especially for optimized binaries.  It seems kernel
also can stop callchain anytime if it sees a broken frame.

For dwarf unwind, I think it's also hard to tell since it can be
stopped for various reasons like insufficient dump size or broken CFI,
...

Thanks,
Namhyung

Re: [PATCH 2/3] perf callchain: Stop resolving callchains after invalid address

[Ingo Molnar]

* Namhyung Kim <namhyung@kernel.org> wrote:

> Hi Ingo,
> 
> On Thu, Nov 26, 2015 at 08:43:35AM +0100, Ingo Molnar wrote:
> > 
> > * Namhyung Kim <namhyung@kernel.org> wrote:
> > 
> > > Unwinding optimized binaries using frame pointer gives garbage.  Check
> > > callchain address and stop if it's under vm.mmap_min_addr sysctl value.
> > > 
> > > Before:
> > >   $ perf report --stdio --no-children -g callee
> > >   ...
> > > 
> > >    1.37%  perf    [kernel.vmlinux]    [k] smp_call_function_single
> > >                |
> > >                ---smp_call_function_single
> > >                   _perf_event_enable
> > >                   perf_event_for_each_child
> > >                   perf_ioctl
> > >                   do_vfs_ioctl
> > >                   sys_ioctl
> > >                   entry_SYSCALL_64_fastpath
> > >                   __GI___ioctl
> > >                   0
> > >                   0
> > >                   0x1c5aa70
> > >                   0x1c5b910
> > >                   0x1c5aa70
> > >                   0x1c5b910
> > >                   0x1c5aa70
> > >                   0x1c5b910
> > >                   0x1c5aa70
> > >                   0x1c5b910
> > >                   0x1c5aa70
> > >                   0x1c5b910
> > > 		  ...
> > > 
> > > After:
> > >   $ perf report --stdio --no-children -g callee
> > >   ...
> > > 
> > >    1.37%  perf    [kernel.vmlinux]    [k] smp_call_function_single
> > >                |
> > >                ---smp_call_function_single
> > >                   _perf_event_enable
> > >                   perf_event_for_each_child
> > >                   perf_ioctl
> > >                   do_vfs_ioctl
> > >                   sys_ioctl
> > >                   entry_SYSCALL_64_fastpath
> > >                   __GI___ioctl
> > 
> > In addition to that, would it make sense to terminate the callchain with an 
> > indicator that we found something anomalous? Such an extra line:
> > 
> >                     ...
> > 
> > would not be intrusive, but would tell the informed reader that it's not a normal 
> > ending of the call chain.
> > 
> > This assumes that we can tell apart 'normal end of call chain' from 'seems to end 
> > with garbage poiner' cases - can do we that?
> 
> In case of fp unwind, I'm not sure we can determine whether it's
> normal end or not especially for optimized binaries.  It seems kernel
> also can stop callchain anytime if it sees a broken frame.
> 
> For dwarf unwind, I think it's also hard to tell since it can be
> stopped for various reasons like insufficient dump size or broken CFI,

But but. Doesn't your patch 'detect' an anomaly to begin with?

+               /*
+                * Callchain value under mmap_min_addr means it's broken
+                * or the end of callchain.  Stop.
+                */
+               if (ip < mmap_min_addr) {
+                       if (callchain_param.order == ORDER_CALLEE)
+                               break;

all I'm asking for is to indicate it in some low-key visual fashion when we 
encounter such a 'broken' call-chain.

I presume the 'old' way of ending the call-chain was that 'ip' was zero, right? We 
should not print the indicator in that case.

Also, in the dwarf case I'd also see value in indicating if any of these events 
occured:

  > For dwarf unwind, I think it's also hard to tell since it can be stopped for 
  > various reasons like insufficient dump size or broken CFI,

even if we cannot catch all anomalies. Performance analysis must stand firm on a 
hard rock of reliability and dependability, and we should always propagate 
information about possible profiling data corruption/unreliability. That's why we 
print the 'IO overload' messages during perf record for example.

Even if the problem is not caused by perf, but by external factors such as the 
compiler/linker.

Thanks,

	Ingo

Re: [PATCH 2/3] perf callchain: Stop resolving callchains after invalid address

[David Ahern]

On 11/26/15 12:08 AM, Namhyung Kim wrote:
> @@ -528,11 +529,16 @@ int main(int argc, const char **argv)
>   {
>   	const char *cmd;
>   	char sbuf[STRERR_BUFSIZE];
> +	int min_addr;
>
>   	/* The page_size is placed in util object. */
>   	page_size = sysconf(_SC_PAGE_SIZE);
>   	cacheline_size = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
>
> +	if (sysctl__read_int("vm/mmap_min_addr", &min_addr) < 0)

This assumes the record and analysis are done on the same system.

David

Re: [PATCH 2/3] perf callchain: Stop resolving callchains after invalid address

[Namhyung Kim]

Hi David,

On Thu, Nov 26, 2015 at 06:14:57AM -0700, David Ahern wrote:
> On 11/26/15 12:08 AM, Namhyung Kim wrote:
> >@@ -528,11 +529,16 @@ int main(int argc, const char **argv)
> >  {
> >  	const char *cmd;
> >  	char sbuf[STRERR_BUFSIZE];
> >+	int min_addr;
> >
> >  	/* The page_size is placed in util object. */
> >  	page_size = sysconf(_SC_PAGE_SIZE);
> >  	cacheline_size = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
> >
> >+	if (sysctl__read_int("vm/mmap_min_addr", &min_addr) < 0)
> 
> This assumes the record and analysis are done on the same system.

Right.  Maybe we can just use minimal size (or page size?) or save and
pass it through somewhere in the feature bit?

Thanks,
Namhyung

Re: [PATCH 2/3] perf callchain: Stop resolving callchains after invalid address

[David Ahern]

On 11/26/15 8:00 AM, Namhyung Kim wrote:
> Hi David,
>
> On Thu, Nov 26, 2015 at 06:14:57AM -0700, David Ahern wrote:
>> On 11/26/15 12:08 AM, Namhyung Kim wrote:
>>> @@ -528,11 +529,16 @@ int main(int argc, const char **argv)
>>>   {
>>>   	const char *cmd;
>>>   	char sbuf[STRERR_BUFSIZE];
>>> +	int min_addr;
>>>
>>>   	/* The page_size is placed in util object. */
>>>   	page_size = sysconf(_SC_PAGE_SIZE);
>>>   	cacheline_size = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
>>>
>>> +	if (sysctl__read_int("vm/mmap_min_addr", &min_addr) < 0)
>>
>> This assumes the record and analysis are done on the same system.
>
> Right.  Maybe we can just use minimal size (or page size?) or save and
> pass it through somewhere in the feature bit?

no preference, but it should work with cross arch analysis as well 
(e.g., record on arm/ppc and analysis on x86)

Re: [PATCH 2/3] perf callchain: Stop resolving callchains after invalid address

[Jiri Olsa]

On Thu, Nov 26, 2015 at 08:48:17AM -0700, David Ahern wrote:
> On 11/26/15 8:00 AM, Namhyung Kim wrote:
> >Hi David,
> >
> >On Thu, Nov 26, 2015 at 06:14:57AM -0700, David Ahern wrote:
> >>On 11/26/15 12:08 AM, Namhyung Kim wrote:
> >>>@@ -528,11 +529,16 @@ int main(int argc, const char **argv)
> >>>  {
> >>>  	const char *cmd;
> >>>  	char sbuf[STRERR_BUFSIZE];
> >>>+	int min_addr;
> >>>
> >>>  	/* The page_size is placed in util object. */
> >>>  	page_size = sysconf(_SC_PAGE_SIZE);
> >>>  	cacheline_size = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
> >>>
> >>>+	if (sysctl__read_int("vm/mmap_min_addr", &min_addr) < 0)
> >>
> >>This assumes the record and analysis are done on the same system.
> >
> >Right.  Maybe we can just use minimal size (or page size?) or save and
> >pass it through somewhere in the feature bit?
> 
> no preference, but it should work with cross arch analysis as well (e.g.,
> record on arm/ppc and analysis on x86)

I think we should store it in perf.data in features, but seems
like a waste to spend one bit just for this number.

I remember commenting on new CPU related FEATURE data, that would contain
cpu specific data in extensible form like TAG,VALUE,TAG,VALUE..

but I think the design changed or something, because I cannot find it in now ;-)

maybe we could add something like that

jirka

Re: [PATCH 2/3] perf callchain: Stop resolving callchains after invalid address

[Arnaldo Carvalho de Melo]

Em Thu, Nov 26, 2015 at 04:58:57PM +0100, Jiri Olsa escreveu:
> On Thu, Nov 26, 2015 at 08:48:17AM -0700, David Ahern wrote:
> > On 11/26/15 8:00 AM, Namhyung Kim wrote:
> > >Hi David,
> > >
> > >On Thu, Nov 26, 2015 at 06:14:57AM -0700, David Ahern wrote:
> > >>On 11/26/15 12:08 AM, Namhyung Kim wrote:
> > >>>@@ -528,11 +529,16 @@ int main(int argc, const char **argv)
> > >>>  {
> > >>>  	const char *cmd;
> > >>>  	char sbuf[STRERR_BUFSIZE];
> > >>>+	int min_addr;
> > >>>
> > >>>  	/* The page_size is placed in util object. */
> > >>>  	page_size = sysconf(_SC_PAGE_SIZE);
> > >>>  	cacheline_size = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
> > >>>
> > >>>+	if (sysctl__read_int("vm/mmap_min_addr", &min_addr) < 0)

Please put this in that symbol_conf kitchen sink :-)

I'm unsure though if there would be a reason for having both the local
min_addr and the one at perf record time, i.e. from perf.data :-\

> > >>This assumes the record and analysis are done on the same system.
> > >
> > >Right.  Maybe we can just use minimal size (or page size?) or save and
> > >pass it through somewhere in the feature bit?
> > 
> > no preference, but it should work with cross arch analysis as well (e.g.,
> > record on arm/ppc and analysis on x86)
> 
> I think we should store it in perf.data in features, but seems
> like a waste to spend one bit just for this number.
> 
> I remember commenting on new CPU related FEATURE data, that would contain
> cpu specific data in extensible form like TAG,VALUE,TAG,VALUE..
> 
> but I think the design changed or something, because I cannot find it in now ;-)
> 
> maybe we could add something like that

Right, isn't that one part of the perf/stat patchkit?

- Arnaldo

Re: [PATCH 2/3] perf callchain: Stop resolving callchains after invalid address

[Namhyung Kim]

On Thu, Nov 26, 2015 at 01:19:01PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 26, 2015 at 04:58:57PM +0100, Jiri Olsa escreveu:
> > On Thu, Nov 26, 2015 at 08:48:17AM -0700, David Ahern wrote:
> > > On 11/26/15 8:00 AM, Namhyung Kim wrote:
> > > >Hi David,
> > > >
> > > >On Thu, Nov 26, 2015 at 06:14:57AM -0700, David Ahern wrote:
> > > >>On 11/26/15 12:08 AM, Namhyung Kim wrote:
> > > >>>@@ -528,11 +529,16 @@ int main(int argc, const char **argv)
> > > >>>  {
> > > >>>  	const char *cmd;
> > > >>>  	char sbuf[STRERR_BUFSIZE];
> > > >>>+	int min_addr;
> > > >>>
> > > >>>  	/* The page_size is placed in util object. */
> > > >>>  	page_size = sysconf(_SC_PAGE_SIZE);
> > > >>>  	cacheline_size = sysconf(_SC_LEVEL1_DCACHE_LINESIZE);
> > > >>>
> > > >>>+	if (sysctl__read_int("vm/mmap_min_addr", &min_addr) < 0)
> 
> Please put this in that symbol_conf kitchen sink :-)
> 
> I'm unsure though if there would be a reason for having both the local
> min_addr and the one at perf record time, i.e. from perf.data :-\

Otherwise, it can use different value and can treat valid addresses as
invalid especially if the difference is huge.

Btw, I think it should be kernel to cut off these invalid callchains
in the first place, no?


> 
> > > >>This assumes the record and analysis are done on the same system.
> > > >
> > > >Right.  Maybe we can just use minimal size (or page size?) or save and
> > > >pass it through somewhere in the feature bit?
> > > 
> > > no preference, but it should work with cross arch analysis as well (e.g.,
> > > record on arm/ppc and analysis on x86)
> > 
> > I think we should store it in perf.data in features, but seems
> > like a waste to spend one bit just for this number.
> > 
> > I remember commenting on new CPU related FEATURE data, that would contain
> > cpu specific data in extensible form like TAG,VALUE,TAG,VALUE..
> > 
> > but I think the design changed or something, because I cannot find it in now ;-)
> > 
> > maybe we could add something like that
> 
> Right, isn't that one part of the perf/stat patchkit?

I'll take a look.  Jiri, could you give the link if you have it?


Thanks,
Namhyung

[PATCH 3/3] perf callchain: Honor hide_unresolved

[Namhyung Kim]

If user requested to hide unresolved entries, skip unresolved callchains
as well as hist entries.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-report.c | 7 +++----
 tools/perf/util/machine.c   | 5 +++++
 tools/perf/util/symbol.h    | 3 ++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 14428342b47b..8a9c6908f54e 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -45,7 +45,6 @@ struct report {
 	struct perf_tool	tool;
 	struct perf_session	*session;
 	bool			use_tui, use_gtk, use_stdio;
-	bool			hide_unresolved;
 	bool			dont_use_callchains;
 	bool			show_full_info;
 	bool			show_threads;
@@ -146,7 +145,7 @@ static int process_sample_event(struct perf_tool *tool,
 	struct hist_entry_iter iter = {
 		.evsel 			= evsel,
 		.sample 		= sample,
-		.hide_unresolved 	= rep->hide_unresolved,
+		.hide_unresolved 	= symbol_conf.hide_unresolved,
 		.add_entry_cb 		= hist_iter__report_callback,
 	};
 	int ret = 0;
@@ -157,7 +156,7 @@ static int process_sample_event(struct perf_tool *tool,
 		return -1;
 	}
 
-	if (rep->hide_unresolved && al.sym == NULL)
+	if (symbol_conf.hide_unresolved && al.sym == NULL)
 		goto out_put;
 
 	if (rep->cpu_list && !test_bit(sample->cpu, rep->cpu_bitmap))
@@ -740,7 +739,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_STRING_NOEMPTY('t', "field-separator", &symbol_conf.field_sep, "separator",
 		   "separator for columns, no spaces will be added between "
 		   "columns '.' is reserved."),
-	OPT_BOOLEAN('U', "hide-unresolved", &report.hide_unresolved,
+	OPT_BOOLEAN('U', "hide-unresolved", &symbol_conf.hide_unresolved,
 		    "Only display entries resolved to a symbol"),
 	OPT_STRING(0, "symfs", &symbol_conf.symfs, "directory",
 		    "Look for files with symbols relative to this directory"),
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 0a2f35e0d737..9843fe680cb0 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1618,6 +1618,8 @@ static int add_callchain_ip(struct thread *thread,
 		}
 	}
 
+	if (symbol_conf.hide_unresolved && al.sym == NULL)
+		return 0;
 	return callchain_cursor_append(&callchain_cursor, al.addr, al.map, al.sym);
 }
 
@@ -1885,6 +1887,9 @@ static int thread__resolve_callchain_sample(struct thread *thread,
 static int unwind_entry(struct unwind_entry *entry, void *arg)
 {
 	struct callchain_cursor *cursor = arg;
+
+	if (symbol_conf.hide_unresolved && entry->sym == NULL)
+		return 0;
 	return callchain_cursor_append(cursor, entry->ip,
 				       entry->map, entry->sym);
 }
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index dcd786e364f2..857f707ac12b 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -108,7 +108,8 @@ struct symbol_conf {
 			show_hist_headers,
 			branch_callstack,
 			has_filter,
-			show_ref_callgraph;
+			show_ref_callgraph,
+			hide_unresolved;
 	const char	*vmlinux_name,
 			*kallsyms_name,
 			*source_prefix,
-- 
2.6.2

Re: [PATCH 3/3] perf callchain: Honor hide_unresolved

[Jiri Olsa]

On Thu, Nov 26, 2015 at 04:08:20PM +0900, Namhyung Kim wrote:
> If user requested to hide unresolved entries, skip unresolved callchains
> as well as hist entries.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

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

thanks,
jirka

Re: [PATCH 3/3] perf callchain: Honor hide_unresolved

[Arnaldo Carvalho de Melo]

Em Thu, Nov 26, 2015 at 09:46:20AM +0100, Jiri Olsa escreveu:
> On Thu, Nov 26, 2015 at 04:08:20PM +0900, Namhyung Kim wrote:
> > If user requested to hide unresolved entries, skip unresolved callchains
> > as well as hist entries.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Applied.

- Arnaldo

[tip:perf/core] perf callchain: Honor hide_unresolved

[tip-bot for Namhyung Kim]

Commit-ID:  b49a8fe52626814968b9a9d27d7ad1cadc5532ed
Gitweb:     http://git.kernel.org/tip/b49a8fe52626814968b9a9d27d7ad1cadc5532ed
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 26 Nov 2015 16:08:20 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 26 Nov 2015 13:19:39 -0300

perf callchain: Honor hide_unresolved

If user requested to hide unresolved entries, skip unresolved callchains
as well as hist entries.

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: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1448521700-32062-3-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-report.c | 7 +++----
 tools/perf/util/machine.c   | 5 +++++
 tools/perf/util/symbol.h    | 3 ++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 1442834..8a9c690 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -45,7 +45,6 @@ struct report {
 	struct perf_tool	tool;
 	struct perf_session	*session;
 	bool			use_tui, use_gtk, use_stdio;
-	bool			hide_unresolved;
 	bool			dont_use_callchains;
 	bool			show_full_info;
 	bool			show_threads;
@@ -146,7 +145,7 @@ static int process_sample_event(struct perf_tool *tool,
 	struct hist_entry_iter iter = {
 		.evsel 			= evsel,
 		.sample 		= sample,
-		.hide_unresolved 	= rep->hide_unresolved,
+		.hide_unresolved 	= symbol_conf.hide_unresolved,
 		.add_entry_cb 		= hist_iter__report_callback,
 	};
 	int ret = 0;
@@ -157,7 +156,7 @@ static int process_sample_event(struct perf_tool *tool,
 		return -1;
 	}
 
-	if (rep->hide_unresolved && al.sym == NULL)
+	if (symbol_conf.hide_unresolved && al.sym == NULL)
 		goto out_put;
 
 	if (rep->cpu_list && !test_bit(sample->cpu, rep->cpu_bitmap))
@@ -740,7 +739,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_STRING_NOEMPTY('t', "field-separator", &symbol_conf.field_sep, "separator",
 		   "separator for columns, no spaces will be added between "
 		   "columns '.' is reserved."),
-	OPT_BOOLEAN('U', "hide-unresolved", &report.hide_unresolved,
+	OPT_BOOLEAN('U', "hide-unresolved", &symbol_conf.hide_unresolved,
 		    "Only display entries resolved to a symbol"),
 	OPT_STRING(0, "symfs", &symbol_conf.symfs, "directory",
 		    "Look for files with symbols relative to this directory"),
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 7f5071a..f0019b7 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1618,6 +1618,8 @@ static int add_callchain_ip(struct thread *thread,
 		}
 	}
 
+	if (symbol_conf.hide_unresolved && al.sym == NULL)
+		return 0;
 	return callchain_cursor_append(&callchain_cursor, al.addr, al.map, al.sym);
 }
 
@@ -1872,6 +1874,9 @@ check_calls:
 static int unwind_entry(struct unwind_entry *entry, void *arg)
 {
 	struct callchain_cursor *cursor = arg;
+
+	if (symbol_conf.hide_unresolved && entry->sym == NULL)
+		return 0;
 	return callchain_cursor_append(cursor, entry->ip,
 				       entry->map, entry->sym);
 }
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index dcd786e..857f707 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -108,7 +108,8 @@ struct symbol_conf {
 			show_hist_headers,
 			branch_callstack,
 			has_filter,
-			show_ref_callgraph;
+			show_ref_callgraph,
+			hide_unresolved;
 	const char	*vmlinux_name,
 			*kallsyms_name,
 			*source_prefix,

Re: [PATCH 1/3] perf top: Fix freeze on --call-graph flat/folded

[Jiri Olsa]

On Thu, Nov 26, 2015 at 04:08:18PM +0900, Namhyung Kim wrote:
> The callchain rbtree is rebuilt periodically, so it needs to
> reinitialize the root everytime.  Otherwise it can be stuck in the
> rbtree insertion with stale pointers.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/callchain.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index fc3b1e0d09ee..564377d2bebf 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -290,6 +290,7 @@ static void
>  sort_chain_flat(struct rb_root *rb_root, struct callchain_root *root,
>  		u64 min_hit, struct callchain_param *param __maybe_unused)
>  {
> +	*rb_root = RB_ROOT;

it seems ok, but I did not find how this could be called twice?

the only sort I can see is done within:
  __hists__insert_output_entry

do we allow resorting of the callchains?

thanks,
jirka

>  	__sort_chain_flat(rb_root, &root->node, min_hit);
>  }
>  
> -- 
> 2.6.2
> 

Re: [PATCH 1/3] perf top: Fix freeze on --call-graph flat/folded

[Namhyung Kim]

Hi Jiri,

On Thu, Nov 26, 2015 at 09:38:53AM +0100, Jiri Olsa wrote:
> On Thu, Nov 26, 2015 at 04:08:18PM +0900, Namhyung Kim wrote:
> > The callchain rbtree is rebuilt periodically, so it needs to
> > reinitialize the root everytime.  Otherwise it can be stuck in the
> > rbtree insertion with stale pointers.
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/callchain.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > index fc3b1e0d09ee..564377d2bebf 100644
> > --- a/tools/perf/util/callchain.c
> > +++ b/tools/perf/util/callchain.c
> > @@ -290,6 +290,7 @@ static void
> >  sort_chain_flat(struct rb_root *rb_root, struct callchain_root *root,
> >  		u64 min_hit, struct callchain_param *param __maybe_unused)
> >  {
> > +	*rb_root = RB_ROOT;
> 
> it seems ok, but I did not find how this could be called twice?
> 
> the only sort I can see is done within:
>   __hists__insert_output_entry
> 
> do we allow resorting of the callchains?

No, but I think it's possible though.

It's called from perf top's display thread.

  display_thread()
  -> while (!done)
     -> perf_top__print_sym_table()
        -> hists__output_resort()
           -> __hists__insert_output_entry()

Thanks,
Namhyung

Re: [PATCH 1/3] perf top: Fix freeze on --call-graph flat/folded

[Jiri Olsa]

On Thu, Nov 26, 2015 at 10:52:56PM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Thu, Nov 26, 2015 at 09:38:53AM +0100, Jiri Olsa wrote:
> > On Thu, Nov 26, 2015 at 04:08:18PM +0900, Namhyung Kim wrote:
> > > The callchain rbtree is rebuilt periodically, so it needs to
> > > reinitialize the root everytime.  Otherwise it can be stuck in the
> > > rbtree insertion with stale pointers.
> > > 
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/util/callchain.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > > index fc3b1e0d09ee..564377d2bebf 100644
> > > --- a/tools/perf/util/callchain.c
> > > +++ b/tools/perf/util/callchain.c
> > > @@ -290,6 +290,7 @@ static void
> > >  sort_chain_flat(struct rb_root *rb_root, struct callchain_root *root,
> > >  		u64 min_hit, struct callchain_param *param __maybe_unused)
> > >  {
> > > +	*rb_root = RB_ROOT;
> > 
> > it seems ok, but I did not find how this could be called twice?
> > 
> > the only sort I can see is done within:
> >   __hists__insert_output_entry
> > 
> > do we allow resorting of the callchains?
> 
> No, but I think it's possible though.
> 
> It's called from perf top's display thread.

ah right.. top ;-) ok

isn't there analogical issue with the other sorts?
graph_abs, graph_rel... I dont see that code doing this

thanks,
jirka

> 
>   display_thread()
>   -> while (!done)
>      -> perf_top__print_sym_table()
>         -> hists__output_resort()
>            -> __hists__insert_output_entry()
> 
> Thanks,
> Namhyung

Re: [PATCH 1/3] perf top: Fix freeze on --call-graph flat/folded

[Namhyung Kim]

On Thu, Nov 26, 2015 at 03:00:34PM +0100, Jiri Olsa wrote:
> On Thu, Nov 26, 2015 at 10:52:56PM +0900, Namhyung Kim wrote:
> > Hi Jiri,
> > 
> > On Thu, Nov 26, 2015 at 09:38:53AM +0100, Jiri Olsa wrote:
> > > On Thu, Nov 26, 2015 at 04:08:18PM +0900, Namhyung Kim wrote:
> > > > The callchain rbtree is rebuilt periodically, so it needs to
> > > > reinitialize the root everytime.  Otherwise it can be stuck in the
> > > > rbtree insertion with stale pointers.
> > > > 
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > ---
> > > >  tools/perf/util/callchain.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > > > index fc3b1e0d09ee..564377d2bebf 100644
> > > > --- a/tools/perf/util/callchain.c
> > > > +++ b/tools/perf/util/callchain.c
> > > > @@ -290,6 +290,7 @@ static void
> > > >  sort_chain_flat(struct rb_root *rb_root, struct callchain_root *root,
> > > >  		u64 min_hit, struct callchain_param *param __maybe_unused)
> > > >  {
> > > > +	*rb_root = RB_ROOT;
> > > 
> > > it seems ok, but I did not find how this could be called twice?
> > > 
> > > the only sort I can see is done within:
> > >   __hists__insert_output_entry
> > > 
> > > do we allow resorting of the callchains?
> > 
> > No, but I think it's possible though.
> > 
> > It's called from perf top's display thread.
> 
> ah right.. top ;-) ok
> 
> isn't there analogical issue with the other sorts?
> graph_abs, graph_rel... I dont see that code doing this

The sort_chain_graph_abs/rel() already do this.  Unlike flat/folded
callchains, they put child into each callchain_node's rbtree.  So they
reinitialize node->rb_root in __sort_chain_graph_abs/rel() and reset
rb_root after finishing sort.

Thanks,
Namhyung

Re: [PATCH 1/3] perf top: Fix freeze on --call-graph flat/folded

[Jiri Olsa]

On Fri, Nov 27, 2015 at 12:10:31AM +0900, Namhyung Kim wrote:
> On Thu, Nov 26, 2015 at 03:00:34PM +0100, Jiri Olsa wrote:
> > On Thu, Nov 26, 2015 at 10:52:56PM +0900, Namhyung Kim wrote:
> > > Hi Jiri,
> > > 
> > > On Thu, Nov 26, 2015 at 09:38:53AM +0100, Jiri Olsa wrote:
> > > > On Thu, Nov 26, 2015 at 04:08:18PM +0900, Namhyung Kim wrote:
> > > > > The callchain rbtree is rebuilt periodically, so it needs to
> > > > > reinitialize the root everytime.  Otherwise it can be stuck in the
> > > > > rbtree insertion with stale pointers.
> > > > > 
> > > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > > ---
> > > > >  tools/perf/util/callchain.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > > > > index fc3b1e0d09ee..564377d2bebf 100644
> > > > > --- a/tools/perf/util/callchain.c
> > > > > +++ b/tools/perf/util/callchain.c
> > > > > @@ -290,6 +290,7 @@ static void
> > > > >  sort_chain_flat(struct rb_root *rb_root, struct callchain_root *root,
> > > > >  		u64 min_hit, struct callchain_param *param __maybe_unused)
> > > > >  {
> > > > > +	*rb_root = RB_ROOT;
> > > > 
> > > > it seems ok, but I did not find how this could be called twice?
> > > > 
> > > > the only sort I can see is done within:
> > > >   __hists__insert_output_entry
> > > > 
> > > > do we allow resorting of the callchains?
> > > 
> > > No, but I think it's possible though.
> > > 
> > > It's called from perf top's display thread.
> > 
> > ah right.. top ;-) ok
> > 
> > isn't there analogical issue with the other sorts?
> > graph_abs, graph_rel... I dont see that code doing this
> 
> The sort_chain_graph_abs/rel() already do this.  Unlike flat/folded
> callchains, they put child into each callchain_node's rbtree.  So they
> reinitialize node->rb_root in __sort_chain_graph_abs/rel() and reset
> rb_root after finishing sort.

ook, thanks for explanation

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

jirka

Re: [PATCH 1/3] perf top: Fix freeze on --call-graph flat/folded

[Arnaldo Carvalho de Melo]

Em Thu, Nov 26, 2015 at 04:08:18PM +0900, Namhyung Kim escreveu:
> The callchain rbtree is rebuilt periodically, so it needs to
> reinitialize the root everytime.  Otherwise it can be stuck in the
> rbtree insertion with stale pointers.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Applied.

- Arnaldo

[tip:perf/core] perf top: Fix freeze on --call-graph flat/folded

[tip-bot for Namhyung Kim]

Commit-ID:  0356218a68551f051998f4fb5074a1eed7a346fe
Gitweb:     http://git.kernel.org/tip/0356218a68551f051998f4fb5074a1eed7a346fe
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 26 Nov 2015 16:08:18 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 26 Nov 2015 13:32:08 -0300

perf top: Fix freeze on --call-graph flat/folded

The callchain rbtree is rebuilt periodically, so it needs to
reinitialize the root everytime.  Otherwise it can be stuck in the
rbtree insertion with stale pointers.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1448521700-32062-1-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/callchain.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index fc3b1e0..564377d 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -290,6 +290,7 @@ static void
 sort_chain_flat(struct rb_root *rb_root, struct callchain_root *root,
 		u64 min_hit, struct callchain_param *param __maybe_unused)
 {
+	*rb_root = RB_ROOT;
 	__sort_chain_flat(rb_root, &root->node, min_hit);
 }