| Message ID | 418023e7-a50d-cb6f-989f-2e6d114ce5d8@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | perf top for arm64? | expand |
On Tue, Dec 10, 2019 at 04:13:49PM +0000, John Garry wrote: > Hi all, > > 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 Arnaldo, maybe this should be just a warning/info, because it seems to be related to annotations only..? get_cpuid is defined only for s390/x86/powerpc, so I guess it won't work on the rest as well jirka > > That's v5.5-rc1 release. > > It seems that we are just missing an arm64 version of get_cpuid() - with the > patch below, I now get as hoped: > > PerfTop: 32857 irqs/sec kernel:85.0% exact: 0.0% lost: 0/0 drop: 0/0 > [4000Hz cycles], (all, 64 CPUs) > ------------------------------------------------------------------------------- > > 8.99% [kernel] [k] arm_smmu_cmdq_issue_cmdlist > 5.80% [kernel] [k] __softirqentry_text_start > 4.49% [kernel] [k] _raw_spin_unlock_irqrestore > 3.48% [kernel] [k] el0_svc_common.constprop.2 > 3.37% [kernel] [k] _raw_write_lock_irqsave > 3.28% [kernel] [k] __local_bh_enable_ip > 3.05% [kernel] [k] __blk_complete_request > 2.07% [kernel] [k] queued_spin_lock_slowpath > 1.93% [vdso] [.] 0x0000000000000484 > > > Was this just missed? Or is there a good reason to omit? > > Thanks, > John > > --->8--- > > Subject: [PATCH] perf: Add perf top support for arm64 > > Copied from get_cpuid_str() essentially... > > Signed-off-by: John Garry <john.garry@huawei.com> > > diff --git a/tools/perf/arch/arm64/util/header.c > b/tools/perf/arch/arm64/util/header.c > index a32e4b72a98f..ecd1f86e29cc 100644 > --- a/tools/perf/arch/arm64/util/header.c > +++ b/tools/perf/arch/arm64/util/header.c > @@ -1,10 +1,12 @@ > #include <stdio.h> > #include <stdlib.h> > #include <perf/cpumap.h> > +#include <util/cpumap.h> > #include <internal/cpumap.h> > #include <api/fs/fs.h> > #include "debug.h" > #include "header.h" > +#include <errno.h> > > #define MIDR "/regs/identification/midr_el1" > #define MIDR_SIZE 19 > @@ -12,6 +14,59 @@ > #define MIDR_VARIANT_SHIFT 20 > #define MIDR_VARIANT_MASK (0xf << MIDR_VARIANT_SHIFT) > > +int > +get_cpuid(char *buffer, size_t sz) > +{ > + char *buf = NULL; > + char path[PATH_MAX]; > + const char *sysfs = sysfs__mountpoint(); > + int cpu; > + u64 midr = 0; > + FILE *file; > + > + if (!sysfs) > + return EINVAL; > + > + buf = malloc(MIDR_SIZE); > + if (!buf) > + return EINVAL; > + > + /* read midr from list of cpus mapped to this pmu */ > + for (cpu = 0; cpu < cpu__max_present_cpu(); cpu++) { > + scnprintf(path, sz, "%s/devices/system/cpu/cpu%d"MIDR, > + sysfs, cpu); > + > + file = fopen(path, "r"); > + if (!file) { > + pr_debug("fopen failed for file %s\n", path); > + continue; > + } > + > + if (!fgets(buf, MIDR_SIZE, file)) { > + fclose(file); > + continue; > + } > + fclose(file); > + > + /* Ignore/clear Variant[23:20] and > + * Revision[3:0] of MIDR > + */ > + midr = strtoul(buf, NULL, 16); > + midr &= (~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK)); > + scnprintf(buffer, MIDR_SIZE, "0x%016lx", midr); > + /* got midr break loop */ > + break; > + } > + > + if (!midr) { > + pr_err("failed to get cpuid string\n"); > + free(buf); > + return EINVAL; > + } > + return 0; > +} > + >
On 10/12/2019 16:36, Jiri Olsa wrote: > On Tue, Dec 10, 2019 at 04:13:49PM +0000, John Garry wrote: >> Hi all, >> >> 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 > Hi Jirka, > 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. Thanks, John > Arnaldo, > maybe this should be just a warning/info, because it seems to be related > to annotations only..? > > get_cpuid is defined only for s390/x86/powerpc, so I guess it won't work > on the rest as well > > jirka > >> >> That's v5.5-rc1 release. >> >> It seems that we are just missing an arm64 version of get_cpuid() - with the >> patch below, I now get as hoped: >> >> PerfTop: 32857 irqs/sec kernel:85.0% exact: 0.0% lost: 0/0 drop: 0/0 >> [4000Hz cycles], (all, 64 CPUs) >> ------------------------------------------------------------------------------- >> >> 8.99% [kernel] [k] arm_smmu_cmdq_issue_cmdlist >> 5.80% [kernel] [k] __softirqentry_text_start >> 4.49% [kernel] [k] _raw_spin_unlock_irqrestore >> 3.48% [kernel] [k] el0_svc_common.constprop.2 >> 3.37% [kernel] [k] _raw_write_lock_irqsave >> 3.28% [kernel] [k] __local_bh_enable_ip >> 3.05% [kernel] [k] __blk_complete_request >> 2.07% [kernel] [k] queued_spin_lock_slowpath >> 1.93% [vdso] [.] 0x0000000000000484 >> >> >> Was this just missed? Or is there a good reason to omit? >> >> Thanks, >> John >> >> --->8--- >> >> Subject: [PATCH] perf: Add perf top support for arm64 >> >> Copied from get_cpuid_str() essentially... >> >> Signed-off-by: John Garry <john.garry@huawei.com> >> >> diff --git a/tools/perf/arch/arm64/util/header.c >> b/tools/perf/arch/arm64/util/header.c >> index a32e4b72a98f..ecd1f86e29cc 100644 >> --- a/tools/perf/arch/arm64/util/header.c >> +++ b/tools/perf/arch/arm64/util/header.c >> @@ -1,10 +1,12 @@ >> #include <stdio.h> >> #include <stdlib.h> >> #include <perf/cpumap.h> >> +#include <util/cpumap.h> >> #include <internal/cpumap.h> >> #include <api/fs/fs.h> >> #include "debug.h" >> #include "header.h" >> +#include <errno.h> >> >> #define MIDR "/regs/identification/midr_el1" >> #define MIDR_SIZE 19 >> @@ -12,6 +14,59 @@ >> #define MIDR_VARIANT_SHIFT 20 >> #define MIDR_VARIANT_MASK (0xf << MIDR_VARIANT_SHIFT) >> >> +int >> +get_cpuid(char *buffer, size_t sz) >> +{ >> + char *buf = NULL; >> + char path[PATH_MAX]; >> + const char *sysfs = sysfs__mountpoint(); >> + int cpu; >> + u64 midr = 0; >> + FILE *file; >> + >> + if (!sysfs) >> + return EINVAL; >> + >> + buf = malloc(MIDR_SIZE); >> + if (!buf) >> + return EINVAL; >> + >> + /* read midr from list of cpus mapped to this pmu */ >> + for (cpu = 0; cpu < cpu__max_present_cpu(); cpu++) { >> + scnprintf(path, sz, "%s/devices/system/cpu/cpu%d"MIDR, >> + sysfs, cpu); >> + >> + file = fopen(path, "r"); >> + if (!file) { >> + pr_debug("fopen failed for file %s\n", path); >> + continue; >> + } >> + >> + if (!fgets(buf, MIDR_SIZE, file)) { >> + fclose(file); >> + continue; >> + } >> + fclose(file); >> + >> + /* Ignore/clear Variant[23:20] and >> + * Revision[3:0] of MIDR >> + */ >> + midr = strtoul(buf, NULL, 16); >> + midr &= (~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK)); >> + scnprintf(buffer, MIDR_SIZE, "0x%016lx", midr); >> + /* got midr break loop */ >> + break; >> + } >> + >> + if (!midr) { >> + pr_err("failed to get cpuid string\n"); >> + free(buf); >> + return EINVAL; >> + } >> + return 0; >> +} >> + >> > > . >
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: > > > Hi all, > > > > > > 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 > > > > Hi Jirka, > > > 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? jirka
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: >>>> Hi all, >>>> >>>> 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 >>> >> >> Hi Jirka, >> >>> 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. Thanks, John
Em Tue, Dec 10, 2019 at 05:36:55PM +0100, Jiri Olsa escreveu: > On Tue, Dec 10, 2019 at 04:13:49PM +0000, John Garry wrote: > > Hi all, > > > > 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 > > Arnaldo, > maybe this should be just a warning/info, because it seems to be related > to annotations only..? Right, my bad, I'll look into making this just a debug message and then check in the annotation code when this is really needed to show an error/popup window :-\ - Arnaldo > get_cpuid is defined only for s390/x86/powerpc, so I guess it won't work > on the rest as well > > jirka > > > > > That's v5.5-rc1 release. > > > > It seems that we are just missing an arm64 version of get_cpuid() - with the > > patch below, I now get as hoped: > > > > PerfTop: 32857 irqs/sec kernel:85.0% exact: 0.0% lost: 0/0 drop: 0/0 > > [4000Hz cycles], (all, 64 CPUs) > > ------------------------------------------------------------------------------- > > > > 8.99% [kernel] [k] arm_smmu_cmdq_issue_cmdlist > > 5.80% [kernel] [k] __softirqentry_text_start > > 4.49% [kernel] [k] _raw_spin_unlock_irqrestore > > 3.48% [kernel] [k] el0_svc_common.constprop.2 > > 3.37% [kernel] [k] _raw_write_lock_irqsave > > 3.28% [kernel] [k] __local_bh_enable_ip > > 3.05% [kernel] [k] __blk_complete_request > > 2.07% [kernel] [k] queued_spin_lock_slowpath > > 1.93% [vdso] [.] 0x0000000000000484 > > > > > > Was this just missed? Or is there a good reason to omit? > > > > Thanks, > > John > > > > --->8--- > > > > Subject: [PATCH] perf: Add perf top support for arm64 > > > > Copied from get_cpuid_str() essentially... > > > > Signed-off-by: John Garry <john.garry@huawei.com> > > > > diff --git a/tools/perf/arch/arm64/util/header.c > > b/tools/perf/arch/arm64/util/header.c > > index a32e4b72a98f..ecd1f86e29cc 100644 > > --- a/tools/perf/arch/arm64/util/header.c > > +++ b/tools/perf/arch/arm64/util/header.c > > @@ -1,10 +1,12 @@ > > #include <stdio.h> > > #include <stdlib.h> > > #include <perf/cpumap.h> > > +#include <util/cpumap.h> > > #include <internal/cpumap.h> > > #include <api/fs/fs.h> > > #include "debug.h" > > #include "header.h" > > +#include <errno.h> > > > > #define MIDR "/regs/identification/midr_el1" > > #define MIDR_SIZE 19 > > @@ -12,6 +14,59 @@ > > #define MIDR_VARIANT_SHIFT 20 > > #define MIDR_VARIANT_MASK (0xf << MIDR_VARIANT_SHIFT) > > > > +int > > +get_cpuid(char *buffer, size_t sz) > > +{ > > + char *buf = NULL; > > + char path[PATH_MAX]; > > + const char *sysfs = sysfs__mountpoint(); > > + int cpu; > > + u64 midr = 0; > > + FILE *file; > > + > > + if (!sysfs) > > + return EINVAL; > > + > > + buf = malloc(MIDR_SIZE); > > + if (!buf) > > + return EINVAL; > > + > > + /* read midr from list of cpus mapped to this pmu */ > > + for (cpu = 0; cpu < cpu__max_present_cpu(); cpu++) { > > + scnprintf(path, sz, "%s/devices/system/cpu/cpu%d"MIDR, > > + sysfs, cpu); > > + > > + file = fopen(path, "r"); > > + if (!file) { > > + pr_debug("fopen failed for file %s\n", path); > > + continue; > > + } > > + > > + if (!fgets(buf, MIDR_SIZE, file)) { > > + fclose(file); > > + continue; > > + } > > + fclose(file); > > + > > + /* Ignore/clear Variant[23:20] and > > + * Revision[3:0] of MIDR > > + */ > > + midr = strtoul(buf, NULL, 16); > > + midr &= (~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK)); > > + scnprintf(buffer, MIDR_SIZE, "0x%016lx", midr); > > + /* got midr break loop */ > > + break; > > + } > > + > > + if (!midr) { > > + pr_err("failed to get cpuid string\n"); > > + free(buf); > > + return EINVAL; > > + } > > + return 0; > > +} > > + > > -- - Arnaldo
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: > > > > > Hi all, > > > > > > > > > > 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 > > > > > > > > > > Hi Jirka, > > > > > > > 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. - Arnaldo
> -----Original Message----- > From: linux-perf-users-owner@vger.kernel.org > <linux-perf-users-owner@vger.kernel.org> On Behalf Of Jiri Olsa > Sent: 2019年12月11日 1:09 > To: John Garry <john.garry@huawei.com> > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>; > peterz@infradead.org; mingo@redhat.com; > alexander.shishkin@linux.intel.com; namhyung@kernel.org; > mark.rutland@arm.com; will@kernel.org; linux-kernel@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>; > linux-perf-users@vger.kernel.org > Subject: Re: perf top for arm64? > > 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: > > > > Hi all, > > > > > > > > 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 > > > > > > > Hi Jirka, > > > > > 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? Hi Jirka, I reported metricgroup cannot work on ARM64 before, however, no one can come up with a solution, could you take a look how to fix it? Thanks a lot! You can refer to below link for more info: [1] https://www.spinics.net/lists/linux-perf-users/msg09190.html (NACK by Will Deason) [2] https://www.spinics.net/lists/linux-perf-users/msg09324.html Best Regards, Joakim Zhang > jirka
Hi John, On 2019/12/11 1: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: >>>> Hi all, >>>> >>>> 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 >>> >> >> Hi Jirka, >> >>> 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? I've also seen the similar problem when I was looking to add support for 'perf kvm stat' on arm64 [1] (which though got stuck due to some other reasons for a very long time :( It would be great if your patch can address this issue! [1] https://lore.kernel.org/patchwork/patch/1087531/ Thanks, Zenghui
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 >
> >> 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, >
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 >
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 > > -- - Arnaldo
diff --git a/tools/perf/arch/arm64/util/header.c b/tools/perf/arch/arm64/util/header.c index a32e4b72a98f..ecd1f86e29cc 100644 --- a/tools/perf/arch/arm64/util/header.c +++ b/tools/perf/arch/arm64/util/header.c @@ -1,10 +1,12 @@ #include <stdio.h> #include <stdlib.h> #include <perf/cpumap.h> +#include <util/cpumap.h> #include <internal/cpumap.h> #include <api/fs/fs.h> #include "debug.h" #include "header.h" +#include <errno.h> #define MIDR "/regs/identification/midr_el1" #define MIDR_SIZE 19 @@ -12,6 +14,59 @@ #define MIDR_VARIANT_SHIFT 20 #define MIDR_VARIANT_MASK (0xf << MIDR_VARIANT_SHIFT) +int +get_cpuid(char *buffer, size_t sz) +{ + char *buf = NULL; + char path[PATH_MAX]; + const char *sysfs = sysfs__mountpoint(); + int cpu; + u64 midr = 0; + FILE *file; + + if (!sysfs) + return EINVAL; + + buf = malloc(MIDR_SIZE); + if (!buf) + return EINVAL; + + /* read midr from list of cpus mapped to this pmu */ + for (cpu = 0; cpu < cpu__max_present_cpu(); cpu++) { + scnprintf(path, sz, "%s/devices/system/cpu/cpu%d"MIDR, + sysfs, cpu); + + file = fopen(path, "r"); + if (!file) { + pr_debug("fopen failed for file %s\n", path); + continue; + } + + if (!fgets(buf, MIDR_SIZE, file)) { + fclose(file); + continue; + } + fclose(file); + + /* Ignore/clear Variant[23:20] and + * Revision[3:0] of MIDR + */ + midr = strtoul(buf, NULL, 16); + midr &= (~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK)); + scnprintf(buffer, MIDR_SIZE, "0x%016lx", midr); + /* got midr break loop */ + break; + } + + if (!midr) { + pr_err("failed to get cpuid string\n"); + free(buf); + return EINVAL; + } + return 0; +} +
Hi all, 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 That's v5.5-rc1 release. It seems that we are just missing an arm64 version of get_cpuid() - with the patch below, I now get as hoped: PerfTop: 32857 irqs/sec kernel:85.0% exact: 0.0% lost: 0/0 drop: 0/0 [4000Hz cycles], (all, 64 CPUs) ------------------------------------------------------------------------------- 8.99% [kernel] [k] arm_smmu_cmdq_issue_cmdlist 5.80% [kernel] [k] __softirqentry_text_start 4.49% [kernel] [k] _raw_spin_unlock_irqrestore 3.48% [kernel] [k] el0_svc_common.constprop.2 3.37% [kernel] [k] _raw_write_lock_irqsave 3.28% [kernel] [k] __local_bh_enable_ip 3.05% [kernel] [k] __blk_complete_request 2.07% [kernel] [k] queued_spin_lock_slowpath 1.93% [vdso] [.] 0x0000000000000484 Was this just missed? Or is there a good reason to omit? Thanks, John --->8--- Subject: [PATCH] perf: Add perf top support for arm64 Copied from get_cpuid_str() essentially... Signed-off-by: John Garry <john.garry@huawei.com>