linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch()
@ 2021-07-23  1:49 Li Huafei
  2021-07-23 19:23 ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 4+ messages in thread
From: Li Huafei @ 2021-07-23  1:49 UTC (permalink / raw)
  To: jolsa, acme
  Cc: peterz, mark.rutland, mingo, alexander.shishkin, namhyung,
	mliska, irogers, dzhu, rickyman7, yao.jin, linux-perf-users,
	linux-kernel, zhangjinhao2

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch()
  2021-07-23  1:49 [PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch() Li Huafei
@ 2021-07-23 19:23 ` Arnaldo Carvalho de Melo
  2021-07-26 10:04   ` James Clark
  0 siblings, 1 reply; 4+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-07-23 19:23 UTC (permalink / raw)
  To: Li Huafei
  Cc: jolsa, peterz, mark.rutland, mingo, alexander.shishkin, namhyung,
	mliska, irogers, dzhu, rickyman7, yao.jin, linux-perf-users,
	linux-kernel, zhangjinhao2

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch()
  2021-07-23 19:23 ` Arnaldo Carvalho de Melo
@ 2021-07-26 10:04   ` James Clark
  2021-07-26 13:04     ` Li Huafei
  0 siblings, 1 reply; 4+ messages in thread
From: James Clark @ 2021-07-26 10:04 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Li Huafei
  Cc: jolsa, peterz, mark.rutland, mingo, alexander.shishkin, namhyung,
	mliska, irogers, dzhu, rickyman7, yao.jin, linux-perf-users,
	linux-kernel, zhangjinhao2



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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch()
  2021-07-26 10:04   ` James Clark
@ 2021-07-26 13:04     ` Li Huafei
  0 siblings, 0 replies; 4+ messages in thread
From: Li Huafei @ 2021-07-26 13:04 UTC (permalink / raw)
  To: James Clark, Arnaldo Carvalho de Melo
  Cc: jolsa, peterz, mark.rutland, mingo, alexander.shishkin, namhyung,
	mliska, irogers, dzhu, rickyman7, yao.jin, linux-perf-users,
	linux-kernel, zhangjinhao2



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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-07-26 13:04 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23  1:49 [PATCH] perf env: Normalize aarch64.* to arm64 in normalize_arch() Li Huafei
2021-07-23 19:23 ` Arnaldo Carvalho de Melo
2021-07-26 10:04   ` James Clark
2021-07-26 13:04     ` Li Huafei

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).