Re: [PATCH RESEND WITH CCs v3 3/4] perf tools: enable dwarf_callchain_users on aarch64

From: Alexandre Truong <alexandre.truong@arm.com>
To: Leo Yan <leo.yan@linaro.org>
Cc: linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
	John Garry <john.garry@huawei.com>, Will Deacon <will@kernel.org>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
	Kemeng Shi <shikemeng@huawei.com>,
	Ian Rogers <irogers@google.com>, Andi Kleen <ak@linux.intel.com>,
	Kan Liang <kan.liang@linux.intel.com>,
	Jin Yao <yao.jin@linux.intel.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Al Grant <al.grant@arm.com>, James Clark <james.clark@arm.com>,
	Wilco Dijkstra <wilco.dijkstra@arm.com>
Subject: Re: [PATCH RESEND WITH CCs v3 3/4] perf tools: enable dwarf_callchain_users on aarch64
Date: Tue, 9 Mar 2021 16:10:01 +0000	[thread overview]
Message-ID: <0ef6a6b0-dc0b-5ef4-778f-c93645c44d9f@arm.com> (raw)
In-Reply-To: <20210305140714.GB5478@leoy-ThinkPad-X240s>

Hi Leo,

Thanks for your message, I'll apply your suggestion for the v4 of the patch.

Regards,

Alexandre

On 3/5/21 2:07 PM, Leo Yan wrote:
> On Fri, Mar 05, 2021 at 07:51:20PM +0800, Leo Yan wrote:
>
> [...]
>
>>> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
>>> index 2a845d6cac09..93661a3eaeb1 100644
>>> --- a/tools/perf/builtin-report.c
>>> +++ b/tools/perf/builtin-report.c
>>> @@ -405,6 +405,10 @@ static int report__setup_sample_type(struct report *rep)
>>>
>>>     callchain_param_setup(sample_type);
>>>
>>> +   if (callchain_param.record_mode == CALLCHAIN_FP &&
>>> +                   strncmp(rep->session->header.env.arch, "aarch64", 7) == 0)
>>> +           dwarf_callchain_users = true;
>>> +
>>
>> I don't have knowledge for dwarf or FP.
>>
>> This patch is suspicious for me that since it only fixes the issue for
>> "perf report" command, but it cannot support "perf script".
>>
>> I did a quick testing for "perf script" command with the test code from
>> patch 04, seems to me it cannot fix the fp omitting issue for
>> "perf script" command:
>>
>>    arm64_fp_test 11211  2282.355095:     176307 cycles:
>>                aaaac2e40740 f2+0x10 (/root/arm64_fp_test)
>>                aaaac2e4061c main+0xc (/root/arm64_fp_test)
>>                ffff961fbd24 __libc_start_main+0xe4 (/usr/lib/aarch64-linux-gnu/libc-2.28.so)
>>                aaaac2e4065c _start+0x34 (/root/arm64_fp_test)
>>
>> Could you check for this?  Thanks!
>
> Maybe we can consolidate the setting for the global variable
> "dwarf_callchain_users" with below change; this can help us to cover
> the tools for most cases.  I used the below change to replact patch
> 03, "perf report" and "perf script" both can work well with it.
>
> Please note, if you want to move forward with this way, it's better to
> use a saperate patch for firstly refactoring the function
> script__setup_sample_type() by using the general API
> callchain_param_setup() to replace the duplicate code pieces for
> callchain parameter setting up.
>
> After that, you could apply the reset change for adding new parameter
> "arch" for the function script__setup_sample_type().
>
>
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 2a845d6cac09..ca2e8c9096ea 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1090,7 +1090,8 @@ static int process_attr(struct perf_tool *tool __maybe_unused,
>        * on events sample_type.
>        */
>       sample_type = evlist__combined_sample_type(*pevlist);
> -     callchain_param_setup(sample_type);
> +     callchain_param_setup(sample_type,
> +                           perf_env__arch((*pevlist)->env));
>       return 0;
>   }
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 5915f19cee55..c49212c135b2 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -2250,7 +2250,8 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
>        * on events sample_type.
>        */
>       sample_type = evlist__combined_sample_type(evlist);
> -     callchain_param_setup(sample_type);
> +     callchain_param_setup(sample_type,
> +                           perf_env__arch((*pevlist)->env));
>
>       /* Enable fields for callchain entries */
>       if (symbol_conf.use_callchain &&
> @@ -3309,16 +3310,8 @@ static void script__setup_sample_type(struct perf_script *script)
>       struct perf_session *session = script->session;
>       u64 sample_type = evlist__combined_sample_type(session->evlist);
>
> -     if (symbol_conf.use_callchain || symbol_conf.cumulate_callchain) {
> -             if ((sample_type & PERF_SAMPLE_REGS_USER) &&
> -                 (sample_type & PERF_SAMPLE_STACK_USER)) {
> -                     callchain_param.record_mode = CALLCHAIN_DWARF;
> -                     dwarf_callchain_users = true;
> -             } else if (sample_type & PERF_SAMPLE_BRANCH_STACK)
> -                     callchain_param.record_mode = CALLCHAIN_LBR;
> -             else
> -                     callchain_param.record_mode = CALLCHAIN_FP;
> -     }
> +     callchain_param_setup(sample_type,
> +                           perf_env__arch(session->machines.host.env));
>
>       if (script->stitch_lbr && (callchain_param.record_mode != CALLCHAIN_LBR)) {
>               pr_warning("Can't find LBR callchain. Switch off --stitch-lbr.\n"
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 1b60985690bb..d9766b54cd1a 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -1600,7 +1600,7 @@ void callchain_cursor_reset(struct callchain_cursor *cursor)
>               map__zput(node->ms.map);
>   }
>
> -void callchain_param_setup(u64 sample_type)
> +void callchain_param_setup(u64 sample_type, const char *arch)
>   {
>       if (symbol_conf.use_callchain || symbol_conf.cumulate_callchain) {
>               if ((sample_type & PERF_SAMPLE_REGS_USER) &&
> @@ -1612,6 +1612,14 @@ void callchain_param_setup(u64 sample_type)
>               else
>                       callchain_param.record_mode = CALLCHAIN_FP;
>       }
> +
> +     /*
> +      * Fixup for arm64 due to the frame pointer was omitted for the
> +      * caller of the leaf frame.
> +      */
> +     if (callchain_param.record_mode == CALLCHAIN_FP &&
> +         strncmp(arch, "arm64", 6) == 0)
> +             dwarf_callchain_users = true;
>   }
>
>   static bool chain_match(struct callchain_list *base_chain,
> diff --git a/tools/perf/util/callchain.h b/tools/perf/util/callchain.h
> index 77fba053c677..d95615daed73 100644
> --- a/tools/perf/util/callchain.h
> +++ b/tools/perf/util/callchain.h
> @@ -300,7 +300,7 @@ int callchain_branch_counts(struct callchain_root *root,
>                           u64 *branch_count, u64 *predicted_count,
>                           u64 *abort_count, u64 *cycles_count);
>
> -void callchain_param_setup(u64 sample_type);
> +void callchain_param_setup(u64 sample_type, const char *arch);
>
>   bool callchain_cnode_matched(struct callchain_node *base_cnode,
>                            struct callchain_node *pair_cnode);
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.