From: Namhyung Kim <namhyung@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: acme@kernel.org, adrian.hunter@intel.com,
alexander.shishkin@linux.intel.com, james.clark@linaro.org,
jolsa@kernel.org, linux-kernel@vger.kernel.org,
linux-perf-users@vger.kernel.org, mingo@redhat.com,
peterz@infradead.org
Subject: Re: [PATCH v2 2/2] perf stat: Fix opt->value type for parse_cache_level
Date: Thu, 26 Mar 2026 15:02:29 -0700 [thread overview]
Message-ID: <acWs9Vrj4AA4YypB@google.com> (raw)
In-Reply-To: <20260319233349.306012-2-irogers@google.com>
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@google.com>
> ---
> 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
>
next prev parent reply other threads:[~2026-03-26 22:02 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-19 22:12 [PATCH v1 1/2] perf lock: Fix option value type in parse_max_stack Ian Rogers
2026-03-19 22:12 ` [PATCH v1 2/2] perf stat: Fix opt->value type for parse_cache_level Ian Rogers
2026-03-19 23:33 ` [PATCH v2 1/2] perf lock: Fix option value type in parse_max_stack Ian Rogers
2026-03-19 23:33 ` [PATCH v2 2/2] perf stat: Fix opt->value type for parse_cache_level Ian Rogers
2026-03-26 22:02 ` Namhyung Kim [this message]
2026-03-27 21:01 ` [PATCH v2 1/2] perf lock: Fix option value type in parse_max_stack Namhyung Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=acWs9Vrj4AA4YypB@google.com \
--to=namhyung@kernel.org \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=irogers@google.com \
--cc=james.clark@linaro.org \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox