[v1] tools/perf: Fix ratio_to_prev event parsing test

[PATCH] tools/perf: Fix ratio_to_prev event parsing test

Posted by thomas.falcon@intel.com 1 week, 2 days ago

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

Re: [PATCH] tools/perf: Fix ratio_to_prev event parsing test

Posted by Mi, Dapeng 1 week, 1 day ago

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.

Re: [PATCH] tools/perf: Fix ratio_to_prev event parsing test

Posted by Falcon, Thomas 1 week, 1 day ago

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.