Re: [PATCH v8 0/7] Event parsing fixes

[James Clark]

On 26/09/2024 5:38 am, Namhyung Kim wrote:
> On Wed, Sep 25, 2024 at 03:13:38PM +0100, James Clark wrote:
> > I rebased this one and made some other fixes so that I could test it,
> > so I thought I'd repost it here in case it's helpful. I also added a
> > new test.
> >
> > But for the testing it all looks ok.
> >
> > There is one small difference where it now hides _all_ default
> > <not supported> events, when previously it would only hide some
> > selected subset of events like "stalled-cycles-frontend". I think
> > this is now more consistent across platforms because, for example,
> > Apple M only has cycles and instructions, and the rest of the
> > default events would always show as <not supported> there.
> >
> > Tested on Raptor Lake, Kaby Lake, Juno, N1, Ampere (with the DSU
> > cycles PMU) and I also faked an Apple M on Juno.
>
> Hmm.. I got a segfault with 'perf stat true' on my Zen2 box.
>
>    $ gdb -q -args ./perf stat true
>    Reading symbols from ./perf...
>    (gdb) r
>    Starting program: /home/namhyung/tmp/perf stat true
>    [Thread debugging using libthread_db enabled]
>    Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>    
>    Program received signal SIGSEGV, Segmentation fault.
>    0x00005555557e04b5 in perf_pmu__is_fake (pmu=0x0) at util/pmu.c:1173
>    1173		return pmu->type == PERF_PMU_TYPE_FAKE;
>    (gdb) bt
>    #0  0x00005555557e04b5 in perf_pmu__is_fake (pmu=0x0) at util/pmu.c:1173
>    #1  0x00005555558c1b8b in evsel__sys_has_perf_metrics (evsel=0x5555560cc4b0) at arch/x86/util/evsel.c:50
>    #2  0x00005555558c1c33 in arch_evsel__must_be_in_group (evsel=0x5555560cc4b0) at arch/x86/util/evsel.c:64
>    #3  0x00005555557773a4 in parse_events__sort_events_and_fix_groups (list=0x7fffffff9ad0) at util/parse-events.c:2098
>    #4  0x0000555555777793 in __parse_events (evlist=0x5555560aa880,
>        str=0x5555558d6498 "context-switches,cpu-migrations,page-faults,instructions,cycles,stalled-cycles-frontend,stalled-cycles-backend,branches,branch-misses", pmu_filter=0x0, err=0x7fffffff9bd0, fake_pmu=false, warn_if_reordered=true, fake_tp=false) at util/parse-events.c:2186
>    #5  0x00005555555c787f in parse_events (evlist=0x5555560aa880,
>        str=0x5555558d6498 "context-switches,cpu-migrations,page-faults,instructions,cycles,stalled-cycles-frontend,stalled-cycles-backend,branches,branch-misses", err=0x7fffffff9bd0) at util/parse-events.h:41
>    #6  0x00005555555cce39 in add_default_events () at builtin-stat.c:1977
>    #7  0x00005555555cf928 in cmd_stat (argc=1, argv=0x7fffffffd840) at builtin-stat.c:2724
>    #8  0x000055555564cb81 in run_builtin (p=0x555556024548 <commands+360>, argc=2, argv=0x7fffffffd840) at perf.c:351
>    #9  0x000055555564ce28 in handle_internal_command (argc=2, argv=0x7fffffffd840) at perf.c:404
>    #10 0x000055555564cf81 in run_argv (argcp=0x7fffffffd63c, argv=0x7fffffffd630) at perf.c:448
>    #11 0x000055555564d2cf in main (argc=2, argv=0x7fffffffd840) at perf.c:562
>
> Thanks,
> Namhyung

Oh yeah that's from the rebase. Looks like pmu is never NULL here on Arm 
and hybrid x86 so I didn't see it, but I do see it on non-hybrid x86. It 
just needs a NULL check:

  bool evsel__sys_has_perf_metrics(const struct evsel *evsel)
  {
  ...
  	pmu = evsel->pmu;
	if (pmu && perf_pmu__is_fake(pmu))
		pmu = NULL;

I also noticed a new issue though, the change to make all default stat 
events skippable=true hides permission errors, so now instead of the 
hint to look at perf_event_paranoid you just get nothing, so I need to 
fix that:

  $ cat /proc/sys/kernel/perf_event_paranoid
  4

  $ perf stat true

    Performance counter stats for 'true':


       0.000431617 seconds time elapsed

       0.000445000 seconds user
       0.000000000 seconds sys

I'm thinking it either needs to never skip EPERM or some other errors, 
or maybe make cycles always non skippable. Probably the first option is 
better.

James