Re: [PATCH 10/28] perf report: Cache cumulative callchains
On Tue, Jan 14, 2014 at 08:55:50AM +0900, Namhyung Kim wrote:
> Hi Jiri,
>
> Thanks for reminding me. :)
>
> On Thu, 9 Jan 2014 19:06:28 +0100, Jiri Olsa wrote:
> > On Wed, Jan 08, 2014 at 05:46:15PM +0900, Namhyung Kim wrote:
>
> [SNIP]
> >> + /*
> >> + * Check if there's duplicate entries in the callchain.
> >> + * It's possible that it has cycles or recursive calls.
> >> + */
> >> + for (i = 0; i < iter->curr; i++) {
> >> + if (hist_entry__cmp(he_cache[i], _tmp) == 0)
> >
> > we need here:
> > iter->he = he_cache[i];
> >
> >> + return 0;
> >> + }
> >
> > otherwise iter->he and al are not in sync and
> > hist_entry__inc_addr_samples fails in hist_iter_cb
>
> Right. But the point of the he_cache is to skip duplicate entries. So
> I'd like to change it like setting it to NULL and check it before the
> callback function:
>
> while (iter->next_entry(iter, al)) {
> err = iter->add_next_entry(iter, al);
> if (err)
> break;
>
> if (iter->he && iter->add_entry_cb) {
> err = iter->add_entry_cb(iter, al, ...);
> if (err)
> break;
> }
> }
>
> What do you think?
yep, better ;-) I saw you sent v6.. I'll check that
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/28] perf report: Cache cumulative callchains
On Tue, Jan 14, 2014 at 08:55:50AM +0900, Namhyung Kim wrote:
Hi Jiri,
Thanks for reminding me. :)
On Thu, 9 Jan 2014 19:06:28 +0100, Jiri Olsa wrote:
On Wed, Jan 08, 2014 at 05:46:15PM +0900, Namhyung Kim wrote:
[SNIP]
+ /*
+ * Check if there's duplicate entries in the callchain.
+ * It's possible that it has cycles or recursive calls.
+ */
+ for (i = 0; i iter-curr; i++) {
+ if (hist_entry__cmp(he_cache[i], he_tmp) == 0)
we need here:
iter-he = he_cache[i];
+ return 0;
+ }
otherwise iter-he and al are not in sync and
hist_entry__inc_addr_samples fails in hist_iter_cb
Right. But the point of the he_cache is to skip duplicate entries. So
I'd like to change it like setting it to NULL and check it before the
callback function:
while (iter-next_entry(iter, al)) {
err = iter-add_next_entry(iter, al);
if (err)
break;
if (iter-he iter-add_entry_cb) {
err = iter-add_entry_cb(iter, al, ...);
if (err)
break;
}
}
What do you think?
yep, better ;-) I saw you sent v6.. I'll check that
jirka
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/28] perf report: Cache cumulative callchains
Hi Jiri,
Thanks for reminding me. :)
On Thu, 9 Jan 2014 19:06:28 +0100, Jiri Olsa wrote:
> On Wed, Jan 08, 2014 at 05:46:15PM +0900, Namhyung Kim wrote:
[SNIP]
>> +/*
>> + * Check if there's duplicate entries in the callchain.
>> + * It's possible that it has cycles or recursive calls.
>> + */
>> +for (i = 0; i < iter->curr; i++) {
>> +if (hist_entry__cmp(he_cache[i], _tmp) == 0)
>
> we need here:
> iter->he = he_cache[i];
>
>> +return 0;
>> +}
>
> otherwise iter->he and al are not in sync and
> hist_entry__inc_addr_samples fails in hist_iter_cb
Right. But the point of the he_cache is to skip duplicate entries. So
I'd like to change it like setting it to NULL and check it before the
callback function:
while (iter->next_entry(iter, al)) {
err = iter->add_next_entry(iter, al);
if (err)
break;
if (iter->he && iter->add_entry_cb) {
err = iter->add_entry_cb(iter, al, ...);
if (err)
break;
}
}
What do you think?
Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/28] perf report: Cache cumulative callchains
Hi Jiri, On Sat, 11 Jan 2014 17:02:54 +0100, Jiri Olsa wrote: > On Wed, Jan 08, 2014 at 05:46:15PM +0900, Namhyung Kim wrote: >> It is possble that a callchain has cycles or recursive calls. In that >> case it'll end up having entries more than 100% overhead in the >> output. In order to prevent such entries, cache each callchain node >> and skip if same entry already cumulated. [SNIP] >> +node = callchain_cursor_current(_cursor); >> +if (node == NULL) >> +return 0; > > some leftover? looks like nop.. Ah, right. I'll get rid of it. Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/28] perf report: Cache cumulative callchains
Hi Jiri, On Sat, 11 Jan 2014 17:02:54 +0100, Jiri Olsa wrote: On Wed, Jan 08, 2014 at 05:46:15PM +0900, Namhyung Kim wrote: It is possble that a callchain has cycles or recursive calls. In that case it'll end up having entries more than 100% overhead in the output. In order to prevent such entries, cache each callchain node and skip if same entry already cumulated. [SNIP] +node = callchain_cursor_current(callchain_cursor); +if (node == NULL) +return 0; some leftover? looks like nop.. Ah, right. I'll get rid of it. Thanks, Namhyung -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/28] perf report: Cache cumulative callchains
Hi Jiri,
Thanks for reminding me. :)
On Thu, 9 Jan 2014 19:06:28 +0100, Jiri Olsa wrote:
On Wed, Jan 08, 2014 at 05:46:15PM +0900, Namhyung Kim wrote:
[SNIP]
+/*
+ * Check if there's duplicate entries in the callchain.
+ * It's possible that it has cycles or recursive calls.
+ */
+for (i = 0; i iter-curr; i++) {
+if (hist_entry__cmp(he_cache[i], he_tmp) == 0)
we need here:
iter-he = he_cache[i];
+return 0;
+}
otherwise iter-he and al are not in sync and
hist_entry__inc_addr_samples fails in hist_iter_cb
Right. But the point of the he_cache is to skip duplicate entries. So
I'd like to change it like setting it to NULL and check it before the
callback function:
while (iter-next_entry(iter, al)) {
err = iter-add_next_entry(iter, al);
if (err)
break;
if (iter-he iter-add_entry_cb) {
err = iter-add_entry_cb(iter, al, ...);
if (err)
break;
}
}
What do you think?
Thanks,
Namhyung
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/28] perf report: Cache cumulative callchains
On Wed, Jan 08, 2014 at 05:46:15PM +0900, Namhyung Kim wrote:
> It is possble that a callchain has cycles or recursive calls. In that
> case it'll end up having entries more than 100% overhead in the
> output. In order to prevent such entries, cache each callchain node
> and skip if same entry already cumulated.
>
> Cc: Arun Sharma
> Cc: Frederic Weisbecker
> Signed-off-by: Namhyung Kim
> ---
> tools/perf/builtin-report.c | 54
> ++---
> 1 file changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 06330736485b..087d01cac1aa 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -363,7 +363,27 @@ static int
> iter_prepare_cumulative_entry(struct hist_entry_iter *iter __maybe_unused,
> struct addr_location *al __maybe_unused)
> {
> + struct callchain_cursor_node *node;
> + struct hist_entry **he_cache;
> +
> callchain_cursor_commit(_cursor);
> +
> + /*
> + * This is for detecting cycles or recursions so that they're
> + * cumulated only one time to prevent entries more than 100%
> + * overhead.
> + */
> + he_cache = malloc(sizeof(*he_cache) * (PERF_MAX_STACK_DEPTH + 1));
> + if (he_cache == NULL)
> + return -ENOMEM;
> +
> + iter->priv = he_cache;
> + iter->curr = 0;
> +
> + node = callchain_cursor_current(_cursor);
> + if (node == NULL)
> + return 0;
some leftover? looks like nop..
jirka
> +
> return 0;
> }
>
SNIP
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/28] perf report: Cache cumulative callchains
On Wed, Jan 08, 2014 at 05:46:15PM +0900, Namhyung Kim wrote:
It is possble that a callchain has cycles or recursive calls. In that
case it'll end up having entries more than 100% overhead in the
output. In order to prevent such entries, cache each callchain node
and skip if same entry already cumulated.
Cc: Arun Sharma asha...@fb.com
Cc: Frederic Weisbecker fweis...@gmail.com
Signed-off-by: Namhyung Kim namhy...@kernel.org
---
tools/perf/builtin-report.c | 54
++---
1 file changed, 51 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 06330736485b..087d01cac1aa 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -363,7 +363,27 @@ static int
iter_prepare_cumulative_entry(struct hist_entry_iter *iter __maybe_unused,
struct addr_location *al __maybe_unused)
{
+ struct callchain_cursor_node *node;
+ struct hist_entry **he_cache;
+
callchain_cursor_commit(callchain_cursor);
+
+ /*
+ * This is for detecting cycles or recursions so that they're
+ * cumulated only one time to prevent entries more than 100%
+ * overhead.
+ */
+ he_cache = malloc(sizeof(*he_cache) * (PERF_MAX_STACK_DEPTH + 1));
+ if (he_cache == NULL)
+ return -ENOMEM;
+
+ iter-priv = he_cache;
+ iter-curr = 0;
+
+ node = callchain_cursor_current(callchain_cursor);
+ if (node == NULL)
+ return 0;
some leftover? looks like nop..
jirka
+
return 0;
}
SNIP
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/28] perf report: Cache cumulative callchains
On Wed, Jan 08, 2014 at 05:46:15PM +0900, Namhyung Kim wrote:
SNIP
> goto out;
> }
>
> - if (al->map->groups == >machine->kmaps) {
> - if (machine__is_host(iter->machine)) {
> + if (al->map->groups == >machine->kmaps) {
> + if (machine__is_host(al->machine)) {
> al->cpumode = PERF_RECORD_MISC_KERNEL;
> al->level = 'k';
> } else {
> @@ -417,7 +440,7 @@ iter_next_cumulative_entry(struct hist_entry_iter *iter,
> al->level = 'g';
> }
> } else {
> - if (machine__is_host(iter->machine)) {
> + if (machine__is_host(al->machine)) {
> al->cpumode = PERF_RECORD_MISC_USER;
> al->level = '.';
> } else if (perf_guest) {
> @@ -440,7 +463,29 @@ iter_add_next_cumulative_entry(struct hist_entry_iter
> *iter,
> {
> struct perf_evsel *evsel = iter->evsel;
> struct perf_sample *sample = iter->sample;
> + struct hist_entry **he_cache = iter->priv;
> struct hist_entry *he;
> + struct hist_entry he_tmp = {
> + .cpu = al->cpu,
> + .thread = al->thread,
> + .comm = thread__comm(al->thread),
> + .ip = al->addr,
> + .ms = {
> + .map = al->map,
> + .sym = al->sym,
> + },
> + .parent = iter->parent,
> + };
> + int i;
> +
> + /*
> + * Check if there's duplicate entries in the callchain.
> + * It's possible that it has cycles or recursive calls.
> + */
> + for (i = 0; i < iter->curr; i++) {
> + if (hist_entry__cmp(he_cache[i], _tmp) == 0)
we need here:
iter->he = he_cache[i];
> + return 0;
> + }
otherwise iter->he and al are not in sync and
hist_entry__inc_addr_samples fails in hist_iter_cb
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 10/28] perf report: Cache cumulative callchains
On Wed, Jan 08, 2014 at 05:46:15PM +0900, Namhyung Kim wrote:
SNIP
goto out;
}
- if (al-map-groups == iter-machine-kmaps) {
- if (machine__is_host(iter-machine)) {
+ if (al-map-groups == al-machine-kmaps) {
+ if (machine__is_host(al-machine)) {
al-cpumode = PERF_RECORD_MISC_KERNEL;
al-level = 'k';
} else {
@@ -417,7 +440,7 @@ iter_next_cumulative_entry(struct hist_entry_iter *iter,
al-level = 'g';
}
} else {
- if (machine__is_host(iter-machine)) {
+ if (machine__is_host(al-machine)) {
al-cpumode = PERF_RECORD_MISC_USER;
al-level = '.';
} else if (perf_guest) {
@@ -440,7 +463,29 @@ iter_add_next_cumulative_entry(struct hist_entry_iter
*iter,
{
struct perf_evsel *evsel = iter-evsel;
struct perf_sample *sample = iter-sample;
+ struct hist_entry **he_cache = iter-priv;
struct hist_entry *he;
+ struct hist_entry he_tmp = {
+ .cpu = al-cpu,
+ .thread = al-thread,
+ .comm = thread__comm(al-thread),
+ .ip = al-addr,
+ .ms = {
+ .map = al-map,
+ .sym = al-sym,
+ },
+ .parent = iter-parent,
+ };
+ int i;
+
+ /*
+ * Check if there's duplicate entries in the callchain.
+ * It's possible that it has cycles or recursive calls.
+ */
+ for (i = 0; i iter-curr; i++) {
+ if (hist_entry__cmp(he_cache[i], he_tmp) == 0)
we need here:
iter-he = he_cache[i];
+ return 0;
+ }
otherwise iter-he and al are not in sync and
hist_entry__inc_addr_samples fails in hist_iter_cb
jirka
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
