Re: [PATCH v2] tools/perf: Fix ratio_to_prev event parsing test
From: Falcon, Thomas
Date: Thu Mar 26 2026 - 20:38:17 EST
On Thu, 2026-03-26 at 14:32 -0700, Ian Rogers wrote:
> On Thu, Mar 26, 2026 at 11:01 AM Thomas Falcon
> <thomas.falcon@xxxxxxxxx> wrote:
> >
> > test__ratio_to_prev() assumed the first event in a group is the
> > leader,
> > which is not the case when the event is expanded into two event
> > groups
> > on hybrid PMU's with auto counter reload support. This patch
> > updates the
> > test to iterate over the event group generated for each core PMU.
> > Also
> > update "wrong leader" test to check that the subordinate event has
> > the
> > correct leader instead of checking that it is not the group leader.
> >
> > Signed-off-by: Thomas Falcon <thomas.falcon@xxxxxxxxx>
> > Fixes: 56be0fe5f62c ("perf record: Add auto counter reload parse
> > and regression tests")
> > ---
> > v2: made changes suggested by Dapeng Mi
> > https://lore.kernel.org/all/997b6291-7429-4eaa-8467-1fd88e100616@xxxxxxxxxxxxxxx/
> > ---
> > tools/perf/tests/parse-events.c | 47 +++++++++++++++++++----------
> > ----
> > 1 file changed, 27 insertions(+), 20 deletions(-)
> >
> > diff --git a/tools/perf/tests/parse-events.c
> > b/tools/perf/tests/parse-events.c
> > index 1d3cc224fbc2..4487508c5658 100644
> > --- a/tools/perf/tests/parse-events.c
> > +++ b/tools/perf/tests/parse-events.c
> > @@ -1796,31 +1796,38 @@ static bool test__acr_valid(void)
> >
> > static int test__ratio_to_prev(struct evlist *evlist)
> > {
> > - struct evsel *evsel;
> > + struct evsel *evsel, *leader;
> >
> > TEST_ASSERT_VAL("wrong number of entries", 2 *
> > perf_pmus__num_core_pmus() == evlist->core.nr_entries);
> >
> > - evlist__for_each_entry(evlist, evsel) {
> > - if (!perf_pmu__has_format(evsel->pmu, "acr_mask"))
> > + for (int i = 0; i < num_core_entries(evlist); i++) {
> > + leader = (i == 0 ? evlist__first(evlist) :
> > evsel__next(evsel));
> > + if (!perf_pmu__has_format(leader->pmu, "acr_mask"))
>
> If the p-core non-ACR supporting events are first, won't this mean
> the
> loop immediately terminates? I wonder the loop should be:
> ```
> evlist__for_each_entry(evlist, evsel) {
> if (evsel != evsel__leader(evsel) ||
> !perf_pmu__has_format(evsel->pmu, "acr_mask"))
> continue; /* Not a group leader or the event is
> for a non-ACR supporting PMU. */
> ```
> then the regular tests on the config2, etc. Then evsel__next to get
> the instructions event, etc.
Hi Ian, thanks for catching that! I will send a new version soon.
Thanks,
Tom
>
> Thanks,
> Ian
>
>
> > return TEST_OK;
> >
> > - if (evsel == evlist__first(evlist)) {
> > - TEST_ASSERT_VAL("wrong config2", 0 ==
> > evsel->core.attr.config2);
> > - TEST_ASSERT_VAL("wrong leader",
> > evsel__is_group_leader(evsel));
> > - TEST_ASSERT_VAL("wrong core.nr_members",
> > evsel->core.nr_members == 2);
> > - TEST_ASSERT_VAL("wrong group_idx",
> > evsel__group_idx(evsel) == 0);
> > - TEST_ASSERT_EVSEL("unexpected event",
> > - evsel__match(evsel,
> > HARDWARE, HW_CPU_CYCLES),
> > - evsel);
> > - } else {
> > - TEST_ASSERT_VAL("wrong config2", 0 ==
> > evsel->core.attr.config2);
> > - TEST_ASSERT_VAL("wrong leader",
> > !evsel__is_group_leader(evsel));
> > - TEST_ASSERT_VAL("wrong core.nr_members",
> > evsel->core.nr_members == 0);
> > - TEST_ASSERT_VAL("wrong group_idx",
> > evsel__group_idx(evsel) == 1);
> > - TEST_ASSERT_EVSEL("unexpected event",
> > - evsel__match(evsel,
> > HARDWARE, HW_INSTRUCTIONS),
> > - evsel);
> > - }
> > + /* cycles */
> > + TEST_ASSERT_VAL("wrong config2", 0 == leader-
> > >core.attr.config2);
> > + TEST_ASSERT_VAL("wrong leader",
> > evsel__is_group_leader(leader));
> > + TEST_ASSERT_VAL("wrong core.nr_members", leader-
> > >core.nr_members == 2);
> > + TEST_ASSERT_VAL("wrong group_idx",
> > evsel__group_idx(leader) == 0);
> > + TEST_ASSERT_EVSEL("unexpected event",
> > + evsel__match(leader, HARDWARE,
> > HW_CPU_CYCLES),
> > + leader);
> > + /*
> > + * The period value gets configured within
> > evlist__config,
> > + * while this test executes only parse events
> > method.
> > + */
> > + TEST_ASSERT_VAL("wrong period", 0 == leader-
> > >core.attr.sample_period);
> > +
> > + /* instructions/period=200000,ratio-to-prev=2.0/
> > */
> > + evsel = evsel__next(leader);
> > + TEST_ASSERT_VAL("wrong config2", 0 == evsel-
> > >core.attr.config2);
> > + TEST_ASSERT_VAL("wrong leader",
> > evsel__has_leader(evsel, leader));
> > + TEST_ASSERT_VAL("wrong core.nr_members", evsel-
> > >core.nr_members == 0);
> > + TEST_ASSERT_VAL("wrong group_idx",
> > evsel__group_idx(evsel) == 1);
> > + TEST_ASSERT_EVSEL("unexpected event",
> > + evsel__match(evsel, HARDWARE,
> > HW_INSTRUCTIONS),
> > + evsel);
> > /*
> > * The period value gets configured within
> > evlist__config,
> > * while this test executes only parse events
> > method.
> > --
> > 2.43.0
> >
>