Messages in this thread |  | | Subject | Re: [PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch() | From | Li Huafei <> | Date | Mon, 26 Jul 2021 21:04:28 +0800 |
| |
On 2021/7/26 18:04, James Clark wrote: > > > On 23/07/2021 20:23, Arnaldo Carvalho de Melo wrote: >> Em Fri, Jul 23, 2021 at 09:49:44AM +0800, Li Huafei escreveu: >>> On my aarch64 big endian machine, the perf annotate does not work. >>> >>> # perf annotate >>> Percent | Source code & Disassembly of [kernel.kallsyms] for cycles (253 samples, percent: local period) >>> -------------------------------------------------------------------------------------------------------------- >>> Percent | Source code & Disassembly of [kernel.kallsyms] for cycles (1 samples, percent: local period) >>> ------------------------------------------------------------------------------------------------------------ >>> Percent | Source code & Disassembly of [kernel.kallsyms] for cycles (47 samples, percent: local period) >>> ------------------------------------------------------------------------------------------------------------- >>> ... >>> >>> This is because the arch_find() function uses the normalized architecture >>> name provided by normalize_arch(), and my machine's architecture name >>> aarch64_be is not normalized to arm64. Like other architectures such as >>> arm and powerpc, we can fuzzy match the architecture names associated with >>> aarch64.* and normalize them. >> >> This looks ok modulo fixing the problem and adding that extra pr_err() >> in a single patch, please split this into two.
OK, I've sent the patch set for v2, which puts pr_err() in a single patch: https://lore.kernel.org/patchwork/patch/1467154/
>> >> Also I fail to see why c34df25b40c2 introduced this problem :-\ > > I checked the parent commit of c34df25b40c2 and set the architecture to "aarch64_be" > but it doesn't work either, so I also don't see any regressions. >
Sorry, this may be my mistake. It should be that perf annotate adds support for aarch64_be, not that there is a bug in the existing code. In v2, I've removed the "Fixes" tag.
>> >> Can some ARM person ack/review this, please? >> >> - Arnaldo >> >>> Fixes: c34df25b40c2 ("perf annotate: Add symbol__annotate function") >>> Signed-off-by: Li Huafei <lihuafei1@huawei.com> >>> --- >>> tools/perf/util/annotate.c | 4 +++- >>> tools/perf/util/env.c | 2 +- >>> 2 files changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c >>> index aa04a3655236..cb280de3369f 100644 >>> --- a/tools/perf/util/annotate.c >>> +++ b/tools/perf/util/annotate.c >>> @@ -2192,8 +2192,10 @@ int symbol__annotate(struct map_symbol *ms, struct evsel *evsel, >>> return errno; >>> >>> args.arch = arch = arch__find(arch_name); >>> - if (arch == NULL) >>> + if (arch == NULL) { >>> + pr_err("%s: unsupported arch %s\n", __func__, arch_name); >>> return ENOTSUP; >>> + } >>> >>> if (parch) >>> *parch = arch; >>> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c >>> index cec2e6cad8aa..a91da1e9b201 100644 >>> --- a/tools/perf/util/env.c >>> +++ b/tools/perf/util/env.c >>> @@ -349,7 +349,7 @@ static const char *normalize_arch(char *arch) >>> return "x86"; >>> if (!strcmp(arch, "sun4u") || !strncmp(arch, "sparc", 5)) >>> return "sparc"; >>> - if (!strcmp(arch, "aarch64") || !strcmp(arch, "arm64")) >>> + if (!strncmp(arch, "aarch64", 7) || !strcmp(arch, "arm64")) > > This seems ok to me, but a quick google shows some references to "arm64_be". > I don't know if this could ever get into perf, but most of the other ones > search for prefixes, so it probably doesn't hurt to do strcmp(arch, "arm64", 5) > as well.
In v2, I also added a normalization of "arm64_be". After this patch, perf annotate seems to work fine on my aarch64_be machine. And I checked other source files using perf_env__arch():
- arch/common.c - builtin-trace.c - util/sample-raw.c - util/thread-stack.c - util/unwind-libunwind.c
Looks like it's OK.
Thank you Arnaldo and James for the review.
Huafei > > Thanks > James > >>> return "arm64"; >>> if (!strncmp(arch, "arm", 3) || !strcmp(arch, "sa110")) >>> return "arm"; >>> -- >>> 2.17.1 >>> >> > . >
|  |