| Message ID | 20240925141357.1033087-1-james.clark@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Event parsing fixes | expand |
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 > > Changes since v7: > * Resolve conflicts and rebase onto perf-tools-next 1de5b5dcb835 > * Fix build error by using the new perf_pmu__is_fake() > > Changes since v6: > * Fix empty PMU name in perf report > * Rebase onto perf-tools-next 003265bb6f02 > > Changes since v5: > * Test on x86 non hybrid > * Assume 1 PMU in the test when no PMUs expose /cpus file > > Changes since v4: > > * Hide all <not supported> default events when not verbose > * Remove previous note about <not supported> behavior from the cover > letter and replace it with a new note about the new behavior > > Changes since v3: > > * Rebase onto perf-tools-next 6236ebe07 > * Fix Intel TPEBS counting mode test > * Fix arm-spe build > * Add support for DT devices in stat test > * Add a new test for hybrid perf stat default arguments > > Ian Rogers (5): > perf evsel: Add alternate_hw_config and use in evsel__match > perf stat: Uniquify event name improvements > perf stat: Remove evlist__add_default_attrs use strings > perf evsel x86: Make evsel__has_perf_metrics work for legacy events > perf evsel: Remove pmu_name > > James Clark (2): > perf test: Make stat test work on DT devices > perf test: Add a test for default perf stat command > > tools/perf/arch/arm64/util/arm-spe.c | 4 +- > tools/perf/arch/x86/util/evlist.c | 74 +---- > tools/perf/arch/x86/util/evsel.c | 35 ++- > tools/perf/builtin-diff.c | 6 +- > tools/perf/builtin-stat.c | 291 +++++++----------- > tools/perf/tests/parse-events.c | 2 +- > tools/perf/tests/shell/stat.sh | 37 ++- > .../perf/tests/shell/test_stat_intel_tpebs.sh | 11 +- > tools/perf/util/evlist.c | 46 +-- > tools/perf/util/evlist.h | 12 - > tools/perf/util/evsel.c | 28 +- > tools/perf/util/evsel.h | 22 +- > tools/perf/util/metricgroup.c | 4 +- > tools/perf/util/parse-events.c | 60 ++-- > tools/perf/util/parse-events.h | 8 +- > tools/perf/util/parse-events.y | 2 +- > tools/perf/util/pmu.c | 8 +- > tools/perf/util/pmu.h | 3 +- > tools/perf/util/stat-display.c | 109 +++++-- > tools/perf/util/stat-shadow.c | 14 +- > tools/perf/util/stat.c | 2 +- > 21 files changed, 363 insertions(+), 415 deletions(-) > > -- > 2.34.1 >
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
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. Changes since v7: * Resolve conflicts and rebase onto perf-tools-next 1de5b5dcb835 * Fix build error by using the new perf_pmu__is_fake() Changes since v6: * Fix empty PMU name in perf report * Rebase onto perf-tools-next 003265bb6f02 Changes since v5: * Test on x86 non hybrid * Assume 1 PMU in the test when no PMUs expose /cpus file Changes since v4: * Hide all <not supported> default events when not verbose * Remove previous note about <not supported> behavior from the cover letter and replace it with a new note about the new behavior Changes since v3: * Rebase onto perf-tools-next 6236ebe07 * Fix Intel TPEBS counting mode test * Fix arm-spe build * Add support for DT devices in stat test * Add a new test for hybrid perf stat default arguments Ian Rogers (5): perf evsel: Add alternate_hw_config and use in evsel__match perf stat: Uniquify event name improvements perf stat: Remove evlist__add_default_attrs use strings perf evsel x86: Make evsel__has_perf_metrics work for legacy events perf evsel: Remove pmu_name James Clark (2): perf test: Make stat test work on DT devices perf test: Add a test for default perf stat command tools/perf/arch/arm64/util/arm-spe.c | 4 +- tools/perf/arch/x86/util/evlist.c | 74 +---- tools/perf/arch/x86/util/evsel.c | 35 ++- tools/perf/builtin-diff.c | 6 +- tools/perf/builtin-stat.c | 291 +++++++----------- tools/perf/tests/parse-events.c | 2 +- tools/perf/tests/shell/stat.sh | 37 ++- .../perf/tests/shell/test_stat_intel_tpebs.sh | 11 +- tools/perf/util/evlist.c | 46 +-- tools/perf/util/evlist.h | 12 - tools/perf/util/evsel.c | 28 +- tools/perf/util/evsel.h | 22 +- tools/perf/util/metricgroup.c | 4 +- tools/perf/util/parse-events.c | 60 ++-- tools/perf/util/parse-events.h | 8 +- tools/perf/util/parse-events.y | 2 +- tools/perf/util/pmu.c | 8 +- tools/perf/util/pmu.h | 3 +- tools/perf/util/stat-display.c | 109 +++++-- tools/perf/util/stat-shadow.c | 14 +- tools/perf/util/stat.c | 2 +- 21 files changed, 363 insertions(+), 415 deletions(-)