* [PATCH] tools/perf: Fix ratio_to_prev event parsing test
@ 2026-03-24 22:25 thomas.falcon
2026-03-25 3:12 ` Mi, Dapeng
0 siblings, 1 reply; 3+ messages in thread
From: thomas.falcon @ 2026-03-24 22:25 UTC (permalink / raw)
To: linux-perf-users
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
James Clark, Dapeng Mi, Thomas Falcon
From: Thomas Falcon <thomas.falcon@intel.com>
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@intel.com>
Fixes: 56be0fe5f62c ("perf record: Add auto counter reload parse and regression tests")
---
tools/perf/tests/parse-events.c | 45 +++++++++++++++++++--------------
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 1d3cc224fbc2..cd48e6b2bb57 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) {
+ for (int i = 0; i < num_core_entries(evlist); i++) {
+ evsel = leader = (i == 0 ? evlist__first(evlist) : evsel__next(evsel));
if (!perf_pmu__has_format(evsel->pmu, "acr_mask"))
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 == 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);
+ /*
+ * The period value gets configured within evlist__config,
+ * while this test executes only parse events method.
+ */
+ TEST_ASSERT_VAL("wrong period", 0 == evsel->core.attr.sample_period);
+
+ /* instructions/period=200000,ratio-to-prev=2.0/ */
+ evsel = evsel__next(evsel);
+ 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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] tools/perf: Fix ratio_to_prev event parsing test
2026-03-24 22:25 [PATCH] tools/perf: Fix ratio_to_prev event parsing test thomas.falcon
@ 2026-03-25 3:12 ` Mi, Dapeng
2026-03-25 20:59 ` Falcon, Thomas
0 siblings, 1 reply; 3+ messages in thread
From: Mi, Dapeng @ 2026-03-25 3:12 UTC (permalink / raw)
To: thomas.falcon, linux-perf-users
Cc: linux-kernel, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
James Clark
On 3/25/2026 6:25 AM, thomas.falcon@intel.com wrote:
> From: Thomas Falcon <thomas.falcon@intel.com>
>
> 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@intel.com>
> Fixes: 56be0fe5f62c ("perf record: Add auto counter reload parse and regression tests")
> ---
> tools/perf/tests/parse-events.c | 45 +++++++++++++++++++--------------
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 1d3cc224fbc2..cd48e6b2bb57 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) {
> + for (int i = 0; i < num_core_entries(evlist); i++) {
> + evsel = leader = (i == 0 ? evlist__first(evlist) : evsel__next(evsel));
could we use the "leader" to replace "evsel" for all the "TEST_ASSERT_VAL"
of cycles event? That make code look more clearer. Then the above sentence
can be simplified to this,
```
leader = (i == 0 ? evlist__first(evlist) : evsel__next(evsel));
```
> if (!perf_pmu__has_format(evsel->pmu, "acr_mask"))
> 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 == 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);
> + /*
> + * The period value gets configured within evlist__config,
> + * while this test executes only parse events method.
> + */
> + TEST_ASSERT_VAL("wrong period", 0 == evsel->core.attr.sample_period);
> +
> + /* instructions/period=200000,ratio-to-prev=2.0/ */
> + evsel = evsel__next(evsel);
and here, the code needs to be changed to
```
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.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] tools/perf: Fix ratio_to_prev event parsing test
2026-03-25 3:12 ` Mi, Dapeng
@ 2026-03-25 20:59 ` Falcon, Thomas
0 siblings, 0 replies; 3+ messages in thread
From: Falcon, Thomas @ 2026-03-25 20:59 UTC (permalink / raw)
To: dapeng1.mi@linux.intel.com, linux-perf-users@vger.kernel.org
Cc: james.clark@linaro.org, alexander.shishkin@linux.intel.com,
peterz@infradead.org, mark.rutland@arm.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, acme@kernel.org,
namhyung@kernel.org, jolsa@kernel.org, Rogers, Ian,
Hunter, Adrian
On Wed, 2026-03-25 at 11:12 +0800, Mi, Dapeng wrote:
>
> On 3/25/2026 6:25 AM, thomas.falcon@intel.com wrote:
> > From: Thomas Falcon <thomas.falcon@intel.com>
> >
> > 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@intel.com>
> > Fixes: 56be0fe5f62c ("perf record: Add auto counter reload parse
> > and regression tests")
> > ---
> > tools/perf/tests/parse-events.c | 45 +++++++++++++++++++----------
> > ----
> > 1 file changed, 26 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/perf/tests/parse-events.c
> > b/tools/perf/tests/parse-events.c
> > index 1d3cc224fbc2..cd48e6b2bb57 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) {
> > + for (int i = 0; i < num_core_entries(evlist); i++) {
> > + evsel = leader = (i == 0 ? evlist__first(evlist) :
> > evsel__next(evsel));
>
> could we use the "leader" to replace "evsel" for all the
> "TEST_ASSERT_VAL"
> of cycles event? That make code look more clearer. Then the above
> sentence
> can be simplified to this,
>
> ```
> leader = (i == 0 ? evlist__first(evlist) : evsel__next(evsel));
>
> ```
>
Hi Dapeng, the current patch follows a style similar to some of the
other event parsing tests, though I don't see why it couldn't be
simplified in this way. I will submit a v2 with the changes soon.
Thanks,
Tom
>
> > if (!perf_pmu__has_format(evsel->pmu, "acr_mask"))
> > 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 == 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);
> > + /*
> > + * The period value gets configured within
> > evlist__config,
> > + * while this test executes only parse events
> > method.
> > + */
> > + TEST_ASSERT_VAL("wrong period", 0 == evsel-
> > >core.attr.sample_period);
> > +
> > + /* instructions/period=200000,ratio-to-prev=2.0/
> > */
> > + evsel = evsel__next(evsel);
>
> and here, the code needs to be changed to
>
> ```
>
> 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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-03-25 20:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-24 22:25 [PATCH] tools/perf: Fix ratio_to_prev event parsing test thomas.falcon
2026-03-25 3:12 ` Mi, Dapeng
2026-03-25 20:59 ` Falcon, Thomas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox