[PATCH v1 0/2] perf tests: Add tests for uncore and perf metric event sorting

[PATCH v1 0/2] perf tests: Add tests for uncore and perf metric event sorting

[Ian Rogers]

A thread changing event sorting highlighted a lack of testing for the
more complicated uncore and x86 perf metric event sorting:
https://lore.kernel.org/linux-perf-users/CAP-5=fWRgDo7UnJAD4C--d=mVPRhOEWZVyU7nVM1YEp3jncAgg@mail.gmail.com/

Ian Rogers (2):
  perf tests: Add test for uncore event sorting
  perf arch x86 tests: Add test for topdown event sorting

 tools/perf/arch/x86/tests/topdown.c     | 137 +++++++++++++++++++++++-
 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 +++++++++++++++++++++
 5 files changed, 264 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/tests/uncore-event-sorting.c

-- 
2.53.0.1018.g2bb0e51243-goog

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

[Ian Rogers]

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>
---
 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);
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (evsel__is_group_leader(evsel)) {
+			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;
+}
+
+DEFINE_SUITE("Uncore event sorting", uncore_event_sorting);
-- 
2.53.0.1018.g2bb0e51243-goog

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

[Chen, Zide]

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);
> +
> +	evlist__for_each_entry(evlist, evsel) {
> +		if (evsel__is_group_leader(evsel)) {
> +			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;
> +}
> +
> +DEFINE_SUITE("Uncore event sorting", uncore_event_sorting);

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

[Namhyung Kim]

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);
> 

[PATCH v1 2/2] perf arch x86 tests: Add test for topdown event sorting

[Ian Rogers]

Add a test to capture the comment in
tools/perf/arch/x86/util/evlist.c. Test that slots and
topdown-retiring get appropriately sorted with respect to instructions
when they're all specified together. When the PMU requires topdown
event grouping (indicated by the pressence of the slots event) metric
events should be after slots, which should be the group leader.

Add a related test that when the slots event isn't given it is
injected into the appropriate group.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/x86/tests/topdown.c | 137 +++++++++++++++++++++++++++-
 1 file changed, 136 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
index 3ee4e5e71be3..aca7faa16fc7 100644
--- a/tools/perf/arch/x86/tests/topdown.c
+++ b/tools/perf/arch/x86/tests/topdown.c
@@ -75,4 +75,139 @@ static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest
 	return ret;
 }
 
-DEFINE_SUITE("x86 topdown", x86_topdown);
+static int test_sort(const char *str, int expected_slots_group_size,
+		     int expected_instructions_group_size)
+{
+	struct evlist *evlist;
+	struct parse_events_error err;
+	struct evsel *evsel;
+	int ret;
+
+	evlist = evlist__new();
+	if (!evlist)
+		return TEST_FAIL;
+
+	parse_events_error__init(&err);
+	ret = parse_events(evlist, str, &err);
+	if (ret) {
+		pr_debug("parse_events failed for %s\n", str);
+		goto out_err;
+	}
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (evsel__is_group_leader(evsel)) {
+			if (strstr(evsel->name, "slots")) {
+				/*
+				 * Slots as a leader means the PMU is for a perf
+				 * metric group as the slots event isn't present
+				 * when not.
+				 */
+				TEST_ASSERT_EQUAL("slots group size", evsel->core.nr_members,
+						  expected_slots_group_size);
+				if (expected_slots_group_size == 3) {
+					struct evsel *next = evsel__next(evsel);
+					struct evsel *next2 = evsel__next(next);
+
+					TEST_ASSERT_VAL("slots second event is instructions",
+							strstr(next->name, "instructions")
+							!= NULL);
+					TEST_ASSERT_VAL("slots third event is topdown-retiring",
+							strstr(next2->name, "topdown-retiring")
+							!= NULL);
+				} else if (expected_slots_group_size == 2) {
+					struct evsel *next = evsel__next(evsel);
+
+					TEST_ASSERT_VAL("slots second event is topdown-retiring",
+							strstr(next->name, "topdown-retiring")
+							!= NULL);
+				}
+			} else if (strstr(evsel->name, "instructions")) {
+				TEST_ASSERT_EQUAL("instructions group size", evsel->core.nr_members,
+						  expected_instructions_group_size);
+				if (expected_instructions_group_size == 2) {
+					/*
+					 * The instructions event leads a group
+					 * with a topdown-retiring event,
+					 * neither of which need reordering for
+					 * perf metric event support.
+					 */
+					struct evsel *next = evsel__next(evsel);
+
+					TEST_ASSERT_VAL("instructions second event is topdown-retiring",
+							strstr(next->name, "topdown-retiring")
+							!= NULL);
+				}
+			} else if (strstr(evsel->name, "topdown-retiring")) {
+				/*
+				 * A perf metric event where the PMU doesn't
+				 * require slots as a leader.
+				 */
+				TEST_ASSERT_EQUAL("topdown-retiring group size",
+						  evsel->core.nr_members, 1);
+			} else if (strstr(evsel->name, "cycles")) {
+				TEST_ASSERT_EQUAL("cycles group size", evsel->core.nr_members, 1);
+			}
+		}
+	}
+
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	return TEST_OK;
+
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	return TEST_FAIL;
+}
+
+static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
+				     int subtest __maybe_unused)
+{
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	TEST_ASSERT_EQUAL("all events in a group",
+			  test_sort("{instructions,topdown-retiring,slots}", 3, 2), TEST_OK);
+	TEST_ASSERT_EQUAL("all events not in a group",
+			  test_sort("instructions,topdown-retiring,slots", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event in a group but topdown metrics events outside the group",
+			  test_sort("{instructions,slots},topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event and topdown metrics events in two groups",
+			  test_sort("{instructions,slots},{topdown-retiring}", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event and metrics event are not in a group and not adjacent",
+			  test_sort("{instructions,slots},cycles,topdown-retiring", 2, 1), TEST_OK);
+
+	return TEST_OK;
+}
+
+static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unused,
+					     int subtest __maybe_unused)
+{
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	TEST_ASSERT_EQUAL("all events in a group",
+			  test_sort("{instructions,topdown-retiring}", 3, 2), TEST_OK);
+	TEST_ASSERT_EQUAL("all events not in a group",
+			  test_sort("instructions,topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event in a group but topdown metrics events outside the group",
+			  test_sort("{instructions},topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event and topdown metrics events in two groups",
+			  test_sort("{instructions},{topdown-retiring}", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event and metrics event are not in a group and not adjacent",
+			  test_sort("{instructions},cycles,topdown-retiring", 2, 1), TEST_OK);
+
+	return TEST_OK;
+}
+
+static struct test_case x86_topdown_tests[] = {
+	TEST_CASE("topdown events", x86_topdown),
+	TEST_CASE("topdown sorting", x86_topdown_sorting),
+	TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
+	{ .name = NULL, }
+};
+
+struct test_suite suite__x86_topdown = {
+	.desc = "x86 topdown",
+	.test_cases = x86_topdown_tests,
+};
-- 
2.53.0.1018.g2bb0e51243-goog

Re: [PATCH v1 2/2] perf arch x86 tests: Add test for topdown event sorting

[Chen, Zide]

On 3/25/2026 11:30 AM, Ian Rogers wrote:
> Add a test to capture the comment in
> tools/perf/arch/x86/util/evlist.c. Test that slots and
> topdown-retiring get appropriately sorted with respect to instructions
> when they're all specified together. When the PMU requires topdown
> event grouping (indicated by the pressence of the slots event) metric
> events should be after slots, which should be the group leader.
> 
> Add a related test that when the slots event isn't given it is
> injected into the appropriate group.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/x86/tests/topdown.c | 137 +++++++++++++++++++++++++++-
>  1 file changed, 136 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
> index 3ee4e5e71be3..aca7faa16fc7 100644
> --- a/tools/perf/arch/x86/tests/topdown.c
> +++ b/tools/perf/arch/x86/tests/topdown.c
> @@ -75,4 +75,139 @@ static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest
>  	return ret;
>  }
>  
> -DEFINE_SUITE("x86 topdown", x86_topdown);
> +static int test_sort(const char *str, int expected_slots_group_size,
> +		     int expected_instructions_group_size)
> +{
> +	struct evlist *evlist;
> +	struct parse_events_error err;
> +	struct evsel *evsel;
> +	int ret;
> +
> +	evlist = evlist__new();
> +	if (!evlist)
> +		return TEST_FAIL;
> +
> +	parse_events_error__init(&err);
> +	ret = parse_events(evlist, str, &err);
> +	if (ret) {
> +		pr_debug("parse_events failed for %s\n", str);
> +		goto out_err;
> +	}
> +
> +	evlist__for_each_entry(evlist, evsel) {
> +		if (evsel__is_group_leader(evsel)) {
> +			if (strstr(evsel->name, "slots")) {
> +				/*
> +				 * Slots as a leader means the PMU is for a perf
> +				 * metric group as the slots event isn't present
> +				 * when not.
> +				 */
> +				TEST_ASSERT_EQUAL("slots group size", evsel->core.nr_members,
> +						  expected_slots_group_size);
> +				if (expected_slots_group_size == 3) {
> +					struct evsel *next = evsel__next(evsel);
> +					struct evsel *next2 = evsel__next(next);
> +
> +					TEST_ASSERT_VAL("slots second event is instructions",
> +							strstr(next->name, "instructions")
> +							!= NULL);
> +					TEST_ASSERT_VAL("slots third event is topdown-retiring",
> +							strstr(next2->name, "topdown-retiring")
> +							!= NULL);
> +				} else if (expected_slots_group_size == 2) {
> +					struct evsel *next = evsel__next(evsel);
> +
> +					TEST_ASSERT_VAL("slots second event is topdown-retiring",
> +							strstr(next->name, "topdown-retiring")
> +							!= NULL);
> +				}
> +			} else if (strstr(evsel->name, "instructions")) {
> +				TEST_ASSERT_EQUAL("instructions group size", evsel->core.nr_members,
> +						  expected_instructions_group_size);
> +				if (expected_instructions_group_size == 2) {
> +					/*
> +					 * The instructions event leads a group
> +					 * with a topdown-retiring event,
> +					 * neither of which need reordering for
> +					 * perf metric event support.
> +					 */
> +					struct evsel *next = evsel__next(evsel);
> +
> +					TEST_ASSERT_VAL("instructions second event is topdown-retiring",
> +							strstr(next->name, "topdown-retiring")
> +							!= NULL);
> +				}
> +			} else if (strstr(evsel->name, "topdown-retiring")) {
> +				/*
> +				 * A perf metric event where the PMU doesn't
> +				 * require slots as a leader.
> +				 */
> +				TEST_ASSERT_EQUAL("topdown-retiring group size",
> +						  evsel->core.nr_members, 1);
> +			} else if (strstr(evsel->name, "cycles")) {
> +				TEST_ASSERT_EQUAL("cycles group size", evsel->core.nr_members, 1);
> +			}
> +		}
> +	}
> +
> +	evlist__delete(evlist);
> +	parse_events_error__exit(&err);
> +	return TEST_OK;
> +
> +out_err:
> +	evlist__delete(evlist);
> +	parse_events_error__exit(&err);
> +	return TEST_FAIL;
> +}
> +
> +static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
> +				     int subtest __maybe_unused)
> +{
> +	if (!topdown_sys_has_perf_metrics())
> +		return TEST_OK;

I'm wondering if it makes more sense to return TEST_SKIP? As well as for
other calls to topdown_sys_has_perf_metrics().

Other than that, Tested-by: Zide Chen <zide.chen@intel.com>



> +	TEST_ASSERT_EQUAL("all events in a group",
> +			  test_sort("{instructions,topdown-retiring,slots}", 3, 2), TEST_OK);
> +	TEST_ASSERT_EQUAL("all events not in a group",
> +			  test_sort("instructions,topdown-retiring,slots", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("slots event in a group but topdown metrics events outside the group",
> +			  test_sort("{instructions,slots},topdown-retiring", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("slots event and topdown metrics events in two groups",
> +			  test_sort("{instructions,slots},{topdown-retiring}", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("slots event and metrics event are not in a group and not adjacent",
> +			  test_sort("{instructions,slots},cycles,topdown-retiring", 2, 1), TEST_OK);
> +
> +	return TEST_OK;
> +}
> +
> +static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unused,
> +					     int subtest __maybe_unused)
> +{
> +	if (!topdown_sys_has_perf_metrics())
> +		return TEST_OK;
> +
> +	TEST_ASSERT_EQUAL("all events in a group",
> +			  test_sort("{instructions,topdown-retiring}", 3, 2), TEST_OK);
> +	TEST_ASSERT_EQUAL("all events not in a group",
> +			  test_sort("instructions,topdown-retiring", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("event in a group but topdown metrics events outside the group",
> +			  test_sort("{instructions},topdown-retiring", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("event and topdown metrics events in two groups",
> +			  test_sort("{instructions},{topdown-retiring}", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("event and metrics event are not in a group and not adjacent",
> +			  test_sort("{instructions},cycles,topdown-retiring", 2, 1), TEST_OK);
> +
> +	return TEST_OK;
> +}
> +
> +static struct test_case x86_topdown_tests[] = {
> +	TEST_CASE("topdown events", x86_topdown),
> +	TEST_CASE("topdown sorting", x86_topdown_sorting),
> +	TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
> +	{ .name = NULL, }
> +};
> +
> +struct test_suite suite__x86_topdown = {
> +	.desc = "x86 topdown",
> +	.test_cases = x86_topdown_tests,
> +};

Re: [PATCH v1 2/2] perf arch x86 tests: Add test for topdown event sorting

[Namhyung Kim]

On Mon, Mar 30, 2026 at 02:53:45PM -0700, Chen, Zide wrote:
> 
> 
> On 3/25/2026 11:30 AM, Ian Rogers wrote:
> > Add a test to capture the comment in
> > tools/perf/arch/x86/util/evlist.c. Test that slots and
> > topdown-retiring get appropriately sorted with respect to instructions
> > when they're all specified together. When the PMU requires topdown
> > event grouping (indicated by the pressence of the slots event) metric
> > events should be after slots, which should be the group leader.
> > 
> > Add a related test that when the slots event isn't given it is
> > injected into the appropriate group.
> > 
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/arch/x86/tests/topdown.c | 137 +++++++++++++++++++++++++++-
> >  1 file changed, 136 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
> > index 3ee4e5e71be3..aca7faa16fc7 100644
> > --- a/tools/perf/arch/x86/tests/topdown.c
> > +++ b/tools/perf/arch/x86/tests/topdown.c
> > @@ -75,4 +75,139 @@ static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest
> >  	return ret;
> >  }
> >  
> > -DEFINE_SUITE("x86 topdown", x86_topdown);
> > +static int test_sort(const char *str, int expected_slots_group_size,
> > +		     int expected_instructions_group_size)
> > +{
> > +	struct evlist *evlist;
> > +	struct parse_events_error err;
> > +	struct evsel *evsel;
> > +	int ret;
> > +
> > +	evlist = evlist__new();
> > +	if (!evlist)
> > +		return TEST_FAIL;
> > +
> > +	parse_events_error__init(&err);
> > +	ret = parse_events(evlist, str, &err);
> > +	if (ret) {
> > +		pr_debug("parse_events failed for %s\n", str);
> > +		goto out_err;
> > +	}
> > +
> > +	evlist__for_each_entry(evlist, evsel) {
> > +		if (evsel__is_group_leader(evsel)) {

Same thing.  Please reduce the indentation.


> > +			if (strstr(evsel->name, "slots")) {
> > +				/*
> > +				 * Slots as a leader means the PMU is for a perf
> > +				 * metric group as the slots event isn't present
> > +				 * when not.
> > +				 */
> > +				TEST_ASSERT_EQUAL("slots group size", evsel->core.nr_members,
> > +						  expected_slots_group_size);
> > +				if (expected_slots_group_size == 3) {
> > +					struct evsel *next = evsel__next(evsel);
> > +					struct evsel *next2 = evsel__next(next);
> > +
> > +					TEST_ASSERT_VAL("slots second event is instructions",
> > +							strstr(next->name, "instructions")
> > +							!= NULL);
> > +					TEST_ASSERT_VAL("slots third event is topdown-retiring",
> > +							strstr(next2->name, "topdown-retiring")
> > +							!= NULL);
> > +				} else if (expected_slots_group_size == 2) {
> > +					struct evsel *next = evsel__next(evsel);
> > +
> > +					TEST_ASSERT_VAL("slots second event is topdown-retiring",
> > +							strstr(next->name, "topdown-retiring")
> > +							!= NULL);
> > +				}
> > +			} else if (strstr(evsel->name, "instructions")) {
> > +				TEST_ASSERT_EQUAL("instructions group size", evsel->core.nr_members,
> > +						  expected_instructions_group_size);
> > +				if (expected_instructions_group_size == 2) {
> > +					/*
> > +					 * The instructions event leads a group
> > +					 * with a topdown-retiring event,
> > +					 * neither of which need reordering for
> > +					 * perf metric event support.
> > +					 */
> > +					struct evsel *next = evsel__next(evsel);
> > +
> > +					TEST_ASSERT_VAL("instructions second event is topdown-retiring",
> > +							strstr(next->name, "topdown-retiring")
> > +							!= NULL);
> > +				}
> > +			} else if (strstr(evsel->name, "topdown-retiring")) {
> > +				/*
> > +				 * A perf metric event where the PMU doesn't
> > +				 * require slots as a leader.
> > +				 */
> > +				TEST_ASSERT_EQUAL("topdown-retiring group size",
> > +						  evsel->core.nr_members, 1);
> > +			} else if (strstr(evsel->name, "cycles")) {
> > +				TEST_ASSERT_EQUAL("cycles group size", evsel->core.nr_members, 1);
> > +			}
> > +		}
> > +	}
> > +
> > +	evlist__delete(evlist);
> > +	parse_events_error__exit(&err);
> > +	return TEST_OK;
> > +
> > +out_err:
> > +	evlist__delete(evlist);
> > +	parse_events_error__exit(&err);
> > +	return TEST_FAIL;

Also, please share the code and use a different return value.

> > +}
> > +
> > +static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
> > +				     int subtest __maybe_unused)
> > +{
> > +	if (!topdown_sys_has_perf_metrics())
> > +		return TEST_OK;
> 
> I'm wondering if it makes more sense to return TEST_SKIP? As well as for
> other calls to topdown_sys_has_perf_metrics().

Makes sense.

Thanks,
Namhyung

> 
> Other than that, Tested-by: Zide Chen <zide.chen@intel.com>
> 
> 
> 
> > +	TEST_ASSERT_EQUAL("all events in a group",
> > +			  test_sort("{instructions,topdown-retiring,slots}", 3, 2), TEST_OK);
> > +	TEST_ASSERT_EQUAL("all events not in a group",
> > +			  test_sort("instructions,topdown-retiring,slots", 2, 1), TEST_OK);
> > +	TEST_ASSERT_EQUAL("slots event in a group but topdown metrics events outside the group",
> > +			  test_sort("{instructions,slots},topdown-retiring", 2, 1), TEST_OK);
> > +	TEST_ASSERT_EQUAL("slots event and topdown metrics events in two groups",
> > +			  test_sort("{instructions,slots},{topdown-retiring}", 2, 1), TEST_OK);
> > +	TEST_ASSERT_EQUAL("slots event and metrics event are not in a group and not adjacent",
> > +			  test_sort("{instructions,slots},cycles,topdown-retiring", 2, 1), TEST_OK);
> > +
> > +	return TEST_OK;
> > +}
> > +
> > +static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unused,
> > +					     int subtest __maybe_unused)
> > +{
> > +	if (!topdown_sys_has_perf_metrics())
> > +		return TEST_OK;
> > +
> > +	TEST_ASSERT_EQUAL("all events in a group",
> > +			  test_sort("{instructions,topdown-retiring}", 3, 2), TEST_OK);
> > +	TEST_ASSERT_EQUAL("all events not in a group",
> > +			  test_sort("instructions,topdown-retiring", 2, 1), TEST_OK);
> > +	TEST_ASSERT_EQUAL("event in a group but topdown metrics events outside the group",
> > +			  test_sort("{instructions},topdown-retiring", 2, 1), TEST_OK);
> > +	TEST_ASSERT_EQUAL("event and topdown metrics events in two groups",
> > +			  test_sort("{instructions},{topdown-retiring}", 2, 1), TEST_OK);
> > +	TEST_ASSERT_EQUAL("event and metrics event are not in a group and not adjacent",
> > +			  test_sort("{instructions},cycles,topdown-retiring", 2, 1), TEST_OK);
> > +
> > +	return TEST_OK;
> > +}
> > +
> > +static struct test_case x86_topdown_tests[] = {
> > +	TEST_CASE("topdown events", x86_topdown),
> > +	TEST_CASE("topdown sorting", x86_topdown_sorting),
> > +	TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
> > +	{ .name = NULL, }
> > +};
> > +
> > +struct test_suite suite__x86_topdown = {
> > +	.desc = "x86 topdown",
> > +	.test_cases = x86_topdown_tests,
> > +};
> 

[PATCH v2 0/2] perf tests: Add tests for uncore and perf metric event sorting

[Ian Rogers]

A thread changing event sorting highlighted a lack of testing for the
more complicated uncore and x86 perf metric event sorting:
https://lore.kernel.org/linux-perf-users/CAP-5=fWRgDo7UnJAD4C--d=mVPRhOEWZVyU7nVM1YEp3jncAgg@mail.gmail.com/

v2: Address indentation and other nits from Namhyung. Add Zide Chen's
    tested-by tags.

v1: https://lore.kernel.org/lkml/20260325183045.1229502-1-irogers@google.com/

Ian Rogers (2):
  perf tests: Add test for uncore event sorting
  perf arch x86 tests: Add test for topdown event sorting

 tools/perf/arch/x86/tests/topdown.c     | 129 +++++++++++++++++++++++-
 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 | 122 ++++++++++++++++++++++
 5 files changed, 253 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/tests/uncore-event-sorting.c

-- 
2.53.0.1018.g2bb0e51243-goog

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

[Ian Rogers]

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 | 122 ++++++++++++++++++++++++
 4 files changed, 125 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..7e61e46283d5
--- /dev/null
+++ b/tools/perf/tests/uncore-event-sorting.c
@@ -0,0 +1,122 @@
+// 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");
+		ret = TEST_FAIL;
+		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);
+
+	evlist__for_each_entry(evlist, evsel) {
+		struct evsel *next;
+
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		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(evsel), m.event1) != NULL);
+		TEST_ASSERT_VAL("Second event name", strstr(evsel__name(next), m.event2) != NULL);
+	}
+	ret = TEST_OK;
+
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	free(pmu_prefix);
+	free(m.event1);
+	free(m.event2);
+	return ret;
+}
+
+DEFINE_SUITE("Uncore event sorting", uncore_event_sorting);
-- 
2.53.0.1018.g2bb0e51243-goog

[PATCH v2 2/2] perf arch x86 tests: Add test for topdown event sorting

[Ian Rogers]

Add a test to capture the comment in
tools/perf/arch/x86/util/evlist.c. Test that slots and
topdown-retiring get appropriately sorted with respect to instructions
when they're all specified together. When the PMU requires topdown
event grouping (indicated by the pressence of the slots event) metric
events should be after slots, which should be the group leader.

Add a related test that when the slots event isn't given it is
injected into the appropriate group.

Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Zide Chen <zide.chen@intel.com>
---
 tools/perf/arch/x86/tests/topdown.c | 129 +++++++++++++++++++++++++++-
 1 file changed, 128 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
index 3ee4e5e71be3..b1a9fa28bdfd 100644
--- a/tools/perf/arch/x86/tests/topdown.c
+++ b/tools/perf/arch/x86/tests/topdown.c
@@ -75,4 +75,131 @@ static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest
 	return ret;
 }
 
-DEFINE_SUITE("x86 topdown", x86_topdown);
+static int test_sort(const char *str, int expected_slots_group_size,
+		     int expected_instructions_group_size)
+{
+	struct evlist *evlist;
+	struct parse_events_error err;
+	struct evsel *evsel;
+	int ret = TEST_FAIL;
+
+	evlist = evlist__new();
+	if (!evlist)
+		return TEST_FAIL;
+
+	parse_events_error__init(&err);
+	ret = parse_events(evlist, str, &err);
+	if (ret) {
+		pr_debug("parse_events failed for %s\n", str);
+		goto out_err;
+	}
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		if (strstr(evsel->name, "slots")) {
+			/*
+			 * Slots as a leader means the PMU is for a perf metric
+			 * group as the slots event isn't present when not.
+			 */
+			TEST_ASSERT_EQUAL("slots group size", evsel->core.nr_members,
+					expected_slots_group_size);
+			if (expected_slots_group_size == 3) {
+				struct evsel *next = evsel__next(evsel);
+				struct evsel *next2 = evsel__next(next);
+
+				TEST_ASSERT_VAL("slots second event is instructions",
+						strstr(next->name, "instructions")
+						!= NULL);
+				TEST_ASSERT_VAL("slots third event is topdown-retiring",
+						strstr(next2->name, "topdown-retiring") != NULL);
+			} else if (expected_slots_group_size == 2) {
+				struct evsel *next = evsel__next(evsel);
+
+				TEST_ASSERT_VAL("slots second event is topdown-retiring",
+						strstr(next->name, "topdown-retiring") != NULL);
+			}
+		} else if (strstr(evsel->name, "instructions")) {
+			TEST_ASSERT_EQUAL("instructions group size", evsel->core.nr_members,
+					  expected_instructions_group_size);
+			if (expected_instructions_group_size == 2) {
+				/*
+				 * The instructions event leads a group with a
+				 * topdown-retiring event, neither of which need
+				 * reordering for perf metric event support.
+				 */
+				struct evsel *next = evsel__next(evsel);
+
+				TEST_ASSERT_VAL("instructions second event is topdown-retiring",
+						strstr(next->name, "topdown-retiring") != NULL);
+			}
+		} else if (strstr(evsel->name, "topdown-retiring")) {
+			/*
+			 * A perf metric event where the PMU doesn't require
+			 * slots as a leader.
+			 */
+			TEST_ASSERT_EQUAL("topdown-retiring group size", evsel->core.nr_members, 1);
+		} else if (strstr(evsel->name, "cycles")) {
+			TEST_ASSERT_EQUAL("cycles group size", evsel->core.nr_members, 1);
+		}
+	}
+
+	ret = TEST_OK;
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	return ret;
+}
+
+static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
+				     int subtest __maybe_unused)
+{
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	TEST_ASSERT_EQUAL("all events in a group",
+			  test_sort("{instructions,topdown-retiring,slots}", 3, 2), TEST_OK);
+	TEST_ASSERT_EQUAL("all events not in a group",
+			  test_sort("instructions,topdown-retiring,slots", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event in a group but topdown metrics events outside the group",
+			  test_sort("{instructions,slots},topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event and topdown metrics events in two groups",
+			  test_sort("{instructions,slots},{topdown-retiring}", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event and metrics event are not in a group and not adjacent",
+			  test_sort("{instructions,slots},cycles,topdown-retiring", 2, 1), TEST_OK);
+
+	return TEST_OK;
+}
+
+static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unused,
+					     int subtest __maybe_unused)
+{
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	TEST_ASSERT_EQUAL("all events in a group",
+			  test_sort("{instructions,topdown-retiring}", 3, 2), TEST_OK);
+	TEST_ASSERT_EQUAL("all events not in a group",
+			  test_sort("instructions,topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event in a group but topdown metrics events outside the group",
+			  test_sort("{instructions},topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event and topdown metrics events in two groups",
+			  test_sort("{instructions},{topdown-retiring}", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event and metrics event are not in a group and not adjacent",
+			  test_sort("{instructions},cycles,topdown-retiring", 2, 1), TEST_OK);
+
+	return TEST_OK;
+}
+
+static struct test_case x86_topdown_tests[] = {
+	TEST_CASE("topdown events", x86_topdown),
+	TEST_CASE("topdown sorting", x86_topdown_sorting),
+	TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
+	{ .name = NULL, }
+};
+
+struct test_suite suite__x86_topdown = {
+	.desc = "x86 topdown",
+	.test_cases = x86_topdown_tests,
+};
-- 
2.53.0.1018.g2bb0e51243-goog

[PATCH v3 0/2] Add tests for uncore and perf metric event sorting

[Ian Rogers]

A thread changing event sorting highlighted a lack of testing for the
more complicated uncore and x86 perf metric event sorting:
https://lore.kernel.org/linux-perf-users/CAP-5=fWRgDo7UnJAD4C--d=mVPRhOEWZVyU7nVM1YEp3jncAgg@mail.gmail.com/

v3: Address sashiko nits on using evsel__name and ensuring slots is injected.

v2: Address indentation and other nits from Namhyung. Add Zide Chen's
    tested-by tags.
    https://lore.kernel.org/lkml/20260331165207.4016392-1-irogers@google.com/

v1: https://lore.kernel.org/lkml/20260325183045.1229502-1-irogers@google.com/

Ian Rogers (2):
  perf tests: Add test for uncore event sorting
  perf arch x86 tests: Add test for topdown event sorting

 tools/perf/arch/x86/tests/topdown.c     | 134 +++++++++++++++++++++++-
 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 | 122 +++++++++++++++++++++
 5 files changed, 258 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/tests/uncore-event-sorting.c

-- 
2.53.0.1118.gaef5881109-goog

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

[Ian Rogers]

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 | 122 ++++++++++++++++++++++++
 4 files changed, 125 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..7e61e46283d5
--- /dev/null
+++ b/tools/perf/tests/uncore-event-sorting.c
@@ -0,0 +1,122 @@
+// 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");
+		ret = TEST_FAIL;
+		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);
+
+	evlist__for_each_entry(evlist, evsel) {
+		struct evsel *next;
+
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		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(evsel), m.event1) != NULL);
+		TEST_ASSERT_VAL("Second event name", strstr(evsel__name(next), m.event2) != NULL);
+	}
+	ret = TEST_OK;
+
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	free(pmu_prefix);
+	free(m.event1);
+	free(m.event2);
+	return ret;
+}
+
+DEFINE_SUITE("Uncore event sorting", uncore_event_sorting);
-- 
2.53.0.1118.gaef5881109-goog

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

[Namhyung Kim]

On Tue, Mar 31, 2026 at 11:54:18AM -0700, 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/}".

I got this:

  --- start ---                                                                   
  test child forked, pid 1168839                                                  
  Using CPUID AuthenticAMD-23-31-0                                                
  Parsing: {amd_iommu/amd_iommu_0/ign_rd_wr_mmio_1ff8h//,amd_iommu/amd_iommu_0/cmd_processed_inv//}
  parse_events failed                                                             
  ---- end(-1) ----
    6: Uncore event sorting                                            : FAILED!

Thanks,
Namhyung

> 
> 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 | 122 ++++++++++++++++++++++++
>  4 files changed, 125 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..7e61e46283d5
> --- /dev/null
> +++ b/tools/perf/tests/uncore-event-sorting.c
> @@ -0,0 +1,122 @@
> +// 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");
> +		ret = TEST_FAIL;
> +		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);
> +
> +	evlist__for_each_entry(evlist, evsel) {
> +		struct evsel *next;
> +
> +		if (!evsel__is_group_leader(evsel))
> +			continue;
> +
> +		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(evsel), m.event1) != NULL);
> +		TEST_ASSERT_VAL("Second event name", strstr(evsel__name(next), m.event2) != NULL);
> +	}
> +	ret = TEST_OK;
> +
> +out_err:
> +	evlist__delete(evlist);
> +	parse_events_error__exit(&err);
> +	free(pmu_prefix);
> +	free(m.event1);
> +	free(m.event2);
> +	return ret;
> +}
> +
> +DEFINE_SUITE("Uncore event sorting", uncore_event_sorting);
> -- 
> 2.53.0.1118.gaef5881109-goog
> 

[PATCH v3 2/2] perf arch x86 tests: Add test for topdown event sorting

[Ian Rogers]

Add a test to capture the comment in
tools/perf/arch/x86/util/evlist.c. Test that slots and
topdown-retiring get appropriately sorted with respect to instructions
when they're all specified together. When the PMU requires topdown
event grouping (indicated by the pressence of the slots event) metric
events should be after slots, which should be the group leader.

Add a related test that when the slots event isn't given it is
injected into the appropriate group.

Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Zide Chen <zide.chen@intel.com>
---
 tools/perf/arch/x86/tests/topdown.c | 134 +++++++++++++++++++++++++++-
 1 file changed, 133 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
index 3ee4e5e71be3..ee8d673a0e7b 100644
--- a/tools/perf/arch/x86/tests/topdown.c
+++ b/tools/perf/arch/x86/tests/topdown.c
@@ -75,4 +75,136 @@ static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest
 	return ret;
 }
 
-DEFINE_SUITE("x86 topdown", x86_topdown);
+static int test_sort(const char *str, int expected_slots_group_size,
+		     int expected_instructions_group_size)
+{
+	struct evlist *evlist;
+	struct parse_events_error err;
+	struct evsel *evsel;
+	int ret = TEST_FAIL;
+	bool slots_seen = false;
+
+	evlist = evlist__new();
+	if (!evlist)
+		return TEST_FAIL;
+
+	parse_events_error__init(&err);
+	ret = parse_events(evlist, str, &err);
+	if (ret) {
+		pr_debug("parse_events failed for %s\n", str);
+		goto out_err;
+	}
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		if (strstr(evsel__name(evsel), "slots")) {
+			/*
+			 * Slots as a leader means the PMU is for a perf metric
+			 * group as the slots event isn't present when not.
+			 */
+			slots_seen = true;
+			TEST_ASSERT_EQUAL("slots group size", evsel->core.nr_members,
+					expected_slots_group_size);
+			if (expected_slots_group_size == 3) {
+				struct evsel *next = evsel__next(evsel);
+				struct evsel *next2 = evsel__next(next);
+
+				TEST_ASSERT_VAL("slots second event is instructions",
+						strstr(evsel__name(next), "instructions")
+						!= NULL);
+				TEST_ASSERT_VAL("slots third event is topdown-retiring",
+						strstr(evsel__name(next2), "topdown-retiring")
+						!= NULL);
+			} else if (expected_slots_group_size == 2) {
+				struct evsel *next = evsel__next(evsel);
+
+				TEST_ASSERT_VAL("slots second event is topdown-retiring",
+						strstr(evsel__name(next), "topdown-retiring")
+						!= NULL);
+			}
+		} else if (strstr(evsel__name(evsel), "instructions")) {
+			TEST_ASSERT_EQUAL("instructions group size", evsel->core.nr_members,
+					  expected_instructions_group_size);
+			if (expected_instructions_group_size == 2) {
+				/*
+				 * The instructions event leads a group with a
+				 * topdown-retiring event, neither of which need
+				 * reordering for perf metric event support.
+				 */
+				struct evsel *next = evsel__next(evsel);
+
+				TEST_ASSERT_VAL("instructions second event is topdown-retiring",
+						strstr(evsel__name(next), "topdown-retiring")
+						!= NULL);
+			}
+		} else if (strstr(evsel__name(evsel), "topdown-retiring")) {
+			/*
+			 * A perf metric event where the PMU doesn't require
+			 * slots as a leader.
+			 */
+			TEST_ASSERT_EQUAL("topdown-retiring group size", evsel->core.nr_members, 1);
+		} else if (strstr(evsel__name(evsel), "cycles")) {
+			TEST_ASSERT_EQUAL("cycles group size", evsel->core.nr_members, 1);
+		}
+	}
+	TEST_ASSERT_VAL("slots seen", slots_seen);
+	ret = TEST_OK;
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	return ret;
+}
+
+static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
+				     int subtest __maybe_unused)
+{
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	TEST_ASSERT_EQUAL("all events in a group",
+			  test_sort("{instructions,topdown-retiring,slots}", 3, 2), TEST_OK);
+	TEST_ASSERT_EQUAL("all events not in a group",
+			  test_sort("instructions,topdown-retiring,slots", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event in a group but topdown metrics events outside the group",
+			  test_sort("{instructions,slots},topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event and topdown metrics events in two groups",
+			  test_sort("{instructions,slots},{topdown-retiring}", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event and metrics event are not in a group and not adjacent",
+			  test_sort("{instructions,slots},cycles,topdown-retiring", 2, 1), TEST_OK);
+
+	return TEST_OK;
+}
+
+static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unused,
+					     int subtest __maybe_unused)
+{
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	TEST_ASSERT_EQUAL("all events in a group",
+			  test_sort("{instructions,topdown-retiring}", 3, 2), TEST_OK);
+	TEST_ASSERT_EQUAL("all events not in a group",
+			  test_sort("instructions,topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event in a group but topdown metrics events outside the group",
+			  test_sort("{instructions},topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event and topdown metrics events in two groups",
+			  test_sort("{instructions},{topdown-retiring}", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event and metrics event are not in a group and not adjacent",
+			  test_sort("{instructions},cycles,topdown-retiring", 2, 1), TEST_OK);
+
+	return TEST_OK;
+}
+
+static struct test_case x86_topdown_tests[] = {
+	TEST_CASE("topdown events", x86_topdown),
+	TEST_CASE("topdown sorting", x86_topdown_sorting),
+	TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
+	{ .name = NULL, }
+};
+
+struct test_suite suite__x86_topdown = {
+	.desc = "x86 topdown",
+	.test_cases = x86_topdown_tests,
+};
-- 
2.53.0.1118.gaef5881109-goog

Re: [PATCH v3 2/2] perf arch x86 tests: Add test for topdown event sorting

[Namhyung Kim]

On Tue, Mar 31, 2026 at 11:54:19AM -0700, Ian Rogers wrote:
> Add a test to capture the comment in
> tools/perf/arch/x86/util/evlist.c. Test that slots and
> topdown-retiring get appropriately sorted with respect to instructions
> when they're all specified together. When the PMU requires topdown
> event grouping (indicated by the pressence of the slots event) metric
> events should be after slots, which should be the group leader.
> 
> Add a related test that when the slots event isn't given it is
> injected into the appropriate group.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Tested-by: Zide Chen <zide.chen@intel.com>
> ---
>  tools/perf/arch/x86/tests/topdown.c | 134 +++++++++++++++++++++++++++-
>  1 file changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
> index 3ee4e5e71be3..ee8d673a0e7b 100644
> --- a/tools/perf/arch/x86/tests/topdown.c
> +++ b/tools/perf/arch/x86/tests/topdown.c
> @@ -75,4 +75,136 @@ static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest
>  	return ret;
>  }
>  
> -DEFINE_SUITE("x86 topdown", x86_topdown);
> +static int test_sort(const char *str, int expected_slots_group_size,
> +		     int expected_instructions_group_size)
> +{
> +	struct evlist *evlist;
> +	struct parse_events_error err;
> +	struct evsel *evsel;
> +	int ret = TEST_FAIL;
> +	bool slots_seen = false;
> +
> +	evlist = evlist__new();
> +	if (!evlist)
> +		return TEST_FAIL;
> +
> +	parse_events_error__init(&err);
> +	ret = parse_events(evlist, str, &err);
> +	if (ret) {
> +		pr_debug("parse_events failed for %s\n", str);

It should set ret to TEST_FAIL.  I'll add that this time.

Thanks,
Namhyung


> +		goto out_err;
> +	}
> +
> +	evlist__for_each_entry(evlist, evsel) {
> +		if (!evsel__is_group_leader(evsel))
> +			continue;
> +
> +		if (strstr(evsel__name(evsel), "slots")) {
> +			/*
> +			 * Slots as a leader means the PMU is for a perf metric
> +			 * group as the slots event isn't present when not.
> +			 */
> +			slots_seen = true;
> +			TEST_ASSERT_EQUAL("slots group size", evsel->core.nr_members,
> +					expected_slots_group_size);
> +			if (expected_slots_group_size == 3) {
> +				struct evsel *next = evsel__next(evsel);
> +				struct evsel *next2 = evsel__next(next);
> +
> +				TEST_ASSERT_VAL("slots second event is instructions",
> +						strstr(evsel__name(next), "instructions")
> +						!= NULL);
> +				TEST_ASSERT_VAL("slots third event is topdown-retiring",
> +						strstr(evsel__name(next2), "topdown-retiring")
> +						!= NULL);
> +			} else if (expected_slots_group_size == 2) {
> +				struct evsel *next = evsel__next(evsel);
> +
> +				TEST_ASSERT_VAL("slots second event is topdown-retiring",
> +						strstr(evsel__name(next), "topdown-retiring")
> +						!= NULL);
> +			}
> +		} else if (strstr(evsel__name(evsel), "instructions")) {
> +			TEST_ASSERT_EQUAL("instructions group size", evsel->core.nr_members,
> +					  expected_instructions_group_size);
> +			if (expected_instructions_group_size == 2) {
> +				/*
> +				 * The instructions event leads a group with a
> +				 * topdown-retiring event, neither of which need
> +				 * reordering for perf metric event support.
> +				 */
> +				struct evsel *next = evsel__next(evsel);
> +
> +				TEST_ASSERT_VAL("instructions second event is topdown-retiring",
> +						strstr(evsel__name(next), "topdown-retiring")
> +						!= NULL);
> +			}
> +		} else if (strstr(evsel__name(evsel), "topdown-retiring")) {
> +			/*
> +			 * A perf metric event where the PMU doesn't require
> +			 * slots as a leader.
> +			 */
> +			TEST_ASSERT_EQUAL("topdown-retiring group size", evsel->core.nr_members, 1);
> +		} else if (strstr(evsel__name(evsel), "cycles")) {
> +			TEST_ASSERT_EQUAL("cycles group size", evsel->core.nr_members, 1);
> +		}
> +	}
> +	TEST_ASSERT_VAL("slots seen", slots_seen);
> +	ret = TEST_OK;
> +out_err:
> +	evlist__delete(evlist);
> +	parse_events_error__exit(&err);
> +	return ret;
> +}
> +
> +static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
> +				     int subtest __maybe_unused)
> +{
> +	if (!topdown_sys_has_perf_metrics())
> +		return TEST_OK;
> +
> +	TEST_ASSERT_EQUAL("all events in a group",
> +			  test_sort("{instructions,topdown-retiring,slots}", 3, 2), TEST_OK);
> +	TEST_ASSERT_EQUAL("all events not in a group",
> +			  test_sort("instructions,topdown-retiring,slots", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("slots event in a group but topdown metrics events outside the group",
> +			  test_sort("{instructions,slots},topdown-retiring", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("slots event and topdown metrics events in two groups",
> +			  test_sort("{instructions,slots},{topdown-retiring}", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("slots event and metrics event are not in a group and not adjacent",
> +			  test_sort("{instructions,slots},cycles,topdown-retiring", 2, 1), TEST_OK);
> +
> +	return TEST_OK;
> +}
> +
> +static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unused,
> +					     int subtest __maybe_unused)
> +{
> +	if (!topdown_sys_has_perf_metrics())
> +		return TEST_OK;
> +
> +	TEST_ASSERT_EQUAL("all events in a group",
> +			  test_sort("{instructions,topdown-retiring}", 3, 2), TEST_OK);
> +	TEST_ASSERT_EQUAL("all events not in a group",
> +			  test_sort("instructions,topdown-retiring", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("event in a group but topdown metrics events outside the group",
> +			  test_sort("{instructions},topdown-retiring", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("event and topdown metrics events in two groups",
> +			  test_sort("{instructions},{topdown-retiring}", 2, 1), TEST_OK);
> +	TEST_ASSERT_EQUAL("event and metrics event are not in a group and not adjacent",
> +			  test_sort("{instructions},cycles,topdown-retiring", 2, 1), TEST_OK);
> +
> +	return TEST_OK;
> +}
> +
> +static struct test_case x86_topdown_tests[] = {
> +	TEST_CASE("topdown events", x86_topdown),
> +	TEST_CASE("topdown sorting", x86_topdown_sorting),
> +	TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
> +	{ .name = NULL, }
> +};
> +
> +struct test_suite suite__x86_topdown = {
> +	.desc = "x86 topdown",
> +	.test_cases = x86_topdown_tests,
> +};
> -- 
> 2.53.0.1118.gaef5881109-goog
> 

[PATCH v4 0/2] perf tests: Add uncore and x86 topdown event sorting tests

[Ian Rogers]

A patch thread changing event sorting highlighted a lack of testing
for the more complicated uncore and x86 perf metric event sorting:
https://lore.kernel.org/linux-perf-users/CAP-5=fWRgDo7UnJAD4C--d=mVPRhOEWZVyU7nVM1YEp3jncAgg@mail.gmail.com/

v4: Fix malformed event group string formatting in the uncore event
    sorting test when running on AMD systems (where sysfs event
    aliases lack JSON descriptions - reported by Namhyung). Added a
    helper to strip redundant PMU prefixes and trailing slashes to
    ensure parse_events() receives valid syntax across all
    architectures.

v3: Address sashiko nits on using evsel__name and ensuring slots is injected.
https://lore.kernel.org/linux-perf-users/20260331185419.4085479-1-irogers@google.com/

v2: Address indentation and other nits from Namhyung. Add Zide Chen's
    tested-by tags.
https://lore.kernel.org/lkml/20260331165207.4016392-1-irogers@google.com/

v1: https://lore.kernel.org/lkml/20260325183045.1229502-1-irogers@google.com/

Ian Rogers (2):
  perf tests: Add test for uncore event sorting
  perf arch x86 tests: Add test for topdown event sorting

 tools/perf/arch/x86/tests/topdown.c     | 134 +++++++++++++++++++-
 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 | 155 ++++++++++++++++++++++++
 5 files changed, 291 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/tests/uncore-event-sorting.c

-- 
2.54.0.563.g4f69b47b94-goog

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

[Ian Rogers]

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 | 155 ++++++++++++++++++++++++
 4 files changed, 158 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..cab5ef49e2be
--- /dev/null
+++ b/tools/perf/tests/uncore-event-sorting.c
@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+#include <string.h>
+
+#include "debug.h"
+#include "evlist.h"
+#include "parse-events.h"
+#include "pmu.h"
+#include "pmus.h"
+#include "tests.h"
+
+struct match_state {
+	char *event1;
+	char *event2;
+};
+
+static char *clean_event_name(struct pmu_event_info *info)
+{
+	const char *name = info->name;
+	const char *pmu_name = info->pmu->name;
+	size_t pmu_len = strlen(pmu_name);
+	char *res;
+	size_t len;
+
+	if (!strncmp(name, pmu_name, pmu_len) && name[pmu_len] == '/')
+		name += pmu_len + 1;
+
+	res = strdup(name);
+	if (!res)
+		return NULL;
+
+	len = strlen(res);
+	if (len > 0 && res[len - 1] == '/')
+		res[len - 1] = '\0';
+
+	return res;
+}
+
+static int event_cb(void *state, struct pmu_event_info *info)
+{
+	struct match_state *m = state;
+	char *clean_name;
+
+	if (m->event1 && m->event2)
+		return 1;
+
+	clean_name = clean_event_name(info);
+	if (!clean_name)
+		return 0;
+
+	if (!m->event1) {
+		m->event1 = clean_name;
+	} else if (!m->event2) {
+		if (strcmp(m->event1, clean_name)) {
+			m->event2 = clean_name;
+			return 1;
+		}
+		free(clean_name);
+	} else {
+		free(clean_name);
+	}
+	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");
+		ret = TEST_FAIL;
+		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);
+
+	evlist__for_each_entry(evlist, evsel) {
+		struct evsel *next;
+
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		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(evsel), m.event1) != NULL);
+		TEST_ASSERT_VAL("Second event name", strstr(evsel__name(next), m.event2) != NULL);
+	}
+	ret = TEST_OK;
+
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	free(pmu_prefix);
+	free(m.event1);
+	free(m.event2);
+	return ret;
+}
+
+DEFINE_SUITE("Uncore event sorting", uncore_event_sorting);
-- 
2.54.0.563.g4f69b47b94-goog

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The test fails to verify that events from multiple PMUs were actually parsed, allowing it to silently pass without exercising the multi-PMU sorting logic.
--

commit 7547d1b7750e7305c495d31d775f2a8e3af0325a
Author: Ian Rogers <irogers@google.com>

perf tests: Add test for uncore event sorting

This commit adds a test to verify that uncore events with a common PMU prefix
are properly resorted when parsed. It sets up a scenario matching multiple
sibling PMUs and checks that the resulting event list correctly groups the
events by PMU.

> diff --git a/tools/perf/tests/uncore-event-sorting.c b/tools/perf/tests/uncore-event-sorting.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/perf/tests/uncore-event-sorting.c
[ ... ]
> +	parse_events_error__init(&err);
> +	ret = parse_events(evlist, buf, &err);
> +	if (ret) {
> +		pr_debug("parse_events failed\n");
> +		ret = TEST_FAIL;
> +		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);

If parse_events() encounters a regression and only matches a single PMU instead
of expanding to all siblings, evlist->core.nr_entries would evaluate to 2.

Since 2 is greater than 0 and a multiple of 2, wouldn't the test silently pass
without actually exercising the multi-PMU sorting logic it was designed for?

Should this test assert that evlist->core.nr_entries >= 4 to guarantee that
the mixed PMU resorting scenario is genuinely validated?

> +
> +	evlist__for_each_entry(evlist, evsel) {
> +		struct evsel *next;
> +
> +		if (!evsel__is_group_leader(evsel))
> +			continue;
> +
> +		next = evsel__next(evsel);
> +		TEST_ASSERT_EQUAL("Group size is 2", evsel->core.nr_members, 2);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517232849.2391357-1-irogers@google.com?part=1

[PATCH v4 2/2] perf arch x86 tests: Add test for topdown event sorting

[Ian Rogers]

Add a test to capture the comment in
tools/perf/arch/x86/util/evlist.c. Test that slots and
topdown-retiring get appropriately sorted with respect to instructions
when they're all specified together. When the PMU requires topdown
event grouping (indicated by the pressence of the slots event) metric
events should be after slots, which should be the group leader.

Add a related test that when the slots event isn't given it is
injected into the appropriate group.

Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Zide Chen <zide.chen@intel.com>
---
 tools/perf/arch/x86/tests/topdown.c | 134 +++++++++++++++++++++++++++-
 1 file changed, 133 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
index 3ee4e5e71be3..ee8d673a0e7b 100644
--- a/tools/perf/arch/x86/tests/topdown.c
+++ b/tools/perf/arch/x86/tests/topdown.c
@@ -75,4 +75,136 @@ static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest
 	return ret;
 }
 
-DEFINE_SUITE("x86 topdown", x86_topdown);
+static int test_sort(const char *str, int expected_slots_group_size,
+		     int expected_instructions_group_size)
+{
+	struct evlist *evlist;
+	struct parse_events_error err;
+	struct evsel *evsel;
+	int ret = TEST_FAIL;
+	bool slots_seen = false;
+
+	evlist = evlist__new();
+	if (!evlist)
+		return TEST_FAIL;
+
+	parse_events_error__init(&err);
+	ret = parse_events(evlist, str, &err);
+	if (ret) {
+		pr_debug("parse_events failed for %s\n", str);
+		goto out_err;
+	}
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		if (strstr(evsel__name(evsel), "slots")) {
+			/*
+			 * Slots as a leader means the PMU is for a perf metric
+			 * group as the slots event isn't present when not.
+			 */
+			slots_seen = true;
+			TEST_ASSERT_EQUAL("slots group size", evsel->core.nr_members,
+					expected_slots_group_size);
+			if (expected_slots_group_size == 3) {
+				struct evsel *next = evsel__next(evsel);
+				struct evsel *next2 = evsel__next(next);
+
+				TEST_ASSERT_VAL("slots second event is instructions",
+						strstr(evsel__name(next), "instructions")
+						!= NULL);
+				TEST_ASSERT_VAL("slots third event is topdown-retiring",
+						strstr(evsel__name(next2), "topdown-retiring")
+						!= NULL);
+			} else if (expected_slots_group_size == 2) {
+				struct evsel *next = evsel__next(evsel);
+
+				TEST_ASSERT_VAL("slots second event is topdown-retiring",
+						strstr(evsel__name(next), "topdown-retiring")
+						!= NULL);
+			}
+		} else if (strstr(evsel__name(evsel), "instructions")) {
+			TEST_ASSERT_EQUAL("instructions group size", evsel->core.nr_members,
+					  expected_instructions_group_size);
+			if (expected_instructions_group_size == 2) {
+				/*
+				 * The instructions event leads a group with a
+				 * topdown-retiring event, neither of which need
+				 * reordering for perf metric event support.
+				 */
+				struct evsel *next = evsel__next(evsel);
+
+				TEST_ASSERT_VAL("instructions second event is topdown-retiring",
+						strstr(evsel__name(next), "topdown-retiring")
+						!= NULL);
+			}
+		} else if (strstr(evsel__name(evsel), "topdown-retiring")) {
+			/*
+			 * A perf metric event where the PMU doesn't require
+			 * slots as a leader.
+			 */
+			TEST_ASSERT_EQUAL("topdown-retiring group size", evsel->core.nr_members, 1);
+		} else if (strstr(evsel__name(evsel), "cycles")) {
+			TEST_ASSERT_EQUAL("cycles group size", evsel->core.nr_members, 1);
+		}
+	}
+	TEST_ASSERT_VAL("slots seen", slots_seen);
+	ret = TEST_OK;
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	return ret;
+}
+
+static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
+				     int subtest __maybe_unused)
+{
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	TEST_ASSERT_EQUAL("all events in a group",
+			  test_sort("{instructions,topdown-retiring,slots}", 3, 2), TEST_OK);
+	TEST_ASSERT_EQUAL("all events not in a group",
+			  test_sort("instructions,topdown-retiring,slots", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event in a group but topdown metrics events outside the group",
+			  test_sort("{instructions,slots},topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event and topdown metrics events in two groups",
+			  test_sort("{instructions,slots},{topdown-retiring}", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event and metrics event are not in a group and not adjacent",
+			  test_sort("{instructions,slots},cycles,topdown-retiring", 2, 1), TEST_OK);
+
+	return TEST_OK;
+}
+
+static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unused,
+					     int subtest __maybe_unused)
+{
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	TEST_ASSERT_EQUAL("all events in a group",
+			  test_sort("{instructions,topdown-retiring}", 3, 2), TEST_OK);
+	TEST_ASSERT_EQUAL("all events not in a group",
+			  test_sort("instructions,topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event in a group but topdown metrics events outside the group",
+			  test_sort("{instructions},topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event and topdown metrics events in two groups",
+			  test_sort("{instructions},{topdown-retiring}", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event and metrics event are not in a group and not adjacent",
+			  test_sort("{instructions},cycles,topdown-retiring", 2, 1), TEST_OK);
+
+	return TEST_OK;
+}
+
+static struct test_case x86_topdown_tests[] = {
+	TEST_CASE("topdown events", x86_topdown),
+	TEST_CASE("topdown sorting", x86_topdown_sorting),
+	TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
+	{ .name = NULL, }
+};
+
+struct test_suite suite__x86_topdown = {
+	.desc = "x86 topdown",
+	.test_cases = x86_topdown_tests,
+};
-- 
2.54.0.563.g4f69b47b94-goog

Re: [PATCH v4 2/2] perf arch x86 tests: Add test for topdown event sorting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Memory leak in test_sort() on test assertion failure.
- [Medium] Dead code and completely ignored parameters in test_sort() due to a logical contradiction in test design.
--

commit 412c12ca56b31a24a283d86a447c891e7731f0d0
Author: Ian Rogers <irogers@google.com>

perf arch x86 tests: Add test for topdown event sorting

This commit adds a test to verify that slots and topdown-retiring events
are correctly sorted with respect to instructions. It checks that metric
events are placed after slots, which should be the group leader when
required by the PMU. It also verifies that the slots event is appropriately
injected into groups when not specified.

> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
> index 3ee4e5e71be3b..ee8d673a0e7b1 100644
> --- a/tools/perf/arch/x86/tests/topdown.c
> +++ b/tools/perf/arch/x86/tests/topdown.c
[ ... ]
> +		if (strstr(evsel__name(evsel), "slots")) {
> +			/*
> +			 * Slots as a leader means the PMU is for a perf metric
> +			 * group as the slots event isn't present when not.
> +			 */
> +			slots_seen = true;
> +			TEST_ASSERT_EQUAL("slots group size", evsel->core.nr_members,
> +					expected_slots_group_size);

Do the TEST_ASSERT_EQUAL and TEST_ASSERT_VAL macros leak evlist and err
if an assertion fails?

These macros execute a direct return TEST_FAIL when a condition is not met.

This would immediately abort the function, bypassing the out_err cleanup
block and leaking both the evlist structure and dynamically allocated
error strings.

[ ... ]
> +		} else if (strstr(evsel__name(evsel), "instructions")) {
> +			TEST_ASSERT_EQUAL("instructions group size", evsel->core.nr_members,
> +					  expected_instructions_group_size);
> +			if (expected_instructions_group_size == 2) {
> +				/*
> +				 * The instructions event leads a group with a
> +				 * topdown-retiring event, neither of which need
> +				 * reordering for perf metric event support.
> +				 */
> +				struct evsel *next = evsel__next(evsel);
> +
> +				TEST_ASSERT_VAL("instructions second event is topdown-retiring",
> +						strstr(evsel__name(next), "topdown-retiring")
> +						!= NULL);
> +			}

Is this expected_instructions_group_size check unreachable dead code?

Both parent test functions (test__x86_topdown_sorting and
test__x86_topdown_slots_injection) exit unconditionally with TEST_OK if
topdown_sys_has_perf_metrics() is false.

Because the test only executes on systems that enforce "slots" as a group
leader, "slots" is always injected or reordered to lead the group.

The evlist iteration loop explicitly skips non-leaders via:

    if (!evsel__is_group_leader(evsel))
        continue;

Consequently, "instructions" is never evaluated as the leader of a group
containing "topdown-retiring", making this block unreachable.

Furthermore, tests pass 2 for expected_instructions_group_size in cases
like "{instructions,topdown-retiring,slots}", which contains 3 events.
If "instructions" were actually evaluated as the leader, the assertion
would incorrectly fail. 

The test passes because this parameter is ignored. Does this indicate a
logical contradiction in the test's design?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517232849.2391357-1-irogers@google.com?part=2

[PATCH v5 0/2] perf tests: Add uncore and x86 topdown event sorting tests

[Ian Rogers]

A patch thread changing event sorting highlighted a lack of testing
for the more complicated uncore and x86 perf metric event sorting:
https://lore.kernel.org/linux-perf-users/CAP-5=fWRgDo7UnJAD4C--d=mVPRhOEWZVyU7nVM1YEp3jncAgg@mail.gmail.com/
  
v5: - Strengthen uncore event sorting test assertions to require at
      least 4 total events (>= 2 groups of 2 events) instead of > 0
      events, guaranteeing that multi-PMU wildcard expansion is
      genuinely exercised across multiple sibling PMUs (reported by Sashiko).
    - Fix memory leaks on assertion failure across both uncore and
      topdown test suites by replacing standard TEST_ASSERT macros
      with local CHECK macros that correctly branch to out_err cleanup
      handlers (reported by Sashiko).
    - Fix test_sort() in the x86 topdown test to correctly return TEST_FAIL (-1)
      instead of parse_events() internal error codes (e.g., -EINVAL) when event
      parsing fails (reported by Namhyung).
  
v4: Fix malformed event group string formatting in the uncore event
    sorting test when running on AMD systems (where sysfs event
    aliases lack JSON descriptions - reported by Namhyung). Added a
    helper to strip redundant PMU prefixes and trailing slashes to
    ensure parse_events() receives valid syntax across all
    architectures.
  
v3: Address sashiko nits on using evsel__name and ensuring slots is injected.
https://lore.kernel.org/linux-perf-users/20260331185419.4085479-1-irogers@google.com/
  
v2: Address indentation and other nits from Namhyung. Add Zide Chen's
    tested-by tags.
https://lore.kernel.org/lkml/20260331165207.4016392-1-irogers@google.com/
  
v1: https://lore.kernel.org/lkml/20260325183045.1229502-1-irogers@google.com/

Ian Rogers (2):
  perf tests: Add test for uncore event sorting
  perf arch x86 tests: Add test for topdown event sorting

 tools/perf/arch/x86/tests/topdown.c     | 148 +++++++++++++++++++-
 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 | 174 ++++++++++++++++++++++++
 5 files changed, 324 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/tests/uncore-event-sorting.c

-- 
2.54.0.563.g4f69b47b94-goog

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

[Ian Rogers]

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 | 174 ++++++++++++++++++++++++
 4 files changed, 177 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..8d10b0d84fe7
--- /dev/null
+++ b/tools/perf/tests/uncore-event-sorting.c
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+#include <string.h>
+
+#include "debug.h"
+#include "evlist.h"
+#include "parse-events.h"
+#include "pmu.h"
+#include "pmus.h"
+#include "tests.h"
+
+struct match_state {
+	char *event1;
+	char *event2;
+};
+
+static char *clean_event_name(struct pmu_event_info *info)
+{
+	const char *name = info->name;
+	const char *pmu_name = info->pmu->name;
+	size_t pmu_len = strlen(pmu_name);
+	char *res;
+	size_t len;
+
+	if (!strncmp(name, pmu_name, pmu_len) && name[pmu_len] == '/')
+		name += pmu_len + 1;
+
+	res = strdup(name);
+	if (!res)
+		return NULL;
+
+	len = strlen(res);
+	if (len > 0 && res[len - 1] == '/')
+		res[len - 1] = '\0';
+
+	return res;
+}
+
+static int event_cb(void *state, struct pmu_event_info *info)
+{
+	struct match_state *m = state;
+	char *clean_name;
+
+	if (m->event1 && m->event2)
+		return 1;
+
+	clean_name = clean_event_name(info);
+	if (!clean_name)
+		return 0;
+
+	if (!m->event1) {
+		m->event1 = clean_name;
+	} else if (!m->event2) {
+		if (strcmp(m->event1, clean_name)) {
+			m->event2 = clean_name;
+			return 1;
+		}
+		free(clean_name);
+	} else {
+		free(clean_name);
+	}
+	return 0;
+}
+
+#define CHECK_COND(cond, text)					\
+do {								\
+	if (!(cond)) {						\
+		pr_debug("FAILED %s:%d %s\n", __FILE__, __LINE__, text); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+#define CHECK_EQUAL(val, expected, text)			\
+do {								\
+	if ((val) != (expected)) {				\
+		pr_debug("FAILED %s:%d %s (%d != %d)\n",	\
+			 __FILE__, __LINE__, text, (val), (expected)); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (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");
+		ret = TEST_FAIL;
+		goto out_err;
+	}
+
+	CHECK_COND(evlist->core.nr_entries >= 4, "Number of events is >= 4");
+	CHECK_EQUAL(evlist->core.nr_entries % 2, 0, "Number of events is a multiple of 2");
+
+	evlist__for_each_entry(evlist, evsel) {
+		struct evsel *next;
+
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		next = evsel__next(evsel);
+		CHECK_EQUAL(evsel->core.nr_members, 2, "Group size is 2");
+		CHECK_COND(evsel->pmu == next->pmu, "PMU match");
+		CHECK_COND(strstr(evsel__name(evsel), m.event1) != NULL, "First event name");
+		CHECK_COND(strstr(evsel__name(next), m.event2) != NULL, "Second event name");
+	}
+	ret = TEST_OK;
+
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	free(pmu_prefix);
+	free(m.event1);
+	free(m.event2);
+	return ret;
+}
+
+DEFINE_SUITE("Uncore event sorting", uncore_event_sorting);
-- 
2.54.0.563.g4f69b47b94-goog

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Memory leak of dynamically allocated strings on error and skip paths.
--

commit df52a005ba8bea64e813a32dded17fb34900d4b6
Author: Ian Rogers <irogers@google.com>

perf tests: Add test for uncore event sorting

Add a test for uncore event sorting matching multiple PMUs to ensure evsels
are resorted properly when parsed with mixed PMUs.

> diff --git a/tools/perf/tests/uncore-event-sorting.c b/tools/perf/tests/uncore-event-sorting.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/perf/tests/uncore-event-sorting.c

[ ... ]

> +	while ((pmu = perf_pmus__scan(pmu)) != NULL) {

[ ... ]

> +		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;
> +	}

Does this early return bypass cleanup and leak m.event1 and m.event2?

If strndup() fails to allocate memory during the loop, pmu_prefix remains
NULL. The loop breaks, and this check returns TEST_SKIP immediately without
freeing the strings allocated during the perf_pmu__for_each_event() callbacks.

> +
> +	evlist = evlist__new();
> +	if (!evlist)
> +		return TEST_FAIL;

Does returning directly here leak pmu_prefix, m.event1, and m.event2?

If this is updated to use the out_err cleanup path instead, will it
dereference uninitialized stack memory?

Calling goto out_err; here would jump to parse_events_error__exit(&err)
before parse_events_error__init(&err) has had a chance to initialize it.

> +
> +	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);

[ ... ]

> +out_err:
> +	evlist__delete(evlist);
> +	parse_events_error__exit(&err);
> +	free(pmu_prefix);
> +	free(m.event1);
> +	free(m.event2);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518003704.2434308-1-irogers@google.com?part=1

[PATCH v5 2/2] perf arch x86 tests: Add test for topdown event sorting

[Ian Rogers]

Add a test to capture the comment in
tools/perf/arch/x86/util/evlist.c. Test that slots and
topdown-retiring get appropriately sorted with respect to instructions
when they're all specified together. When the PMU requires topdown
event grouping (indicated by the pressence of the slots event) metric
events should be after slots, which should be the group leader.

Add a related test that when the slots event isn't given it is
injected into the appropriate group.

Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Zide Chen <zide.chen@intel.com>
---
 tools/perf/arch/x86/tests/topdown.c | 148 +++++++++++++++++++++++++++-
 1 file changed, 147 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
index 3ee4e5e71be3..9e9dbea7998d 100644
--- a/tools/perf/arch/x86/tests/topdown.c
+++ b/tools/perf/arch/x86/tests/topdown.c
@@ -75,4 +75,150 @@ static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest
 	return ret;
 }
 
-DEFINE_SUITE("x86 topdown", x86_topdown);
+#define CHECK_COND(cond, text)					\
+do {								\
+	if (!(cond)) {						\
+		pr_debug("FAILED %s:%d %s\n", __FILE__, __LINE__, text); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+#define CHECK_EQUAL(val, expected, text)			\
+do {								\
+	if ((val) != (expected)) {				\
+		pr_debug("FAILED %s:%d %s (%d != %d)\n",	\
+			 __FILE__, __LINE__, text, (val), (expected)); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+static int test_sort(const char *str, int expected_slots_group_size,
+		     int expected_instructions_group_size)
+{
+	struct evlist *evlist;
+	struct parse_events_error err;
+	struct evsel *evsel;
+	int ret = TEST_FAIL;
+	bool slots_seen = false;
+
+	evlist = evlist__new();
+	if (!evlist)
+		return TEST_FAIL;
+
+	parse_events_error__init(&err);
+	if (parse_events(evlist, str, &err)) {
+		pr_debug("parse_events failed for %s\n", str);
+		goto out_err;
+	}
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		if (strstr(evsel__name(evsel), "slots")) {
+			/*
+			 * Slots as a leader means the PMU is for a perf metric
+			 * group as the slots event isn't present when not.
+			 */
+			slots_seen = true;
+			CHECK_EQUAL(evsel->core.nr_members, expected_slots_group_size,
+				    "slots group size");
+			if (expected_slots_group_size == 3) {
+				struct evsel *next = evsel__next(evsel);
+				struct evsel *next2 = evsel__next(next);
+
+				CHECK_COND(strstr(evsel__name(next), "instructions") != NULL,
+					   "slots second event is instructions");
+				CHECK_COND(strstr(evsel__name(next2), "topdown-retiring") != NULL,
+					   "slots third event is topdown-retiring");
+			} else if (expected_slots_group_size == 2) {
+				struct evsel *next = evsel__next(evsel);
+
+				CHECK_COND(strstr(evsel__name(next), "topdown-retiring") != NULL,
+					   "slots second event is topdown-retiring");
+			}
+		} else if (strstr(evsel__name(evsel), "instructions")) {
+			CHECK_EQUAL(evsel->core.nr_members, expected_instructions_group_size,
+				    "instructions group size");
+			if (expected_instructions_group_size == 2) {
+				/*
+				 * The instructions event leads a group with a
+				 * topdown-retiring event, neither of which need
+				 * reordering for perf metric event support.
+				 */
+				struct evsel *next = evsel__next(evsel);
+
+				CHECK_COND(strstr(evsel__name(next), "topdown-retiring") != NULL,
+					   "instructions second event is topdown-retiring");
+			}
+		} else if (strstr(evsel__name(evsel), "topdown-retiring")) {
+			/*
+			 * A perf metric event where the PMU doesn't require
+			 * slots as a leader.
+			 */
+			CHECK_EQUAL(evsel->core.nr_members, 1, "topdown-retiring group size");
+		} else if (strstr(evsel__name(evsel), "cycles")) {
+			CHECK_EQUAL(evsel->core.nr_members, 1, "cycles group size");
+		}
+	}
+	CHECK_COND(slots_seen, "slots seen");
+	ret = TEST_OK;
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	return ret;
+}
+
+static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
+				     int subtest __maybe_unused)
+{
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	TEST_ASSERT_EQUAL("all events in a group",
+			  test_sort("{instructions,topdown-retiring,slots}", 3, 2), TEST_OK);
+	TEST_ASSERT_EQUAL("all events not in a group",
+			  test_sort("instructions,topdown-retiring,slots", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event in a group but topdown metrics events outside the group",
+			  test_sort("{instructions,slots},topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event and topdown metrics events in two groups",
+			  test_sort("{instructions,slots},{topdown-retiring}", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("slots event and metrics event are not in a group and not adjacent",
+			  test_sort("{instructions,slots},cycles,topdown-retiring", 2, 1), TEST_OK);
+
+	return TEST_OK;
+}
+
+static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unused,
+					     int subtest __maybe_unused)
+{
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	TEST_ASSERT_EQUAL("all events in a group",
+			  test_sort("{instructions,topdown-retiring}", 3, 2), TEST_OK);
+	TEST_ASSERT_EQUAL("all events not in a group",
+			  test_sort("instructions,topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event in a group but topdown metrics events outside the group",
+			  test_sort("{instructions},topdown-retiring", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event and topdown metrics events in two groups",
+			  test_sort("{instructions},{topdown-retiring}", 2, 1), TEST_OK);
+	TEST_ASSERT_EQUAL("event and metrics event are not in a group and not adjacent",
+			  test_sort("{instructions},cycles,topdown-retiring", 2, 1), TEST_OK);
+
+	return TEST_OK;
+}
+
+static struct test_case x86_topdown_tests[] = {
+	TEST_CASE("topdown events", x86_topdown),
+	TEST_CASE("topdown sorting", x86_topdown_sorting),
+	TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
+	{ .name = NULL, }
+};
+
+struct test_suite suite__x86_topdown = {
+	.desc = "x86 topdown",
+	.test_cases = x86_topdown_tests,
+};
-- 
2.54.0.563.g4f69b47b94-goog

Re: [PATCH v5 2/2] perf arch x86 tests: Add test for topdown event sorting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The validation block for `expected_instructions_group_size == 2` in `test_sort()` is structurally unreachable dead code.
- [Low] Passing `test_sort()` directly to `TEST_ASSERT_EQUAL` causes the test function to execute twice upon failure.
--

commit 418562cab64daaf4f2d774b19e333bea0ea7f81a
Author: Ian Rogers <irogers@google.com>

perf arch x86 tests: Add test for topdown event sorting

This commit adds a test to verify that slots and topdown-retiring get
appropriately sorted with respect to instructions when specified together. It
also adds a related test to ensure the slots event is injected into the
appropriate group when not explicitly provided.

> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
> index 3ee4e5e71be3b..9e9dbea7998dc 100644
> --- a/tools/perf/arch/x86/tests/topdown.c
> +++ b/tools/perf/arch/x86/tests/topdown.c
[ ... ]
> +	evlist__for_each_entry(evlist, evsel) {
> +		if (!evsel__is_group_leader(evsel))
> +			continue;
[ ... ]
> +		} else if (strstr(evsel__name(evsel), "instructions")) {
> +			CHECK_EQUAL(evsel->core.nr_members, expected_instructions_group_size,
> +				    "instructions group size");
> +			if (expected_instructions_group_size == 2) {
> +				/*
> +				 * The instructions event leads a group with a
> +				 * topdown-retiring event, neither of which need
> +				 * reordering for perf metric event support.
> +				 */
> +				struct evsel *next = evsel__next(evsel);
> +
> +				CHECK_COND(strstr(evsel__name(next), "topdown-retiring") != NULL,
> +					   "instructions second event is topdown-retiring");
> +			}

Is this validation block for expected_instructions_group_size == 2 reachable?

The loop explicitly skips events that are not group leaders at the top.
However, in the test cases passing 2 for expected_instructions_group_size,
the slots event is injected as the group leader for all three events.

Consequently, instructions becomes a subordinate group member and is skipped
by the loop entirely, which bypasses these test assertions.

[ ... ]
> +static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
> +				     int subtest __maybe_unused)
> +{
> +	if (!topdown_sys_has_perf_metrics())
> +		return TEST_OK;
> +
> +	TEST_ASSERT_EQUAL("all events in a group",
> +			  test_sort("{instructions,topdown-retiring,slots}", 3, 2), TEST_OK);

Does passing test_sort() directly to TEST_ASSERT_EQUAL cause the test to
execute twice upon failure?

The TEST_ASSERT_EQUAL macro in tools/perf/tests/tests.h evaluates the
provided value a second time in its pr_debug statement if the assertion
fails.

Since test_sort() allocates an evlist, parses events, and prints internal
debug logs, evaluating it twice creates duplicate work and duplicate debug
output.

Should the return value of test_sort() be stored in a local variable before
being passed to the assertion macro?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518003704.2434308-1-irogers@google.com?part=2

[PATCH v6 0/2] perf tests: Add uncore and x86 topdown event sorting tests

[Ian Rogers]

A patch thread changing event sorting highlighted a lack of testing
for the more complicated uncore and x86 perf metric event sorting:
https://lore.kernel.org/linux-perf-users/CAP-5=fWRgDo7UnJAD4C--d=mVPRhOEWZVyU7nVM1YEp3jncAgg@mail.gmail.com/

v6: - Ensure robust error initialization and resource cleanup in both
      uncore and topdown tests by initializing evlist to NULL and
      parse_events_error at function entry, preventing uninitialized
      stack access or resource leaks on early return paths (reported
      by Sashiko).
    - Prevent double evaluation and duplicate debug logging upon test
      assertion failure in the x86 topdown test by caching test_sort()
      return values into local variables (reported by Sashiko).

v5: - Strengthen uncore event sorting test assertions to require at
      least 4 total events (>= 2 groups of 2 events) instead of > 0
      events, guaranteeing that multi-PMU wildcard expansion is
      genuinely exercised across multiple sibling PMUs (reported by Sashiko).
    - Fix memory leaks on assertion failure across both uncore and
      topdown test suites by replacing standard TEST_ASSERT macros
      with local CHECK macros that correctly branch to out_err cleanup
      handlers (reported by Sashiko).
    - Fix test_sort() in the x86 topdown test to correctly return TEST_FAIL (-1)
      instead of parse_events() internal error codes (e.g., -EINVAL) when event
      parsing fails (reported by Namhyung).
  
v4: Fix malformed event group string formatting in the uncore event
    sorting test when running on AMD systems (where sysfs event
    aliases lack JSON descriptions - reported by Namhyung). Added a
    helper to strip redundant PMU prefixes and trailing slashes to
    ensure parse_events() receives valid syntax across all
    architectures.
  
v3: Address sashiko nits on using evsel__name and ensuring slots is injected.
https://lore.kernel.org/linux-perf-users/20260331185419.4085479-1-irogers@google.com/
  
v2: Address indentation and other nits from Namhyung. Add Zide Chen's
    tested-by tags.
https://lore.kernel.org/lkml/20260331165207.4016392-1-irogers@google.com/
  
v1: https://lore.kernel.org/lkml/20260325183045.1229502-1-irogers@google.com/

Ian Rogers (2):
  perf tests: Add test for uncore event sorting
  perf arch x86 tests: Add test for topdown event sorting

 tools/perf/arch/x86/tests/topdown.c     | 159 ++++++++++++++++++++-
 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 | 178 ++++++++++++++++++++++++
 5 files changed, 339 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/tests/uncore-event-sorting.c

-- 
2.54.0.563.g4f69b47b94-goog

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

[Ian Rogers]

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 | 178 ++++++++++++++++++++++++
 4 files changed, 181 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..88bda7a89a05
--- /dev/null
+++ b/tools/perf/tests/uncore-event-sorting.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+#include <string.h>
+
+#include "debug.h"
+#include "evlist.h"
+#include "parse-events.h"
+#include "pmu.h"
+#include "pmus.h"
+#include "tests.h"
+
+struct match_state {
+	char *event1;
+	char *event2;
+};
+
+static char *clean_event_name(struct pmu_event_info *info)
+{
+	const char *name = info->name;
+	const char *pmu_name = info->pmu->name;
+	size_t pmu_len = strlen(pmu_name);
+	char *res;
+	size_t len;
+
+	if (!strncmp(name, pmu_name, pmu_len) && name[pmu_len] == '/')
+		name += pmu_len + 1;
+
+	res = strdup(name);
+	if (!res)
+		return NULL;
+
+	len = strlen(res);
+	if (len > 0 && res[len - 1] == '/')
+		res[len - 1] = '\0';
+
+	return res;
+}
+
+static int event_cb(void *state, struct pmu_event_info *info)
+{
+	struct match_state *m = state;
+	char *clean_name;
+
+	if (m->event1 && m->event2)
+		return 1;
+
+	clean_name = clean_event_name(info);
+	if (!clean_name)
+		return 0;
+
+	if (!m->event1) {
+		m->event1 = clean_name;
+	} else if (!m->event2) {
+		if (strcmp(m->event1, clean_name)) {
+			m->event2 = clean_name;
+			return 1;
+		}
+		free(clean_name);
+	} else {
+		free(clean_name);
+	}
+	return 0;
+}
+
+#define CHECK_COND(cond, text)					\
+do {								\
+	if (!(cond)) {						\
+		pr_debug("FAILED %s:%d %s\n", __FILE__, __LINE__, text); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+#define CHECK_EQUAL(val, expected, text)			\
+do {								\
+	if ((val) != (expected)) {				\
+		pr_debug("FAILED %s:%d %s (%d != %d)\n",	\
+			 __FILE__, __LINE__, text, (val), (expected)); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+static int test__uncore_event_sorting(struct test_suite *test __maybe_unused,
+				      int subtest __maybe_unused)
+{
+	struct evlist *evlist = NULL;
+	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;
+
+	parse_events_error__init(&err);
+
+	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");
+		ret = TEST_SKIP;
+		goto out_err;
+	}
+
+	evlist = evlist__new();
+	if (!evlist) {
+		ret = TEST_FAIL;
+		goto out_err;
+	}
+
+	snprintf(buf, sizeof(buf), "{%s/%s/,%s/%s/}", pmu_prefix, m.event1, pmu_prefix, m.event2);
+	pr_debug("Parsing: %s\n", buf);
+
+	ret = parse_events(evlist, buf, &err);
+	if (ret) {
+		pr_debug("parse_events failed\n");
+		ret = TEST_FAIL;
+		goto out_err;
+	}
+
+	CHECK_COND(evlist->core.nr_entries >= 4, "Number of events is >= 4");
+	CHECK_EQUAL(evlist->core.nr_entries % 2, 0, "Number of events is a multiple of 2");
+
+	evlist__for_each_entry(evlist, evsel) {
+		struct evsel *next;
+
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		next = evsel__next(evsel);
+		CHECK_EQUAL(evsel->core.nr_members, 2, "Group size is 2");
+		CHECK_COND(evsel->pmu == next->pmu, "PMU match");
+		CHECK_COND(strstr(evsel__name(evsel), m.event1) != NULL, "First event name");
+		CHECK_COND(strstr(evsel__name(next), m.event2) != NULL, "Second event name");
+	}
+	ret = TEST_OK;
+
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	free(pmu_prefix);
+	free(m.event1);
+	free(m.event2);
+	return ret;
+}
+
+DEFINE_SUITE("Uncore event sorting", uncore_event_sorting);
-- 
2.54.0.563.g4f69b47b94-goog

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Double free of `m.event1` when scanning uncore PMUs.
--

commit 5412f65ccaa1b5aa1cce690a35fcf6de242d9132
Author: Ian Rogers <irogers@google.com>

perf tests: Add test for uncore event sorting

This commit adds a test for uncore event sorting that matches multiple PMUs.
It ensures that event groups are correctly resorted when initial parsing
results in mixed PMUs due to common prefixes.

> diff --git a/tools/perf/tests/uncore-event-sorting.c b/tools/perf/tests/uncore-event-sorting.c
> new file mode 100644
> index 0000000000000..88bda7a89a057
> --- /dev/null
> +++ b/tools/perf/tests/uncore-event-sorting.c
[ ... ]
> +static int test__uncore_event_sorting(struct test_suite *test __maybe_unused,
> +				      int subtest __maybe_unused)
> +{
> +	struct evlist *evlist = NULL;
> +	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;
> +
> +	parse_events_error__init(&err);
> +
> +	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);
> +	}

Could this lead to a double free of m.event1?

If perf_pmu__for_each_event() allocates m.event1 but not m.event2, the code
falls through the condition and frees m.event1 without setting it back to NULL.

If the loop subsequently terminates, or if the next iteration triggers an early
continue before m.event1 is reset to NULL, the function will eventually
proceed to the out_err label.

[ ... ]
> +out_err:
> +	evlist__delete(evlist);
> +	parse_events_error__exit(&err);
> +	free(pmu_prefix);
> +	free(m.event1);

When it reaches this point, will it call free(m.event1) again on the dangling
pointer?

> +	free(m.event2);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518023225.2472122-1-irogers@google.com?part=1

[PATCH v6 2/2] perf arch x86 tests: Add test for topdown event sorting

[Ian Rogers]

Add a test to capture the comment in
tools/perf/arch/x86/util/evlist.c. Test that slots and
topdown-retiring get appropriately sorted with respect to instructions
when they're all specified together. When the PMU requires topdown
event grouping (indicated by the pressence of the slots event) metric
events should be after slots, which should be the group leader.

Add a related test that when the slots event isn't given it is
injected into the appropriate group.

Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Zide Chen <zide.chen@intel.com>
---
 tools/perf/arch/x86/tests/topdown.c | 159 +++++++++++++++++++++++++++-
 1 file changed, 158 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
index 3ee4e5e71be3..dbee32bf973e 100644
--- a/tools/perf/arch/x86/tests/topdown.c
+++ b/tools/perf/arch/x86/tests/topdown.c
@@ -75,4 +75,161 @@ static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest
 	return ret;
 }
 
-DEFINE_SUITE("x86 topdown", x86_topdown);
+#define CHECK_COND(cond, text)					\
+do {								\
+	if (!(cond)) {						\
+		pr_debug("FAILED %s:%d %s\n", __FILE__, __LINE__, text); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+#define CHECK_EQUAL(val, expected, text)			\
+do {								\
+	if ((val) != (expected)) {				\
+		pr_debug("FAILED %s:%d %s (%d != %d)\n",	\
+			 __FILE__, __LINE__, text, (val), (expected)); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+static int test_sort(const char *str, int expected_slots_group_size,
+		     int expected_instructions_group_size)
+{
+	struct evlist *evlist = NULL;
+	struct parse_events_error err;
+	struct evsel *evsel;
+	int ret = TEST_FAIL;
+	bool slots_seen = false;
+
+	parse_events_error__init(&err);
+
+	evlist = evlist__new();
+	if (!evlist)
+		goto out_err;
+
+	if (parse_events(evlist, str, &err)) {
+		pr_debug("parse_events failed for %s\n", str);
+		goto out_err;
+	}
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		if (strstr(evsel__name(evsel), "slots")) {
+			/*
+			 * Slots as a leader means the PMU is for a perf metric
+			 * group as the slots event isn't present when not.
+			 */
+			slots_seen = true;
+			CHECK_EQUAL(evsel->core.nr_members, expected_slots_group_size,
+				    "slots group size");
+			if (expected_slots_group_size == 3) {
+				struct evsel *next = evsel__next(evsel);
+				struct evsel *next2 = evsel__next(next);
+
+				CHECK_COND(strstr(evsel__name(next), "instructions") != NULL,
+					   "slots second event is instructions");
+				CHECK_COND(strstr(evsel__name(next2), "topdown-retiring") != NULL,
+					   "slots third event is topdown-retiring");
+			} else if (expected_slots_group_size == 2) {
+				struct evsel *next = evsel__next(evsel);
+
+				CHECK_COND(strstr(evsel__name(next), "topdown-retiring") != NULL,
+					   "slots second event is topdown-retiring");
+			}
+		} else if (strstr(evsel__name(evsel), "instructions")) {
+			CHECK_EQUAL(evsel->core.nr_members, expected_instructions_group_size,
+				    "instructions group size");
+			if (expected_instructions_group_size == 2) {
+				/*
+				 * The instructions event leads a group with a
+				 * topdown-retiring event, neither of which need
+				 * reordering for perf metric event support.
+				 */
+				struct evsel *next = evsel__next(evsel);
+
+				CHECK_COND(strstr(evsel__name(next), "topdown-retiring") != NULL,
+					   "instructions second event is topdown-retiring");
+			}
+		} else if (strstr(evsel__name(evsel), "topdown-retiring")) {
+			/*
+			 * A perf metric event where the PMU doesn't require
+			 * slots as a leader.
+			 */
+			CHECK_EQUAL(evsel->core.nr_members, 1, "topdown-retiring group size");
+		} else if (strstr(evsel__name(evsel), "cycles")) {
+			CHECK_EQUAL(evsel->core.nr_members, 1, "cycles group size");
+		}
+	}
+	CHECK_COND(slots_seen, "slots seen");
+	ret = TEST_OK;
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	return ret;
+}
+
+static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
+				     int subtest __maybe_unused)
+{
+	int ret;
+
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	ret = test_sort("{instructions,topdown-retiring,slots}", 3, 2);
+	TEST_ASSERT_EQUAL("all events in a group", ret, TEST_OK);
+	ret = test_sort("instructions,topdown-retiring,slots", 2, 1);
+	TEST_ASSERT_EQUAL("all events not in a group", ret, TEST_OK);
+	ret = test_sort("{instructions,slots},topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("slots event in a group but topdown metrics events outside the group",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions,slots},{topdown-retiring}", 2, 1);
+	TEST_ASSERT_EQUAL("slots event and topdown metrics events in two groups",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions,slots},cycles,topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("slots event and metrics event are not in a group and not adjacent",
+			  ret, TEST_OK);
+
+	return TEST_OK;
+}
+
+static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unused,
+					     int subtest __maybe_unused)
+{
+	int ret;
+
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	ret = test_sort("{instructions,topdown-retiring}", 3, 2);
+	TEST_ASSERT_EQUAL("all events in a group", ret, TEST_OK);
+	ret = test_sort("instructions,topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("all events not in a group", ret, TEST_OK);
+	ret = test_sort("{instructions},topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("event in a group but topdown metrics events outside the group",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions},{topdown-retiring}", 2, 1);
+	TEST_ASSERT_EQUAL("event and topdown metrics events in two groups",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions},cycles,topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("event and metrics event are not in a group and not adjacent",
+			  ret, TEST_OK);
+
+	return TEST_OK;
+}
+
+static struct test_case x86_topdown_tests[] = {
+	TEST_CASE("topdown events", x86_topdown),
+	TEST_CASE("topdown sorting", x86_topdown_sorting),
+	TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
+	{ .name = NULL, }
+};
+
+struct test_suite suite__x86_topdown = {
+	.desc = "x86 topdown",
+	.test_cases = x86_topdown_tests,
+};
-- 
2.54.0.563.g4f69b47b94-goog

Re: [PATCH v6 2/2] perf arch x86 tests: Add test for topdown event sorting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The logic for handling 'expected_instructions_group_size == 2' in test_sort() is unreachable dead code.
--

commit 593335141c0ab35f13d4085b28aa7eaa6387b85d
Author: Ian Rogers <irogers@google.com>
Subject: perf arch x86 tests: Add test for topdown event sorting

This commit adds tests to ensure that slots, topdown-retiring, and
instructions events are appropriately sorted and grouped when specified
together.

> diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
> index 3ee4e5e71be3b..dbee32bf973e3 100644
> --- a/tools/perf/arch/x86/tests/topdown.c
> +++ b/tools/perf/arch/x86/tests/topdown.c

[ ... ]

> +	evlist__for_each_entry(evlist, evsel) {
> +		if (!evsel__is_group_leader(evsel))
> +			continue;

[ ... ]

> +		} else if (strstr(evsel__name(evsel), "instructions")) {
> +			CHECK_EQUAL(evsel->core.nr_members, expected_instructions_group_size,
> +				    "instructions group size");
> +			if (expected_instructions_group_size == 2) {
> +				/*
> +				 * The instructions event leads a group with a
> +				 * topdown-retiring event, neither of which need
> +				 * reordering for perf metric event support.
> +				 */
> +				struct evsel *next = evsel__next(evsel);
> +
> +				CHECK_COND(strstr(evsel__name(next), "topdown-retiring") != NULL,
> +					   "instructions second event is topdown-retiring");
> +			}

Is this expected_instructions_group_size == 2 block reachable?

Looking at test_sort(), the loop iterates over evsels and skips any that are
not group leaders.

The only test cases invoking test_sort() with expected_instructions_group_size
set to 2 are those that also expect a slots group size of 3, such as:

test_sort("{instructions,topdown-retiring,slots}", 3, 2);

When slots groups all 3 events together, it becomes the group leader.
Consequently, instructions acts as a group member (not a leader) and is
skipped by the loop's continue statement.

Additionally, the test enforces that slots must be a leader by requiring
slots_seen = true to pass.

Does this mean there are no scenarios in the tests where instructions can act
as a leader of size 2, rendering this validation block dead code?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518023225.2472122-1-irogers@google.com?part=2

[PATCH v7 0/2] perf tests: Add uncore and x86 topdown event sorting tests

[Ian Rogers]

A patch thread changing event sorting highlighted a lack of testing
for the more complicated uncore and x86 perf metric event sorting:
https://lore.kernel.org/linux-perf-users/CAP-5=fWRgDo7UnJAD4C--d=mVPRhOEWZVyU7nVM1YEp3jncAgg@mail.gmail.com/

v7: - Prevent double-free of m.event1 and dangling pointer access upon
      PMU scanning loop termination or early continue in the uncore
      event sorting test by utilizing standard perf zfree() helpers
      (reported by Sashiko).
    - Document Intel Hybrid CPU architectural invariants in the x86
      topdown test to clarify why instructions correctly leads a group
      of 2 on E-cores (cpu_atom) where slots is unsupported and
      filtered out (reported by Sashiko).

v6: - Ensure robust error initialization and resource cleanup in both
      uncore and topdown tests by initializing evlist to NULL and
      parse_events_error at function entry, preventing uninitialized
      stack access or resource leaks on early return paths (reported
      by Sashiko).
    - Prevent double evaluation and duplicate debug logging upon test
      assertion failure in the x86 topdown test by caching test_sort()
      return values into local variables (reported by Sashiko).

v5: - Strengthen uncore event sorting test assertions to require at
      least 4 total events (>= 2 groups of 2 events) instead of > 0
      events, guaranteeing that multi-PMU wildcard expansion is
      genuinely exercised across multiple sibling PMUs (reported by Sashiko).
    - Fix memory leaks on assertion failure across both uncore and
      topdown test suites by replacing standard TEST_ASSERT macros
      with local CHECK macros that correctly branch to out_err cleanup
      handlers (reported by Sashiko).
    - Fix test_sort() in the x86 topdown test to correctly return TEST_FAIL (-1)
      instead of parse_events() internal error codes (e.g., -EINVAL) when event
      parsing fails (reported by Namhyung).
  
v4: Fix malformed event group string formatting in the uncore event
    sorting test when running on AMD systems (where sysfs event
    aliases lack JSON descriptions - reported by Namhyung). Added a
    helper to strip redundant PMU prefixes and trailing slashes to
    ensure parse_events() receives valid syntax across all
    architectures.
  
v3: Address sashiko nits on using evsel__name and ensuring slots is injected.
https://lore.kernel.org/linux-perf-users/20260331185419.4085479-1-irogers@google.com/
  
v2: Address indentation and other nits from Namhyung. Add Zide Chen's
    tested-by tags.
https://lore.kernel.org/lkml/20260331165207.4016392-1-irogers@google.com/
  
v1: https://lore.kernel.org/lkml/20260325183045.1229502-1-irogers@google.com/

Ian Rogers (2):
  perf tests: Add test for uncore event sorting
  perf arch x86 tests: Add test for topdown event sorting

 tools/perf/arch/x86/tests/topdown.c     | 166 +++++++++++++++++++++-
 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 | 178 ++++++++++++++++++++++++
 5 files changed, 346 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/tests/uncore-event-sorting.c

-- 
2.54.0.563.g4f69b47b94-goog

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

[Ian Rogers]

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 | 178 ++++++++++++++++++++++++
 4 files changed, 181 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..594fe7ff7bf6
--- /dev/null
+++ b/tools/perf/tests/uncore-event-sorting.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+#include <string.h>
+
+#include "debug.h"
+#include "evlist.h"
+#include "parse-events.h"
+#include "pmu.h"
+#include "pmus.h"
+#include "tests.h"
+
+struct match_state {
+	char *event1;
+	char *event2;
+};
+
+static char *clean_event_name(struct pmu_event_info *info)
+{
+	const char *name = info->name;
+	const char *pmu_name = info->pmu->name;
+	size_t pmu_len = strlen(pmu_name);
+	char *res;
+	size_t len;
+
+	if (!strncmp(name, pmu_name, pmu_len) && name[pmu_len] == '/')
+		name += pmu_len + 1;
+
+	res = strdup(name);
+	if (!res)
+		return NULL;
+
+	len = strlen(res);
+	if (len > 0 && res[len - 1] == '/')
+		res[len - 1] = '\0';
+
+	return res;
+}
+
+static int event_cb(void *state, struct pmu_event_info *info)
+{
+	struct match_state *m = state;
+	char *clean_name;
+
+	if (m->event1 && m->event2)
+		return 1;
+
+	clean_name = clean_event_name(info);
+	if (!clean_name)
+		return 0;
+
+	if (!m->event1) {
+		m->event1 = clean_name;
+	} else if (!m->event2) {
+		if (strcmp(m->event1, clean_name)) {
+			m->event2 = clean_name;
+			return 1;
+		}
+		free(clean_name);
+	} else {
+		free(clean_name);
+	}
+	return 0;
+}
+
+#define CHECK_COND(cond, text)					\
+do {								\
+	if (!(cond)) {						\
+		pr_debug("FAILED %s:%d %s\n", __FILE__, __LINE__, text); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+#define CHECK_EQUAL(val, expected, text)			\
+do {								\
+	if ((val) != (expected)) {				\
+		pr_debug("FAILED %s:%d %s (%d != %d)\n",	\
+			 __FILE__, __LINE__, text, (val), (expected)); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+static int test__uncore_event_sorting(struct test_suite *test __maybe_unused,
+				      int subtest __maybe_unused)
+{
+	struct evlist *evlist = NULL;
+	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;
+
+	parse_events_error__init(&err);
+
+	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;
+		}
+		zfree(&m.event1);
+	}
+
+	if (!pmu_prefix) {
+		pr_debug("No suitable uncore PMU found\n");
+		ret = TEST_SKIP;
+		goto out_err;
+	}
+
+	evlist = evlist__new();
+	if (!evlist) {
+		ret = TEST_FAIL;
+		goto out_err;
+	}
+
+	snprintf(buf, sizeof(buf), "{%s/%s/,%s/%s/}", pmu_prefix, m.event1, pmu_prefix, m.event2);
+	pr_debug("Parsing: %s\n", buf);
+
+	ret = parse_events(evlist, buf, &err);
+	if (ret) {
+		pr_debug("parse_events failed\n");
+		ret = TEST_FAIL;
+		goto out_err;
+	}
+
+	CHECK_COND(evlist->core.nr_entries >= 4, "Number of events is >= 4");
+	CHECK_EQUAL(evlist->core.nr_entries % 2, 0, "Number of events is a multiple of 2");
+
+	evlist__for_each_entry(evlist, evsel) {
+		struct evsel *next;
+
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		next = evsel__next(evsel);
+		CHECK_EQUAL(evsel->core.nr_members, 2, "Group size is 2");
+		CHECK_COND(evsel->pmu == next->pmu, "PMU match");
+		CHECK_COND(strstr(evsel__name(evsel), m.event1) != NULL, "First event name");
+		CHECK_COND(strstr(evsel__name(next), m.event2) != NULL, "Second event name");
+	}
+	ret = TEST_OK;
+
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	zfree(&pmu_prefix);
+	zfree(&m.event1);
+	zfree(&m.event2);
+	return ret;
+}
+
+DEFINE_SUITE("Uncore event sorting", uncore_event_sorting);
-- 
2.54.0.563.g4f69b47b94-goog

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Unreachable else block in event_cb().
--

commit 73f78d43a9da9383b1db8bb57a775b964c9f8810
Author: Ian Rogers <irogers@google.com>

perf tests: Add test for uncore event sorting

Add a test for uncore event sorting matching multiple PMUs. Uncore PMUs
may have a common prefix, and parsing an event group for those PMUs
should result in correctly sorted evsels.

> diff --git a/tools/perf/tests/uncore-event-sorting.c b/tools/perf/tests/uncore-event-sorting.c
> new file mode 100644
> index 0000000000000..594fe7ff7bf6a
> --- /dev/null
> +++ b/tools/perf/tests/uncore-event-sorting.c

[ ... ]

> +static int event_cb(void *state, struct pmu_event_info *info)
> +{
> +	struct match_state *m = state;
> +	char *clean_name;
> +
> +	if (m->event1 && m->event2)
> +		return 1;
> +
> +	clean_name = clean_event_name(info);
> +	if (!clean_name)
> +		return 0;
> +
> +	if (!m->event1) {
> +		m->event1 = clean_name;
> +	} else if (!m->event2) {
> +		if (strcmp(m->event1, clean_name)) {
> +			m->event2 = clean_name;
> +			return 1;
> +		}
> +		free(clean_name);
> +	} else {
> +		free(clean_name);
> +	}

Is this else block unreachable?

Since there is an early return at the start of the function when both
m->event1 and m->event2 are non-NULL, we can only reach this point if at
least one of them is NULL.

If the first branch (!m->event1) is not taken, m->event1 is non-NULL.
If the second branch (!m->event2) is not taken, m->event2 is non-NULL.

This implies both are non-NULL, which would have triggered the early return
at the top of the function, leaving this block as dead code.

> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518043140.2522141-1-irogers@google.com?part=1

[PATCH v7 2/2] perf arch x86 tests: Add test for topdown event sorting

[Ian Rogers]

Add a test to capture the comment in
tools/perf/arch/x86/util/evlist.c. Test that slots and
topdown-retiring get appropriately sorted with respect to instructions
when they're all specified together. When the PMU requires topdown
event grouping (indicated by the pressence of the slots event) metric
events should be after slots, which should be the group leader.

Add a related test that when the slots event isn't given it is
injected into the appropriate group.

Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Zide Chen <zide.chen@intel.com>
---
 tools/perf/arch/x86/tests/topdown.c | 166 +++++++++++++++++++++++++++-
 1 file changed, 165 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
index 3ee4e5e71be3..221f2c4bbb61 100644
--- a/tools/perf/arch/x86/tests/topdown.c
+++ b/tools/perf/arch/x86/tests/topdown.c
@@ -75,4 +75,168 @@ static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest
 	return ret;
 }
 
-DEFINE_SUITE("x86 topdown", x86_topdown);
+#define CHECK_COND(cond, text)					\
+do {								\
+	if (!(cond)) {						\
+		pr_debug("FAILED %s:%d %s\n", __FILE__, __LINE__, text); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+#define CHECK_EQUAL(val, expected, text)			\
+do {								\
+	if ((val) != (expected)) {				\
+		pr_debug("FAILED %s:%d %s (%d != %d)\n",	\
+			 __FILE__, __LINE__, text, (val), (expected)); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+static int test_sort(const char *str, int expected_slots_group_size,
+		     int expected_instructions_group_size)
+{
+	struct evlist *evlist = NULL;
+	struct parse_events_error err;
+	struct evsel *evsel;
+	int ret = TEST_FAIL;
+	bool slots_seen = false;
+
+	parse_events_error__init(&err);
+
+	evlist = evlist__new();
+	if (!evlist)
+		goto out_err;
+
+	if (parse_events(evlist, str, &err)) {
+		pr_debug("parse_events failed for %s\n", str);
+		goto out_err;
+	}
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		if (strstr(evsel__name(evsel), "slots")) {
+			/*
+			 * Slots as a leader means the PMU is for a perf metric
+			 * group as the slots event isn't present when not.
+			 */
+			slots_seen = true;
+			CHECK_EQUAL(evsel->core.nr_members, expected_slots_group_size,
+				    "slots group size");
+			if (expected_slots_group_size == 3) {
+				struct evsel *next = evsel__next(evsel);
+				struct evsel *next2 = evsel__next(next);
+
+				CHECK_COND(strstr(evsel__name(next), "instructions") != NULL,
+					   "slots second event is instructions");
+				CHECK_COND(strstr(evsel__name(next2), "topdown-retiring") != NULL,
+					   "slots third event is topdown-retiring");
+			} else if (expected_slots_group_size == 2) {
+				struct evsel *next = evsel__next(evsel);
+
+				CHECK_COND(strstr(evsel__name(next), "topdown-retiring") != NULL,
+					   "slots second event is topdown-retiring");
+			}
+		} else if (strstr(evsel__name(evsel), "instructions")) {
+			CHECK_EQUAL(evsel->core.nr_members, expected_instructions_group_size,
+				    "instructions group size");
+			if (expected_instructions_group_size == 2) {
+				/*
+				 * On Intel hybrid CPUs (e.g., Alder Lake/
+				 * Raptor Lake), E-cores (cpu_atom) do not
+				 * support/enforce the slots event. When
+				 * parsing event groups containing slots
+				 * across all PMUs, slots is automatically
+				 * filtered out from cpu_atom, leaving
+				 * {cpu_atom/instructions/,
+				 *  cpu_atom/topdown-retiring/}. On cpu_atom,
+				 * instructions correctly leads this group of
+				 * 2 without slots reordering.
+				 */
+				struct evsel *next = evsel__next(evsel);
+
+				CHECK_COND(strstr(evsel__name(next), "topdown-retiring") != NULL,
+					   "instructions second event is topdown-retiring");
+			}
+		} else if (strstr(evsel__name(evsel), "topdown-retiring")) {
+			/*
+			 * A perf metric event where the PMU doesn't require
+			 * slots as a leader.
+			 */
+			CHECK_EQUAL(evsel->core.nr_members, 1, "topdown-retiring group size");
+		} else if (strstr(evsel__name(evsel), "cycles")) {
+			CHECK_EQUAL(evsel->core.nr_members, 1, "cycles group size");
+		}
+	}
+	CHECK_COND(slots_seen, "slots seen");
+	ret = TEST_OK;
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	return ret;
+}
+
+static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
+				     int subtest __maybe_unused)
+{
+	int ret;
+
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	ret = test_sort("{instructions,topdown-retiring,slots}", 3, 2);
+	TEST_ASSERT_EQUAL("all events in a group", ret, TEST_OK);
+	ret = test_sort("instructions,topdown-retiring,slots", 2, 1);
+	TEST_ASSERT_EQUAL("all events not in a group", ret, TEST_OK);
+	ret = test_sort("{instructions,slots},topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("slots event in a group but topdown metrics events outside the group",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions,slots},{topdown-retiring}", 2, 1);
+	TEST_ASSERT_EQUAL("slots event and topdown metrics events in two groups",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions,slots},cycles,topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("slots event and metrics event are not in a group and not adjacent",
+			  ret, TEST_OK);
+
+	return TEST_OK;
+}
+
+static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unused,
+					     int subtest __maybe_unused)
+{
+	int ret;
+
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	ret = test_sort("{instructions,topdown-retiring}", 3, 2);
+	TEST_ASSERT_EQUAL("all events in a group", ret, TEST_OK);
+	ret = test_sort("instructions,topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("all events not in a group", ret, TEST_OK);
+	ret = test_sort("{instructions},topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("event in a group but topdown metrics events outside the group",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions},{topdown-retiring}", 2, 1);
+	TEST_ASSERT_EQUAL("event and topdown metrics events in two groups",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions},cycles,topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("event and metrics event are not in a group and not adjacent",
+			  ret, TEST_OK);
+
+	return TEST_OK;
+}
+
+static struct test_case x86_topdown_tests[] = {
+	TEST_CASE("topdown events", x86_topdown),
+	TEST_CASE("topdown sorting", x86_topdown_sorting),
+	TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
+	{ .name = NULL, }
+};
+
+struct test_suite suite__x86_topdown = {
+	.desc = "x86 topdown",
+	.test_cases = x86_topdown_tests,
+};
-- 
2.54.0.563.g4f69b47b94-goog

[PATCH v8 0/2] perf tests: Add uncore and x86 topdown event sorting tests

[Ian Rogers]

A patch thread changing event sorting highlighted a lack of testing
for the more complicated uncore and x86 perf metric event sorting:
https://lore.kernel.org/linux-perf-users/CAP-5=fWRgDo7UnJAD4C--d=mVPRhOEWZVyU7nVM1YEp3jncAgg@mail.gmail.com/

v8: Simplify event_cb in the uncore event sorting test by removing the
    unreachable dead-code else block (reported by Sashiko).

v7: - Prevent double-free of m.event1 and dangling pointer access upon
      PMU scanning loop termination or early continue in the uncore
      event sorting test by utilizing standard perf zfree() helpers
      (reported by Sashiko).
    - Document Intel Hybrid CPU architectural invariants in the x86
      topdown test to clarify why instructions correctly leads a group
      of 2 on E-cores (cpu_atom) where slots is unsupported and
      filtered out (reported by Sashiko).

v6: - Ensure robust error initialization and resource cleanup in both
      uncore and topdown tests by initializing evlist to NULL and
      parse_events_error at function entry, preventing uninitialized
      stack access or resource leaks on early return paths (reported
      by Sashiko).
    - Prevent double evaluation and duplicate debug logging upon test
      assertion failure in the x86 topdown test by caching test_sort()
      return values into local variables (reported by Sashiko).

v5: - Strengthen uncore event sorting test assertions to require at
      least 4 total events (>= 2 groups of 2 events) instead of > 0
      events, guaranteeing that multi-PMU wildcard expansion is
      genuinely exercised across multiple sibling PMUs (reported by Sashiko).
    - Fix memory leaks on assertion failure across both uncore and
      topdown test suites by replacing standard TEST_ASSERT macros
      with local CHECK macros that correctly branch to out_err cleanup
      handlers (reported by Sashiko).
    - Fix test_sort() in the x86 topdown test to correctly return TEST_FAIL (-1)
      instead of parse_events() internal error codes (e.g., -EINVAL) when event
      parsing fails (reported by Namhyung).
  
v4: Fix malformed event group string formatting in the uncore event
    sorting test when running on AMD systems (where sysfs event
    aliases lack JSON descriptions - reported by Namhyung). Added a
    helper to strip redundant PMU prefixes and trailing slashes to
    ensure parse_events() receives valid syntax across all
    architectures.
  
v3: Address sashiko nits on using evsel__name and ensuring slots is injected.
https://lore.kernel.org/linux-perf-users/20260331185419.4085479-1-irogers@google.com/
  
v2: Address indentation and other nits from Namhyung. Add Zide Chen's
    tested-by tags.
https://lore.kernel.org/lkml/20260331165207.4016392-1-irogers@google.com/
  
v1: https://lore.kernel.org/lkml/20260325183045.1229502-1-irogers@google.com/

Ian Rogers (2):
  perf tests: Add test for uncore event sorting
  perf arch x86 tests: Add test for topdown event sorting

 tools/perf/arch/x86/tests/topdown.c     | 166 +++++++++++++++++++++-
 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 | 176 ++++++++++++++++++++++++
 5 files changed, 344 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/tests/uncore-event-sorting.c

-- 
2.54.0.563.g4f69b47b94-goog

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

[Ian Rogers]

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 | 176 ++++++++++++++++++++++++
 4 files changed, 179 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..7d2fc304e21f
--- /dev/null
+++ b/tools/perf/tests/uncore-event-sorting.c
@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+#include <string.h>
+
+#include "debug.h"
+#include "evlist.h"
+#include "parse-events.h"
+#include "pmu.h"
+#include "pmus.h"
+#include "tests.h"
+
+struct match_state {
+	char *event1;
+	char *event2;
+};
+
+static char *clean_event_name(struct pmu_event_info *info)
+{
+	const char *name = info->name;
+	const char *pmu_name = info->pmu->name;
+	size_t pmu_len = strlen(pmu_name);
+	char *res;
+	size_t len;
+
+	if (!strncmp(name, pmu_name, pmu_len) && name[pmu_len] == '/')
+		name += pmu_len + 1;
+
+	res = strdup(name);
+	if (!res)
+		return NULL;
+
+	len = strlen(res);
+	if (len > 0 && res[len - 1] == '/')
+		res[len - 1] = '\0';
+
+	return res;
+}
+
+static int event_cb(void *state, struct pmu_event_info *info)
+{
+	struct match_state *m = state;
+	char *clean_name;
+
+	if (m->event1 && m->event2)
+		return 1;
+
+	clean_name = clean_event_name(info);
+	if (!clean_name)
+		return 0;
+
+	if (!m->event1) {
+		m->event1 = clean_name;
+	} else {
+		if (strcmp(m->event1, clean_name)) {
+			m->event2 = clean_name;
+			return 1;
+		}
+		free(clean_name);
+	}
+	return 0;
+}
+
+#define CHECK_COND(cond, text)					\
+do {								\
+	if (!(cond)) {						\
+		pr_debug("FAILED %s:%d %s\n", __FILE__, __LINE__, text); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+#define CHECK_EQUAL(val, expected, text)			\
+do {								\
+	if ((val) != (expected)) {				\
+		pr_debug("FAILED %s:%d %s (%d != %d)\n",	\
+			 __FILE__, __LINE__, text, (val), (expected)); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+static int test__uncore_event_sorting(struct test_suite *test __maybe_unused,
+				      int subtest __maybe_unused)
+{
+	struct evlist *evlist = NULL;
+	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;
+
+	parse_events_error__init(&err);
+
+	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;
+		}
+		zfree(&m.event1);
+	}
+
+	if (!pmu_prefix) {
+		pr_debug("No suitable uncore PMU found\n");
+		ret = TEST_SKIP;
+		goto out_err;
+	}
+
+	evlist = evlist__new();
+	if (!evlist) {
+		ret = TEST_FAIL;
+		goto out_err;
+	}
+
+	snprintf(buf, sizeof(buf), "{%s/%s/,%s/%s/}", pmu_prefix, m.event1, pmu_prefix, m.event2);
+	pr_debug("Parsing: %s\n", buf);
+
+	ret = parse_events(evlist, buf, &err);
+	if (ret) {
+		pr_debug("parse_events failed\n");
+		ret = TEST_FAIL;
+		goto out_err;
+	}
+
+	CHECK_COND(evlist->core.nr_entries >= 4, "Number of events is >= 4");
+	CHECK_EQUAL(evlist->core.nr_entries % 2, 0, "Number of events is a multiple of 2");
+
+	evlist__for_each_entry(evlist, evsel) {
+		struct evsel *next;
+
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		next = evsel__next(evsel);
+		CHECK_EQUAL(evsel->core.nr_members, 2, "Group size is 2");
+		CHECK_COND(evsel->pmu == next->pmu, "PMU match");
+		CHECK_COND(strstr(evsel__name(evsel), m.event1) != NULL, "First event name");
+		CHECK_COND(strstr(evsel__name(next), m.event2) != NULL, "Second event name");
+	}
+	ret = TEST_OK;
+
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	zfree(&pmu_prefix);
+	zfree(&m.event1);
+	zfree(&m.event2);
+	return ret;
+}
+
+DEFINE_SUITE("Uncore event sorting", uncore_event_sorting);
-- 
2.54.0.563.g4f69b47b94-goog

[PATCH v8 2/2] perf arch x86 tests: Add test for topdown event sorting

[Ian Rogers]

Add a test to capture the comment in
tools/perf/arch/x86/util/evlist.c. Test that slots and
topdown-retiring get appropriately sorted with respect to instructions
when they're all specified together. When the PMU requires topdown
event grouping (indicated by the pressence of the slots event) metric
events should be after slots, which should be the group leader.

Add a related test that when the slots event isn't given it is
injected into the appropriate group.

Signed-off-by: Ian Rogers <irogers@google.com>
Tested-by: Zide Chen <zide.chen@intel.com>
---
 tools/perf/arch/x86/tests/topdown.c | 166 +++++++++++++++++++++++++++-
 1 file changed, 165 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/tests/topdown.c b/tools/perf/arch/x86/tests/topdown.c
index 3ee4e5e71be3..221f2c4bbb61 100644
--- a/tools/perf/arch/x86/tests/topdown.c
+++ b/tools/perf/arch/x86/tests/topdown.c
@@ -75,4 +75,168 @@ static int test__x86_topdown(struct test_suite *test __maybe_unused, int subtest
 	return ret;
 }
 
-DEFINE_SUITE("x86 topdown", x86_topdown);
+#define CHECK_COND(cond, text)					\
+do {								\
+	if (!(cond)) {						\
+		pr_debug("FAILED %s:%d %s\n", __FILE__, __LINE__, text); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+#define CHECK_EQUAL(val, expected, text)			\
+do {								\
+	if ((val) != (expected)) {				\
+		pr_debug("FAILED %s:%d %s (%d != %d)\n",	\
+			 __FILE__, __LINE__, text, (val), (expected)); \
+		ret = TEST_FAIL;				\
+		goto out_err;					\
+	}							\
+} while (0)
+
+static int test_sort(const char *str, int expected_slots_group_size,
+		     int expected_instructions_group_size)
+{
+	struct evlist *evlist = NULL;
+	struct parse_events_error err;
+	struct evsel *evsel;
+	int ret = TEST_FAIL;
+	bool slots_seen = false;
+
+	parse_events_error__init(&err);
+
+	evlist = evlist__new();
+	if (!evlist)
+		goto out_err;
+
+	if (parse_events(evlist, str, &err)) {
+		pr_debug("parse_events failed for %s\n", str);
+		goto out_err;
+	}
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (!evsel__is_group_leader(evsel))
+			continue;
+
+		if (strstr(evsel__name(evsel), "slots")) {
+			/*
+			 * Slots as a leader means the PMU is for a perf metric
+			 * group as the slots event isn't present when not.
+			 */
+			slots_seen = true;
+			CHECK_EQUAL(evsel->core.nr_members, expected_slots_group_size,
+				    "slots group size");
+			if (expected_slots_group_size == 3) {
+				struct evsel *next = evsel__next(evsel);
+				struct evsel *next2 = evsel__next(next);
+
+				CHECK_COND(strstr(evsel__name(next), "instructions") != NULL,
+					   "slots second event is instructions");
+				CHECK_COND(strstr(evsel__name(next2), "topdown-retiring") != NULL,
+					   "slots third event is topdown-retiring");
+			} else if (expected_slots_group_size == 2) {
+				struct evsel *next = evsel__next(evsel);
+
+				CHECK_COND(strstr(evsel__name(next), "topdown-retiring") != NULL,
+					   "slots second event is topdown-retiring");
+			}
+		} else if (strstr(evsel__name(evsel), "instructions")) {
+			CHECK_EQUAL(evsel->core.nr_members, expected_instructions_group_size,
+				    "instructions group size");
+			if (expected_instructions_group_size == 2) {
+				/*
+				 * On Intel hybrid CPUs (e.g., Alder Lake/
+				 * Raptor Lake), E-cores (cpu_atom) do not
+				 * support/enforce the slots event. When
+				 * parsing event groups containing slots
+				 * across all PMUs, slots is automatically
+				 * filtered out from cpu_atom, leaving
+				 * {cpu_atom/instructions/,
+				 *  cpu_atom/topdown-retiring/}. On cpu_atom,
+				 * instructions correctly leads this group of
+				 * 2 without slots reordering.
+				 */
+				struct evsel *next = evsel__next(evsel);
+
+				CHECK_COND(strstr(evsel__name(next), "topdown-retiring") != NULL,
+					   "instructions second event is topdown-retiring");
+			}
+		} else if (strstr(evsel__name(evsel), "topdown-retiring")) {
+			/*
+			 * A perf metric event where the PMU doesn't require
+			 * slots as a leader.
+			 */
+			CHECK_EQUAL(evsel->core.nr_members, 1, "topdown-retiring group size");
+		} else if (strstr(evsel__name(evsel), "cycles")) {
+			CHECK_EQUAL(evsel->core.nr_members, 1, "cycles group size");
+		}
+	}
+	CHECK_COND(slots_seen, "slots seen");
+	ret = TEST_OK;
+out_err:
+	evlist__delete(evlist);
+	parse_events_error__exit(&err);
+	return ret;
+}
+
+static int test__x86_topdown_sorting(struct test_suite *test __maybe_unused,
+				     int subtest __maybe_unused)
+{
+	int ret;
+
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	ret = test_sort("{instructions,topdown-retiring,slots}", 3, 2);
+	TEST_ASSERT_EQUAL("all events in a group", ret, TEST_OK);
+	ret = test_sort("instructions,topdown-retiring,slots", 2, 1);
+	TEST_ASSERT_EQUAL("all events not in a group", ret, TEST_OK);
+	ret = test_sort("{instructions,slots},topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("slots event in a group but topdown metrics events outside the group",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions,slots},{topdown-retiring}", 2, 1);
+	TEST_ASSERT_EQUAL("slots event and topdown metrics events in two groups",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions,slots},cycles,topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("slots event and metrics event are not in a group and not adjacent",
+			  ret, TEST_OK);
+
+	return TEST_OK;
+}
+
+static int test__x86_topdown_slots_injection(struct test_suite *test __maybe_unused,
+					     int subtest __maybe_unused)
+{
+	int ret;
+
+	if (!topdown_sys_has_perf_metrics())
+		return TEST_OK;
+
+	ret = test_sort("{instructions,topdown-retiring}", 3, 2);
+	TEST_ASSERT_EQUAL("all events in a group", ret, TEST_OK);
+	ret = test_sort("instructions,topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("all events not in a group", ret, TEST_OK);
+	ret = test_sort("{instructions},topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("event in a group but topdown metrics events outside the group",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions},{topdown-retiring}", 2, 1);
+	TEST_ASSERT_EQUAL("event and topdown metrics events in two groups",
+			  ret, TEST_OK);
+	ret = test_sort("{instructions},cycles,topdown-retiring", 2, 1);
+	TEST_ASSERT_EQUAL("event and metrics event are not in a group and not adjacent",
+			  ret, TEST_OK);
+
+	return TEST_OK;
+}
+
+static struct test_case x86_topdown_tests[] = {
+	TEST_CASE("topdown events", x86_topdown),
+	TEST_CASE("topdown sorting", x86_topdown_sorting),
+	TEST_CASE("topdown slots injection", x86_topdown_slots_injection),
+	{ .name = NULL, }
+};
+
+struct test_suite suite__x86_topdown = {
+	.desc = "x86 topdown",
+	.test_cases = x86_topdown_tests,
+};
-- 
2.54.0.563.g4f69b47b94-goog

Re: [PATCH v8 0/2] perf tests: Add uncore and x86 topdown event sorting tests

[Ian Rogers]

On Sun, May 17, 2026 at 11:29 PM Ian Rogers <irogers@google.com> wrote:
>
> A patch thread changing event sorting highlighted a lack of testing
> for the more complicated uncore and x86 perf metric event sorting:
> https://lore.kernel.org/linux-perf-users/CAP-5=fWRgDo7UnJAD4C--d=mVPRhOEWZVyU7nVM1YEp3jncAgg@mail.gmail.com/

Purely test changes with green analysis from Sashiko:
https://sashiko.dev/#/patchset/20260518062904.2591616-1-irogers%40google.com
It would be nice to get help landing this.

Thanks,
Ian

> v8: Simplify event_cb in the uncore event sorting test by removing the
>     unreachable dead-code else block (reported by Sashiko).
>
> v7: - Prevent double-free of m.event1 and dangling pointer access upon
>       PMU scanning loop termination or early continue in the uncore
>       event sorting test by utilizing standard perf zfree() helpers
>       (reported by Sashiko).
>     - Document Intel Hybrid CPU architectural invariants in the x86
>       topdown test to clarify why instructions correctly leads a group
>       of 2 on E-cores (cpu_atom) where slots is unsupported and
>       filtered out (reported by Sashiko).
>
> v6: - Ensure robust error initialization and resource cleanup in both
>       uncore and topdown tests by initializing evlist to NULL and
>       parse_events_error at function entry, preventing uninitialized
>       stack access or resource leaks on early return paths (reported
>       by Sashiko).
>     - Prevent double evaluation and duplicate debug logging upon test
>       assertion failure in the x86 topdown test by caching test_sort()
>       return values into local variables (reported by Sashiko).
>
> v5: - Strengthen uncore event sorting test assertions to require at
>       least 4 total events (>= 2 groups of 2 events) instead of > 0
>       events, guaranteeing that multi-PMU wildcard expansion is
>       genuinely exercised across multiple sibling PMUs (reported by Sashiko).
>     - Fix memory leaks on assertion failure across both uncore and
>       topdown test suites by replacing standard TEST_ASSERT macros
>       with local CHECK macros that correctly branch to out_err cleanup
>       handlers (reported by Sashiko).
>     - Fix test_sort() in the x86 topdown test to correctly return TEST_FAIL (-1)
>       instead of parse_events() internal error codes (e.g., -EINVAL) when event
>       parsing fails (reported by Namhyung).
>
> v4: Fix malformed event group string formatting in the uncore event
>     sorting test when running on AMD systems (where sysfs event
>     aliases lack JSON descriptions - reported by Namhyung). Added a
>     helper to strip redundant PMU prefixes and trailing slashes to
>     ensure parse_events() receives valid syntax across all
>     architectures.
>
> v3: Address sashiko nits on using evsel__name and ensuring slots is injected.
> https://lore.kernel.org/linux-perf-users/20260331185419.4085479-1-irogers@google.com/
>
> v2: Address indentation and other nits from Namhyung. Add Zide Chen's
>     tested-by tags.
> https://lore.kernel.org/lkml/20260331165207.4016392-1-irogers@google.com/
>
> v1: https://lore.kernel.org/lkml/20260325183045.1229502-1-irogers@google.com/
>
> Ian Rogers (2):
>   perf tests: Add test for uncore event sorting
>   perf arch x86 tests: Add test for topdown event sorting
>
>  tools/perf/arch/x86/tests/topdown.c     | 166 +++++++++++++++++++++-
>  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 | 176 ++++++++++++++++++++++++
>  5 files changed, 344 insertions(+), 1 deletion(-)
>  create mode 100644 tools/perf/tests/uncore-event-sorting.c
>
> --
> 2.54.0.563.g4f69b47b94-goog
>

Re: [PATCH v8 0/2] perf tests: Add uncore and x86 topdown event sorting tests

[Namhyung Kim]

On Wed, May 20, 2026 at 08:31:31AM -0700, Ian Rogers wrote:
> On Sun, May 17, 2026 at 11:29 PM Ian Rogers <irogers@google.com> wrote:
> >
> > A patch thread changing event sorting highlighted a lack of testing
> > for the more complicated uncore and x86 perf metric event sorting:
> > https://lore.kernel.org/linux-perf-users/CAP-5=fWRgDo7UnJAD4C--d=mVPRhOEWZVyU7nVM1YEp3jncAgg@mail.gmail.com/
> 
> Purely test changes with green analysis from Sashiko:
> https://sashiko.dev/#/patchset/20260518062904.2591616-1-irogers%40google.com
> It would be nice to get help landing this.

On my AMD machine, it looks good now.

  $ sudo tools/perf/perf test -v sorting
    6: Uncore event sorting                                            : Ok
   76.2: topdown sorting                                               : Ok

Tested-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung