2017-11-23 08:56:59

by Milian Wolff

[permalink] [raw]
Subject: Re: [PATCH AUTOSEL for 4.14 18/51] perf callchain: Compare symbol name for inlined frames when matching

On Wednesday, November 22, 2017 11:25:40 PM CET [email protected]
wrote:
> From: Milian Wolff <[email protected]>
>
> [ Upstream commit 9856240ad3269f2fdab0b2fa4400ef8aab792061 ]

Hello Alexander,

this is the first time I encounter AUTOSEL. I just want to check: The patch
below depends on others in a whole series that reworks the handling of inline
frames. Why is only this one getting selected? I don't even think it can work
stand-alone?

Thanks

> The fake symbols we create for inlined frames will represent different
> functions but can use the symbol start address. This leads to issues
> when different inline branches all lead to the same function.
>
> Before:
> ~~~~~
> $ perf report -s sym -i perf.inlining.data --inline --stdio -g function
> ...
> --38.86%--_start
> __libc_start_main
> main
>
> --37.57%--std::norm<double> (inlined)
> std::_Norm_helper<true>::_S_do_it<double>
> (inlined)
>
> --36.36%--std::abs<double> (inlined)
> std::__complex_abs (inlined)
>
>
> --12.24%--std::linear_congruential_engine<unsigned long, 16807ul, 0ul,
> 2147483647ul>::operator() (inlined) std::__detail::__mod<unsigned long,
> 2147483647ul, 16807ul, 0ul> (inlined) std::__detail::_Mod<unsigned long,
> 2147483647ul, 16807ul, 0ul, true, true>::__calc (inlined) ~~~~~
>
> Note that this backtrace representation is completely bogus.
> Complex abs does not call the linear congruential engine! It
> is just a side-effect of a longer inlined stack being appended
> to a shorter, different inlined stack, both of which originate
> in the same function (main).
>
> This patch fixes the issue:
>
> ~~~~~
> $ perf report -s sym -i perf.inlining.data --inline --stdio -g function
> ...
> --38.86%--_start
> __libc_start_main
> main
>
> |--35.59%--std::uniform_real_distribution<double>::op
> |erator()<std::linear_congruential_engine<unsigned
> |long, 16807ul, 0ul, 2147483647ul> > (inlined)
|
> | std::uniform_real_distribution<double>::op
> | erator()<std::linear_congruential_engine<u
> | nsigned long, 16807ul, 0ul, 2147483647ul>
> | > (inlined) |
> | --34.37%--std::__detail::_Adaptor<std::li
> | near_congruential_engine<unsigned long,
> | 16807ul, 0ul, 2147483647ul>,
> | double>::operator() (inlined)
|
> | std::generate_canonical<double,
> | 53ul,
> | std::linear_congruential_engin
> | e<unsigned long, 16807ul, 0ul,
> | 2147483647ul> > (inlined)
|
> | --12.24%--std::linear_congruen
> | tial_engine<unsigned long,
> | 16807ul, 0ul,
> | 2147483647ul>::operator()
> | (inlined)
|
> | std::__detail::__mod
> | <unsigned long,
> | 2147483647ul,
> | 16807ul, 0ul>
> | (inlined)
> | std::__detail::_Mod<
> | unsigned long,
> | 2147483647ul,
> | 16807ul, 0ul, true,
> | true>::__calc
> | (inlined)
> --1.99%--std::norm<double> (inlined)
> std::_Norm_helper<true>::_S_do_it<double>
> (inlined) std::abs<double> (inlined)
> std::__complex_abs (inlined)
> ~~~~~
>
> Signed-off-by: Milian Wolff <[email protected]>
> Reviewed-by: Jiri Olsa <[email protected]>
> Reviewed-by: Namhyung Kim <[email protected]>
> Cc: David Ahern <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ravi Bangoria <[email protected]>
> Cc: Yao Jin <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> [ Fix up conflict with c1fbc0cf81f1 ("perf callchain: Compare dsos (as well)
> for CCKEY_FUNCTION"), remove unneeded hunk ] Signed-off-by: Arnaldo
> Carvalho de Melo <[email protected]>
>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
> tools/perf/util/callchain.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 6031933d811c..3cc51e852cf0 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -697,6 +697,14 @@ static enum match_result match_chain(struct
> callchain_cursor_node *node, }
>
> if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
> + /*
> + * Compare inlined frames based on their symbol name because
> + * different inlined frames will have the same symbol start
> + */
> + if (cnode->ms.sym->inlined || node->sym->inlined)
> + return match_chain_strings(cnode->ms.sym->name,
> + node->sym->name);
> +
> left = cnode->ms.sym->start;
> right = sym->start;
> left_dso = cnode->ms.map->dso;


--
Milian Wolff | [email protected] | Senior Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts


Attachments:
smime.p7s (3.74 kB)