Re: [PATCH v2] perf pmu: Skip test on Arm64 when #slots is zero

Re: [PATCH v2] perf pmu: Skip test on Arm64 when #slots is zero

[Leo Yan]

On Tue, Apr 21, 2026 at 03:20:08PM +0100, James Clark wrote:

[...]

> > This commit introduces a new function is_expected_broken_metric() to
> > identify broken metrics, and treats metrics containing "#slots" as
> > expected broken when #slots == 0 on Arm64 platforms.
> > 
> > Fixes: 3a61fd866ef9 ("perf expr: Return -EINVAL for syntax error in expr__find_ids()")
> > Signed-off-by: Leo Yan <leo.yan@arm.com>
> 
> Reviewed-by: James Clark <james.clark@linaro.org>

Gentle ping. Thanks!

Re: [PATCH v2] perf pmu: Skip test on Arm64 when #slots is zero

[Ian Rogers]

On Fri, Apr 10, 2026 at 4:13 AM Leo Yan <leo.yan@arm.com> wrote:
>
> Some Arm64 PMUs expose 'caps/slots' as 0 when the slot count is not
> implemented, tool_pmu__read_event() currently returns false for this,
> so metrics that reference #slots are reported as syntax error.
>
> Since the commit 3a61fd866ef9 ("perf expr: Return -EINVAL for syntax
> error in expr__find_ids()"), these syntax errors are populated as
> failures and make the PMU metric test fail:
>
>     9.3: Parsing of PMU event table metrics:
>     --- start ---
>     ...
>
>     Found metric 'backend_bound'
>     metric expr 100 * (stall_slot_backend / (#slots * cpu_cycles)) for backend_bound
>     parsing metric: 100 * (stall_slot_backend / (#slots * cpu_cycles))
>     Failure to read '#slots'
>     literal: #slots = nan
>     syntax error
>     Fail to parse metric or group `backend_bound'
>
>     ...
>     ---- end(-1) ----
>     9.3: Parsing of PMU event table metrics    : FAILED!
>
> This commit introduces a new function is_expected_broken_metric() to
> identify broken metrics, and treats metrics containing "#slots" as
> expected broken when #slots == 0 on Arm64 platforms.
>
> Fixes: 3a61fd866ef9 ("perf expr: Return -EINVAL for syntax error in expr__find_ids()")
> Signed-off-by: Leo Yan <leo.yan@arm.com>

We get this failure for these metrics on x86 when building perf with
JEVENTS_ARCH=all. Rather than expecting the parse failure perhaps we
should just always return true in tool_pmu__read_event:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/tool_pmu.c#n421
I guess the problem there is that when these metrics are broken (no
slots value) you can't distinguish this case from other valid cases.

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

Thanks,
Ian

> ---
> Changes in v2:
> - Checked pm->metric_expr instead of pm->metric_name and removed the
>   negation before strstr() suggested by sashiko.
> - Link to v1: https://lore.kernel.org/r/20260410-perf_fix_pmu_metrics_test-v1-1-18a5d80f71b6@arm.com
> ---
>  tools/perf/tests/pmu-events.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index a997168621688007c85495e2d9f6f459c2471516..b1609a7e1d8c9427e6bf9500380e8aac6167c7fa 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -15,6 +15,7 @@
>  #include "util/expr.h"
>  #include "util/hashmap.h"
>  #include "util/parse-events.h"
> +#include "util/tool_pmu.h"
>  #include "metricgroup.h"
>  #include "stat.h"
>
> @@ -817,6 +818,26 @@ struct metric {
>         struct metric_ref metric_ref;
>  };
>
> +static bool is_expected_broken_metric(const struct pmu_metric *pm)
> +{
> +       if (!strcmp(pm->metric_name, "M1") || !strcmp(pm->metric_name, "M2") ||
> +           !strcmp(pm->metric_name, "M3"))
> +               return true;
> +
> +#if defined(__aarch64__)
> +       /*
> +        * Arm64 platforms may return "#slots == 0", which is treated as a
> +        * syntax error by the parser. Don't test these metrics when running
> +        * on such platforms.
> +        */
> +       if (strstr(pm->metric_expr, "#slots") &&
> +           !tool_pmu__cpu_slots_per_cycle())
> +               return true;
> +#endif
> +
> +       return false;
> +}
> +
>  static int test__parsing_callback(const struct pmu_metric *pm,
>                                   const struct pmu_metrics_table *table,
>                                   void *data)
> @@ -852,8 +873,7 @@ static int test__parsing_callback(const struct pmu_metric *pm,
>
>         err = metricgroup__parse_groups_test(evlist, table, pm->metric_name);
>         if (err) {
> -               if (!strcmp(pm->metric_name, "M1") || !strcmp(pm->metric_name, "M2") ||
> -                   !strcmp(pm->metric_name, "M3")) {
> +               if (is_expected_broken_metric(pm)) {
>                         (*failures)--;
>                         pr_debug("Expected broken metric %s skipping\n", pm->metric_name);
>                         err = 0;
>
> ---
> base-commit: 4cf1f549bbcdfea9c20df52994bb342677472dcd
> change-id: 20260408-perf_fix_pmu_metrics_test-73c4dedc8585
>
> Best regards,
> --
> Leo Yan <leo.yan@arm.com>
>

Re: [PATCH v2] perf pmu: Skip test on Arm64 when #slots is zero

[Leo Yan]

On Wed, May 13, 2026 at 06:10:00AM -0700, Ian Rogers wrote:

[...]

> We get this failure for these metrics on x86 when building perf with
> JEVENTS_ARCH=all. Rather than expecting the parse failure perhaps we
> should just always return true in tool_pmu__read_event:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/tool_pmu.c#n421

I considered returning true when slots == 0 so can mute parser error
and allow the test to pass. However, if so platforms which do not
support #slots would be able to use those metrics and generate
meaningless statistics.

I would keep the parser errors so this is a reminding when users wrongly
use unsupported metrics.

> I guess the problem there is that when these metrics are broken (no
> slots value) you can't distinguish this case from other valid cases.

IMO, this is a test design issue: tests should validate metrics while
remaining hardware-agnostic. Hardware-specific cases should either run
only on supported platforms, or the tests should be refined to run
transparently across different hardware.

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

Thanks for review!

Leo