Message-ID: <CAP-5=fV20jibAu82Q3U8FnavHp7dxG=5UvKgMBp7YBe5FG+gXw@mail.gmail.com>
Date:   Wed, 12 Apr 2023 09:35:52 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     "Zhang, Tinghao" <tinghao.zhang@...el.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        "Hunter, Adrian" <adrian.hunter@...el.com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-perf-users <linux-perf-users@...r.kernel.org>,
        "Liang, Kan" <kan.liang@...ux.intel.com>,
        "Yi, Ammy" <ammy.yi@...el.com>
Subject: Re: [PATCH] perf tests: Fix tests in 'Parse event definition strings'

On Wed, Apr 12, 2023 at 3:16 AM Zhang, Tinghao <tinghao.zhang@...el.com> wrote:
>
>
> On 4/11/2023 9:50 PM, Ian Rogers wrote:
>
> On Tue, Apr 11, 2023, 2:49 AM <tinghao.zhang@...el.com> wrote:
>>
>> From: Tinghao Zhang <tinghao.zhang@...el.com>
>>
>> The 'Test event parsing' test set under 'Parse event definion strings'
>> does not apply to hybrid events. Since hybrid events have a dedicated
>> test set 'Test parsing of hybrid CPU events', skip these tests on hybrid
>> platforms.
>>
>> Fixes: 7741e03e808a ("perf test: Parse events break apart tests")
>> Reported-by: Yi Ammy <ammy.yi@...el.com>
>> Signed-off-by: Tinghao Zhang <tinghao.zhang@...el.com>
>
>
> This feels wrong and could make a different problem. Could you send the failure you are seeing? There is a verbose flag (-vv) that you can pass to perf test.
>
> Thanks,
> Ian
>
> Hi Ian,
>
> I think these test cases are not designed for hybrid platforms.  As the current implementation, if one general event type is used on hybrid platform,
> perf tool would generate seperate event for each pmu. But tests in 'Test event parsing' asserts only one evlist entry per event type. Another problem is that for


Hi Tinghao,

So you are right that the tests weren't designed for hybrid platforms
because they didn't exist when the tests were created, but the bigger
issue is what should perf be doing in these cases. On ARM's BIG.little
version of "hybrid" there is a single PMU and generic events that work
on both kind of core. But even with a single core we can get into
issues that an event can be defined for more than one PMU. For
example, a GPU may have an instructions-retired count. In sysfs we'd
have /sys/devices/my-gpu and /sys/devices/cpu. If the user asks for
instructions-retired and has suitable permission, should the perf tool
display instructions-retired for the GPU or just the cpu? My belief is
that we should display my-gpu/instructions-retired/ and
cpu/instructions-retired/. If the user wants to then limit to the CPU
one, they can use the full PMU and event name
cpu/instructions-retired/.

Why does this matter? Well if we have a common plan for how to deal
with events that match on more than 1 PMU then hybrid can just follow
the plan and we can remove all of the hybrid code from the perf tool.
Yes, the tests made an assumption there'd only be 1 of a kind of PMU
for an event, so we need to change the test to assume that there will
be at least 1, or that the number should match the number of PMUs or
core PMUs.

> PERF_TYPE_HARDWARE and PERF_TYPE_HW_CACHE, attr.config has a different format for hybrid events, so we see attr.config errors. Also, it is not applicable
> to put events from different PMUs to one group, but here some tests put general events in one group. As for the patch that first introduces hybrid test cases
> 2541cb63ac0c ("perf tests: Add hybrid cases for 'Parse event definition strings' test") , this test set is skipped. I think it's better if we skip this test set for hybrid pmus.

So I think:
2541cb63ac0c ("perf tests: Add hybrid cases for 'Parse event
definition strings' test")
was added quickly in order that nothing be blocked, but with hindsight
it regrettably didn't make a generic solution that could be used for a
range of PMUs. You may be interested to see that the problem of being
too CPU centric for perf is something reported by Intel's GPU team:
https://www.kernel.org/doc/html/next/gpu/i915.html#issues-hit-with-first-prototype-based-on-core-perf
I'd rather that the code were not hybrid or CPU specific, but
everything was based off the PMU abstraction. For the attributes we'll
need to improve the test to make it more generic to handle matching on
multiple PMUs.

> Below is the perf test result on one Intel Meteor Lake platform
>
>   6: Parse event definition strings                                  :
>   6.1: Test event parsing                                            :
> --- start ---
> test child forked, pid 44965
> running test 0 'syscalls:sys_enter_openat'
> Using CPUID GenuineIntel-6-AA-2
> intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
> running test 1 'syscalls:*'
> running test 2 'r1a'
> FAILED tests/parse-events.c:87 wrong number of entries
> Event test failure: test 2 'r1a'running test 3 '1:1'
> running test 4 'instructions'
> FAILED tests/parse-events.c:107 wrong number of entries
> Event test failure: test 4 'instructions'running test 5 'cycles/period=100000,config2/'
> FAILED tests/parse-events.c:118 wrong number of entries
> Event test failure: test 5 'cycles/period=100000,config2/'running test 6 'faults'
> running test 7 'L1-dcache-load-miss'
> ------------------------------------------------------------
> perf_event_attr:
>   type                             3
>   config                           0x400010000
>   disabled                         1
> ------------------------------------------------------------
> sys_perf_event_open: pid 0  cpu -1  group_fd -1  flags 0x8 = 3
> ------------------------------------------------------------
> perf_event_attr:
>   type                             3
>   config                           0x800010000
>   disabled                         1
> ------------------------------------------------------------
> sys_perf_event_open: pid 0  cpu -1  group_fd -1  flags 0x8
> sys_perf_event_open failed, error -2
> FAILED tests/parse-events.c:152 wrong config
> Event test failure: test 7 'L1-dcache-load-miss'running test 8 'mem:0'
> running test 9 'mem:0:x'
> running test 10 'mem:0:r'
> running test 11 'mem:0:w'
> running test 12 'syscalls:sys_enter_openat:k'
> running test 13 'syscalls:*:u'
> running test 14 'r1a:kp'
> FAILED tests/parse-events.c:87 wrong number of entries
> Event test failure: test 14 'r1a:kp'running test 15 '1:1:hp'
> running test 16 'instructions:h'
> FAILED tests/parse-events.c:107 wrong number of entries
> Event test failure: test 16 'instructions:h'running test 17 'faults:u'
> running test 18 'L1-dcache-load-miss:kp'
> ------------------------------------------------------------
> perf_event_attr:
>   type                             3
>   config                           0x400010000
>   disabled                         1
> ------------------------------------------------------------
> sys_perf_event_open: pid 0  cpu -1  group_fd -1  flags 0x8 = 39
> ------------------------------------------------------------
> perf_event_attr:
>   type                             3
>   config                           0x800010000
>   disabled                         1
> ------------------------------------------------------------
> sys_perf_event_open: pid 0  cpu -1  group_fd -1  flags 0x8
> sys_perf_event_open failed, error -2
> FAILED tests/parse-events.c:152 wrong config
> Event test failure: test 18 'L1-dcache-load-miss:kp'running test 19 'mem:0:u'
> running test 20 'mem:0:x:k'
> running test 21 'mem:0:r:hp'
> running test 22 'mem:0:w:up'
> running test 23 'r1,syscalls:sys_enter_openat:k,1:1:hp'
> FAILED tests/parse-events.c:466 wrong number of entries
> Event test failure: test 23 'r1,syscalls:sys_enter_openat:k,1:1:hp'running test 24 'instructions:G'
> FAILED tests/parse-events.c:107 wrong number of entries
> Event test failure: test 24 'instructions:G'running test 25 'instructions:H'
> FAILED tests/parse-events.c:107 wrong number of entries
> Event test failure: test 25 'instructions:H'running test 26 'mem:0:rw'
> running test 27 'mem:0:rw:kp'
> running test 28 '{instructions:k,cycles:upp}'
> FAILED tests/parse-events.c:679 wrong number of entries
> Event test failure: test 28 '{instructions:k,cycles:upp}'running test 29 '{faults:k,cache-references}:u,cycles:k'
> FAILED tests/parse-events.c:721 wrong number of entries
> Event test failure: test 29 '{faults:k,cache-references}:u,cycles:k'running test 30 'group1{syscalls:sys_enter_openat:H,cycles:kppp},group2{cycles,1:3}:G,instructions:u'
> FAILED tests/parse-events.c:777 wrong number of entries
> Event test failure: test 30 'group1{syscalls:sys_enter_openat:H,cycles:kppp},group2{cycles,1:3}:G,instructions:u'running test 31 '{cycles:u,instructions:kp}:p'
> FAILED tests/parse-events.c:870 wrong number of entries
> Event test failure: test 31 '{cycles:u,instructions:kp}:p'running test 32 '{cycles,instructions}:G,{cycles:G,instructions:G},cycles'
> FAILED tests/parse-events.c:914 wrong number of entries
> Event test failure: test 32 '{cycles,instructions}:G,{cycles:G,instructions:G},cycles'running test 33 '*:*'
> running test 34 '{cycles,cache-misses:G}:H'
> FAILED tests/parse-events.c:1000 wrong number of entries
> Event test failure: test 34 '{cycles,cache-misses:G}:H'running test 35 '{cycles,cache-misses:H}:G'
> FAILED tests/parse-events.c:1040 wrong number of entries
> Event test failure: test 35 '{cycles,cache-misses:H}:G'running test 36 '{cycles:G,cache-misses:H}:u'
> FAILED tests/parse-events.c:1080 wrong number of entries
> Event test failure: test 36 '{cycles:G,cache-misses:H}:u'running test 37 '{cycles:G,cache-misses:H}:uG'
> FAILED tests/parse-events.c:1120 wrong number of entries
> Event test failure: test 37 '{cycles:G,cache-misses:H}:uG'running test 38 '{cycles,cache-misses,branch-misses}:S'
> FAILED tests/parse-events.c:1160 wrong number of entries
> Event test failure: test 38 '{cycles,cache-misses,branch-misses}:S'running test 39 '{instructions,branch-misses}:Su'
> FAILED tests/parse-events.c:1213 wrong number of entries
> Event test failure: test 39 '{instructions,branch-misses}:Su'running test 40 'instructions:uDp'
> FAILED tests/parse-events.c:107 wrong number of entries
> Event test failure: test 40 'instructions:uDp'running test 41 '{cycles,cache-misses,branch-misses}:D'
> FAILED tests/parse-events.c:1265 wrong number of entries
> Event test failure: test 41 '{cycles,cache-misses,branch-misses}:D'running test 42 'mem:0/1'
> running test 43 'mem:0/2:w'
> running test 44 'mem:0/4:rw:u'
> running test 45 'instructions:I'
> FAILED tests/parse-events.c:107 wrong number of entries
> Event test failure: test 45 'instructions:I'running test 46 'instructions:kIG'
> FAILED tests/parse-events.c:107 wrong number of entries
> Event test failure: test 46 'instructions:kIG'running test 47 'task-clock:P,cycles'
> FAILED tests/parse-events.c:1382 wrong number of entries
> Event test failure: test 47 'task-clock:P,cycles'running test 48 'instructions/name=insn/'
> running test 49 'r1234/name=rawpmu/'
> running test 50 '4:0x6530160/name=numpmu/'
> WARNING: event 'numpmu' not valid (bits 16-17,20,22 of config '6530160' not supported by kernel)!
> running test 51 'L1-dcache-misses/name=cachepmu/'
> ------------------------------------------------------------
> perf_event_attr:
>   type                             3
>   config                           0x400010000
>   disabled                         1
> ------------------------------------------------------------
> sys_perf_event_open: pid 0  cpu -1  group_fd -1  flags 0x8 = 40
> ------------------------------------------------------------
> perf_event_attr:
>   type                             3
>   config                           0x800010000
>   disabled                         1
> ------------------------------------------------------------
> sys_perf_event_open: pid 0  cpu -1  group_fd -1  flags 0x8
> sys_perf_event_open failed, error -2
> running test 52 'intel_pt//u'
> running test 53 'cycles/name='COMPLEX_CYCLES_NAME:orig=cycles,desc=chip-clock-ticks'/Duk'
> running test 54 'cycles//u'
> FAILED tests/parse-events.c:1457 wrong config
> Event test failure: test 54 'cycles//u'running test 55 'cycles:k'
> FAILED tests/parse-events.c:1467 wrong config
> Event test failure: test 55 'cycles:k'running test 56 'instructions:uep'
> FAILED tests/parse-events.c:107 wrong number of entries
> Event test failure: test 56 'instructions:uep'running test 57 '{cycles,cache-misses,branch-misses}:e'
> FAILED tests/parse-events.c:1309 wrong number of entries
> Event test failure: test 57 '{cycles,cache-misses,branch-misses}:e'test child finished with -1
> ---- end ----
>
> Parse event definition strings subtest 1: FAILED!

Thanks! So "wrong number of entries" is the dominant failure and
should be straightforward to make more PMU agnostic. Tests that are
tied to a PMU like:

running test 52 'intel_pt//u'

succeed and it seems sad to lose coverage of them with this change.
Overall, I think we should work to remove the hybrid test and make
sure we get coverage in the general test. That means just skipping the
test is the wrong thing to do.

Thanks,
Ian

> Thanks,
>
> Tinghao
>>
>> ---
>>  tools/perf/tests/parse-events.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
>> index 71a5cb343311..c6949e458e61 100644
>> --- a/tools/perf/tests/parse-events.c
>> +++ b/tools/perf/tests/parse-events.c
>> @@ -2146,6 +2146,9 @@ static int test_events(const struct evlist_test *events, int cnt)
>>
>>  static int test__events2(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
>>  {
>> +       if (perf_pmu__has_hybrid())
>> +               return TEST_SKIP;
>> +
>>         return test_events(test__events, ARRAY_SIZE(test__events));
>>  }
>>
>> @@ -2421,7 +2424,7 @@ static int test__pmu_events_alias2(struct test_suite *test __maybe_unused,
>>  static struct test_case tests__parse_events[] = {
>>         TEST_CASE_REASON("Test event parsing",
>>                          events2,
>> -                        "permissions"),
>> +                        "permissions or hybrid"),
>>         TEST_CASE_REASON("Test parsing of \"hybrid\" CPU events",
>>                          hybrid,
>>                         "not hybrid"),
>> --
>> 2.34.1
>>

Powered by blists - more mailing lists