Re: [PATCH] perf tests: Fix tests in 'Parse event definition strings'

From: Zhang, Tinghao
Date: Thu Apr 13 2023 - 07:32:26 EST

Next message: David Hildenbrand: "Re: [PATCH v6 3/3] mm: vmscan: refactor updating current->reclaim_state"
Previous message: Roger Quadros: "Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature"
In reply to: Ian Rogers: "Re: [PATCH] perf tests: Fix tests in 'Parse event definition strings'"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 4/13/2023 5:36 AM, Ian Rogers wrote:

On Wed, Apr 12, 2023 at 1:39 PM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:

On 2023-04-12 12:35 p.m., Ian Rogers wrote:

On Wed, Apr 12, 2023 at 3:16 AM Zhang, Tinghao <tinghao.zhang@xxxxxxxxx> wrote:

On 4/11/2023 9:50 PM, Ian Rogers wrote:

On Tue, Apr 11, 2023, 2:49 AM <tinghao.zhang@xxxxxxxxx> wrote:

From: Tinghao Zhang <tinghao.zhang@xxxxxxxxx>

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@xxxxxxxxx>
Signed-off-by: Tinghao Zhang <tinghao.zhang@xxxxxxxxx>

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 we have different PMUs for Intel hybrid. The event list is quite
different between e-core and p-core. There are some features which only
available for one type of core. I don't think we can have a single PMU
for Intel hybrid.

So we make the test and the expectation match.

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.

How about the group? Using weak group?

No need, just make sure to call parse_events__sort_events_and_fix_groups:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2167

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.

The current "test__events" doesn't generic enough. There is
L1-dcache-load-miss which apparently CPU specific. Also ARCH specific
events, e.g., kvm-s390, intel_pt. And groups.

How should we make it generic? Any suggestions?

s390 uses #ifdefs, intel_pt uses the intel_pt_valid call to enable the
test - this returns false if the intel_pt PMU isn't detected. I have a
fix for legacy events like L1-dcache-load-miss, the hybrid hack of
having the parser run the parser inside itself is gross, we should
just fix the parser. But given use of these legacy events is
marginal/deprecated it isn't clear we need to support them on hybrid.
Fwiw, I want to refactor the parsing code into a generic event to
perf_event_attr library that is more permissible than GPLv2, so I need
to find some time to make it happen.

Yeah I think we need to refactor these test cases to base on the generic PMU abstractions.

And for test cases that are not generic, maybe we could create and move these to arch

specific test cases?

For the parser change Ian you suggest, don't know what's the impact on the current design?

Thanks,

Tinghao

Thanks,
Ian

Thanks,
Kan

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

Next message: David Hildenbrand: "Re: [PATCH v6 3/3] mm: vmscan: refactor updating current->reclaim_state"
Previous message: Roger Quadros: "Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature"
In reply to: Ian Rogers: "Re: [PATCH] perf tests: Fix tests in 'Parse event definition strings'"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]