From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Jiri Olsa <jolsa@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
David Ahern <dsahern@gmail.com>,
Stephane Eranian <eranian@google.com>,
Andi Kleen <andi@firstfloor.org>, Wang Nan <wangnan0@huawei.com>
Subject: Re: [PATCH 06/21] perf hists: Return error from hists__collapse_resort()
Date: Tue, 2 Feb 2016 13:56:34 -0300 [thread overview]
Message-ID: <20160202165634.GG32488@kernel.org> (raw)
In-Reply-To: <20160202165205.GF32488@kernel.org>
Em Tue, Feb 02, 2016 at 01:52:05PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Feb 02, 2016 at 11:39:48PM +0900, Namhyung Kim escreveu:
> > Currently hists__collapse_resort() and hists__collapse_insert_entry()
> > don't return error code. Now callchain_merge() can check error case,
> > abort and pass the error to the user. Later patch can add more work
> > which can be failed too.
> >
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > tools/perf/util/hist.c | 27 +++++++++++++++++----------
> > tools/perf/util/hist.h | 4 ++--
> > 2 files changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index 098310bc4489..b476b599e415 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -1016,8 +1016,8 @@ void hist_entry__delete(struct hist_entry *he)
> > * collapse the histogram
> > */
> >
> > -bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
> > - struct rb_root *root, struct hist_entry *he)
> > +int hists__collapse_insert_entry(struct hists *hists, struct rb_root *root,
> > + struct hist_entry *he)
> > {
> > struct rb_node **p = &root->rb_node;
> > struct rb_node *parent = NULL;
> > @@ -1037,12 +1037,13 @@ bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
> >
> > if (symbol_conf.use_callchain) {
> > callchain_cursor_reset(&callchain_cursor);
> > - callchain_merge(&callchain_cursor,
> > - iter->callchain,
> > - he->callchain);
> > + if (callchain_merge(&callchain_cursor,
> > + iter->callchain,
> > + he->callchain) < 0)
> > + return -1;
>
> So now we're exiting on error here, whereas before we would
> unconditionally call hist_entry__delete() right after this branch... Is
> that right?
> > }
> > hist_entry__delete(he);
> > - return false;
> > + return 0;
> > }
>
> Looking at it...
Below it does:
rb_erase(&n->rb_node_in, root);
- if (hists__collapse_insert_entry(hists, &hists->entries_collapsed, n)) {
+ ret = hists__collapse_insert_entry(hists, &hists->entries_collapsed, n);
+ if (ret < 0)
+ return -1;
So, we remove it from the rb tree it is in, and previously we expected
that hists__collapse_insert_entry would hist_entry__delete(n), while
now, if callchain_merge() fails, we leak it, no?
- Arnaldo
> >
> > if (cmp < 0)
> > @@ -1054,7 +1055,7 @@ bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
> >
> > rb_link_node(&he->rb_node_in, parent, p);
> > rb_insert_color(&he->rb_node_in, root);
> > - return true;
> > + return 1;
> > }
> >
> > struct rb_root *hists__get_rotate_entries_in(struct hists *hists)
> > @@ -1080,14 +1081,15 @@ static void hists__apply_filters(struct hists *hists, struct hist_entry *he)
> > hists__filter_entry_by_socket(hists, he);
> > }
> >
> > -void hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
> > +int hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
> > {
> > struct rb_root *root;
> > struct rb_node *next;
> > struct hist_entry *n;
> > + int ret;
> >
> > if (!sort__need_collapse)
> > - return;
> > + return 0;
> >
> > hists->nr_entries = 0;
> >
> > @@ -1102,7 +1104,11 @@ void hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
> > next = rb_next(&n->rb_node_in);
> >
> > rb_erase(&n->rb_node_in, root);
> > - if (hists__collapse_insert_entry(hists, &hists->entries_collapsed, n)) {
> > + ret = hists__collapse_insert_entry(hists, &hists->entries_collapsed, n);
> > + if (ret < 0)
> > + return -1;
> > +
> > + if (ret) {
> > /*
> > * If it wasn't combined with one of the entries already
> > * collapsed, we need to apply the filters that may have
> > @@ -1113,6 +1119,7 @@ void hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
> > if (prog)
> > ui_progress__update(prog, 1);
> > }
> > + return 0;
> > }
> >
> > static int hist_entry__sort(struct hist_entry *a, struct hist_entry *b)
> > diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> > index d4ec4822a103..9c72f8331d1c 100644
> > --- a/tools/perf/util/hist.h
> > +++ b/tools/perf/util/hist.h
> > @@ -129,7 +129,7 @@ int hist_entry__sort_snprintf(struct hist_entry *he, char *bf, size_t size,
> > void hist_entry__delete(struct hist_entry *he);
> >
> > void hists__output_resort(struct hists *hists, struct ui_progress *prog);
> > -void hists__collapse_resort(struct hists *hists, struct ui_progress *prog);
> > +int hists__collapse_resort(struct hists *hists, struct ui_progress *prog);
> >
> > void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel);
> > void hists__delete_entries(struct hists *hists);
> > @@ -188,7 +188,7 @@ int hists__init(void);
> > int __hists__init(struct hists *hists);
> >
> > struct rb_root *hists__get_rotate_entries_in(struct hists *hists);
> > -bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
> > +int hists__collapse_insert_entry(struct hists *hists,
> > struct rb_root *root, struct hist_entry *he);
> >
> > struct perf_hpp {
> > --
> > 2.6.4
next prev parent reply other threads:[~2016-02-02 16:56 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-02 14:39 [PATCHSET 00/21] perf tools: Add support for hierachy view (v4) Namhyung Kim
2016-02-02 14:39 ` [PATCH 01/21] perf callchain: Check return value of add_child() Namhyung Kim
2016-02-02 14:39 ` [PATCH 02/21] perf callchain: Check return value of fill_node() Namhyung Kim
2016-02-02 14:39 ` [PATCH 03/21] perf callchain: Add enum match_result for match_chain() Namhyung Kim
2016-02-02 14:39 ` [PATCH 04/21] perf callchain: Check return value of split_add_child() Namhyung Kim
2016-02-02 14:39 ` [PATCH 05/21] perf callchain: Check return value of append_chain_children() Namhyung Kim
2016-02-02 14:39 ` [PATCH 06/21] perf hists: Return error from hists__collapse_resort() Namhyung Kim
2016-02-02 16:52 ` Arnaldo Carvalho de Melo
2016-02-02 16:56 ` Arnaldo Carvalho de Melo [this message]
2016-02-03 14:10 ` Namhyung Kim
2016-02-02 14:39 ` [PATCH 07/21] perf report: Check error during report__collapse_hists() Namhyung Kim
2016-02-02 14:39 ` [PATCH 08/21] perf hists: Basic support of hierarchical report view Namhyung Kim
2016-02-02 14:39 ` [PATCH 09/21] perf hists: Resort hist entries with hierarchy Namhyung Kim
2016-02-02 14:39 ` [PATCH 10/21] perf hists: Add helper functions for hierarchy mode Namhyung Kim
2016-02-02 14:39 ` [PATCH 11/21] perf hists: Support filtering in " Namhyung Kim
2016-02-02 14:39 ` [PATCH 12/21] perf ui/stdio: Implement hierarchy output mode Namhyung Kim
2016-02-02 14:39 ` [PATCH 13/21] perf ui/stdio: Align column header for hierarchy output Namhyung Kim
2016-02-02 14:39 ` [PATCH 14/21] perf hists browser: Count number of hierarchy entries Namhyung Kim
2016-02-02 14:39 ` [PATCH 15/21] perf hists browser: Support collapsing/expanding whole entries in hierarchy Namhyung Kim
2016-02-02 14:39 ` [PATCH 16/21] perf hists browser: Implement hierarchy output Namhyung Kim
2016-02-02 14:39 ` [PATCH 17/21] perf hists browser: Align column header in hierarchy mode Namhyung Kim
2016-02-02 14:40 ` [PATCH 18/21] perf ui/gtk: Implement hierarchy output mode Namhyung Kim
2016-02-02 14:40 ` [PATCH 19/21] perf report: Add --hierarchy option Namhyung Kim
2016-02-02 14:40 ` [PATCH 20/21] perf hists: Support decaying in hierarchy mode Namhyung Kim
2016-02-02 14:40 ` [PATCH 21/21] perf top: Add --hierarchy option Namhyung Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160202165634.GG32488@kernel.org \
--to=acme@kernel.org \
--cc=andi@firstfloor.org \
--cc=dsahern@gmail.com \
--cc=eranian@google.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=wangnan0@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.