[PATCH] perf c2c report: Fix crash for empty browser

[PATCH] perf c2c report: Fix crash for empty browser

[Jiri Olsa]

Do not try to display entry details if there's
not any. Currently this ends up in crash:
  $ perf c2c report
  perf: Segmentation fault

Reported-by: rodia@autistici.org
Link: http://lkml.kernel.org/n/tip-3d7qjz9x49ay9ncerfordcv3@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-c2c.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index f2ea85ee573f..f3aa9d02a5ab 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2349,6 +2349,9 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
 	" s             Toggle full length of symbol and source line columns \n"
 	" q             Return back to cacheline list \n";
 
+	if (!he)
+		return 0;
+
 	/* Display compact version first. */
 	c2c.symbol_full = false;
 
-- 
2.17.1

Re: [PATCH] perf c2c report: Fix crash for empty browser

[Arnaldo Carvalho de Melo]

Em Tue, Jul 24, 2018 at 08:20:08AM +0200, Jiri Olsa escreveu:
> Do not try to display entry details if there's
> not any. Currently this ends up in crash:
>   $ perf c2c report
>   perf: Segmentation fault

How to replicate this?

I tried:

$ perf record sleep 1
$ perf c2c report

But it didn't segfault

 
> Reported-by: rodia@autistici.org
> Link: http://lkml.kernel.org/n/tip-3d7qjz9x49ay9ncerfordcv3@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/builtin-c2c.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index f2ea85ee573f..f3aa9d02a5ab 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -2349,6 +2349,9 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
>  	" s             Toggle full length of symbol and source line columns \n"
>  	" q             Return back to cacheline list \n";
>  
> +	if (!he)
> +		return 0;
> +
>  	/* Display compact version first. */
>  	c2c.symbol_full = false;
>  
> -- 
> 2.17.1

Re: [PATCH] perf c2c report: Fix crash for empty browser

[rodia]

On 2018-07-26 19:30, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jul 24, 2018 at 08:20:08AM +0200, Jiri Olsa escreveu:
>> Do not try to display entry details if there's
>> not any. Currently this ends up in crash:
>>   $ perf c2c report
>>   perf: Segmentation fault
> 
> How to replicate this?
> 
> I tried:
> 
> $ perf record sleep 1
> $ perf c2c report
> 
> But it didn't segfault

Similarly I have tried :
$ perf record sleep 1
$ perf c2c report
Then Press `d` to show the cache-line contents.
This replies the segfault on my machine (4.17.8-1).
The patch mentioned above should solve it, even tough I am not sure as I 
haven't been able to recompile the kernel.

The segfault by itself seems to be due to the report logic, as it did 
not expect to report on an empty browser.
What has stepped me back is that application which I have been testing 
with rely on multiple threads instantiated through pthread, which should 
be counted in user-level threads right? But they still seem to return an 
empty browser.

When instead c2c is runned system-wide, with an application running on 
multiple threads like firefox or julia, cache hits are measured and also 
they are traced back in the source code.

Anyway, thanks for your support!

Re: [PATCH] perf c2c report: Fix crash for empty browser

[Jiri Olsa]

On Thu, Jul 26, 2018 at 11:31:34PM +0000, rodia@autistici.org wrote:
> On 2018-07-26 19:30, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jul 24, 2018 at 08:20:08AM +0200, Jiri Olsa escreveu:
> > > Do not try to display entry details if there's
> > > not any. Currently this ends up in crash:
> > >   $ perf c2c report
> > >   perf: Segmentation fault
> > 
> > How to replicate this?
> > 
> > I tried:
> > 
> > $ perf record sleep 1
> > $ perf c2c report
> > 
> > But it didn't segfault
> 
> Similarly I have tried :
> $ perf record sleep 1
> $ perf c2c report
> Then Press `d` to show the cache-line contents.

yep, sry I forgot to mention you need to press the 'd' to show details


> This replies the segfault on my machine (4.17.8-1).
> The patch mentioned above should solve it, even tough I am not sure as I
> haven't been able to recompile the kernel.

no need to recompile kernel

> 
> The segfault by itself seems to be due to the report logic, as it did not
> expect to report on an empty browser.
> What has stepped me back is that application which I have been testing with
> rely on multiple threads instantiated through pthread, which should be
> counted in user-level threads right? But they still seem to return an empty
> browser.

right, c2c scans read/write accesses and tries to find false sharing
cases maybe there was nothing to be found

> When instead c2c is runned system-wide, with an application running on
> multiple threads like firefox or julia, cache hits are measured and also
> they are traced back in the source code.

I got a cache line (attached) for 'perf bench sched messaging'
NOT being traced system wide and just for user (you'll get plenty
of detected cachelines in kernel space):

jirka


---
[root@krava perf]# ./perf c2c record --all-user -- ./perf bench sched messaging -l 100000
[root@krava perf]# ./perf c2c report --stdio

=================================================
           Shared Data Cache Line Table          
=================================================
#
#        ----------- Cacheline ----------    Total      Tot  ----- LLC Load Hitm -----  ---- Store Reference ----  --- Load Dram ----      LLC    Total  ----- Core Load Hit -----  -- LLC Load Hit --
# Index             Address  Node  PA cnt  records     Hitm    Total      Lcl      Rmt    Total    L1Hit   L1Miss       Lcl       Rmt  Ld Miss    Loads       FB       L1       L2       Llc       Rmt
# .....  ..................  ....  ......  .......  .......  .......  .......  .......  .......  .......  .......  ........  ........  .......  .......  .......  .......  .......  ........  ........
#
      0      0x7fff5b729cc0     0       1       44  100.00%        1        1        0       21       21        0         2         0        2       23        0        0        9        11         0

=================================================
      Shared Cache Line Distribution Pareto      
=================================================
#
#        ----- HITM -----  -- Store Refs --  --------- Data address ---------                               ---------- cycles ----------    Total       cpu                               Shared                             
#   Num      Rmt      Lcl   L1 Hit  L1 Miss              Offset  Node  PA cnt      Pid        Code address  rmt hitm  lcl hitm      load  records       cnt           Symbol              Object            Source:Line  Node
# .....  .......  .......  .......  .......  ..................  ....  ......  .......  ..................  ........  ........  ........  .......  ........  ...............  ..................  .....................  ....
#
  -------------------------------------------------------------
      0        0        1       21        0      0x7fff5b729cc0
  -------------------------------------------------------------
           0.00%  100.00%    0.00%    0.00%                0x38     0       1    17356      0x7febaf7e1a46         0       142       101       23         4  [.] __libc_read  libpthread-2.27.so  read.c:28               0
           0.00%    0.00%  100.00%    0.00%                0x38     0       1    17356            0x494e4e         0         0         0       21         4  [.] receiver     perf                sched-messaging.c:129   0

Re: [PATCH] perf c2c report: Fix crash for empty browser

[Arnaldo Carvalho de Melo]

Adding Joe to the CC list.

Em Fri, Jul 27, 2018 at 09:06:23AM +0200, Jiri Olsa escreveu:
> On Thu, Jul 26, 2018 at 11:31:34PM +0000, rodia@autistici.org wrote:
> > On 2018-07-26 19:30, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Jul 24, 2018 at 08:20:08AM +0200, Jiri Olsa escreveu:
> > > > Do not try to display entry details if there's
> > > > not any. Currently this ends up in crash:
> > > >   $ perf c2c report
> > > >   perf: Segmentation fault

> > > How to replicate this?

> > > I tried:

> > > $ perf record sleep 1
> > > $ perf c2c report

> > > But it didn't segfault

> > Similarly I have tried :
> > $ perf record sleep 1
> > $ perf c2c report
> > Then Press `d` to show the cache-line contents.

> yep, sry I forgot to mention you need to press the 'd' to show details
 
> > This replies the segfault on my machine (4.17.8-1).
> > The patch mentioned above should solve it, even tough I am not sure as I
> > haven't been able to recompile the kernel.
 
> no need to recompile kernel
 
> > The segfault by itself seems to be due to the report logic, as it did not
> > expect to report on an empty browser.
> > What has stepped me back is that application which I have been testing with
> > rely on multiple threads instantiated through pthread, which should be
> > counted in user-level threads right? But they still seem to return an empty
> > browser.
 
> right, c2c scans read/write accesses and tries to find false sharing
> cases maybe there was nothing to be found
 
> > When instead c2c is runned system-wide, with an application running on
> > multiple threads like firefox or julia, cache hits are measured and also
> > they are traced back in the source code.
 
> I got a cache line (attached) for 'perf bench sched messaging'
> NOT being traced system wide and just for user (you'll get plenty
> of detected cachelines in kernel space):

With that info in mind, we get:

[root@seventh ~]# perf record sleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.001 MB perf.data (6 samples) ]
[root@seventh ~]# 
[root@seventh ~]# 
[root@seventh ~]# perf c2c report # and press 'd'
perf: Segmentation fault
-------- backtrace --------
perf[0x5b1d2a]
/lib64/libc.so.6(+0x346df)[0x7fcb566e36df]
perf[0x46fcae]
perf[0x4a9f1e]
perf[0x4aa220]
perf(main+0x301)[0x42c561]
/lib64/libc.so.6(__libc_start_main+0xe9)[0x7fcb566cff29]
perf(_start+0x29)[0x42c999]
[root@seventh ~]#

With your patches the segfault is gone, but I'd do a follow up patch to
show some message telling the user why 'd' showed nothing and
instructing him about what is missing, i.e. is this done on a perf.data
file that has no events of interest? Suggest using 'perf c2c record' or
'perf record -e events,of,interest,to,perf,c2c', was this done on some
workload where no false sharing was detected? Say so, etc.

I applied your patch with a more detailed commit log to state how this
can reproduced, etc, as usual:

https://git.kernel.org/acme/c/983eb6aa7098

- Arnaldo

Re: [PATCH] perf c2c report: Fix crash for empty browser

[Jiri Olsa]

On Fri, Jul 27, 2018 at 11:13:40AM -0300, Arnaldo Carvalho de Melo wrote:
> Adding Joe to the CC list.
> 
> Em Fri, Jul 27, 2018 at 09:06:23AM +0200, Jiri Olsa escreveu:
> > On Thu, Jul 26, 2018 at 11:31:34PM +0000, rodia@autistici.org wrote:
> > > On 2018-07-26 19:30, Arnaldo Carvalho de Melo wrote:
> > > > Em Tue, Jul 24, 2018 at 08:20:08AM +0200, Jiri Olsa escreveu:
> > > > > Do not try to display entry details if there's
> > > > > not any. Currently this ends up in crash:
> > > > >   $ perf c2c report
> > > > >   perf: Segmentation fault
> 
> > > > How to replicate this?
> 
> > > > I tried:
> 
> > > > $ perf record sleep 1
> > > > $ perf c2c report
> 
> > > > But it didn't segfault
> 
> > > Similarly I have tried :
> > > $ perf record sleep 1
> > > $ perf c2c report
> > > Then Press `d` to show the cache-line contents.
> 
> > yep, sry I forgot to mention you need to press the 'd' to show details
>  
> > > This replies the segfault on my machine (4.17.8-1).
> > > The patch mentioned above should solve it, even tough I am not sure as I
> > > haven't been able to recompile the kernel.
>  
> > no need to recompile kernel
>  
> > > The segfault by itself seems to be due to the report logic, as it did not
> > > expect to report on an empty browser.
> > > What has stepped me back is that application which I have been testing with
> > > rely on multiple threads instantiated through pthread, which should be
> > > counted in user-level threads right? But they still seem to return an empty
> > > browser.
>  
> > right, c2c scans read/write accesses and tries to find false sharing
> > cases maybe there was nothing to be found
>  
> > > When instead c2c is runned system-wide, with an application running on
> > > multiple threads like firefox or julia, cache hits are measured and also
> > > they are traced back in the source code.
>  
> > I got a cache line (attached) for 'perf bench sched messaging'
> > NOT being traced system wide and just for user (you'll get plenty
> > of detected cachelines in kernel space):
> 
> With that info in mind, we get:
> 
> [root@seventh ~]# perf record sleep 1
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.001 MB perf.data (6 samples) ]
> [root@seventh ~]# 
> [root@seventh ~]# 
> [root@seventh ~]# perf c2c report # and press 'd'
> perf: Segmentation fault
> -------- backtrace --------
> perf[0x5b1d2a]
> /lib64/libc.so.6(+0x346df)[0x7fcb566e36df]
> perf[0x46fcae]
> perf[0x4a9f1e]
> perf[0x4aa220]
> perf(main+0x301)[0x42c561]
> /lib64/libc.so.6(__libc_start_main+0xe9)[0x7fcb566cff29]
> perf(_start+0x29)[0x42c999]
> [root@seventh ~]#
> 
> With your patches the segfault is gone, but I'd do a follow up patch to
> show some message telling the user why 'd' showed nothing and
> instructing him about what is missing, i.e. is this done on a perf.data
> file that has no events of interest? Suggest using 'perf c2c record' or
> 'perf record -e events,of,interest,to,perf,c2c', was this done on some
> workload where no false sharing was detected? Say so, etc.

ok, will try to come up with something

> 
> I applied your patch with a more detailed commit log to state how this
> can reproduced, etc, as usual:
> 
> https://git.kernel.org/acme/c/983eb6aa7098

I checked, looks good, thanks

jirka

[tip:perf/core] perf c2c report: Fix crash for empty browser

[tip-bot for Jiri Olsa]

Commit-ID:  73978332572ccf5e364c31e9a70ba953f8202b46
Gitweb:     https://git.kernel.org/tip/73978332572ccf5e364c31e9a70ba953f8202b46
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Tue, 24 Jul 2018 08:20:08 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 31 Jul 2018 10:53:20 -0300

perf c2c report: Fix crash for empty browser

'perf c2c' scans read/write accesses and tries to find false sharing
cases, so when the events it wants were not asked for or ended up not
taking place, we get no histograms.

So do not try to display entry details if there's not any. Currently
this ends up in crash:

  $ perf c2c report # then press 'd'
  perf: Segmentation fault
  $

Committer testing:

Before:

Record a perf.data file without events of interest to 'perf c2c report',
then call it and press 'd':

  # perf record sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.001 MB perf.data (6 samples) ]
  # perf c2c report
  perf: Segmentation fault
  -------- backtrace --------
  perf[0x5b1d2a]
  /lib64/libc.so.6(+0x346df)[0x7fcb566e36df]
  perf[0x46fcae]
  perf[0x4a9f1e]
  perf[0x4aa220]
  perf(main+0x301)[0x42c561]
  /lib64/libc.so.6(__libc_start_main+0xe9)[0x7fcb566cff29]
  perf(_start+0x29)[0x42c999]
  #

After the patch the segfault doesn't take place, a follow up patch to
tell the user why nothing changes when 'd' is pressed would be good.

Reported-by: rodia@autistici.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Joe Mario <jmario@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Fixes: f1c5fd4d0bb9 ("perf c2c report: Add TUI cacheline browser")
Link: http://lkml.kernel.org/r/20180724062008.26126-1-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-c2c.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index f2ea85ee573f..f3aa9d02a5ab 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2349,6 +2349,9 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
 	" s             Toggle full length of symbol and source line columns \n"
 	" q             Return back to cacheline list \n";
 
+	if (!he)
+		return 0;
+
 	/* Display compact version first. */
 	c2c.symbol_full = false;