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

["Falcon, Thomas" <thomas.falcon@intel.com>]

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.