diff mbox series

[PATCHES] Fix 'perf top' breakage on architectures not providing get_cpuid() Re: perf top for arm64?

Message ID 20191211133319.GA15181@kernel.org (mailing list archive)
State New, archived
Headers show
Series [PATCHES] Fix 'perf top' breakage on architectures not providing get_cpuid() Re: perf top for arm64? | expand

Commit Message

Arnaldo Carvalho de Melo Dec. 11, 2019, 1:33 p.m. UTC
Em Tue, Dec 10, 2019 at 04:51:13PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Dec 10, 2019 at 05:17:56PM +0000, John Garry escreveu:
> > On 10/12/2019 17:08, Jiri Olsa wrote:
> > > On Tue, Dec 10, 2019 at 04:52:52PM +0000, John Garry wrote:
> > > > On 10/12/2019 16:36, Jiri Olsa wrote:
> > > > > On Tue, Dec 10, 2019 at 04:13:49PM +0000, John Garry wrote:
> > > > > > I find to my surprise that "perf top" does not work for arm64:

> > > > > > root@ubuntu:/home/john/linux# tools/perf/perf top
> > > > > > Couldn't read the cpuid for this machine: No such file or directory

> > > > > there was recent change that check on cpuid and quits:
> > > > >     608127f73779 perf top: Initialize perf_env->cpuid, needed by the per arch annotation init routine

> > > > ok, this is new code. I obviously didn't check the git history...

> > > > But, apart from this, there are many other places where get_cpuid() is
> > > > called. I wonder what else we're missing out on, and whether we should still
> > > > add it.

> > > right, I was just wondering how come vendor events are working for you,
> > > but realized we have get_cpuid_str being called in there ;-)

> > > I think we should add it as you have it prepared already,
> > > could you post it with bigger changelog that would explain
> > > where it's being used for arm?

> > ok, I can look to do that.

> > But, as you know, we still need to fix perf top for other architectures
> > affected.

> Right, I need to make that just a pr_debug() message and then check in
> the annotation code when that is needed to see if it is set, if not,
> then show a popup error message and refuse to do whatever annotation
> feature requires that.

> Anyway, your patch should make sense and provide info that the ARM64
> annotation may use now or in the future.

So can you take a look at the two patches below and provide me Acked-by
and/or Reviewed-by and/or Tested-by?


From 53c6cde6a71a1a9283763bd2e938b229b50c2cd5 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Wed, 11 Dec 2019 10:09:24 -0300
Subject: [PATCH 1/2] perf arch: Make the default get_cpuid() return compatible
 error

Some of the functions calling get_cpuid() propagate back the error it
returns, and all are using errno (positive) values, make the weak
default get_cpuid() function return ENOSYS to be consistent and to allow
checking if this is an arch not providing this function or if a provided
one is having trouble getting the cpuid, to decide if the warning should
be provided to the user or just a debug message should be emitted.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Garry <john.garry@huawei.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Will Deacon <will@kernel.org>
Link: https://lkml.kernel.org/n/tip-lxwjr0cd2eggzx04a780ffrv@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mark Rutland Dec. 11, 2019, 2:46 p.m. UTC | #1
On Wed, Dec 11, 2019 at 10:33:19AM -0300, Arnaldo Carvalho de Melo wrote:
> So can you take a look at the two patches below and provide me Acked-by
> and/or Reviewed-by and/or Tested-by?

I just gave this a spin. With vanilla v5.5-rc1 perf top behaves as John
reported, and with these applied atop perf works as expected:

Tested-by: Mark Rutland <mark.rutland@arm.com>

From scaning the code, the only other user of get_cpuid() that ends up
giving up is perf kvm, but AFAICT that never supported arm64, so that
looks sound to me:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> From 53c6cde6a71a1a9283763bd2e938b229b50c2cd5 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Wed, 11 Dec 2019 10:09:24 -0300
> Subject: [PATCH 1/2] perf arch: Make the default get_cpuid() return compatible
>  error
> 
> Some of the functions calling get_cpuid() propagate back the error it
> returns, and all are using errno (positive) values, make the weak
> default get_cpuid() function return ENOSYS to be consistent and to allow
> checking if this is an arch not providing this function or if a provided
> one is having trouble getting the cpuid, to decide if the warning should
> be provided to the user or just a debug message should be emitted.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Link: https://lkml.kernel.org/n/tip-lxwjr0cd2eggzx04a780ffrv@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/header.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index becc2d109423..4d39a75551a0 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -850,7 +850,7 @@ int __weak strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
>   */
>  int __weak get_cpuid(char *buffer __maybe_unused, size_t sz __maybe_unused)
>  {
> -	return -1;
> +	return ENOSYS; /* Not implemented */
>  }
>  
>  static int write_cpuid(struct feat_fd *ff,
> -- 
> 2.21.0
> 
> From c6c6a3e2eb6982e37294abcac389effd298cf730 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Wed, 11 Dec 2019 10:21:59 -0300
> Subject: [PATCH 2/2] perf top: Do not bail out when perf_env__read_cpuid()
>  returns ENOSYS
> 
> 'perf top' stopped working on hw architectures that do not provide a
> get_cpuid() implementation and thus fallback to the weak get_cpuid()
> default function.
> 
> This is done because at annotation time we may need it in the arch
> specific annotation init routine, but that is only being used by arches
> that do provide a get_cpuid() implementation:
> 
>   $ find tools/  -name "*.[ch]" | xargs grep 'evlist->env'
>   tools/perf/builtin-top.c:	top.evlist->env = &perf_env;
>   tools/perf/util/evsel.c:		return evsel->evlist->env;
>   tools/perf/util/s390-cpumsf.c:	sf->machine_type = s390_cpumsf_get_type(session->evlist->env->cpuid);
>   tools/perf/util/header.c:	session->evlist->env = &header->env;
>   tools/perf/util/sample-raw.c:	const char *arch_pf = perf_env__arch(evlist->env);
>   $
> 
>   $ find tools/perf/arch  -name "*.[ch]" | xargs grep -w get_cpuid
>   tools/perf/arch/x86/util/auxtrace.c:	ret = get_cpuid(buffer, sizeof(buffer));
>   tools/perf/arch/x86/util/header.c:get_cpuid(char *buffer, size_t sz)
>   tools/perf/arch/powerpc/util/header.c:get_cpuid(char *buffer, size_t sz)
>   tools/perf/arch/s390/util/header.c: * Implementation of get_cpuid().
>   tools/perf/arch/s390/util/header.c:int get_cpuid(char *buffer, size_t sz)
>   tools/perf/arch/s390/util/header.c:	if (buf && get_cpuid(buf, 128))
>   $
> 
> For 'report' or 'script', i.e. tools working on perf.data files, that is
> setup while reading the header, its just top that needs to explicitely
> read it at tool start.
> 
> Reported-by: John Garry <john.garry@huawei.com>
> Analysed-by: Jiri Olsa <jolsa@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Link: https://lkml.kernel.org/n/tip-lxwjr0cd2eggzx04a780ffrv@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/builtin-top.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index dc80044bc46f..795e353de095 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1568,9 +1568,13 @@ int cmd_top(int argc, const char **argv)
>  	 */
>  	status = perf_env__read_cpuid(&perf_env);
>  	if (status) {
> -		pr_err("Couldn't read the cpuid for this machine: %s\n",
> -		       str_error_r(errno, errbuf, sizeof(errbuf)));
> -		goto out_delete_evlist;
> +		/*
> +		 * Some arches do not provide a get_cpuid(), so just use pr_debug, otherwise
> +		 * warn the user explicitely.
> +		 */
> +		eprintf(status == ENOSYS ? 1 : 0, verbose,
> +			"Couldn't read the cpuid for this machine: %s\n",
> +			str_error_r(errno, errbuf, sizeof(errbuf)));
>  	}
>  	top.evlist->env = &perf_env;
>  
> -- 
> 2.21.0
>
John Garry Dec. 11, 2019, 2:50 p.m. UTC | #2
> 
>> Right, I need to make that just a pr_debug() message and then check in
>> the annotation code when that is needed to see if it is set, if not,
>> then show a popup error message and refuse to do whatever annotation
>> feature requires that.
> 
>> Anyway, your patch should make sense and provide info that the ARM64
>> annotation may use now or in the future.
> 
> So can you take a look at the two patches below and provide me Acked-by
> and/or Reviewed-by and/or Tested-by?
> 
> 

>>From 53c6cde6a71a1a9283763bd2e938b229b50c2cd5 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Wed, 11 Dec 2019 10:09:24 -0300
> Subject: [PATCH 1/2] perf arch: Make the default get_cpuid() return compatible
>   error
> 
> Some of the functions calling get_cpuid() propagate back the error it
> returns, and all are using errno (positive) values, make the weak
> default get_cpuid() function return ENOSYS to be consistent and to allow
> checking if this is an arch not providing this function or if a provided
> one is having trouble getting the cpuid, to decide if the warning should
> be provided to the user or just a debug message should be emitted.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: John Garry <john.garry@huawei.com>

For both patches:
Tested-by: John Garry <john.garry@huawei.com> #arm64

I'll still look to get the arm64 version of get_cpuid() patch sent.

> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Link: https://lkml.kernel.org/n/tip-lxwjr0cd2eggzx04a780ffrv@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>   tools/perf/util/header.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index becc2d109423..4d39a75551a0 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -850,7 +850,7 @@ int __weak strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
>    */
>   int __weak get_cpuid(char *buffer __maybe_unused, size_t sz __maybe_unused)
>   {
> -	return -1;
> +	return ENOSYS; /* Not implemented */
>   }
>   
>   static int write_cpuid(struct feat_fd *ff,
>
Jiri Olsa Dec. 11, 2019, 2:54 p.m. UTC | #3
On Wed, Dec 11, 2019 at 10:33:19AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 10, 2019 at 04:51:13PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Dec 10, 2019 at 05:17:56PM +0000, John Garry escreveu:
> > > On 10/12/2019 17:08, Jiri Olsa wrote:
> > > > On Tue, Dec 10, 2019 at 04:52:52PM +0000, John Garry wrote:
> > > > > On 10/12/2019 16:36, Jiri Olsa wrote:
> > > > > > On Tue, Dec 10, 2019 at 04:13:49PM +0000, John Garry wrote:
> > > > > > > I find to my surprise that "perf top" does not work for arm64:
> 
> > > > > > > root@ubuntu:/home/john/linux# tools/perf/perf top
> > > > > > > Couldn't read the cpuid for this machine: No such file or directory
> 
> > > > > > there was recent change that check on cpuid and quits:
> > > > > >     608127f73779 perf top: Initialize perf_env->cpuid, needed by the per arch annotation init routine
> 
> > > > > ok, this is new code. I obviously didn't check the git history...
> 
> > > > > But, apart from this, there are many other places where get_cpuid() is
> > > > > called. I wonder what else we're missing out on, and whether we should still
> > > > > add it.
> 
> > > > right, I was just wondering how come vendor events are working for you,
> > > > but realized we have get_cpuid_str being called in there ;-)
> 
> > > > I think we should add it as you have it prepared already,
> > > > could you post it with bigger changelog that would explain
> > > > where it's being used for arm?
> 
> > > ok, I can look to do that.
> 
> > > But, as you know, we still need to fix perf top for other architectures
> > > affected.
> 
> > Right, I need to make that just a pr_debug() message and then check in
> > the annotation code when that is needed to see if it is set, if not,
> > then show a popup error message and refuse to do whatever annotation
> > feature requires that.
> 
> > Anyway, your patch should make sense and provide info that the ARM64
> > annotation may use now or in the future.
> 
> So can you take a look at the two patches below and provide me Acked-by
> and/or Reviewed-by and/or Tested-by?

looks good to me, but don't have arm server at the moment..

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

> 
> 
> From 53c6cde6a71a1a9283763bd2e938b229b50c2cd5 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Wed, 11 Dec 2019 10:09:24 -0300
> Subject: [PATCH 1/2] perf arch: Make the default get_cpuid() return compatible
>  error
> 
> Some of the functions calling get_cpuid() propagate back the error it
> returns, and all are using errno (positive) values, make the weak
> default get_cpuid() function return ENOSYS to be consistent and to allow
> checking if this is an arch not providing this function or if a provided
> one is having trouble getting the cpuid, to decide if the warning should
> be provided to the user or just a debug message should be emitted.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Link: https://lkml.kernel.org/n/tip-lxwjr0cd2eggzx04a780ffrv@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/header.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index becc2d109423..4d39a75551a0 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -850,7 +850,7 @@ int __weak strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
>   */
>  int __weak get_cpuid(char *buffer __maybe_unused, size_t sz __maybe_unused)
>  {
> -	return -1;
> +	return ENOSYS; /* Not implemented */
>  }
>  
>  static int write_cpuid(struct feat_fd *ff,
> -- 
> 2.21.0
> 
> From c6c6a3e2eb6982e37294abcac389effd298cf730 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Wed, 11 Dec 2019 10:21:59 -0300
> Subject: [PATCH 2/2] perf top: Do not bail out when perf_env__read_cpuid()
>  returns ENOSYS
> 
> 'perf top' stopped working on hw architectures that do not provide a
> get_cpuid() implementation and thus fallback to the weak get_cpuid()
> default function.
> 
> This is done because at annotation time we may need it in the arch
> specific annotation init routine, but that is only being used by arches
> that do provide a get_cpuid() implementation:
> 
>   $ find tools/  -name "*.[ch]" | xargs grep 'evlist->env'
>   tools/perf/builtin-top.c:	top.evlist->env = &perf_env;
>   tools/perf/util/evsel.c:		return evsel->evlist->env;
>   tools/perf/util/s390-cpumsf.c:	sf->machine_type = s390_cpumsf_get_type(session->evlist->env->cpuid);
>   tools/perf/util/header.c:	session->evlist->env = &header->env;
>   tools/perf/util/sample-raw.c:	const char *arch_pf = perf_env__arch(evlist->env);
>   $
> 
>   $ find tools/perf/arch  -name "*.[ch]" | xargs grep -w get_cpuid
>   tools/perf/arch/x86/util/auxtrace.c:	ret = get_cpuid(buffer, sizeof(buffer));
>   tools/perf/arch/x86/util/header.c:get_cpuid(char *buffer, size_t sz)
>   tools/perf/arch/powerpc/util/header.c:get_cpuid(char *buffer, size_t sz)
>   tools/perf/arch/s390/util/header.c: * Implementation of get_cpuid().
>   tools/perf/arch/s390/util/header.c:int get_cpuid(char *buffer, size_t sz)
>   tools/perf/arch/s390/util/header.c:	if (buf && get_cpuid(buf, 128))
>   $
> 
> For 'report' or 'script', i.e. tools working on perf.data files, that is
> setup while reading the header, its just top that needs to explicitely
> read it at tool start.
> 
> Reported-by: John Garry <john.garry@huawei.com>
> Analysed-by: Jiri Olsa <jolsa@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Link: https://lkml.kernel.org/n/tip-lxwjr0cd2eggzx04a780ffrv@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/builtin-top.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index dc80044bc46f..795e353de095 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1568,9 +1568,13 @@ int cmd_top(int argc, const char **argv)
>  	 */
>  	status = perf_env__read_cpuid(&perf_env);
>  	if (status) {
> -		pr_err("Couldn't read the cpuid for this machine: %s\n",
> -		       str_error_r(errno, errbuf, sizeof(errbuf)));
> -		goto out_delete_evlist;
> +		/*
> +		 * Some arches do not provide a get_cpuid(), so just use pr_debug, otherwise
> +		 * warn the user explicitely.
> +		 */
> +		eprintf(status == ENOSYS ? 1 : 0, verbose,
> +			"Couldn't read the cpuid for this machine: %s\n",
> +			str_error_r(errno, errbuf, sizeof(errbuf)));
>  	}
>  	top.evlist->env = &perf_env;
>  
> -- 
> 2.21.0
>
Arnaldo Carvalho de Melo Dec. 11, 2019, 3:28 p.m. UTC | #4
Em Wed, Dec 11, 2019 at 02:46:43PM +0000, Mark Rutland escreveu:
> On Wed, Dec 11, 2019 at 10:33:19AM -0300, Arnaldo Carvalho de Melo wrote:
> > So can you take a look at the two patches below and provide me Acked-by
> > and/or Reviewed-by and/or Tested-by?
> 
> I just gave this a spin. With vanilla v5.5-rc1 perf top behaves as John
> reported, and with these applied atop perf works as expected:
> 
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> 
> >From scaning the code, the only other user of get_cpuid() that ends up
> giving up is perf kvm, but AFAICT that never supported arm64, so that
> looks sound to me:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks guys, added all the tags, appreciated, I also added this:

Fixes: 608127f73779 ("perf top: Initialize perf_env->cpuid, needed by the per arch annotation init routine")

Thanks Jiri for pointing to it.

- Arnaldo
 
> Thanks,
> Mark.
> 
> > From 53c6cde6a71a1a9283763bd2e938b229b50c2cd5 Mon Sep 17 00:00:00 2001
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Date: Wed, 11 Dec 2019 10:09:24 -0300
> > Subject: [PATCH 1/2] perf arch: Make the default get_cpuid() return compatible
> >  error
> > 
> > Some of the functions calling get_cpuid() propagate back the error it
> > returns, and all are using errno (positive) values, make the weak
> > default get_cpuid() function return ENOSYS to be consistent and to allow
> > checking if this is an arch not providing this function or if a provided
> > one is having trouble getting the cpuid, to decide if the warning should
> > be provided to the user or just a debug message should be emitted.
> > 
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: John Garry <john.garry@huawei.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Will Deacon <will@kernel.org>
> > Link: https://lkml.kernel.org/n/tip-lxwjr0cd2eggzx04a780ffrv@git.kernel.org
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> >  tools/perf/util/header.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index becc2d109423..4d39a75551a0 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -850,7 +850,7 @@ int __weak strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
> >   */
> >  int __weak get_cpuid(char *buffer __maybe_unused, size_t sz __maybe_unused)
> >  {
> > -	return -1;
> > +	return ENOSYS; /* Not implemented */
> >  }
> >  
> >  static int write_cpuid(struct feat_fd *ff,
> > -- 
> > 2.21.0
> > 
> > From c6c6a3e2eb6982e37294abcac389effd298cf730 Mon Sep 17 00:00:00 2001
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Date: Wed, 11 Dec 2019 10:21:59 -0300
> > Subject: [PATCH 2/2] perf top: Do not bail out when perf_env__read_cpuid()
> >  returns ENOSYS
> > 
> > 'perf top' stopped working on hw architectures that do not provide a
> > get_cpuid() implementation and thus fallback to the weak get_cpuid()
> > default function.
> > 
> > This is done because at annotation time we may need it in the arch
> > specific annotation init routine, but that is only being used by arches
> > that do provide a get_cpuid() implementation:
> > 
> >   $ find tools/  -name "*.[ch]" | xargs grep 'evlist->env'
> >   tools/perf/builtin-top.c:	top.evlist->env = &perf_env;
> >   tools/perf/util/evsel.c:		return evsel->evlist->env;
> >   tools/perf/util/s390-cpumsf.c:	sf->machine_type = s390_cpumsf_get_type(session->evlist->env->cpuid);
> >   tools/perf/util/header.c:	session->evlist->env = &header->env;
> >   tools/perf/util/sample-raw.c:	const char *arch_pf = perf_env__arch(evlist->env);
> >   $
> > 
> >   $ find tools/perf/arch  -name "*.[ch]" | xargs grep -w get_cpuid
> >   tools/perf/arch/x86/util/auxtrace.c:	ret = get_cpuid(buffer, sizeof(buffer));
> >   tools/perf/arch/x86/util/header.c:get_cpuid(char *buffer, size_t sz)
> >   tools/perf/arch/powerpc/util/header.c:get_cpuid(char *buffer, size_t sz)
> >   tools/perf/arch/s390/util/header.c: * Implementation of get_cpuid().
> >   tools/perf/arch/s390/util/header.c:int get_cpuid(char *buffer, size_t sz)
> >   tools/perf/arch/s390/util/header.c:	if (buf && get_cpuid(buf, 128))
> >   $
> > 
> > For 'report' or 'script', i.e. tools working on perf.data files, that is
> > setup while reading the header, its just top that needs to explicitely
> > read it at tool start.
> > 
> > Reported-by: John Garry <john.garry@huawei.com>
> > Analysed-by: Jiri Olsa <jolsa@kernel.org>
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Will Deacon <will@kernel.org>
> > Link: https://lkml.kernel.org/n/tip-lxwjr0cd2eggzx04a780ffrv@git.kernel.org
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> >  tools/perf/builtin-top.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> > index dc80044bc46f..795e353de095 100644
> > --- a/tools/perf/builtin-top.c
> > +++ b/tools/perf/builtin-top.c
> > @@ -1568,9 +1568,13 @@ int cmd_top(int argc, const char **argv)
> >  	 */
> >  	status = perf_env__read_cpuid(&perf_env);
> >  	if (status) {
> > -		pr_err("Couldn't read the cpuid for this machine: %s\n",
> > -		       str_error_r(errno, errbuf, sizeof(errbuf)));
> > -		goto out_delete_evlist;
> > +		/*
> > +		 * Some arches do not provide a get_cpuid(), so just use pr_debug, otherwise
> > +		 * warn the user explicitely.
> > +		 */
> > +		eprintf(status == ENOSYS ? 1 : 0, verbose,
> > +			"Couldn't read the cpuid for this machine: %s\n",
> > +			str_error_r(errno, errbuf, sizeof(errbuf)));
> >  	}
> >  	top.evlist->env = &perf_env;
> >  
> > -- 
> > 2.21.0
> >
diff mbox series

Patch

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index becc2d109423..4d39a75551a0 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -850,7 +850,7 @@  int __weak strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
  */
 int __weak get_cpuid(char *buffer __maybe_unused, size_t sz __maybe_unused)
 {
-	return -1;
+	return ENOSYS; /* Not implemented */
 }
 
 static int write_cpuid(struct feat_fd *ff,
-- 
2.21.0

From c6c6a3e2eb6982e37294abcac389effd298cf730 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Wed, 11 Dec 2019 10:21:59 -0300
Subject: [PATCH 2/2] perf top: Do not bail out when perf_env__read_cpuid()
 returns ENOSYS

'perf top' stopped working on hw architectures that do not provide a
get_cpuid() implementation and thus fallback to the weak get_cpuid()
default function.

This is done because at annotation time we may need it in the arch
specific annotation init routine, but that is only being used by arches
that do provide a get_cpuid() implementation:

  $ find tools/  -name "*.[ch]" | xargs grep 'evlist->env'
  tools/perf/builtin-top.c:	top.evlist->env = &perf_env;
  tools/perf/util/evsel.c:		return evsel->evlist->env;
  tools/perf/util/s390-cpumsf.c:	sf->machine_type = s390_cpumsf_get_type(session->evlist->env->cpuid);
  tools/perf/util/header.c:	session->evlist->env = &header->env;
  tools/perf/util/sample-raw.c:	const char *arch_pf = perf_env__arch(evlist->env);
  $

  $ find tools/perf/arch  -name "*.[ch]" | xargs grep -w get_cpuid
  tools/perf/arch/x86/util/auxtrace.c:	ret = get_cpuid(buffer, sizeof(buffer));
  tools/perf/arch/x86/util/header.c:get_cpuid(char *buffer, size_t sz)
  tools/perf/arch/powerpc/util/header.c:get_cpuid(char *buffer, size_t sz)
  tools/perf/arch/s390/util/header.c: * Implementation of get_cpuid().
  tools/perf/arch/s390/util/header.c:int get_cpuid(char *buffer, size_t sz)
  tools/perf/arch/s390/util/header.c:	if (buf && get_cpuid(buf, 128))
  $

For 'report' or 'script', i.e. tools working on perf.data files, that is
setup while reading the header, its just top that needs to explicitely
read it at tool start.

Reported-by: John Garry <john.garry@huawei.com>
Analysed-by: Jiri Olsa <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Will Deacon <will@kernel.org>
Link: https://lkml.kernel.org/n/tip-lxwjr0cd2eggzx04a780ffrv@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index dc80044bc46f..795e353de095 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1568,9 +1568,13 @@  int cmd_top(int argc, const char **argv)
 	 */
 	status = perf_env__read_cpuid(&perf_env);
 	if (status) {
-		pr_err("Couldn't read the cpuid for this machine: %s\n",
-		       str_error_r(errno, errbuf, sizeof(errbuf)));
-		goto out_delete_evlist;
+		/*
+		 * Some arches do not provide a get_cpuid(), so just use pr_debug, otherwise
+		 * warn the user explicitely.
+		 */
+		eprintf(status == ENOSYS ? 1 : 0, verbose,
+			"Couldn't read the cpuid for this machine: %s\n",
+			str_error_r(errno, errbuf, sizeof(errbuf)));
 	}
 	top.evlist->env = &perf_env;