linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix num_events calculation in lazy loading
@ 2024-05-10  2:47 Jia He
  2024-05-10  2:47 ` [PATCH 1/2] perf pmu: Allow finishing loading json events when !events_table Jia He
  2024-05-10  2:47 ` [PATCH 2/2] perf pmu: Fix num_events calculation Jia He
  0 siblings, 2 replies; 8+ messages in thread
From: Jia He @ 2024-05-10  2:47 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Kan Liang, James Clark, linux-perf-users,
	linux-kernel, Jia He

I noticed "perf list" reported the error as follows on an Armv8 Neoverse
N2 server:
Unexpected event smmuv3_pmcg_3f062/smmuv3_pmcg_3f062/transaction//

The root cause is due to the incorrect calculation in
perf_pmu__num_events().

Jia He (2):
  perf pmu: Allow finishing loading json events when !events_table
  perf pmu: Fix num_events calculation

 tools/perf/util/pmu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.34.1


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

* [PATCH 1/2] perf pmu: Allow finishing loading json events when !events_table
  2024-05-10  2:47 [PATCH 0/2] Fix num_events calculation in lazy loading Jia He
@ 2024-05-10  2:47 ` Jia He
  2024-05-10  2:47 ` [PATCH 2/2] perf pmu: Fix num_events calculation Jia He
  1 sibling, 0 replies; 8+ messages in thread
From: Jia He @ 2024-05-10  2:47 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Kan Liang, James Clark, linux-perf-users,
	linux-kernel, Jia He

Otherwise, cpu_aliases_added is never set to true on an Arm v8a
Neoverse N2 server.

Signed-off-by: Jia He <justin.he@arm.com>
---
 tools/perf/util/pmu.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index f39cbbc1a7ec..a1eef7b2e389 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -915,13 +915,11 @@ void pmu_add_cpu_aliases_table(struct perf_pmu *pmu, const struct pmu_events_tab
 
 static void pmu_add_cpu_aliases(struct perf_pmu *pmu)
 {
-	if (!pmu->events_table)
-		return;
-
 	if (pmu->cpu_aliases_added)
 		return;
 
-	pmu_add_cpu_aliases_table(pmu, pmu->events_table);
+	if (pmu->events_table)
+		pmu_add_cpu_aliases_table(pmu, pmu->events_table);
 	pmu->cpu_aliases_added = true;
 }
 
-- 
2.34.1


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

* [PATCH 2/2] perf pmu: Fix num_events calculation
  2024-05-10  2:47 [PATCH 0/2] Fix num_events calculation in lazy loading Jia He
  2024-05-10  2:47 ` [PATCH 1/2] perf pmu: Allow finishing loading json events when !events_table Jia He
@ 2024-05-10  2:47 ` Jia He
  2024-05-10  6:16   ` Ian Rogers
  1 sibling, 1 reply; 8+ messages in thread
From: Jia He @ 2024-05-10  2:47 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim
  Cc: Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Kan Liang, James Clark, linux-perf-users,
	linux-kernel, Jia He

When pe is NULL in the function perf_pmu__new_alias(), the total number
of events is added to loaded_json_aliases. However, if pmu->events_table
is NULL and cpu_aliases_added is false, the calculation for the events
number in perf_pmu__num_events() is incorrect.

Then cause the error report after "perf list":
Unexpected event smmuv3_pmcg_3f062/smmuv3_pmcg_3f062/transaction//

Fix it by adding loaded_json_aliases in the calculation under the
mentioned conditions.

Test it also with "perf bench internals pmu-scan" and there is no
regression.

Fixes: e6ff1eed3584 ("perf pmu: Lazily add JSON events")
Signed-off-by: Jia He <justin.he@arm.com>
---
 tools/perf/util/pmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index a1eef7b2e389..a53224e2ce7e 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1639,6 +1639,8 @@ size_t perf_pmu__num_events(struct perf_pmu *pmu)
 		 nr += pmu->loaded_json_aliases;
 	else if (pmu->events_table)
 		nr += pmu_events_table__num_events(pmu->events_table, pmu) - pmu->loaded_json_aliases;
+	else
+		nr += pmu->loaded_json_aliases;
 
 	return pmu->selectable ? nr + 1 : nr;
 }
-- 
2.34.1


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

* Re: [PATCH 2/2] perf pmu: Fix num_events calculation
  2024-05-10  2:47 ` [PATCH 2/2] perf pmu: Fix num_events calculation Jia He
@ 2024-05-10  6:16   ` Ian Rogers
  2024-05-10  6:52     ` Justin He
  0 siblings, 1 reply; 8+ messages in thread
From: Ian Rogers @ 2024-05-10  6:16 UTC (permalink / raw)
  To: Jia He
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Adrian Hunter, Kan Liang, James Clark, linux-perf-users,
	linux-kernel

On Thu, May 9, 2024 at 7:47 PM Jia He <justin.he@arm.com> wrote:
>
> When pe is NULL in the function perf_pmu__new_alias(), the total number
> of events is added to loaded_json_aliases. However, if pmu->events_table
> is NULL and cpu_aliases_added is false, the calculation for the events
> number in perf_pmu__num_events() is incorrect.
>
> Then cause the error report after "perf list":
> Unexpected event smmuv3_pmcg_3f062/smmuv3_pmcg_3f062/transaction//
>
> Fix it by adding loaded_json_aliases in the calculation under the
> mentioned conditions.
>
> Test it also with "perf bench internals pmu-scan" and there is no
> regression.
>
> Fixes: e6ff1eed3584 ("perf pmu: Lazily add JSON events")
> Signed-off-by: Jia He <justin.he@arm.com>
> ---
>  tools/perf/util/pmu.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index a1eef7b2e389..a53224e2ce7e 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1639,6 +1639,8 @@ size_t perf_pmu__num_events(struct perf_pmu *pmu)
>                  nr += pmu->loaded_json_aliases;
>         else if (pmu->events_table)
>                 nr += pmu_events_table__num_events(pmu->events_table, pmu) - pmu->loaded_json_aliases;
> +       else
> +               nr += pmu->loaded_json_aliases;

Thanks for working on this! The "struct pmu_event *pe" in new_alias is
an entry from the json data, and "pmu->events_table" should NULL if
there is no json data. I believe the code is assuming that these lines
aren't necessary as it shouldn't be possible to load json data if the
json events table doesn't exist for the PMU - if there is no json data
then loaded_json_aliases should be 0 and no addition is necessary. I'm
wondering why this case isn't true for you.

Thanks,
Ian

>
>         return pmu->selectable ? nr + 1 : nr;
>  }
> --
> 2.34.1
>

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

* RE: [PATCH 2/2] perf pmu: Fix num_events calculation
  2024-05-10  6:16   ` Ian Rogers
@ 2024-05-10  6:52     ` Justin He
  2024-05-10 20:41       ` Ian Rogers
  0 siblings, 1 reply; 8+ messages in thread
From: Justin He @ 2024-05-10  6:52 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Adrian Hunter, Kan Liang, James Clark,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	nd

Hi, Ian

> -----Original Message-----
> From: Ian Rogers <irogers@google.com>
> Sent: Friday, May 10, 2024 2:17 PM
> To: Justin He <Justin.He@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>; Ingo Molnar <mingo@redhat.com>;
> Arnaldo Carvalho de Melo <acme@kernel.org>; Namhyung Kim
> <namhyung@kernel.org>; Mark Rutland <Mark.Rutland@arm.com>; Alexander
> Shishkin <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>;
> Adrian Hunter <adrian.hunter@intel.com>; Kan Liang
> <kan.liang@linux.intel.com>; James Clark <James.Clark@arm.com>;
> linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] perf pmu: Fix num_events calculation
> 
> On Thu, May 9, 2024 at 7:47 PM Jia He <justin.he@arm.com> wrote:
> >
> > When pe is NULL in the function perf_pmu__new_alias(), the total
> > number of events is added to loaded_json_aliases. However, if
> > pmu->events_table is NULL and cpu_aliases_added is false, the
> > calculation for the events number in perf_pmu__num_events() is incorrect.
> >
> > Then cause the error report after "perf list":
> > Unexpected event
> smmuv3_pmcg_3f062/smmuv3_pmcg_3f062/transaction//
> >
> > Fix it by adding loaded_json_aliases in the calculation under the
> > mentioned conditions.
> >
> > Test it also with "perf bench internals pmu-scan" and there is no
> > regression.
> >
> > Fixes: e6ff1eed3584 ("perf pmu: Lazily add JSON events")
> > Signed-off-by: Jia He <justin.he@arm.com>
> > ---
> >  tools/perf/util/pmu.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index
> > a1eef7b2e389..a53224e2ce7e 100644
> > --- a/tools/perf/util/pmu.c
> > +++ b/tools/perf/util/pmu.c
> > @@ -1639,6 +1639,8 @@ size_t perf_pmu__num_events(struct perf_pmu
> *pmu)
> >                  nr += pmu->loaded_json_aliases;
> >         else if (pmu->events_table)
> >                 nr +=
> pmu_events_table__num_events(pmu->events_table,
> > pmu) - pmu->loaded_json_aliases;
> > +       else
> > +               nr += pmu->loaded_json_aliases;
> 
> Thanks for working on this! The "struct pmu_event *pe" in new_alias is an entry
> from the json data, and "pmu->events_table" should NULL if there is no json
> data. I believe the code is assuming that these lines aren't necessary as it
> shouldn't be possible to load json data if the json events table doesn't exist for
> the PMU - if there is no json data then loaded_json_aliases should be 0 and no
> addition is necessary. I'm wondering why this case isn't true for you.
On my Armv8a N2 server, "pmu->events_table" is NULL because perf_pmu__find_events_table()
return NULL.

I also noticed that pmu->loaded_json_aliases is *not* 0. The missing adding calculation will cause
perf_pmu__num_events() less than normal case and will trigger latter check failure in
perf_pmus__print_pmu_events__callback().
At last, perf list will report many lines similar as:
Unexpected event smmuv3_pmcg_3f062/smmuv3_pmcg_3f062/transaction//


--
Cheers,
Justin (Jia He)

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

* Re: [PATCH 2/2] perf pmu: Fix num_events calculation
  2024-05-10  6:52     ` Justin He
@ 2024-05-10 20:41       ` Ian Rogers
  2024-05-11  0:50         ` Ian Rogers
  0 siblings, 1 reply; 8+ messages in thread
From: Ian Rogers @ 2024-05-10 20:41 UTC (permalink / raw)
  To: Justin He, John Garry
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Adrian Hunter, Kan Liang, James Clark,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	nd

On Thu, May 9, 2024 at 11:52 PM Justin He <Justin.He@arm.com> wrote:
>
> Hi, Ian
>
> > -----Original Message-----
> > From: Ian Rogers <irogers@google.com>
> > Sent: Friday, May 10, 2024 2:17 PM
> > To: Justin He <Justin.He@arm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>; Ingo Molnar <mingo@redhat.com>;
> > Arnaldo Carvalho de Melo <acme@kernel.org>; Namhyung Kim
> > <namhyung@kernel.org>; Mark Rutland <Mark.Rutland@arm.com>; Alexander
> > Shishkin <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>;
> > Adrian Hunter <adrian.hunter@intel.com>; Kan Liang
> > <kan.liang@linux.intel.com>; James Clark <James.Clark@arm.com>;
> > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/2] perf pmu: Fix num_events calculation
> >
> > On Thu, May 9, 2024 at 7:47 PM Jia He <justin.he@arm.com> wrote:
> > >
> > > When pe is NULL in the function perf_pmu__new_alias(), the total
> > > number of events is added to loaded_json_aliases. However, if
> > > pmu->events_table is NULL and cpu_aliases_added is false, the
> > > calculation for the events number in perf_pmu__num_events() is incorrect.
> > >
> > > Then cause the error report after "perf list":
> > > Unexpected event
> > smmuv3_pmcg_3f062/smmuv3_pmcg_3f062/transaction//
> > >
> > > Fix it by adding loaded_json_aliases in the calculation under the
> > > mentioned conditions.
> > >
> > > Test it also with "perf bench internals pmu-scan" and there is no
> > > regression.
> > >
> > > Fixes: e6ff1eed3584 ("perf pmu: Lazily add JSON events")
> > > Signed-off-by: Jia He <justin.he@arm.com>
> > > ---
> > >  tools/perf/util/pmu.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index
> > > a1eef7b2e389..a53224e2ce7e 100644
> > > --- a/tools/perf/util/pmu.c
> > > +++ b/tools/perf/util/pmu.c
> > > @@ -1639,6 +1639,8 @@ size_t perf_pmu__num_events(struct perf_pmu
> > *pmu)
> > >                  nr += pmu->loaded_json_aliases;
> > >         else if (pmu->events_table)
> > >                 nr +=
> > pmu_events_table__num_events(pmu->events_table,
> > > pmu) - pmu->loaded_json_aliases;
> > > +       else
> > > +               nr += pmu->loaded_json_aliases;
> >
> > Thanks for working on this! The "struct pmu_event *pe" in new_alias is an entry
> > from the json data, and "pmu->events_table" should NULL if there is no json
> > data. I believe the code is assuming that these lines aren't necessary as it
> > shouldn't be possible to load json data if the json events table doesn't exist for
> > the PMU - if there is no json data then loaded_json_aliases should be 0 and no
> > addition is necessary. I'm wondering why this case isn't true for you.
> On my Armv8a N2 server, "pmu->events_table" is NULL because perf_pmu__find_events_table()
> return NULL.
>
> I also noticed that pmu->loaded_json_aliases is *not* 0. The missing adding calculation will cause
> perf_pmu__num_events() less than normal case and will trigger latter check failure in
> perf_pmus__print_pmu_events__callback().
> At last, perf list will report many lines similar as:
> Unexpected event smmuv3_pmcg_3f062/smmuv3_pmcg_3f062/transaction//

The issue is the sys events which currently are ARM only. Here is an
lldb stack trace:
```
    frame #6: 0x0000aaaaabc9b49c
perf`__assert_fail(assertion="pmu->events_table",
file="tools/perf/util/pmu.c", line=522, function="int
perf_pmu__new_alias(struct perf_pmu *, const char *, const char *,
const char *, FILE *, const struct pmu_event *)")
    frame #7: 0x0000aaaaab41e018
perf`perf_pmu__new_alias(pmu=0x00007377e7e09b40,
name="hnf_brd_snoops_sent", desc="Counts number of multicast snoops
sent (not including SF back invalidation)", val="eventid=9,type=5",
val_fd=0x0000000000000000, pe=0x0000ffffffffde88) at pmu.c:522:3
    frame #8: 0x0000aaaaab4175cc
perf`pmu_add_sys_aliases_iter_fn(pe=0x0000ffffffffde88,
table=0x0000aaaaac299bb0, vdata=0x00007377e7e09b40) at pmu.c:939:3
    frame #9: 0x0000aaaaaaf6d000
perf`pmu_events_table__for_each_event_pmu(table=0x0000aaaaac299bb0,
pmu=0x0000aaaaac299238, fn=(perf`pmu_add_sys_aliases_iter_fn at
pmu.c:931), data=0x00007377e7e09b40) at pmu-events.c:5994:23
    frame #10: 0x0000aaaaaaf6cd78
perf`pmu_events_table__for_each_event(table=0x0000aaaaac299bb0,
pmu=0x0000000000000000, fn=(perf`pmu_add_sys_aliases_iter_fn at
pmu.c:931), data=0x00007377e7e09b40) at pmu-events.c:6057:23
    frame #11: 0x0000aaaaaaf6ec44
perf`pmu_for_each_sys_event(fn=(perf`pmu_add_sys_aliases_iter_fn at
pmu.c:931), data=0x00007377e7e09b40) at pmu-events.c:6295:27
    frame #12: 0x0000aaaaab41738c
perf`pmu_add_sys_aliases(pmu=0x00007377e7e09b40) at pmu.c:955:2
    frame #13: 0x0000aaaaab4179fc
perf`perf_pmu__lookup(pmus=0x0000aaaaac500970, dirfd=34,
name="arm_cmn_0") at pmu.c:1037:2
    frame #14: 0x0000aaaaab4242d0 perf`perf_pmu__find2(dirfd=34,
name="arm_cmn_0") at pmus.c:161:9
    frame #15: 0x0000aaaaab421614 perf`pmu_read_sysfs(core_only=false)
at pmus.c:209:3
    frame #16: 0x0000aaaaab42278c
perf`perf_pmus__scan_skip_duplicates(pmu=0x0000000000000000) at
pmus.c:297:3
    frame #17: 0x0000aaaaab422078
perf`perf_pmus__print_pmu_events(print_cb=0x0000ffffffffec68,
print_state=0x0000ffffffffecd0) at pmus.c:462:16
    frame #18: 0x0000aaaaab425d9c
perf`print_events(print_cb=0x0000ffffffffec68,
print_state=0x0000ffffffffecd0) at print-events.c:409:2
    frame #19: 0x0000aaaaab093ef0 perf`cmd_list(argc=0,
argv=0x0000fffffffff340) at builtin-list.c:592:3
    frame #20: 0x0000aaaaaaf5b490
perf`run_builtin(p=0x0000aaaaac4e8220, argc=1,
argv=0x0000fffffffff340) at perf.c:349:11
    frame #21: 0x0000aaaaaaf5a3e0 perf`handle_internal_command(argc=1,
argv=0x0000fffffffff340) at perf.c:402:8
    frame #22: 0x0000aaaaaaf5b1a0
perf`run_argv(argcp=0x0000fffffffff1c8, argv=0x0000fffffffff1c0) at
perf.c:446:2
    frame #23: 0x0000aaaaaaf59f44 perf`main(argc=1,
argv=0x0000fffffffff340) at perf.c:562:3
```

As such the fix here is incomplete. There may be both sys json events
(detected by PMU/id name) and CPU json events (detected by CPUID). I'm
looking into a fix.

Thanks,
Ian

> --
> Cheers,
> Justin (Jia He)

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

* Re: [PATCH 2/2] perf pmu: Fix num_events calculation
  2024-05-10 20:41       ` Ian Rogers
@ 2024-05-11  0:50         ` Ian Rogers
  2024-05-11 15:52           ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 8+ messages in thread
From: Ian Rogers @ 2024-05-11  0:50 UTC (permalink / raw)
  To: Justin He, John Garry
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Adrian Hunter, Kan Liang, James Clark,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	nd

On Fri, May 10, 2024 at 1:41 PM Ian Rogers <irogers@google.com> wrote:
>
> On Thu, May 9, 2024 at 11:52 PM Justin He <Justin.He@arm.com> wrote:
> >
> > Hi, Ian
> >
> > > -----Original Message-----
> > > From: Ian Rogers <irogers@google.com>
> > > Sent: Friday, May 10, 2024 2:17 PM
> > > To: Justin He <Justin.He@arm.com>
> > > Cc: Peter Zijlstra <peterz@infradead.org>; Ingo Molnar <mingo@redhat.com>;
> > > Arnaldo Carvalho de Melo <acme@kernel.org>; Namhyung Kim
> > > <namhyung@kernel.org>; Mark Rutland <Mark.Rutland@arm.com>; Alexander
> > > Shishkin <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>;
> > > Adrian Hunter <adrian.hunter@intel.com>; Kan Liang
> > > <kan.liang@linux.intel.com>; James Clark <James.Clark@arm.com>;
> > > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 2/2] perf pmu: Fix num_events calculation
> > >
> > > On Thu, May 9, 2024 at 7:47 PM Jia He <justin.he@arm.com> wrote:
> > > >
> > > > When pe is NULL in the function perf_pmu__new_alias(), the total
> > > > number of events is added to loaded_json_aliases. However, if
> > > > pmu->events_table is NULL and cpu_aliases_added is false, the
> > > > calculation for the events number in perf_pmu__num_events() is incorrect.
> > > >
> > > > Then cause the error report after "perf list":
> > > > Unexpected event
> > > smmuv3_pmcg_3f062/smmuv3_pmcg_3f062/transaction//
> > > >
> > > > Fix it by adding loaded_json_aliases in the calculation under the
> > > > mentioned conditions.
> > > >
> > > > Test it also with "perf bench internals pmu-scan" and there is no
> > > > regression.
> > > >
> > > > Fixes: e6ff1eed3584 ("perf pmu: Lazily add JSON events")
> > > > Signed-off-by: Jia He <justin.he@arm.com>
> > > > ---
> > > >  tools/perf/util/pmu.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index
> > > > a1eef7b2e389..a53224e2ce7e 100644
> > > > --- a/tools/perf/util/pmu.c
> > > > +++ b/tools/perf/util/pmu.c
> > > > @@ -1639,6 +1639,8 @@ size_t perf_pmu__num_events(struct perf_pmu
> > > *pmu)
> > > >                  nr += pmu->loaded_json_aliases;
> > > >         else if (pmu->events_table)
> > > >                 nr +=
> > > pmu_events_table__num_events(pmu->events_table,
> > > > pmu) - pmu->loaded_json_aliases;
> > > > +       else
> > > > +               nr += pmu->loaded_json_aliases;
> > >
> > > Thanks for working on this! The "struct pmu_event *pe" in new_alias is an entry
> > > from the json data, and "pmu->events_table" should NULL if there is no json
> > > data. I believe the code is assuming that these lines aren't necessary as it
> > > shouldn't be possible to load json data if the json events table doesn't exist for
> > > the PMU - if there is no json data then loaded_json_aliases should be 0 and no
> > > addition is necessary. I'm wondering why this case isn't true for you.
> > On my Armv8a N2 server, "pmu->events_table" is NULL because perf_pmu__find_events_table()
> > return NULL.
> >
> > I also noticed that pmu->loaded_json_aliases is *not* 0. The missing adding calculation will cause
> > perf_pmu__num_events() less than normal case and will trigger latter check failure in
> > perf_pmus__print_pmu_events__callback().
> > At last, perf list will report many lines similar as:
> > Unexpected event smmuv3_pmcg_3f062/smmuv3_pmcg_3f062/transaction//
>
> The issue is the sys events which currently are ARM only. Here is an
> lldb stack trace:
> ```
>     frame #6: 0x0000aaaaabc9b49c
> perf`__assert_fail(assertion="pmu->events_table",
> file="tools/perf/util/pmu.c", line=522, function="int
> perf_pmu__new_alias(struct perf_pmu *, const char *, const char *,
> const char *, FILE *, const struct pmu_event *)")
>     frame #7: 0x0000aaaaab41e018
> perf`perf_pmu__new_alias(pmu=0x00007377e7e09b40,
> name="hnf_brd_snoops_sent", desc="Counts number of multicast snoops
> sent (not including SF back invalidation)", val="eventid=9,type=5",
> val_fd=0x0000000000000000, pe=0x0000ffffffffde88) at pmu.c:522:3
>     frame #8: 0x0000aaaaab4175cc
> perf`pmu_add_sys_aliases_iter_fn(pe=0x0000ffffffffde88,
> table=0x0000aaaaac299bb0, vdata=0x00007377e7e09b40) at pmu.c:939:3
>     frame #9: 0x0000aaaaaaf6d000
> perf`pmu_events_table__for_each_event_pmu(table=0x0000aaaaac299bb0,
> pmu=0x0000aaaaac299238, fn=(perf`pmu_add_sys_aliases_iter_fn at
> pmu.c:931), data=0x00007377e7e09b40) at pmu-events.c:5994:23
>     frame #10: 0x0000aaaaaaf6cd78
> perf`pmu_events_table__for_each_event(table=0x0000aaaaac299bb0,
> pmu=0x0000000000000000, fn=(perf`pmu_add_sys_aliases_iter_fn at
> pmu.c:931), data=0x00007377e7e09b40) at pmu-events.c:6057:23
>     frame #11: 0x0000aaaaaaf6ec44
> perf`pmu_for_each_sys_event(fn=(perf`pmu_add_sys_aliases_iter_fn at
> pmu.c:931), data=0x00007377e7e09b40) at pmu-events.c:6295:27
>     frame #12: 0x0000aaaaab41738c
> perf`pmu_add_sys_aliases(pmu=0x00007377e7e09b40) at pmu.c:955:2
>     frame #13: 0x0000aaaaab4179fc
> perf`perf_pmu__lookup(pmus=0x0000aaaaac500970, dirfd=34,
> name="arm_cmn_0") at pmu.c:1037:2
>     frame #14: 0x0000aaaaab4242d0 perf`perf_pmu__find2(dirfd=34,
> name="arm_cmn_0") at pmus.c:161:9
>     frame #15: 0x0000aaaaab421614 perf`pmu_read_sysfs(core_only=false)
> at pmus.c:209:3
>     frame #16: 0x0000aaaaab42278c
> perf`perf_pmus__scan_skip_duplicates(pmu=0x0000000000000000) at
> pmus.c:297:3
>     frame #17: 0x0000aaaaab422078
> perf`perf_pmus__print_pmu_events(print_cb=0x0000ffffffffec68,
> print_state=0x0000ffffffffecd0) at pmus.c:462:16
>     frame #18: 0x0000aaaaab425d9c
> perf`print_events(print_cb=0x0000ffffffffec68,
> print_state=0x0000ffffffffecd0) at print-events.c:409:2
>     frame #19: 0x0000aaaaab093ef0 perf`cmd_list(argc=0,
> argv=0x0000fffffffff340) at builtin-list.c:592:3
>     frame #20: 0x0000aaaaaaf5b490
> perf`run_builtin(p=0x0000aaaaac4e8220, argc=1,
> argv=0x0000fffffffff340) at perf.c:349:11
>     frame #21: 0x0000aaaaaaf5a3e0 perf`handle_internal_command(argc=1,
> argv=0x0000fffffffff340) at perf.c:402:8
>     frame #22: 0x0000aaaaaaf5b1a0
> perf`run_argv(argcp=0x0000fffffffff1c8, argv=0x0000fffffffff1c0) at
> perf.c:446:2
>     frame #23: 0x0000aaaaaaf59f44 perf`main(argc=1,
> argv=0x0000fffffffff340) at perf.c:562:3
> ```
>
> As such the fix here is incomplete. There may be both sys json events
> (detected by PMU/id name) and CPU json events (detected by CPUID). I'm
> looking into a fix.

Patch sent:
https://lore.kernel.org/lkml/20240511003601.2666907-1-irogers@google.com/

Thanks,
Ian

> Thanks,
> Ian
>
> > --
> > Cheers,
> > Justin (Jia He)

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

* Re: [PATCH 2/2] perf pmu: Fix num_events calculation
  2024-05-11  0:50         ` Ian Rogers
@ 2024-05-11 15:52           ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-05-11 15:52 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Justin He, John Garry, Peter Zijlstra, Ingo Molnar, Namhyung Kim,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
	Kan Liang, James Clark, linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org, nd

On Fri, May 10, 2024 at 05:50:29PM -0700, Ian Rogers wrote:
> On Fri, May 10, 2024 at 1:41 PM Ian Rogers <irogers@google.com> wrote:
> > As such the fix here is incomplete. There may be both sys json events
> > (detected by PMU/id name) and CPU json events (detected by CPUID). I'm
> > looking into a fix.
> 
> Patch sent:
> https://lore.kernel.org/lkml/20240511003601.2666907-1-irogers@google.com/

I applied the patch to tmp.perf-tools-next, will go to perf-tools-next
soon, would be good for Justin to test and provide a Tested-by.

- Arnaldo

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

end of thread, other threads:[~2024-05-11 15:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-10  2:47 [PATCH 0/2] Fix num_events calculation in lazy loading Jia He
2024-05-10  2:47 ` [PATCH 1/2] perf pmu: Allow finishing loading json events when !events_table Jia He
2024-05-10  2:47 ` [PATCH 2/2] perf pmu: Fix num_events calculation Jia He
2024-05-10  6:16   ` Ian Rogers
2024-05-10  6:52     ` Justin He
2024-05-10 20:41       ` Ian Rogers
2024-05-11  0:50         ` Ian Rogers
2024-05-11 15:52           ` Arnaldo Carvalho de Melo

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