Mailing List Archive: Linux: Kernel

[PATCH 1/2] perf-tool: Don't process samples with no valid machine object

Index | Next | Previous | View Threaded

joerg.roedel at amd

Feb 9, 2012, 8:07 AM

Post #1 of 5 (33 views)
Permalink

[PATCH 1/2] perf-tool: Don't process samples with no valid machine object

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 [at] amd>
---
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

acme at ghostprotocols

Feb 9, 2012, 8:34 AM

Post #2 of 5 (29 views)
Permalink

Re: [PATCH 1/2] perf-tool: Don't process samples with no valid machine object [In reply to]

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 [at] amd>
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

joerg.roedel at amd

Feb 9, 2012, 9:13 AM

Post #3 of 5 (29 views)
Permalink

Re: [PATCH 1/2] perf-tool: Don't process samples with no valid machine object [In reply to]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

acme at ghostprotocols

Feb 10, 2012, 5:31 AM

Post #4 of 5 (29 views)
Permalink

Re: [PATCH 1/2] perf-tool: Don't process samples with no valid machine object [In reply to]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

joerg.roedel at amd

Feb 10, 2012, 9:05 AM

Post #5 of 5 (25 views)
Permalink

[PATCH 1/2] perf-tool: Don't process samples with no valid machine object [In reply to]

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 [at] amd>
---
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads