[PATCH 0/2] perf tools: Always uniquify event names

[PATCH 0/2] perf tools: Always uniquify event names

[James Clark]

The commit where this test starts failing is commit 2e6dc3b9334c ("perf
test parse-events: Without a PMU use cpu-cycles rather than cycles"),
but that commit is only a test change, whereas this fix is a code
change. For that reason I didn't add a fixes tag because I don't think
it's worth the risk of backporting a potential behavioral change just to
fix a test.

I'm also fairly unconfident on what the impact of this change is, other
than there's no change to the test results apart from the mentioned
failure.

It also brings up the question whether we should be running the tests
in both verbose and non-verbose modes by default. With a non trivial
amount of behavior being affected by the flag it seems like it could
catch some issues. Although it would double the runtime, and not all of
the tests will ever be affected.

Signed-off-by: James Clark <james.clark@linaro.org>
---
James Clark (2):
      perf tools: Always uniquify event names
      perf test: Add missing newlines in debug messages

 tools/perf/tests/parse-events.c | 6 +++---
 tools/perf/util/parse-events.c  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
---
base-commit: 267c2e633af6e9461477bed91e428993f8b36ee8
change-id: 20251203-james-parse-events-test-b4c092807640

Best regards,
-- 
James Clark <james.clark@linaro.org>

[PATCH 1/2] perf tools: Always uniquify event names

[James Clark]

evlist__uniquify_evsel_names() only gets called in __parse_events() if
verbose is > 0. This means that the auto added "slots" events stay as
"slots" rather than being expanded to "cpu_core/slots/" unless Perf is
run in verbose mode. This is invisible to users when running Perf stat
because evlist__print_counters() always calls it regardless of verbose
mode before displaying.

The only thing this seems to affect is the test "Parsing of all PMU
events from sysfs" which fails when not run in verbose mode.
test__checkevent_pmu_events() always expects event names to be prefixed
with the pmu name, but this only happens for "slots" events after
evlist__uniquify_evsel_names() is called.

One fix could be to relax the test to accept the non prefixed name in
normal mode. But seeing as Perf stat uniquifies unconditionally, make
parse_events() do the same.

This fixes the following test failure:

  $ perf test "Parsing of all PMU events from sysfs"
  5.2: Parsing of all PMU events from sysfs                    : FAILED!

Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/perf/util/parse-events.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 17c1c36a7bf9..50e88c9d3d46 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2217,12 +2217,12 @@ int __parse_events(struct evlist *evlist, const char *str, const char *pmu_filte
 	evlist__splice_list_tail(evlist, &parse_state.list);
 
 	if (ret2 && warn_if_reordered && !parse_state.wild_card_pmus) {
+		evlist__uniquify_evsel_names(evlist, &stat_config);
 		pr_warning("WARNING: events were regrouped to match PMUs\n");
 
 		if (verbose > 0) {
 			struct strbuf sb = STRBUF_INIT;
 
-			evlist__uniquify_evsel_names(evlist, &stat_config);
 			evlist__format_evsels(evlist, &sb, 2048);
 			pr_debug("evlist after sorting/fixing: '%s'\n", sb.buf);
 			strbuf_release(&sb);

-- 
2.34.1

Re: [PATCH 1/2] perf tools: Always uniquify event names

[Ian Rogers]

On Thu, Dec 4, 2025 at 1:11 AM James Clark <james.clark@linaro.org> wrote:
>
> evlist__uniquify_evsel_names() only gets called in __parse_events() if
> verbose is > 0. This means that the auto added "slots" events stay as
> "slots" rather than being expanded to "cpu_core/slots/" unless Perf is
> run in verbose mode. This is invisible to users when running Perf stat
> because evlist__print_counters() always calls it regardless of verbose
> mode before displaying.
>
> The only thing this seems to affect is the test "Parsing of all PMU
> events from sysfs" which fails when not run in verbose mode.
> test__checkevent_pmu_events() always expects event names to be prefixed
> with the pmu name, but this only happens for "slots" events after
> evlist__uniquify_evsel_names() is called.
>
> One fix could be to relax the test to accept the non prefixed name in
> normal mode. But seeing as Perf stat uniquifies unconditionally, make
> parse_events() do the same.
>
> This fixes the following test failure:
>
>   $ perf test "Parsing of all PMU events from sysfs"
>   5.2: Parsing of all PMU events from sysfs                    : FAILED!
>
> Signed-off-by: James Clark <james.clark@linaro.org>

Reviewed-by: Ian Rogers <irogers@google.com>

I wonder if we should just always uniquify event names. It could be a
thing that is changing as more and more PMUs are available. I'm
reminded that we used to uniquify sometimes with "<event> [<pmu>]"
prior to commit 057f8bfc6f70 ("perf stat: Uniquify event name
improvements").

Thanks,
Ian

> ---
>  tools/perf/util/parse-events.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 17c1c36a7bf9..50e88c9d3d46 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2217,12 +2217,12 @@ int __parse_events(struct evlist *evlist, const char *str, const char *pmu_filte
>         evlist__splice_list_tail(evlist, &parse_state.list);
>
>         if (ret2 && warn_if_reordered && !parse_state.wild_card_pmus) {
> +               evlist__uniquify_evsel_names(evlist, &stat_config);
>                 pr_warning("WARNING: events were regrouped to match PMUs\n");
>
>                 if (verbose > 0) {
>                         struct strbuf sb = STRBUF_INIT;
>
> -                       evlist__uniquify_evsel_names(evlist, &stat_config);
>                         evlist__format_evsels(evlist, &sb, 2048);
>                         pr_debug("evlist after sorting/fixing: '%s'\n", sb.buf);
>                         strbuf_release(&sb);
>
> --
> 2.34.1
>

Re: [PATCH 1/2] perf tools: Always uniquify event names

[Arnaldo Carvalho de Melo]

On Thu, Dec 04, 2025 at 08:34:34AM -0800, Ian Rogers wrote:
> On Thu, Dec 4, 2025 at 1:11 AM James Clark <james.clark@linaro.org> wrote:
> > evlist__uniquify_evsel_names() only gets called in __parse_events() if
> > verbose is > 0. This means that the auto added "slots" events stay as
> > "slots" rather than being expanded to "cpu_core/slots/" unless Perf is
> > run in verbose mode. This is invisible to users when running Perf stat
> > because evlist__print_counters() always calls it regardless of verbose
> > mode before displaying.

> > The only thing this seems to affect is the test "Parsing of all PMU
> > events from sysfs" which fails when not run in verbose mode.
> > test__checkevent_pmu_events() always expects event names to be prefixed
> > with the pmu name, but this only happens for "slots" events after
> > evlist__uniquify_evsel_names() is called.

> > One fix could be to relax the test to accept the non prefixed name in
> > normal mode. But seeing as Perf stat uniquifies unconditionally, make
> > parse_events() do the same.

> > This fixes the following test failure:

> >   $ perf test "Parsing of all PMU events from sysfs"
> >   5.2: Parsing of all PMU events from sysfs                    : FAILED!

> > Signed-off-by: James Clark <james.clark@linaro.org>

> Reviewed-by: Ian Rogers <irogers@google.com>

Thanks, applied to perf-tools-next,

- Arnaldo

[PATCH 2/2] perf test: Add missing newlines in debug messages

[James Clark]

These debug messages bleed into the next log line. Fix it by adding the
missing newlines.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 tools/perf/tests/parse-events.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 128d21dc389f..62aa40d8114a 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -2627,7 +2627,7 @@ static int test_events(const struct evlist_test *events, int cnt)
 		pr_debug("running test %d '%s'\n", i, e.name);
 		test_ret = test_event(&e);
 		if (test_ret != TEST_OK) {
-			pr_debug("Event test failure: test %d '%s'", i, e.name);
+			pr_debug("Event test failure: test %d '%s'\n", i, e.name);
 			ret = combine_test_results(ret, test_ret);
 		}
 	}
@@ -2764,7 +2764,7 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
 
 			test_ret = test_event(&e);
 			if (test_ret != TEST_OK) {
-				pr_debug("Test PMU event failed for '%s'", name);
+				pr_debug("Test PMU event failed for '%s'\n", name);
 				ret = combine_test_results(ret, test_ret);
 			}
 
@@ -2790,7 +2790,7 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
 			e.check = test__checkevent_pmu_events_mix;
 			test_ret = test_event(&e);
 			if (test_ret != TEST_OK) {
-				pr_debug("Test PMU event failed for '%s'", name);
+				pr_debug("Test PMU event failed for '%s'\n", name);
 				ret = combine_test_results(ret, test_ret);
 			}
 		}

-- 
2.34.1

Re: [PATCH 2/2] perf test: Add missing newlines in debug messages

[Ian Rogers]

On Thu, Dec 4, 2025 at 1:11 AM James Clark <james.clark@linaro.org> wrote:
>
> These debug messages bleed into the next log line. Fix it by adding the
> missing newlines.
>
> Signed-off-by: James Clark <james.clark@linaro.org>

Reviewed-by: Ian Rogers <irogers@google.com>

I looked at the Fixes tag situation but there are a few different
patches that did this wrong. Given this is debug messages in a test,
backporting is a huge priority.

Thanks!
Ian

> ---
>  tools/perf/tests/parse-events.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 128d21dc389f..62aa40d8114a 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -2627,7 +2627,7 @@ static int test_events(const struct evlist_test *events, int cnt)
>                 pr_debug("running test %d '%s'\n", i, e.name);
>                 test_ret = test_event(&e);
>                 if (test_ret != TEST_OK) {
> -                       pr_debug("Event test failure: test %d '%s'", i, e.name);
> +                       pr_debug("Event test failure: test %d '%s'\n", i, e.name);
>                         ret = combine_test_results(ret, test_ret);
>                 }
>         }
> @@ -2764,7 +2764,7 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
>
>                         test_ret = test_event(&e);
>                         if (test_ret != TEST_OK) {
> -                               pr_debug("Test PMU event failed for '%s'", name);
> +                               pr_debug("Test PMU event failed for '%s'\n", name);
>                                 ret = combine_test_results(ret, test_ret);
>                         }
>
> @@ -2790,7 +2790,7 @@ static int test__pmu_events(struct test_suite *test __maybe_unused, int subtest
>                         e.check = test__checkevent_pmu_events_mix;
>                         test_ret = test_event(&e);
>                         if (test_ret != TEST_OK) {
> -                               pr_debug("Test PMU event failed for '%s'", name);
> +                               pr_debug("Test PMU event failed for '%s'\n", name);
>                                 ret = combine_test_results(ret, test_ret);
>                         }
>                 }
>
> --
> 2.34.1
>