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 <namhy...@kernel.org> > --- > 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 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/