From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753427AbcDVH7m (ORCPT ); Fri, 22 Apr 2016 03:59:42 -0400 Received: from mga02.intel.com ([134.134.136.20]:23475 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751465AbcDVH7l (ORCPT ); Fri, 22 Apr 2016 03:59:41 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,516,1455004800"; d="scan'208";a="690364800" From: Adrian Hunter Subject: Re: [PATCH 1/5] perf tools: fix incorrect ordering of callchain entries To: Chris Phlipot , acme@kernel.org, mingo@redhat.com, peterz@infradead.org References: <1461056164-14914-1-git-send-email-cphlipot0@gmail.com> Cc: linux-kernel@vger.kernel.org Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki Message-ID: <5719D909.9020303@intel.com> Date: Fri, 22 Apr 2016 10:55:53 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1461056164-14914-1-git-send-email-cphlipot0@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 19/04/16 11:56, Chris Phlipot wrote: > The existing implentation implementation of thread__resolve_callchain, Remove 'implentation' > under certain circumstanes, can assemble callchain entries in the 'circumstanes' -> 'circumstances' > incorrect order. > > A the callchain entries are resolved incorrectly for a sample when all > of the following conditions are met: > > 1. callchain_param.order is set to ORDER_CALLER > > 2. thread__resolve_callchain_sample is able to resolve callchain entries > for the sample. > > 3. unwind__get_entries is also able to resolve callchain entries for the > sample. > > The fix is accomplished by reversing the order in which > thread__resolve_callchain_sample and unwind__get_entries are called > when callchain_param.order is set to ORDER_CALLER. Can you give an example of the commands you used and what the call chain looked like before and after. Also please run ./scripts/checkpatch.pl > > Unwind specific code from thread__resolve_callchain is also moved into a > new static function to improve readability of the fix. > > Signed-off-by: Chris Phlipot > --- > tools/perf/util/machine.c | 57 ++++++++++++++++++++++++++++++++++------------- > 1 file changed, 41 insertions(+), 16 deletions(-) > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > index 0c4dabc..dd086c8 100644 > --- a/tools/perf/util/machine.c > +++ b/tools/perf/util/machine.c > @@ -1806,8 +1806,6 @@ static int thread__resolve_callchain_sample(struct thread *thread, > int skip_idx = -1; > int first_call = 0; > > - callchain_cursor_reset(cursor); > - > if (has_branch_callstack(evsel)) { > err = resolve_lbr_callchain_sample(thread, cursor, sample, parent, > root_al, max_stack); > @@ -1918,20 +1916,12 @@ static int unwind_entry(struct unwind_entry *entry, void *arg) > entry->map, entry->sym); > } > > -int thread__resolve_callchain(struct thread *thread, > - struct callchain_cursor *cursor, > - struct perf_evsel *evsel, > - struct perf_sample *sample, > - struct symbol **parent, > - struct addr_location *root_al, > - int max_stack) > -{ > - int ret = thread__resolve_callchain_sample(thread, cursor, evsel, > - sample, parent, > - root_al, max_stack); > - if (ret) > - return ret; > - > +static int thread__resolve_callchain_unwind(struct thread *thread, > + struct callchain_cursor *cursor, > + struct perf_evsel *evsel, > + struct perf_sample *sample, > + int max_stack) > +{ > /* Can we do dwarf post unwind? */ > if (!((evsel->attr.sample_type & PERF_SAMPLE_REGS_USER) && > (evsel->attr.sample_type & PERF_SAMPLE_STACK_USER))) > @@ -1944,7 +1934,42 @@ int thread__resolve_callchain(struct thread *thread, > > return unwind__get_entries(unwind_entry, cursor, > thread, sample, max_stack); > +} > + > +int thread__resolve_callchain(struct thread *thread, > + struct callchain_cursor *cursor, > + struct perf_evsel *evsel, > + struct perf_sample *sample, > + struct symbol **parent, > + struct addr_location *root_al, > + int max_stack) > +{ > + int ret = 0; > + callchain_cursor_reset(&callchain_cursor); > + > + if (callchain_param.order == ORDER_CALLEE) { > + ret = thread__resolve_callchain_sample(thread, cursor, > + evsel, sample, > + parent, root_al, > + max_stack); > + if (ret) > + return ret; > + ret = thread__resolve_callchain_unwind(thread, cursor, > + evsel, sample, > + max_stack); > + } else { > + ret = thread__resolve_callchain_unwind(thread, cursor, > + evsel, sample, > + max_stack); > + if (ret) > + return ret; > + ret = thread__resolve_callchain_sample(thread, cursor, > + evsel, sample, > + parent, root_al, > + max_stack); > + } > > + return ret; > } > > int machine__for_each_thread(struct machine *machine, >