Re: [PATCH] perf metricgroup: Correctly return syntax error

[James Clark]

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@xxxxxxxxxx/



> 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@xxxxxxx>
> ---
>   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,