linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] perf/report: Report OOM in perf report status line
@ 2019-04-23 10:53 Thomas Richter
  2019-04-26 14:09 ` Arnaldo Carvalho de Melo
  2019-05-03  5:55 ` [tip:perf/urgent] perf report: Report OOM in status line in the GTK UI tip-bot for Thomas Richter
  0 siblings, 2 replies; 9+ messages in thread
From: Thomas Richter @ 2019-04-23 10:53 UTC (permalink / raw)
  To: linux-kernel, linux-perf-users, acme
  Cc: brueckner, schwidefsky, heiko.carstens, jolsa, Thomas Richter

An -ENOMEM error is not reported in the GTK GUI.
Instead this error message pops up on the screen:

[root@m35lp76 perf]# ./perf  report -i perf.data.error68-1

	Processing events... [974K/3M]
	Error:failed to process sample

	0xf4198 [0x8]: failed to process type: 68

However when I use the same perf.data file with --stdio it works:

[root@m35lp76 perf]# ./perf  report -i perf.data.error68-1 --stdio \
		| head -12

  # Total Lost Samples: 0
  #
  # Samples: 76K of event 'cycles'
  # Event count (approx.): 99056160000
  #
  # Overhead  Command          Shared Object      Symbol
  # ........  ...............  .................  .........
  #
     8.81%  find             [kernel.kallsyms]  [k] ftrace_likely_update
     8.74%  swapper          [kernel.kallsyms]  [k] ftrace_likely_update
     8.34%  sshd             [kernel.kallsyms]  [k] ftrace_likely_update
     2.19%  kworker/u512:1-  [kernel.kallsyms]  [k] ftrace_likely_update

The sample precentage is a bit low.....

The GUI always fails in the FINISHED_ROUND event (68) and does not
indicate the reason why.

When happened is the following. Perf report calls a lot of functions and
down deep when a FINISHED_ROUND event is processed, these functions are
called:

  perf_session__process_event()
  + perf_session__process_user_event()
    + process_finished_round()
      + ordered_events__flush()
        + __ordered_events__flush()
	  + do_flush()
	    + ordered_events__deliver_event()
	      + perf_session__deliver_event()
	        + machine__deliver_event()
	          + perf_evlist__deliver_event()
	            + process_sample_event()
	              + hist_entry_iter_add() --> only called in GUI case!!!
	                + hist_iter__report__callback()
	                  + symbol__inc_addr_sample()

	                    Now this functions runs out of memory and
			    returns -ENOMEM. This is reported all the way up
			    until function

perf_session__process_event() returns to its caller, where -ENOMEM is
changed to -EINVAL and processing stops:

 if ((skip = perf_session__process_event(session, event, head)) < 0) {
      pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
	     head, event->header.size, event->header.type);
      err = -EINVAL;
      goto out_err;
 }

This occurred in the FINISHED_ROUND event when it has to process some
10000 entries and ran out of memory.

This patch indicates the root cause and displays it in the status line
of ther perf report GUI.

Output before (on GUI status line):
0xf4198 [0x8]: failed to process type: 68

Output after:
0xf4198 [0x8]: failed to process type: 68 [not enough memory]

Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Reviewed-by: Jiri Olsa <jolsa@redhat.com>
Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>
---
 tools/perf/util/session.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index b17f1c9bc965..e89716175588 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1930,10 +1930,10 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 
 	if (size < sizeof(struct perf_event_header) ||
 	    (skip = rd->process(session, event, file_pos)) < 0) {
-		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
+		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
 		       file_offset + head, event->header.size,
-		       event->header.type);
-		err = -EINVAL;
+		       event->header.type, strerror(-skip));
+		err = skip;
 		goto out;
 	}
 
-- 
2.16.4


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

* Re: [PATCH] perf/report: Report OOM in perf report status line
  2019-04-23 10:53 [PATCH] perf/report: Report OOM in perf report status line Thomas Richter
@ 2019-04-26 14:09 ` Arnaldo Carvalho de Melo
  2019-04-26 14:10   ` Arnaldo Carvalho de Melo
  2019-05-03  5:55 ` [tip:perf/urgent] perf report: Report OOM in status line in the GTK UI tip-bot for Thomas Richter
  1 sibling, 1 reply; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-04-26 14:09 UTC (permalink / raw)
  To: Thomas Richter
  Cc: linux-kernel, linux-perf-users, brueckner, schwidefsky,
	heiko.carstens, jolsa

Em Tue, Apr 23, 2019 at 12:53:03PM +0200, Thomas Richter escreveu:
> An -ENOMEM error is not reported in the GTK GUI.
> Instead this error message pops up on the screen:
> 
> [root@m35lp76 perf]# ./perf  report -i perf.data.error68-1
> 
> 	Processing events... [974K/3M]
> 	Error:failed to process sample
> 
> 	0xf4198 [0x8]: failed to process type: 68

Thanks, applied to perf/urgent.

- Arnaldo

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

* Re: [PATCH] perf/report: Report OOM in perf report status line
  2019-04-26 14:09 ` Arnaldo Carvalho de Melo
@ 2019-04-26 14:10   ` Arnaldo Carvalho de Melo
  2019-04-26 14:16     ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-04-26 14:10 UTC (permalink / raw)
  To: Thomas Richter
  Cc: linux-kernel, linux-perf-users, brueckner, schwidefsky,
	heiko.carstens, jolsa

Em Fri, Apr 26, 2019 at 11:09:07AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Apr 23, 2019 at 12:53:03PM +0200, Thomas Richter escreveu:
> > An -ENOMEM error is not reported in the GTK GUI.
> > Instead this error message pops up on the screen:
> > 
> > [root@m35lp76 perf]# ./perf  report -i perf.data.error68-1
> > 
> > 	Processing events... [974K/3M]
> > 	Error:failed to process sample
> > 
> > 	0xf4198 [0x8]: failed to process type: 68
> 
> Thanks, applied to perf/urgent.

Well, I tried to, now trying to fix this...


[acme@quaco perf]$ m
make: Entering directory '/home/acme/git/perf/tools/perf'
  BUILD:   Doing 'make -j8' parallel build
  INSTALL  GTK UI
  INSTALL  trace_plugins
  CC       /tmp/build/perf/util/session.o
util/session.c: In function ‘perf_session__process_events’:
util/session.c:1936:7: error: ‘skip’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
   err = skip;
   ~~~~^~~~~~
util/session.c:1874:6: note: ‘skip’ was declared here
  s64 skip;
      ^~~~
cc1: all warnings being treated as errors
mv: cannot stat '/tmp/build/perf/util/.session.o.tmp': No such file or directory
make[4]: *** [/home/acme/git/perf/tools/build/Makefile.build:96: /tmp/build/perf/util/session.o] Error 1
make[3]: *** [/home/acme/git/perf/tools/build/Makefile.build:139: util] Error 2
make[2]: *** [Makefile.perf:559: /tmp/build/perf/perf-in.o] Error 2
make[1]: *** [Makefile.perf:215: sub-make] Error 2
make: *** [Makefile:110: install-bin] Error 2
make: Leaving directory '/home/acme/git/perf/tools/perf'

 Performance counter stats for 'make -k O=/tmp/build/perf -C tools/perf install-bin':

     4,913,757,758      cycles:u
     7,432,572,002      instructions:u            #    1.51  insn per cycle

       1.345541349 seconds time elapsed

       1.412713000 seconds user
       0.572540000 seconds sys


[acme@quaco perf]$

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

* Re: [PATCH] perf/report: Report OOM in perf report status line
  2019-04-26 14:10   ` Arnaldo Carvalho de Melo
@ 2019-04-26 14:16     ` Arnaldo Carvalho de Melo
  2019-04-26 14:44       ` Jiri Olsa
  0 siblings, 1 reply; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-04-26 14:16 UTC (permalink / raw)
  To: Thomas Richter
  Cc: linux-kernel, linux-perf-users, brueckner, schwidefsky,
	heiko.carstens, jolsa

Em Fri, Apr 26, 2019 at 11:10:32AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Apr 26, 2019 at 11:09:07AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Apr 23, 2019 at 12:53:03PM +0200, Thomas Richter escreveu:
> > > An -ENOMEM error is not reported in the GTK GUI.
> > > Instead this error message pops up on the screen:
> > > 
> > > [root@m35lp76 perf]# ./perf  report -i perf.data.error68-1
> > > 
> > > 	Processing events... [974K/3M]
> > > 	Error:failed to process sample
> > > 
> > > 	0xf4198 [0x8]: failed to process type: 68
> > 
> > Thanks, applied to perf/urgent.
> 
> Well, I tried to, now trying to fix this...
> 
> 
> [acme@quaco perf]$ m
> make: Entering directory '/home/acme/git/perf/tools/perf'
>   BUILD:   Doing 'make -j8' parallel build
>   INSTALL  GTK UI
>   INSTALL  trace_plugins
>   CC       /tmp/build/perf/util/session.o
> util/session.c: In function ‘perf_session__process_events’:
> util/session.c:1936:7: error: ‘skip’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    err = skip;
>    ~~~~^~~~~~
> util/session.c:1874:6: note: ‘skip’ was declared here
>   s64 skip;
>       ^~~~
> cc1: all warnings being treated as errors
> mv: cannot stat '/tmp/build/perf/util/.session.o.tmp': No such file or directory
> make[4]: *** [/home/acme/git/perf/tools/build/Makefile.build:96: /tmp/build/perf/util/session.o] Error 1
> make[3]: *** [/home/acme/git/perf/tools/build/Makefile.build:139: util] Error 2
> make[2]: *** [Makefile.perf:559: /tmp/build/perf/perf-in.o] Error 2
> make[1]: *** [Makefile.perf:215: sub-make] Error 2
> make: *** [Makefile:110: install-bin] Error 2
> make: Leaving directory '/home/acme/git/perf/tools/perf'

So, here is your patch:

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index b17f1c9bc965..e89716175588 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1930,10 +1930,10 @@ reader__process_events(struct reader *rd, struct perf_session *session,

        if (size < sizeof(struct perf_event_header) ||
            (skip = rd->process(session, event, file_pos)) < 0) {
-               pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
+               pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
                       file_offset + head, event->header.size,
-                      event->header.type);
-               err = -EINVAL;
+                      event->header.type, strerror(-skip));
+               err = skip;
                goto out;
        }

[acme@quaco perf]$

What happens if (size < sizeof(struct perf_event_header)) is true? size
will have an undefined value, so the right thing is to have this patch
on top of yours, so that err get, as before, set to -EINVAL when the
size is less than the perf_event_header sizeof:

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index e89716175588..bad5f87ae001 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1928,6 +1928,8 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 
 	size = event->header.size;
 
+	skip = -EINVAL;
+
 	if (size < sizeof(struct perf_event_header) ||
 	    (skip = rd->process(session, event, file_pos)) < 0) {
 		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",

---

With two Reviewed-by tags, I jumped to quickly at applying, please
compile test next time guys ;-) :-)

- Arnaldo

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

* Re: [PATCH] perf/report: Report OOM in perf report status line
  2019-04-26 14:16     ` Arnaldo Carvalho de Melo
@ 2019-04-26 14:44       ` Jiri Olsa
  2019-04-26 15:12         ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 9+ messages in thread
From: Jiri Olsa @ 2019-04-26 14:44 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Thomas Richter, linux-kernel, linux-perf-users, brueckner,
	schwidefsky, heiko.carstens

On Fri, Apr 26, 2019 at 11:16:07AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > util/session.c: In function ‘perf_session__process_events’:
> > util/session.c:1936:7: error: ‘skip’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >    err = skip;
> >    ~~~~^~~~~~
> > util/session.c:1874:6: note: ‘skip’ was declared here
> >   s64 skip;
> >       ^~~~
> > cc1: all warnings being treated as errors
> > mv: cannot stat '/tmp/build/perf/util/.session.o.tmp': No such file or directory
> > make[4]: *** [/home/acme/git/perf/tools/build/Makefile.build:96: /tmp/build/perf/util/session.o] Error 1
> > make[3]: *** [/home/acme/git/perf/tools/build/Makefile.build:139: util] Error 2
> > make[2]: *** [Makefile.perf:559: /tmp/build/perf/perf-in.o] Error 2
> > make[1]: *** [Makefile.perf:215: sub-make] Error 2
> > make: *** [Makefile:110: install-bin] Error 2
> > make: Leaving directory '/home/acme/git/perf/tools/perf'
> 
> So, here is your patch:
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index b17f1c9bc965..e89716175588 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1930,10 +1930,10 @@ reader__process_events(struct reader *rd, struct perf_session *session,
> 
>         if (size < sizeof(struct perf_event_header) ||
>             (skip = rd->process(session, event, file_pos)) < 0) {
> -               pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
> +               pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
>                        file_offset + head, event->header.size,
> -                      event->header.type);
> -               err = -EINVAL;
> +                      event->header.type, strerror(-skip));
> +               err = skip;
>                 goto out;
>         }
> 
> [acme@quaco perf]$
> 
> What happens if (size < sizeof(struct perf_event_header)) is true? size
> will have an undefined value, so the right thing is to have this patch
> on top of yours, so that err get, as before, set to -EINVAL when the
> size is less than the perf_event_header sizeof:

I'd think you need to squash your change with the original patch
for the bisecting sake, right?

> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index e89716175588..bad5f87ae001 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1928,6 +1928,8 @@ reader__process_events(struct reader *rd, struct perf_session *session,
>  
>  	size = event->header.size;
>  
> +	skip = -EINVAL;
> +
>  	if (size < sizeof(struct perf_event_header) ||
>  	    (skip = rd->process(session, event, file_pos)) < 0) {
>  		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
> 
> ---
> 
> With two Reviewed-by tags, I jumped to quickly at applying, please
> compile test next time guys ;-) :-)

ugh.. need to slow down, this is happening too often now :-\

jirka

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

* Re: [PATCH] perf/report: Report OOM in perf report status line
  2019-04-26 14:44       ` Jiri Olsa
@ 2019-04-26 15:12         ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-04-26 15:12 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Thomas Richter, linux-kernel,
	linux-perf-users, brueckner, schwidefsky, heiko.carstens

Em Fri, Apr 26, 2019 at 04:44:06PM +0200, Jiri Olsa escreveu:
> On Fri, Apr 26, 2019 at 11:16:07AM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > > util/session.c: In function ‘perf_session__process_events’:
> > > util/session.c:1936:7: error: ‘skip’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >    err = skip;
> > >    ~~~~^~~~~~
> > > util/session.c:1874:6: note: ‘skip’ was declared here
> > >   s64 skip;
> > >       ^~~~
> > > cc1: all warnings being treated as errors
> > > mv: cannot stat '/tmp/build/perf/util/.session.o.tmp': No such file or directory
> > > make[4]: *** [/home/acme/git/perf/tools/build/Makefile.build:96: /tmp/build/perf/util/session.o] Error 1
> > > make[3]: *** [/home/acme/git/perf/tools/build/Makefile.build:139: util] Error 2
> > > make[2]: *** [Makefile.perf:559: /tmp/build/perf/perf-in.o] Error 2
> > > make[1]: *** [Makefile.perf:215: sub-make] Error 2
> > > make: *** [Makefile:110: install-bin] Error 2
> > > make: Leaving directory '/home/acme/git/perf/tools/perf'
> > 
> > So, here is your patch:
> > 
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index b17f1c9bc965..e89716175588 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -1930,10 +1930,10 @@ reader__process_events(struct reader *rd, struct perf_session *session,
> > 
> >         if (size < sizeof(struct perf_event_header) ||
> >             (skip = rd->process(session, event, file_pos)) < 0) {
> > -               pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
> > +               pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
> >                        file_offset + head, event->header.size,
> > -                      event->header.type);
> > -               err = -EINVAL;
> > +                      event->header.type, strerror(-skip));
> > +               err = skip;
> >                 goto out;
> >         }
> > 
> > [acme@quaco perf]$
> > 
> > What happens if (size < sizeof(struct perf_event_header)) is true? size
> > will have an undefined value, so the right thing is to have this patch
> > on top of yours, so that err get, as before, set to -EINVAL when the
> > size is less than the perf_event_header sizeof:
> 
> I'd think you need to squash your change with the original patch
> for the bisecting sake, right?

Yes, that is what I did.
 
> > +++ b/tools/perf/util/session.c
> > @@ -1928,6 +1928,8 @@ reader__process_events(struct reader *rd, struct perf_session *session,
> >  
> >  	size = event->header.size;
> >  
> > +	skip = -EINVAL;
> > +
> >  	if (size < sizeof(struct perf_event_header) ||
> >  	    (skip = rd->process(session, event, file_pos)) < 0) {
> >  		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
> > 
> > ---
> > 
> > With two Reviewed-by tags, I jumped to quickly at applying, please
> > compile test next time guys ;-) :-)
 
> ugh.. need to slow down, this is happening too often now :-\

Right, I'll test build everything, and I do like reviewers, who don't?
Its just that when I see reviewed-by tags I end up relaxing a bit :-)

- Arnaldo

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

* [tip:perf/urgent] perf report: Report OOM in status line in the GTK UI
  2019-04-23 10:53 [PATCH] perf/report: Report OOM in perf report status line Thomas Richter
  2019-04-26 14:09 ` Arnaldo Carvalho de Melo
@ 2019-05-03  5:55 ` tip-bot for Thomas Richter
  1 sibling, 0 replies; 9+ messages in thread
From: tip-bot for Thomas Richter @ 2019-05-03  5:55 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, jolsa, tglx, hpa, schwidefsky, mingo, acme,
	heiko.carstens, brueckner, tmricht

Commit-ID:  167e418fa0871c083e2c74508d73012abb01e6f7
Gitweb:     https://git.kernel.org/tip/167e418fa0871c083e2c74508d73012abb01e6f7
Author:     Thomas Richter <tmricht@linux.ibm.com>
AuthorDate: Tue, 23 Apr 2019 12:53:03 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 2 May 2019 16:00:20 -0400

perf report: Report OOM in status line in the GTK UI

An -ENOMEM error is not reported in the GTK GUI.  Instead this error
message pops up on the screen:

[root@m35lp76 perf]# ./perf  report -i perf.data.error68-1

	Processing events... [974K/3M]
	Error:failed to process sample

	0xf4198 [0x8]: failed to process type: 68

However when I use the same perf.data file with --stdio it works:

[root@m35lp76 perf]# ./perf  report -i perf.data.error68-1 --stdio \
		| head -12

  # Total Lost Samples: 0
  #
  # Samples: 76K of event 'cycles'
  # Event count (approx.): 99056160000
  #
  # Overhead  Command          Shared Object      Symbol
  # ........  ...............  .................  .........
  #
     8.81%  find             [kernel.kallsyms]  [k] ftrace_likely_update
     8.74%  swapper          [kernel.kallsyms]  [k] ftrace_likely_update
     8.34%  sshd             [kernel.kallsyms]  [k] ftrace_likely_update
     2.19%  kworker/u512:1-  [kernel.kallsyms]  [k] ftrace_likely_update

The sample precentage is a bit low.....

The GUI always fails in the FINISHED_ROUND event (68) and does not
indicate the reason why.

When happened is the following. Perf report calls a lot of functions and
down deep when a FINISHED_ROUND event is processed, these functions are
called:

  perf_session__process_event()
  + perf_session__process_user_event()
    + process_finished_round()
      + ordered_events__flush()
        + __ordered_events__flush()
	  + do_flush()
	    + ordered_events__deliver_event()
	      + perf_session__deliver_event()
	        + machine__deliver_event()
	          + perf_evlist__deliver_event()
	            + process_sample_event()
	              + hist_entry_iter_add() --> only called in GUI case!!!
	                + hist_iter__report__callback()
	                  + symbol__inc_addr_sample()

	                    Now this functions runs out of memory and
			    returns -ENOMEM. This is reported all the way up
			    until function

perf_session__process_event() returns to its caller, where -ENOMEM is
changed to -EINVAL and processing stops:

 if ((skip = perf_session__process_event(session, event, head)) < 0) {
      pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
	     head, event->header.size, event->header.type);
      err = -EINVAL;
      goto out_err;
 }

This occurred in the FINISHED_ROUND event when it has to process some
10000 entries and ran out of memory.

This patch indicates the root cause and displays it in the status line
of ther perf report GUI.

Output before (on GUI status line):

  0xf4198 [0x8]: failed to process type: 68

Output after:

  0xf4198 [0x8]: failed to process type: 68 [not enough memory]

Committer notes:

the 'skip' variable needs to be initialized to -EINVAL, so that when the
size is less than sizeof(struct perf_event_attr) we avoid this valid
compiler warning:

  util/session.c: In function ‘perf_session__process_events’:
  util/session.c:1936:7: error: ‘skip’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
     err = skip;
     ~~~~^~~~~~
  util/session.c:1874:6: note: ‘skip’ was declared here
    s64 skip;
        ^~~~
  cc1: all warnings being treated as errors

Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>
Reviewed-by: Jiri Olsa <jolsa@redhat.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Link: http://lkml.kernel.org/r/20190423105303.61683-1-tmricht@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/session.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index b17f1c9bc965..bad5f87ae001 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1928,12 +1928,14 @@ more:
 
 	size = event->header.size;
 
+	skip = -EINVAL;
+
 	if (size < sizeof(struct perf_event_header) ||
 	    (skip = rd->process(session, event, file_pos)) < 0) {
-		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
+		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
 		       file_offset + head, event->header.size,
-		       event->header.type);
-		err = -EINVAL;
+		       event->header.type, strerror(-skip));
+		err = skip;
 		goto out;
 	}
 

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

* Re: [PATCH] perf/report: Report OOM in perf report status line
  2019-04-15  9:46 [PATCH] perf/report: Report OOM in perf report status line Thomas Richter
@ 2019-04-15 10:17 ` Hendrik Brueckner
  0 siblings, 0 replies; 9+ messages in thread
From: Hendrik Brueckner @ 2019-04-15 10:17 UTC (permalink / raw)
  To: Thomas Richter
  Cc: linux-kernel, linux-perf-users, acme, brueckner, schwidefsky,
	heiko.carstens

Thomas,

On Mon, Apr 15, 2019 at 11:46:17AM +0200, Thomas Richter wrote:
[...]

> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
> Reviewed-by: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/perf/util/session.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index b17f1c9bc965..e89716175588 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1930,10 +1930,10 @@ reader__process_events(struct reader *rd, struct perf_session *session,
> 
>  	if (size < sizeof(struct perf_event_header) ||
>  	    (skip = rd->process(session, event, file_pos)) < 0) {
> -		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
> +		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
>  		       file_offset + head, event->header.size,
> -		       event->header.type);
> -		err = -EINVAL;
> +		       event->header.type, strerror(-skip));
> +		err = skip;
>  		goto out;
>  	}
> 

Thanks for solving this issue.

Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>


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

* [PATCH] perf/report: Report OOM in perf report status line
@ 2019-04-15  9:46 Thomas Richter
  2019-04-15 10:17 ` Hendrik Brueckner
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Richter @ 2019-04-15  9:46 UTC (permalink / raw)
  To: linux-kernel, linux-perf-users, acme
  Cc: brueckner, schwidefsky, heiko.carstens, Thomas Richter

An -ENOMEM error is not reported in the GTK GUI.
Instead this error message pops up on the screen:

[root@m35lp76 perf]# ./perf  report -i perf.data.error68-1

	Processing events... [974K/3M]
	Error:failed to process sample

	0xf4198 [0x8]: failed to process type: 68

However when I use the same perf.data file with --stdio it works:

[root@m35lp76 perf]# ./perf  report -i perf.data.error68-1 --stdio \
		| head -12

  # Total Lost Samples: 0
  #
  # Samples: 76K of event 'cycles'
  # Event count (approx.): 99056160000
  #
  # Overhead  Command          Shared Object      Symbol
  # ........  ...............  .................  .........
  #
     8.81%  find             [kernel.kallsyms]  [k] ftrace_likely_update
     8.74%  swapper          [kernel.kallsyms]  [k] ftrace_likely_update
     8.34%  sshd             [kernel.kallsyms]  [k] ftrace_likely_update
     2.19%  kworker/u512:1-  [kernel.kallsyms]  [k] ftrace_likely_update

The sample precentage is a bit low.....

The GUI always fails in the FINISHED_ROUND event (68) and does not
indicate the reason why.

When happened is the following. Perf report calls a lot of functions and
down deep when a FINISHED_ROUND event is processed, these functions are
called:

  perf_session__process_event()
  + perf_session__process_user_event()
    + process_finished_round()
      + ordered_events__flush()
        + __ordered_events__flush()
	  + do_flush()
	    + ordered_events__deliver_event()
	      + perf_session__deliver_event()
	        + machine__deliver_event()
	          + perf_evlist__deliver_event()
	            + process_sample_event()
	              + hist_entry_iter_add() --> only called in GUI case!!!
	                + hist_iter__report__callback()
	                  + symbol__inc_addr_sample()

	                    Now this functions runs out of memory and
			    returns -ENOMEM. This is reported all the way up
			    until function

perf_session__process_event() returns to its caller, where -ENOMEM is
changed to -EINVAL and processing stops:

 if ((skip = perf_session__process_event(session, event, head)) < 0) {
      pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
	     head, event->header.size, event->header.type);
      err = -EINVAL;
      goto out_err;
 }

This occurred in the FINISHED_ROUND event when it has to process some
10000 entries and ran out of memory.

This patch indicates the root cause and displays it in the status line
of ther perf report GUI.

Output before (on GUI status line):
0xf4198 [0x8]: failed to process type: 68

Output after:
0xf4198 [0x8]: failed to process type: 68 [not enough memory]

Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Reviewed-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/session.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index b17f1c9bc965..e89716175588 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1930,10 +1930,10 @@ reader__process_events(struct reader *rd, struct perf_session *session,
 
 	if (size < sizeof(struct perf_event_header) ||
 	    (skip = rd->process(session, event, file_pos)) < 0) {
-		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d\n",
+		pr_err("%#" PRIx64 " [%#x]: failed to process type: %d [%s]\n",
 		       file_offset + head, event->header.size,
-		       event->header.type);
-		err = -EINVAL;
+		       event->header.type, strerror(-skip));
+		err = skip;
 		goto out;
 	}
 
-- 
2.16.4


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

end of thread, other threads:[~2019-05-03  5:55 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-23 10:53 [PATCH] perf/report: Report OOM in perf report status line Thomas Richter
2019-04-26 14:09 ` Arnaldo Carvalho de Melo
2019-04-26 14:10   ` Arnaldo Carvalho de Melo
2019-04-26 14:16     ` Arnaldo Carvalho de Melo
2019-04-26 14:44       ` Jiri Olsa
2019-04-26 15:12         ` Arnaldo Carvalho de Melo
2019-05-03  5:55 ` [tip:perf/urgent] perf report: Report OOM in status line in the GTK UI tip-bot for Thomas Richter
  -- strict thread matches above, loose matches on Subject: below --
2019-04-15  9:46 [PATCH] perf/report: Report OOM in perf report status line Thomas Richter
2019-04-15 10:17 ` Hendrik Brueckner

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).