* [PATCH 0/2] Fix perf-tool crashes when kvm guest is running @ 2012-02-09 16:07 Joerg Roedel 2012-02-09 16:07 ` [PATCH 1/2] perf-tool: Don't process samples with no valid machine object Joerg Roedel 2012-02-09 16:07 ` [PATCH 2/2] perf-tool: Change perf_guest default back to false Joerg Roedel 0 siblings, 2 replies; 7+ messages in thread From: Joerg Roedel @ 2012-02-09 16:07 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Ingo Molnar, Paul Mackerras, Peter Zijlstra Cc: linux-kernel, David Ahern, Jason Wang Hi, here are the fixes I promised. These patches should fix the crashes David and Jason have seen by making sure no sample is processed when ther is no valid machine object for it. The second patch also changes the perf_guest default back to false. Regards, Joerg ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] perf-tool: Don't process samples with no valid machine object 2012-02-09 16:07 [PATCH 0/2] Fix perf-tool crashes when kvm guest is running Joerg Roedel @ 2012-02-09 16:07 ` Joerg Roedel 2012-02-09 16:34 ` Arnaldo Carvalho de Melo 2012-02-09 16:07 ` [PATCH 2/2] perf-tool: Change perf_guest default back to false Joerg Roedel 1 sibling, 1 reply; 7+ messages in thread From: Joerg Roedel @ 2012-02-09 16:07 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Ingo Molnar, Paul Mackerras, Peter Zijlstra Cc: linux-kernel, David Ahern, Jason Wang, Joerg Roedel The perf sample processing code relies on a valid machine object. Make sure that this path is only entered when such a object exists. Signed-off-by: Joerg Roedel <joerg.roedel@amd.com> --- tools/perf/builtin-top.c | 2 +- tools/perf/util/session.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c index dd162aa..7c81d1d 100644 --- a/tools/perf/builtin-top.c +++ b/tools/perf/builtin-top.c @@ -805,7 +805,7 @@ static void perf_top__mmap_read_idx(struct perf_top *top, int idx) } - if (event->header.type == PERF_RECORD_SAMPLE) { + if (event->header.type == PERF_RECORD_SAMPLE && machine != NULL) { perf_event__process_sample(&top->tool, event, evsel, &sample, machine); } else if (event->header.type < PERF_RECORD_MAX) { diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c index b5ca2558..c9593c7 100644 --- a/tools/perf/util/session.c +++ b/tools/perf/util/session.c @@ -792,7 +792,7 @@ static int perf_session_deliver_event(struct perf_session *session, switch (event->header.type) { case PERF_RECORD_SAMPLE: dump_sample(session, event, sample); - if (evsel == NULL) { + if (evsel == NULL || machine == NULL) { ++session->hists.stats.nr_unknown_id; return -1; } -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] perf-tool: Don't process samples with no valid machine object 2012-02-09 16:07 ` [PATCH 1/2] perf-tool: Don't process samples with no valid machine object Joerg Roedel @ 2012-02-09 16:34 ` Arnaldo Carvalho de Melo 2012-02-09 17:13 ` Joerg Roedel 0 siblings, 1 reply; 7+ messages in thread From: Arnaldo Carvalho de Melo @ 2012-02-09 16:34 UTC (permalink / raw) To: Joerg Roedel Cc: Ingo Molnar, Paul Mackerras, Peter Zijlstra, linux-kernel, David Ahern, Jason Wang Em Thu, Feb 09, 2012 at 05:07:38PM +0100, Joerg Roedel escreveu: > The perf sample processing code relies on a valid machine > object. Make sure that this path is only entered when such a > object exists. > > Signed-off-by: Joerg Roedel <joerg.roedel@amd.com> > --- > tools/perf/builtin-top.c | 2 +- > tools/perf/util/session.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c > index dd162aa..7c81d1d 100644 > --- a/tools/perf/builtin-top.c > +++ b/tools/perf/builtin-top.c > @@ -805,7 +805,7 @@ static void perf_top__mmap_read_idx(struct perf_top *top, int idx) > } > > > - if (event->header.type == PERF_RECORD_SAMPLE) { > + if (event->header.type == PERF_RECORD_SAMPLE && machine != NULL) { Shouldn't we warn the user, even if just once, on the status (last line on the screen) line? > perf_event__process_sample(&top->tool, event, evsel, > &sample, machine); > } else if (event->header.type < PERF_RECORD_MAX) { > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c > index b5ca2558..c9593c7 100644 > --- a/tools/perf/util/session.c > +++ b/tools/perf/util/session.c > @@ -792,7 +792,7 @@ static int perf_session_deliver_event(struct perf_session *session, > switch (event->header.type) { > case PERF_RECORD_SAMPLE: > dump_sample(session, event, sample); > - if (evsel == NULL) { > + if (evsel == NULL || machine == NULL) { > ++session->hists.stats.nr_unknown_id; > return -1; > } > -- > 1.7.5.4 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] perf-tool: Don't process samples with no valid machine object 2012-02-09 16:34 ` Arnaldo Carvalho de Melo @ 2012-02-09 17:13 ` Joerg Roedel 2012-02-10 13:31 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 7+ messages in thread From: Joerg Roedel @ 2012-02-09 17:13 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Ingo Molnar, Paul Mackerras, Peter Zijlstra, linux-kernel, David Ahern, Jason Wang On Thu, Feb 09, 2012 at 02:34:41PM -0200, Arnaldo Carvalho de Melo wrote: > Em Thu, Feb 09, 2012 at 05:07:38PM +0100, Joerg Roedel escreveu: > > - if (event->header.type == PERF_RECORD_SAMPLE) { > > + if (event->header.type == PERF_RECORD_SAMPLE && machine != NULL) { > > Shouldn't we warn the user, even if just once, on the status (last line > on the screen) line? Probably yes, what would be a good message? I guess something like "no machine object for sample" is not helpful to the user. Maybe something like "Unresolvable sample(s) recorded"? Or something completly different? Joerg -- AMD Operating System Research Center Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] perf-tool: Don't process samples with no valid machine object 2012-02-09 17:13 ` Joerg Roedel @ 2012-02-10 13:31 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 7+ messages in thread From: Arnaldo Carvalho de Melo @ 2012-02-10 13:31 UTC (permalink / raw) To: Joerg Roedel Cc: Ingo Molnar, Paul Mackerras, Peter Zijlstra, linux-kernel, David Ahern, Jason Wang Em Thu, Feb 09, 2012 at 06:13:34PM +0100, Joerg Roedel escreveu: > On Thu, Feb 09, 2012 at 02:34:41PM -0200, Arnaldo Carvalho de Melo wrote: > > Em Thu, Feb 09, 2012 at 05:07:38PM +0100, Joerg Roedel escreveu: > > > - if (event->header.type == PERF_RECORD_SAMPLE) { > > > + if (event->header.type == PERF_RECORD_SAMPLE && machine != NULL) { > > > > Shouldn't we warn the user, even if just once, on the status (last line > > on the screen) line? > > Probably yes, what would be a good message? I guess something like > > "no machine object for sample" > > is not helpful to the user. Maybe something like > > "Unresolvable sample(s) recorded"? > > Or something completly different? This is not completely standardized or harminized across the sources, but we have things like: if (verbose) error("Failed to resolve callchain. Skipping\n"); --- if (perf_event__preprocess_sample(event, machine, &al, sample, NULL) < 0) { error("problem processing %d event, skipping it.\n", event->header.type); return; } --- if (!ip_callchain__valid(sample->callchain, event)) { pr_debug("call-chain problem with event, skipping it.\n"); ++session->hists.stats.nr_invalid_chains; --- What has been done more recently is like the above, account the number of different problems and then, at the end do like: if (session->hists.stats.nr_unknown_id != 0) { ui__warning("%u samples with id not present in the header\n", session->hists.stats.nr_unknown_id); } But this is for perf report or other tools that process all events and then provide some post processed results, just before presenting these results. I guess we could do that too for 'perf top' like tools, that continuously show results while collecting more. Perhaps call ui__warning() if some threshold happens, i.e. if way too many errors of some kind are happening and then at the end provide a summary of errors found. - Arnaldo ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] perf-tool: Change perf_guest default back to false 2012-02-09 16:07 [PATCH 0/2] Fix perf-tool crashes when kvm guest is running Joerg Roedel 2012-02-09 16:07 ` [PATCH 1/2] perf-tool: Don't process samples with no valid machine object Joerg Roedel @ 2012-02-09 16:07 ` Joerg Roedel 1 sibling, 0 replies; 7+ messages in thread From: Joerg Roedel @ 2012-02-09 16:07 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Ingo Molnar, Paul Mackerras, Peter Zijlstra Cc: linux-kernel, David Ahern, Jason Wang, Joerg Roedel Setting perf_guest to true by default makes no sense because the perf subcommands can not setup guest symbol information and thus not process and guest samples. The only exception is perf-kvm which changes the perf_guest value on its own. So change the default for perf_guest back to false. Signed-off-by: Joerg Roedel <joerg.roedel@amd.com> --- tools/perf/util/util.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c index 8131410..fb25d13 100644 --- a/tools/perf/util/util.c +++ b/tools/perf/util/util.c @@ -6,7 +6,7 @@ * XXX We need to find a better place for these things... */ bool perf_host = true; -bool perf_guest = true; +bool perf_guest = false; void event_attr_init(struct perf_event_attr *attr) { -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 0/2 v2] Fix perf-tool crashes when kvm guest is running
@ 2012-02-10 17:05 Joerg Roedel
2012-02-10 17:05 ` [PATCH 1/2] perf-tool: Don't process samples with no valid machine object Joerg Roedel
0 siblings, 1 reply; 7+ messages in thread
From: Joerg Roedel @ 2012-02-10 17:05 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ingo Molnar, Paul Mackerras,
Peter Zijlstra
Cc: linux-kernel, David Ahern, Jason Wang
Hi,
here are the fixes I promised. These patches should fix the crashes
David and Jason have seen by making sure no sample is processed when
ther is no valid machine object for it. The second patch also changes
the perf_guest default back to false.
v1->v2:
* Introduced counter for unprocessable samples
* Added ui warnings when unprocessable samples are found
Regards,
Joerg
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/2] perf-tool: Don't process samples with no valid machine object 2012-02-10 17:05 [PATCH 0/2 v2] Fix perf-tool crashes when kvm guest is running Joerg Roedel @ 2012-02-10 17:05 ` Joerg Roedel 0 siblings, 0 replies; 7+ messages in thread From: Joerg Roedel @ 2012-02-10 17:05 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Ingo Molnar, Paul Mackerras, Peter Zijlstra Cc: linux-kernel, David Ahern, Jason Wang, Joerg Roedel The perf sample processing code relies on a valid machine object. Make sure that this path is only entered when such a object exists. A counter for samples where no machine object exits is also introduced to give the user a message about these samples. Signed-off-by: Joerg Roedel <joerg.roedel@amd.com> --- tools/perf/builtin-top.c | 6 ++++++ tools/perf/util/hist.h | 1 + tools/perf/util/session.c | 10 ++++++++++ 3 files changed, 17 insertions(+), 0 deletions(-) diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c index dd162aa..48e0090 100644 --- a/tools/perf/builtin-top.c +++ b/tools/perf/builtin-top.c @@ -668,6 +668,12 @@ static void perf_event__process_sample(struct perf_tool *tool, return; } + if (!machine) { + pr_err("%u unprocessable samples recorded.", + top->session->hists.stats.nr_unprocessable_samples++); + return; + } + if (event->header.misc & PERF_RECORD_MISC_EXACT_IP) top->exact_samples++; diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h index f55f0a8d..8d5641f 100644 --- a/tools/perf/util/hist.h +++ b/tools/perf/util/hist.h @@ -32,6 +32,7 @@ struct events_stats { u32 nr_unknown_events; u32 nr_invalid_chains; u32 nr_unknown_id; + u32 nr_unprocessable_samples; }; enum hist_column { diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c index b5ca2558..a8d25d9 100644 --- a/tools/perf/util/session.c +++ b/tools/perf/util/session.c @@ -796,6 +796,10 @@ static int perf_session_deliver_event(struct perf_session *session, ++session->hists.stats.nr_unknown_id; return -1; } + if (machine == NULL) { + ++session->hists.stats.nr_unprocessable_samples; + return -1; + } return tool->sample(tool, event, sample, evsel, machine); case PERF_RECORD_MMAP: return tool->mmap(tool, event, sample, machine); @@ -964,6 +968,12 @@ static void perf_session__warn_about_errors(const struct perf_session *session, session->hists.stats.nr_invalid_chains, session->hists.stats.nr_events[PERF_RECORD_SAMPLE]); } + + if (session->hists.stats.nr_unprocessable_samples != 0) { + ui__warning("%u unprocessable samples recorded.\n" + "Do you have a KVM guest running and not using 'perf kvm'?\n", + session->hists.stats.nr_unprocessable_samples); + } } #define session_done() (*(volatile int *)(&session_done)) -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-02-10 17:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-09 16:07 [PATCH 0/2] Fix perf-tool crashes when kvm guest is running Joerg Roedel 2012-02-09 16:07 ` [PATCH 1/2] perf-tool: Don't process samples with no valid machine object Joerg Roedel 2012-02-09 16:34 ` Arnaldo Carvalho de Melo 2012-02-09 17:13 ` Joerg Roedel 2012-02-10 13:31 ` Arnaldo Carvalho de Melo 2012-02-09 16:07 ` [PATCH 2/2] perf-tool: Change perf_guest default back to false Joerg Roedel -- strict thread matches above, loose matches on Subject: below -- 2012-02-10 17:05 [PATCH 0/2 v2] Fix perf-tool crashes when kvm guest is running Joerg Roedel 2012-02-10 17:05 ` [PATCH 1/2] perf-tool: Don't process samples with no valid machine object Joerg Roedel
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).