From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964850AbcBBQ4k (ORCPT ); Tue, 2 Feb 2016 11:56:40 -0500 Received: from mail.kernel.org ([198.145.29.136]:40402 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964833AbcBBQ4i (ORCPT ); Tue, 2 Feb 2016 11:56:38 -0500 Date: Tue, 2 Feb 2016 13:56:34 -0300 From: Arnaldo Carvalho de Melo To: Namhyung Kim Cc: Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern , Stephane Eranian , Andi Kleen , Wang Nan Subject: Re: [PATCH 06/21] perf hists: Return error from hists__collapse_resort() Message-ID: <20160202165634.GG32488@kernel.org> References: <1454424003-19031-1-git-send-email-namhyung@kernel.org> <1454424003-19031-7-git-send-email-namhyung@kernel.org> <20160202165205.GF32488@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160202165205.GF32488@kernel.org> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.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 > > Signed-off-by: Namhyung Kim > > --- > > 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