[PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch()

[PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch()

[Li Huafei]

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.

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"))
 		return "arm64";
 	if (!strncmp(arch, "arm", 3) || !strcmp(arch, "sa110"))
 		return "arm";
-- 
2.17.1

Re: [PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch()

[Arnaldo Carvalho de Melo]

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.

Also I fail to see why c34df25b40c2 introduced this problem :-\

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"))
>  		return "arm64";
>  	if (!strncmp(arch, "arm", 3) || !strcmp(arch, "sa110"))
>  		return "arm";
> -- 
> 2.17.1
> 

-- 

- Arnaldo

Re: [PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch()

[James Clark]

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.
> 
> 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.

> 
> 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.

Thanks
James

>>  		return "arm64";
>>  	if (!strncmp(arch, "arm", 3) || !strcmp(arch, "sa110"))
>>  		return "arm";
>> -- 
>> 2.17.1
>>
> 

Re: [PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch()

[Li Huafei]

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
>>>
>>
> .
>