From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753174AbcDVIDW (ORCPT ); Fri, 22 Apr 2016 04:03:22 -0400 Received: from mga04.intel.com ([192.55.52.120]:58579 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751681AbcDVIDS (ORCPT ); Fri, 22 Apr 2016 04:03:18 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,516,1455004800"; d="scan'208";a="937844390" Subject: Re: [PATCH 1/5] perf tools: fix incorrect ordering of callchain entries To: Adrian Hunter , Chris Phlipot , acme@kernel.org, mingo@redhat.com, peterz@infradead.org References: <1461056164-14914-1-git-send-email-cphlipot0@gmail.com> <5719D909.9020303@intel.com> Cc: linux-kernel@vger.kernel.org, Jiri Olsa From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki Message-ID: <5719D9E1.9020507@intel.com> Date: Fri, 22 Apr 2016 10:59:29 +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: <5719D909.9020303@intel.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 +Jiri since he wrote the original code On 22/04/16 10:55, Adrian Hunter wrote: > 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, >> > >