Re: [PATCH v2 2/2] perf stat: Fix opt->value type for parse_cache_level

[Namhyung Kim]

On Thu, Mar 19, 2026 at 04:33:49PM -0700, Ian Rogers wrote:
> Commit f5803651b4a4 ("perf stat: Choose the most disaggregate command
> line option") changed aggregation option handling for `perf stat` but
> not `perf stat report` leading to parse_cache_level being passed a
> struct in the `perf stat` case but erroneously an aggr_mode enum value
> for `perf stat report`. Change the `perf stat report` aggregation
> handling to use the same opt_aggr_mode as `perf stat`. Also, just pass
> the boolean for consistency with other boolean argument handling.
> 
> Fixes: f5803651b4a4 ("perf stat: Choose the most disaggregate command line option")
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
> v2: Add in fixes suggested by Sashiko.
> ---
>  tools/perf/builtin-stat.c | 43 +++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 73c2ba7e3076..02c1c02d4b27 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -164,7 +164,7 @@ struct opt_aggr_mode {
>  };
>  
>  /* Turn command line option into most generic aggregation mode setting. */
> -static enum aggr_mode opt_aggr_mode_to_aggr_mode(struct opt_aggr_mode *opt_mode)
> +static enum aggr_mode opt_aggr_mode_to_aggr_mode(const struct opt_aggr_mode *opt_mode)
>  {
>  	enum aggr_mode mode = AGGR_GLOBAL;
>  
> @@ -1219,8 +1219,8 @@ static int parse_cache_level(const struct option *opt,
>  			     int unset __maybe_unused)
>  {
>  	int level;
> -	struct opt_aggr_mode *opt_aggr_mode = (struct opt_aggr_mode *)opt->value;
> -	u32 *aggr_level = (u32 *)opt->data;
> +	bool *per_cache = opt->value;
> +	u32 *aggr_level = opt->data;
>  
>  	/*
>  	 * If no string is specified, aggregate based on the topology of
> @@ -1258,7 +1258,7 @@ static int parse_cache_level(const struct option *opt,
>  		return -EINVAL;
>  	}
>  out:
> -	opt_aggr_mode->cache = true;
> +	*per_cache = true;
>  	*aggr_level = level;
>  	return 0;
>  }
> @@ -2305,24 +2305,23 @@ static struct perf_stat perf_stat = {
>  static int __cmd_report(int argc, const char **argv)
>  {
>  	struct perf_session *session;
> +	struct opt_aggr_mode opt_mode = {};
>  	const struct option options[] = {
>  	OPT_STRING('i', "input", &input_name, "file", "input file name"),
> -	OPT_SET_UINT(0, "per-socket", &perf_stat.aggr_mode,
> -		     "aggregate counts per processor socket", AGGR_SOCKET),
> -	OPT_SET_UINT(0, "per-die", &perf_stat.aggr_mode,
> -		     "aggregate counts per processor die", AGGR_DIE),
> -	OPT_SET_UINT(0, "per-cluster", &perf_stat.aggr_mode,
> -		     "aggregate counts perf processor cluster", AGGR_CLUSTER),
> -	OPT_CALLBACK_OPTARG(0, "per-cache", &perf_stat.aggr_mode, &perf_stat.aggr_level,
> -			    "cache level",
> -			    "aggregate count at this cache level (Default: LLC)",
> +		OPT_BOOLEAN(0, "per-thread", &opt_mode.thread, "aggregate counts per thread"),

A wrong indentation.  I'll fix this time.


> +	OPT_BOOLEAN(0, "per-socket", &opt_mode.socket,
> +		    "aggregate counts per processor socket"),
> +	OPT_BOOLEAN(0, "per-die", &opt_mode.die, "aggregate counts per processor die"),
> +	OPT_BOOLEAN(0, "per-cluster", &opt_mode.cluster,
> +		    "aggregate counts per processor cluster"),
> +	OPT_CALLBACK_OPTARG(0, "per-cache", &opt_mode.cache, &perf_stat.aggr_level,
> +			    "cache level", "aggregate count at this cache level (Default: LLC)",
>  			    parse_cache_level),
> -	OPT_SET_UINT(0, "per-core", &perf_stat.aggr_mode,
> -		     "aggregate counts per physical processor core", AGGR_CORE),
> -	OPT_SET_UINT(0, "per-node", &perf_stat.aggr_mode,
> -		     "aggregate counts per numa node", AGGR_NODE),
> -	OPT_SET_UINT('A', "no-aggr", &perf_stat.aggr_mode,
> -		     "disable CPU count aggregation", AGGR_NONE),
> +	OPT_BOOLEAN(0, "per-core", &opt_mode.core,
> +		    "aggregate counts per physical processor core"),
> +	OPT_BOOLEAN(0, "per-node", &opt_mode.node, "aggregate counts per numa node"),
> +	OPT_BOOLEAN('A', "no-aggr", &opt_mode.no_aggr,
> +		    "disable aggregation across CPUs or PMUs"),
>  	OPT_END()
>  	};
>  	struct stat st;
> @@ -2330,6 +2329,10 @@ static int __cmd_report(int argc, const char **argv)
>  
>  	argc = parse_options(argc, argv, options, stat_report_usage, 0);
>  
> +	perf_stat.aggr_mode = opt_aggr_mode_to_aggr_mode(&opt_mode);
> +	if (perf_stat.aggr_mode == AGGR_GLOBAL)
> +		perf_stat.aggr_mode = AGGR_UNSET; /* No option found so leave unset. */

I think it's more intuitive if the function returns AGGR_UNSET if none
of the field was set.  Then we need to change record and stat parts to
set GLOBAL when it's UNSET.  But this could be a separate change.

Thanks,
Namhyung


> +
>  	if (!input_name || !strlen(input_name)) {
>  		if (!fstat(STDIN_FILENO, &st) && S_ISFIFO(st.st_mode))
>  			input_name = "-";
> @@ -2506,7 +2509,7 @@ int cmd_stat(int argc, const char **argv)
>  		OPT_BOOLEAN(0, "per-die", &opt_mode.die, "aggregate counts per processor die"),
>  		OPT_BOOLEAN(0, "per-cluster", &opt_mode.cluster,
>  			"aggregate counts per processor cluster"),
> -		OPT_CALLBACK_OPTARG(0, "per-cache", &opt_mode, &stat_config.aggr_level,
> +		OPT_CALLBACK_OPTARG(0, "per-cache", &opt_mode.cache, &stat_config.aggr_level,
>  				"cache level", "aggregate count at this cache level (Default: LLC)",
>  				parse_cache_level),
>  		OPT_BOOLEAN(0, "per-core", &opt_mode.core,
> -- 
> 2.53.0.959.g497ff81fa9-goog
>