Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751877AbbLJTE3 (ORCPT ); Thu, 10 Dec 2015 14:04:29 -0500 Received: from mail.kernel.org ([198.145.29.136]:45199 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751072AbbLJTE1 (ORCPT ); Thu, 10 Dec 2015 14:04:27 -0500 Date: Thu, 10 Dec 2015 16:04:17 -0300 From: Arnaldo Carvalho de Melo To: Namhyung Kim Cc: Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern , Frederic Weisbecker , Andi Kleen , Stephane Eranian , Adrian Hunter Subject: Re: [PATCH/RFC 02/16] perf top: Fix and cleanup perf_top__record_precise_ip() Message-ID: <20151210190417.GI17996@kernel.org> References: <1449734015-9148-1-git-send-email-namhyung@kernel.org> <1449734015-9148-3-git-send-email-namhyung@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1449734015-9148-3-git-send-email-namhyung@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 Content-Length: 4208 Lines: 120 Em Thu, Dec 10, 2015 at 04:53:21PM +0900, Namhyung Kim escreveu: > At first, it has duplicate ui__has_annotation() and 'sort__has_sym' > and 'use_browser' check. In fact, the ui__has_annotation() should be > removed as it needs to annotate on --stdio as well. And the > top->sym_filter_entry is used only for stdio mode, so make it clear on > the condition too. So this is doing more than one thing and changes decisions about non-trivial operations, can you please break it down into smaller patches? > Also the check will be simplifed if it sets sym before the check. And > omit check for 'he' since its caller (hist_iter__top_callback) only > gets called when iter->he is not NULL. > > Secondly, it converts the 'ip' argument using map__unmap_ip() before > the call and then reverts it using map__map_ip(). This seems > meaningless and strange since only one of them checks the 'map'. For isntance: the following change has value and should be on a separate patch. > Finally, access the he->hists->lock only if there's an error. > > Signed-off-by: Namhyung Kim > --- > tools/perf/builtin-top.c | 52 ++++++++++++++++++++---------------------------- > 1 file changed, 22 insertions(+), 30 deletions(-) > > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c > index 7e2e72e6d9d1..7cd9bb69f5a6 100644 > --- a/tools/perf/builtin-top.c > +++ b/tools/perf/builtin-top.c > @@ -175,42 +175,40 @@ static void perf_top__record_precise_ip(struct perf_top *top, > int counter, u64 ip) > { > struct annotation *notes; > - struct symbol *sym; > + struct symbol *sym = he->ms.sym; > int err = 0; > > - if (he == NULL || he->ms.sym == NULL || > - ((top->sym_filter_entry == NULL || > - top->sym_filter_entry->ms.sym != he->ms.sym) && use_browser != 1)) > + if (sym == NULL || (use_browser == 0 && > + (top->sym_filter_entry == NULL || > + top->sym_filter_entry->ms.sym != sym))) > return; > > - sym = he->ms.sym; > notes = symbol__annotation(sym); > > if (pthread_mutex_trylock(¬es->lock)) > return; > > - ip = he->ms.map->map_ip(he->ms.map, ip); > - > - if (ui__has_annotation()) > - err = hist_entry__inc_addr_samples(he, counter, ip); > + err = hist_entry__inc_addr_samples(he, counter, ip); > > pthread_mutex_unlock(¬es->lock); > > - /* > - * This function is now called with he->hists->lock held. > - * Release it before going to sleep. > - */ > - pthread_mutex_unlock(&he->hists->lock); > + if (unlikely(err)) { > + /* > + * This function is now called with hists->lock held. > + * Release it before going to sleep. > + */ > + pthread_mutex_unlock(&he->hists->lock); > + > + if (err == -ERANGE && !he->ms.map->erange_warned) > + ui__warn_map_erange(he->ms.map, sym, ip); > + else if (err == -ENOMEM) { > + pr_err("Not enough memory for annotating '%s' symbol!\n", > + sym->name); > + sleep(1); > + } > > - if (err == -ERANGE && !he->ms.map->erange_warned) > - ui__warn_map_erange(he->ms.map, sym, ip); > - else if (err == -ENOMEM) { > - pr_err("Not enough memory for annotating '%s' symbol!\n", > - sym->name); > - sleep(1); > + pthread_mutex_lock(&he->hists->lock); > } > - > - pthread_mutex_lock(&he->hists->lock); > } > > static void perf_top__show_details(struct perf_top *top) > @@ -687,14 +685,8 @@ static int hist_iter__top_callback(struct hist_entry_iter *iter, > struct hist_entry *he = iter->he; > struct perf_evsel *evsel = iter->evsel; > > - if (sort__has_sym && single) { > - u64 ip = al->addr; > - > - if (al->map) > - ip = al->map->unmap_ip(al->map, ip); > - > - perf_top__record_precise_ip(top, he, evsel->idx, ip); > - } > + if (sort__has_sym && single) > + perf_top__record_precise_ip(top, he, evsel->idx, al->addr); > > hist__account_cycles(iter->sample->branch_stack, al, iter->sample, > !(top->record_opts.branch_stack & PERF_SAMPLE_BRANCH_ANY)); > -- > 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/