[PATCH] perf metricgroup: Correctly return syntax error

Leo Yan posted 1 patch 5 days, 20 hours ago
tools/perf/util/metricgroup.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[PATCH] perf metricgroup: Correctly return syntax error
Posted by Leo Yan 5 days, 20 hours ago
On arm64 platforms, some CPU variants return zero for the number of slots
when invoking tool_pmu__cpu_slots_per_cycle().  This leads to metric
expression parsing failures, e.g.:

  metric expr 100 * (STALL_SLOT_BACKEND / (CPU_CYCLES * #slots) - BR_MIS_PRED * 3 / CPU_CYCLES) for backend_bound
  parsing metric: 100 * (STALL_SLOT_BACKEND / (CPU_CYCLES * #slots) - BR_MIS_PRED * 3 / CPU_CYCLES)
  Failure to read '#slots' literal: #slots = nan
  syntax error

expr__find_ids() returns a positive value for syntax errors, but this is
not respected.  Fix this by checking for any non-zero return value from
expr__find_ids().

Do not add a Fixes tag, as this is a rare case that can only be reproduced
by deliberately setting the PERF_CPUS env variable.

Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 tools/perf/util/metricgroup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 7e39d469111b2995f5a2636529d6985eac595f76..1119bb1c99dbd9d32601b8aeebed7d5638872366 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -909,10 +909,12 @@ static int __add_metric(struct list_head *metric_list,
 		expr = metric_no_threshold ? pm->metric_name : pm->metric_threshold;
 		visited_node.name = "__threshold__";
 	}
-	if (expr__find_ids(expr, NULL, root_metric->pctx) < 0) {
+
+	/* The expression parser returns a positive value on syntax error */
+	if (expr__find_ids(expr, NULL, root_metric->pctx))
 		/* Broken metric. */
 		ret = -EINVAL;
-	}
+
 	if (!ret) {
 		/* Resolve referenced metrics. */
 		struct perf_pmu *pmu;

---
base-commit: 54fcc7f6ec3944ae7c1b0246a999744e33839cdb
change-id: 20260327-perf_fix_metrics_group-4be55b236f96

Best regards,
-- 
Leo Yan <leo.yan@arm.com>
Re: [PATCH] perf metricgroup: Correctly return syntax error
Posted by James Clark 2 days, 4 hours ago

On 27/03/2026 5:25 pm, Leo Yan wrote:
> On arm64 platforms, some CPU variants return zero for the number of slots
> when invoking tool_pmu__cpu_slots_per_cycle().  This leads to metric
> expression parsing failures, e.g.:
> 
>    metric expr 100 * (STALL_SLOT_BACKEND / (CPU_CYCLES * #slots) - BR_MIS_PRED * 3 / CPU_CYCLES) for backend_bound
>    parsing metric: 100 * (STALL_SLOT_BACKEND / (CPU_CYCLES * #slots) - BR_MIS_PRED * 3 / CPU_CYCLES)
>    Failure to read '#slots' literal: #slots = nan
>    syntax error
> 
> expr__find_ids() returns a positive value for syntax errors, but this is
> not respected.  Fix this by checking for any non-zero return value from
> expr__find_ids().

What command does this impact exactly, and what does the new output look 
like?

Because "failure to read '#slots'" seems like ok output to me. I also 
saw this related commit where it seems like showing NAN is intentional:

https://lore.kernel.org/linux-perf-users/20260203230733.1474840-1-ctshao@google.com/



> 
> Do not add a Fixes tag, as this is a rare case that can only be reproduced
> by deliberately setting the PERF_CPUS env variable.

Slots is always 0 in guests so I think this might not be that rare, 
although that should be fixed in a different way.
Maybe a fixes tag is worthwhile until we fix #slots in guests somehow.

> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   tools/perf/util/metricgroup.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 7e39d469111b2995f5a2636529d6985eac595f76..1119bb1c99dbd9d32601b8aeebed7d5638872366 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -909,10 +909,12 @@ static int __add_metric(struct list_head *metric_list,
>   		expr = metric_no_threshold ? pm->metric_name : pm->metric_threshold;
>   		visited_node.name = "__threshold__";
>   	}
> -	if (expr__find_ids(expr, NULL, root_metric->pctx) < 0) {
> +
> +	/* The expression parser returns a positive value on syntax error */
> +	if (expr__find_ids(expr, NULL, root_metric->pctx))
>   		/* Broken metric. */
>   		ret = -EINVAL;
> -	}
> +
>   	if (!ret) {
>   		/* Resolve referenced metrics. */
>   		struct perf_pmu *pmu;
> 
> ---
> base-commit: 54fcc7f6ec3944ae7c1b0246a999744e33839cdb
> change-id: 20260327-perf_fix_metrics_group-4be55b236f96
> 
> Best regards,