Re: [PATCH v1 1/2] perf tests: Add test for uncore event sorting

[Namhyung Kim <namhyung@kernel.org>]

Hi Ian,

Just a few nitpicks.

On Fri, Mar 27, 2026 at 04:36:47PM -0700, Chen, Zide wrote:
> 
> 
> On 3/25/2026 11:30 AM, Ian Rogers wrote:
> > Add a test for uncore event sorting matching multiple PMUs. Uncore
> > PMUs may have a common prefix, like the PMUs uncore_imc_free_running_0
> > and uncore_imc_free_running_1 have a prefix of
> > uncore_imc_free_running. Parsing an event group like
> > "{data_read,data_write}" for those PMUs should result with two groups
> > "{uncore_imc_free_running_0/data_read/,uncore_imc_free_running_0/data_write/},
> > {uncore_imc_free_running_1/data_read/,uncore_imc_free_running_1/data_write/}"
> > which means the evsels need resorting as when initially parsed the
> > evsels are ordered with mixed PMUs:
> > "{uncore_imc_free_running_0/data_read/,uncore_imc_free_running_1/data_read/,
> > uncore_imc_free_running_0/data_write/,uncore_imc_free_running_1/data_write/}".
> > 
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> 
> Tested-by: Zide Chen <zide.chen@intel.com>
> 
> >  tools/perf/tests/Build                  |   1 +
> >  tools/perf/tests/builtin-test.c         |   1 +
> >  tools/perf/tests/tests.h                |   1 +
> >  tools/perf/tests/uncore-event-sorting.c | 125 ++++++++++++++++++++++++
> >  4 files changed, 128 insertions(+)
> >  create mode 100644 tools/perf/tests/uncore-event-sorting.c
> > 
> > diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
> > index c2a67ce45941..66944a4f4968 100644
> > --- a/tools/perf/tests/Build
> > +++ b/tools/perf/tests/Build
> > @@ -3,6 +3,7 @@
> >  perf-test-y += builtin-test.o
> >  perf-test-y += tests-scripts.o
> >  perf-test-y += parse-events.o
> > +perf-test-y += uncore-event-sorting.o
> >  perf-test-y += dso-data.o
> >  perf-test-y += vmlinux-kallsyms.o
> >  perf-test-y += openat-syscall.o
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index 06507066213b..f2c135891477 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -71,6 +71,7 @@ static struct test_suite *generic_tests[] = {
> >  	&suite__basic_mmap,
> >  	&suite__mem,
> >  	&suite__parse_events,
> > +	&suite__uncore_event_sorting,
> >  	&suite__expr,
> >  	&suite__PERF_RECORD,
> >  	&suite__pmu,
> > diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
> > index f5f1238d1f7f..ee00518bf36f 100644
> > --- a/tools/perf/tests/tests.h
> > +++ b/tools/perf/tests/tests.h
> > @@ -177,6 +177,7 @@ DECLARE_SUITE(sigtrap);
> >  DECLARE_SUITE(event_groups);
> >  DECLARE_SUITE(symbols);
> >  DECLARE_SUITE(util);
> > +DECLARE_SUITE(uncore_event_sorting);
> >  DECLARE_SUITE(subcmd_help);
> >  DECLARE_SUITE(kallsyms_split);
> >  
> > diff --git a/tools/perf/tests/uncore-event-sorting.c b/tools/perf/tests/uncore-event-sorting.c
> > new file mode 100644
> > index 000000000000..91e1a580709b
> > --- /dev/null
> > +++ b/tools/perf/tests/uncore-event-sorting.c
> > @@ -0,0 +1,125 @@
> > +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> > +#include "tests.h"
> > +#include "debug.h"
> > +#include "parse-events.h"
> > +#include "pmu.h"
> > +#include "pmus.h"
> > +#include "evlist.h"
> > +#include <string.h>
> > +
> > +struct match_state {
> > +	char *event1;
> > +	char *event2;
> > +};
> > +
> > +static int event_cb(void *state, struct pmu_event_info *info)
> > +{
> > +	struct match_state *m = state;
> > +
> > +	if (!m->event1) {
> > +		m->event1 = strdup(info->name);
> > +	} else if (!m->event2) {
> > +		if (strcmp(m->event1, info->name)) {
> > +			m->event2 = strdup(info->name);
> > +			return 1;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int test__uncore_event_sorting(struct test_suite *test __maybe_unused,
> > +				      int subtest __maybe_unused)
> > +{
> > +	struct evlist *evlist;
> > +	struct parse_events_error err;
> > +	struct evsel *evsel;
> > +	struct perf_pmu *pmu = NULL;
> > +	char *pmu_prefix = NULL;
> > +	struct match_state m = { NULL, NULL };
> > +	char buf[1024];
> > +	int ret;
> > +
> > +	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> > +		size_t len;
> > +		struct perf_pmu *sibling;
> > +
> > +		if (pmu->is_core)
> > +			continue;
> > +
> > +		len = pmu_name_len_no_suffix(pmu->name);
> > +		if (len == strlen(pmu->name))
> > +			continue;
> > +
> > +		sibling = pmu;
> > +		while ((sibling = perf_pmus__scan(sibling)) != NULL) {
> > +			if (sibling->is_core)
> > +				continue;
> > +			if (pmu_name_len_no_suffix(sibling->name) == len &&
> > +			    !strncmp(pmu->name, sibling->name, len))
> > +				break;
> > +		}
> > +
> > +		if (!sibling)
> > +			continue;
> > +
> > +		m.event1 = m.event2 = NULL;
> > +		perf_pmu__for_each_event(pmu, false, &m, event_cb);
> > +
> > +		if (m.event1 && m.event2) {
> > +			pmu_prefix = strndup(pmu->name, len);
> > +			break;
> > +		}
> > +		free(m.event1);
> > +	}
> > +
> > +	if (!pmu_prefix) {
> > +		pr_debug("No suitable uncore PMU found\n");
> > +		return TEST_SKIP;
> > +	}
> > +
> > +	evlist = evlist__new();
> > +	if (!evlist)
> > +		return TEST_FAIL;
> > +
> > +	snprintf(buf, sizeof(buf), "{%s/%s/,%s/%s/}",
> > +		 pmu_prefix, m.event1, pmu_prefix, m.event2);
> > +	pr_debug("Parsing: %s\n", buf);
> > +
> > +	parse_events_error__init(&err);
> > +	ret = parse_events(evlist, buf, &err);
> > +	if (ret) {
> > +		pr_debug("parse_events failed\n");
> > +		goto out_err;
> > +	}
> > +
> > +	TEST_ASSERT_VAL("Number of events is > 0", evlist->core.nr_entries > 0);
> > +	TEST_ASSERT_EQUAL("Number of events is a multiple of 2", evlist->core.nr_entries % 2, 0);

It'd be nice if we can hide libperf details in general.  But I've
realized there's no API in libperf to return the number of entries
in evlist. :(

> > +
> > +	evlist__for_each_entry(evlist, evsel) {
> > +		if (evsel__is_group_leader(evsel)) {

You can skip the loop if it's not a leader and reduce a level of
indentation.


> > +			struct evsel *next = evsel__next(evsel);
> > +
> > +			TEST_ASSERT_EQUAL("Group size is 2", evsel->core.nr_members, 2);
> > +			TEST_ASSERT_VAL("PMU match", evsel->pmu == next->pmu);
> > +			TEST_ASSERT_VAL("First event name", strstr(evsel->name, m.event1) != NULL);
> > +			TEST_ASSERT_VAL("Second event name", strstr(next->name, m.event2) != NULL);
> > +		}
> > +	}
> > +
> > +	evlist__delete(evlist);
> > +	parse_events_error__exit(&err);
> > +	free(pmu_prefix);
> > +	free(m.event1);
> > +	free(m.event2);
> > +	return TEST_OK;
> > +
> > +out_err:
> > +	evlist__delete(evlist);
> > +	parse_events_error__exit(&err);
> > +	free(pmu_prefix);
> > +	free(m.event1);
> > +	free(m.event2);
> > +	return TEST_FAIL;

Looks like the same logic.  Can we set a return value properly?

Thanks,
Namhyung


> > +}
> > +
> > +DEFINE_SUITE("Uncore event sorting", uncore_event_sorting);
>