Having metric parsing as part of argument processing causes issues as
flags like metric-no-group may be specified later. It also denies the
opportunity to optimize the events on SMT systems where fewer events
may be possible if we know the target is system-wide. Move metric
parsing to after command line option parsing. Because of how stat runs
this moves the parsing after record/report which fail to work with
metrics currently anyway.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/builtin-stat.c | 48 ++++++++++++++++++++++++-----------
tools/perf/util/metricgroup.c | 3 +--
tools/perf/util/metricgroup.h | 2 +-
3 files changed, 35 insertions(+), 18 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 7fb81a44672d..c813b1aa7d7c 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -191,6 +191,7 @@ static bool append_file;
static bool interval_count;
static const char *output_name;
static int output_fd;
+static char *metrics;
struct perf_stat {
bool record;
@@ -1147,14 +1148,21 @@ static int enable_metric_only(const struct option *opt __maybe_unused,
return 0;
}
-static int parse_metric_groups(const struct option *opt,
+static int append_metric_groups(const struct option *opt __maybe_unused,
const char *str,
int unset __maybe_unused)
{
- return metricgroup__parse_groups(opt, str,
- stat_config.metric_no_group,
- stat_config.metric_no_merge,
- &stat_config.metric_events);
+ if (metrics) {
+ char *tmp;
+
+ if (asprintf(&tmp, "%s,%s", metrics, str) < 0)
+ return -ENOMEM;
+ free(metrics);
+ metrics = tmp;
+ } else {
+ metrics = strdup(str);
+ }
+ return 0;
}
static int parse_control_option(const struct option *opt,
@@ -1298,7 +1306,7 @@ static struct option stat_options[] = {
"measure SMI cost"),
OPT_CALLBACK('M', "metrics", &evsel_list, "metric/metric group list",
"monitor specified metrics or metric groups (separated by ,)",
- parse_metric_groups),
+ append_metric_groups),
OPT_BOOLEAN_FLAG(0, "all-kernel", &stat_config.all_kernel,
"Configure all used events to run in kernel space.",
PARSE_OPT_EXCLUSIVE),
@@ -1791,11 +1799,9 @@ static int add_default_attributes(void)
* on an architecture test for such a metric name.
*/
if (metricgroup__has_metric("transaction")) {
- struct option opt = { .value = &evsel_list };
-
- return metricgroup__parse_groups(&opt, "transaction",
+ return metricgroup__parse_groups(evsel_list, "transaction",
stat_config.metric_no_group,
- stat_config.metric_no_merge,
+ stat_config.metric_no_merge,
&stat_config.metric_events);
}
@@ -2260,8 +2266,6 @@ int cmd_stat(int argc, const char **argv)
argc = parse_options_subcommand(argc, argv, stat_options, stat_subcommands,
(const char **) stat_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
- perf_stat__collect_metric_expr(evsel_list);
- perf_stat__init_shadow_stats();
if (stat_config.csv_sep) {
stat_config.csv_output = true;
@@ -2428,6 +2432,23 @@ int cmd_stat(int argc, const char **argv)
target.system_wide = true;
}
+ if ((stat_config.aggr_mode == AGGR_THREAD) && (target.system_wide))
+ target.per_thread = true;
+
+ /*
+ * Metric parsing needs to be delayed as metrics may optimize events
+ * knowing the target is system-wide.
+ */
+ if (metrics) {
+ metricgroup__parse_groups(evsel_list, metrics,
+ stat_config.metric_no_group,
+ stat_config.metric_no_merge,
+ &stat_config.metric_events);
+ zfree(&metrics);
+ }
+ perf_stat__collect_metric_expr(evsel_list);
+ perf_stat__init_shadow_stats();
+
if (add_default_attributes())
goto out;
@@ -2447,9 +2468,6 @@ int cmd_stat(int argc, const char **argv)
}
}
- if ((stat_config.aggr_mode == AGGR_THREAD) && (target.system_wide))
- target.per_thread = true;
-
if (evlist__fix_hybrid_cpus(evsel_list, target.cpu_list)) {
pr_err("failed to use cpu list %s\n", target.cpu_list);
goto out;
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index b144c3e35264..9151346a16ab 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -1646,13 +1646,12 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
return ret;
}
-int metricgroup__parse_groups(const struct option *opt,
+int metricgroup__parse_groups(struct evlist *perf_evlist,
const char *str,
bool metric_no_group,
bool metric_no_merge,
struct rblist *metric_events)
{
- struct evlist *perf_evlist = *(struct evlist **)opt->value;
const struct pmu_events_table *table = pmu_events_table__find();
if (!table)
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index 016b3b1a289a..af9ceadaec0f 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -64,7 +64,7 @@ struct metric_expr {
struct metric_event *metricgroup__lookup(struct rblist *metric_events,
struct evsel *evsel,
bool create);
-int metricgroup__parse_groups(const struct option *opt,
+int metricgroup__parse_groups(struct evlist *perf_evlist,
const char *str,
bool metric_no_group,
bool metric_no_merge,
--
2.37.2.672.g94769d06f0-googEm Tue, Aug 30, 2022 at 09:48:44AM -0700, Ian Rogers escreveu:
> Having metric parsing as part of argument processing causes issues as
> flags like metric-no-group may be specified later. It also denies the
> opportunity to optimize the events on SMT systems where fewer events
> may be possible if we know the target is system-wide. Move metric
> parsing to after command line option parsing. Because of how stat runs
> this moves the parsing after record/report which fail to work with
> metrics currently anyway.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/builtin-stat.c | 48 ++++++++++++++++++++++++-----------
> tools/perf/util/metricgroup.c | 3 +--
> tools/perf/util/metricgroup.h | 2 +-
> 3 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 7fb81a44672d..c813b1aa7d7c 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -191,6 +191,7 @@ static bool append_file;
> static bool interval_count;
> static const char *output_name;
> static int output_fd;
> +static char *metrics;
>
> struct perf_stat {
> bool record;
> @@ -1147,14 +1148,21 @@ static int enable_metric_only(const struct option *opt __maybe_unused,
> return 0;
> }
>
> -static int parse_metric_groups(const struct option *opt,
> +static int append_metric_groups(const struct option *opt __maybe_unused,
> const char *str,
> int unset __maybe_unused)
> {
> - return metricgroup__parse_groups(opt, str,
> - stat_config.metric_no_group,
> - stat_config.metric_no_merge,
> - &stat_config.metric_events);
> + if (metrics) {
> + char *tmp;
> +
> + if (asprintf(&tmp, "%s,%s", metrics, str) < 0)
> + return -ENOMEM;
We check if we managed to allocate memory here, but not later at
strdup()?
> + free(metrics);
> + metrics = tmp;
> + } else {
> + metrics = strdup(str);
> + }
> + return 0;
> }
>
> static int parse_control_option(const struct option *opt,
> @@ -1298,7 +1306,7 @@ static struct option stat_options[] = {
> "measure SMI cost"),
> OPT_CALLBACK('M', "metrics", &evsel_list, "metric/metric group list",
> "monitor specified metrics or metric groups (separated by ,)",
> - parse_metric_groups),
> + append_metric_groups),
> OPT_BOOLEAN_FLAG(0, "all-kernel", &stat_config.all_kernel,
> "Configure all used events to run in kernel space.",
> PARSE_OPT_EXCLUSIVE),
> @@ -1791,11 +1799,9 @@ static int add_default_attributes(void)
> * on an architecture test for such a metric name.
> */
> if (metricgroup__has_metric("transaction")) {
> - struct option opt = { .value = &evsel_list };
> -
> - return metricgroup__parse_groups(&opt, "transaction",
> + return metricgroup__parse_groups(evsel_list, "transaction",
> stat_config.metric_no_group,
> - stat_config.metric_no_merge,
> + stat_config.metric_no_merge,
> &stat_config.metric_events);
> }
>
> @@ -2260,8 +2266,6 @@ int cmd_stat(int argc, const char **argv)
> argc = parse_options_subcommand(argc, argv, stat_options, stat_subcommands,
> (const char **) stat_usage,
> PARSE_OPT_STOP_AT_NON_OPTION);
> - perf_stat__collect_metric_expr(evsel_list);
> - perf_stat__init_shadow_stats();
>
> if (stat_config.csv_sep) {
> stat_config.csv_output = true;
> @@ -2428,6 +2432,23 @@ int cmd_stat(int argc, const char **argv)
> target.system_wide = true;
> }
>
> + if ((stat_config.aggr_mode == AGGR_THREAD) && (target.system_wide))
> + target.per_thread = true;
> +
> + /*
> + * Metric parsing needs to be delayed as metrics may optimize events
> + * knowing the target is system-wide.
> + */
> + if (metrics) {
> + metricgroup__parse_groups(evsel_list, metrics,
> + stat_config.metric_no_group,
> + stat_config.metric_no_merge,
> + &stat_config.metric_events);
> + zfree(&metrics);
> + }
> + perf_stat__collect_metric_expr(evsel_list);
> + perf_stat__init_shadow_stats();
> +
> if (add_default_attributes())
> goto out;
>
> @@ -2447,9 +2468,6 @@ int cmd_stat(int argc, const char **argv)
> }
> }
>
> - if ((stat_config.aggr_mode == AGGR_THREAD) && (target.system_wide))
> - target.per_thread = true;
> -
> if (evlist__fix_hybrid_cpus(evsel_list, target.cpu_list)) {
> pr_err("failed to use cpu list %s\n", target.cpu_list);
> goto out;
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index b144c3e35264..9151346a16ab 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -1646,13 +1646,12 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
> return ret;
> }
>
> -int metricgroup__parse_groups(const struct option *opt,
> +int metricgroup__parse_groups(struct evlist *perf_evlist,
> const char *str,
> bool metric_no_group,
> bool metric_no_merge,
> struct rblist *metric_events)
> {
> - struct evlist *perf_evlist = *(struct evlist **)opt->value;
> const struct pmu_events_table *table = pmu_events_table__find();
>
> if (!table)
> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
> index 016b3b1a289a..af9ceadaec0f 100644
> --- a/tools/perf/util/metricgroup.h
> +++ b/tools/perf/util/metricgroup.h
> @@ -64,7 +64,7 @@ struct metric_expr {
> struct metric_event *metricgroup__lookup(struct rblist *metric_events,
> struct evsel *evsel,
> bool create);
> -int metricgroup__parse_groups(const struct option *opt,
> +int metricgroup__parse_groups(struct evlist *perf_evlist,
> const char *str,
> bool metric_no_group,
> bool metric_no_merge,
> --
> 2.37.2.672.g94769d06f0-goog
--
- Arnaldo
On Wed, Aug 31, 2022 at 7:42 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Tue, Aug 30, 2022 at 09:48:44AM -0700, Ian Rogers escreveu:
> > Having metric parsing as part of argument processing causes issues as
> > flags like metric-no-group may be specified later. It also denies the
> > opportunity to optimize the events on SMT systems where fewer events
> > may be possible if we know the target is system-wide. Move metric
> > parsing to after command line option parsing. Because of how stat runs
> > this moves the parsing after record/report which fail to work with
> > metrics currently anyway.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/builtin-stat.c | 48 ++++++++++++++++++++++++-----------
> > tools/perf/util/metricgroup.c | 3 +--
> > tools/perf/util/metricgroup.h | 2 +-
> > 3 files changed, 35 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 7fb81a44672d..c813b1aa7d7c 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -191,6 +191,7 @@ static bool append_file;
> > static bool interval_count;
> > static const char *output_name;
> > static int output_fd;
> > +static char *metrics;
> >
> > struct perf_stat {
> > bool record;
> > @@ -1147,14 +1148,21 @@ static int enable_metric_only(const struct option *opt __maybe_unused,
> > return 0;
> > }
> >
> > -static int parse_metric_groups(const struct option *opt,
> > +static int append_metric_groups(const struct option *opt __maybe_unused,
> > const char *str,
> > int unset __maybe_unused)
> > {
> > - return metricgroup__parse_groups(opt, str,
> > - stat_config.metric_no_group,
> > - stat_config.metric_no_merge,
> > - &stat_config.metric_events);
> > + if (metrics) {
> > + char *tmp;
> > +
> > + if (asprintf(&tmp, "%s,%s", metrics, str) < 0)
> > + return -ENOMEM;
>
> We check if we managed to allocate memory here, but not later at
> strdup()?
Added to v2.
Thanks,
Ian
> > + free(metrics);
> > + metrics = tmp;
> > + } else {
> > + metrics = strdup(str);
> > + }
> > + return 0;
> > }
> >
> > static int parse_control_option(const struct option *opt,
> > @@ -1298,7 +1306,7 @@ static struct option stat_options[] = {
> > "measure SMI cost"),
> > OPT_CALLBACK('M', "metrics", &evsel_list, "metric/metric group list",
> > "monitor specified metrics or metric groups (separated by ,)",
> > - parse_metric_groups),
> > + append_metric_groups),
> > OPT_BOOLEAN_FLAG(0, "all-kernel", &stat_config.all_kernel,
> > "Configure all used events to run in kernel space.",
> > PARSE_OPT_EXCLUSIVE),
> > @@ -1791,11 +1799,9 @@ static int add_default_attributes(void)
> > * on an architecture test for such a metric name.
> > */
> > if (metricgroup__has_metric("transaction")) {
> > - struct option opt = { .value = &evsel_list };
> > -
> > - return metricgroup__parse_groups(&opt, "transaction",
> > + return metricgroup__parse_groups(evsel_list, "transaction",
> > stat_config.metric_no_group,
> > - stat_config.metric_no_merge,
> > + stat_config.metric_no_merge,
> > &stat_config.metric_events);
> > }
> >
> > @@ -2260,8 +2266,6 @@ int cmd_stat(int argc, const char **argv)
> > argc = parse_options_subcommand(argc, argv, stat_options, stat_subcommands,
> > (const char **) stat_usage,
> > PARSE_OPT_STOP_AT_NON_OPTION);
> > - perf_stat__collect_metric_expr(evsel_list);
> > - perf_stat__init_shadow_stats();
> >
> > if (stat_config.csv_sep) {
> > stat_config.csv_output = true;
> > @@ -2428,6 +2432,23 @@ int cmd_stat(int argc, const char **argv)
> > target.system_wide = true;
> > }
> >
> > + if ((stat_config.aggr_mode == AGGR_THREAD) && (target.system_wide))
> > + target.per_thread = true;
> > +
> > + /*
> > + * Metric parsing needs to be delayed as metrics may optimize events
> > + * knowing the target is system-wide.
> > + */
> > + if (metrics) {
> > + metricgroup__parse_groups(evsel_list, metrics,
> > + stat_config.metric_no_group,
> > + stat_config.metric_no_merge,
> > + &stat_config.metric_events);
> > + zfree(&metrics);
> > + }
> > + perf_stat__collect_metric_expr(evsel_list);
> > + perf_stat__init_shadow_stats();
> > +
> > if (add_default_attributes())
> > goto out;
> >
> > @@ -2447,9 +2468,6 @@ int cmd_stat(int argc, const char **argv)
> > }
> > }
> >
> > - if ((stat_config.aggr_mode == AGGR_THREAD) && (target.system_wide))
> > - target.per_thread = true;
> > -
> > if (evlist__fix_hybrid_cpus(evsel_list, target.cpu_list)) {
> > pr_err("failed to use cpu list %s\n", target.cpu_list);
> > goto out;
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index b144c3e35264..9151346a16ab 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -1646,13 +1646,12 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
> > return ret;
> > }
> >
> > -int metricgroup__parse_groups(const struct option *opt,
> > +int metricgroup__parse_groups(struct evlist *perf_evlist,
> > const char *str,
> > bool metric_no_group,
> > bool metric_no_merge,
> > struct rblist *metric_events)
> > {
> > - struct evlist *perf_evlist = *(struct evlist **)opt->value;
> > const struct pmu_events_table *table = pmu_events_table__find();
> >
> > if (!table)
> > diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
> > index 016b3b1a289a..af9ceadaec0f 100644
> > --- a/tools/perf/util/metricgroup.h
> > +++ b/tools/perf/util/metricgroup.h
> > @@ -64,7 +64,7 @@ struct metric_expr {
> > struct metric_event *metricgroup__lookup(struct rblist *metric_events,
> > struct evsel *evsel,
> > bool create);
> > -int metricgroup__parse_groups(const struct option *opt,
> > +int metricgroup__parse_groups(struct evlist *perf_evlist,
> > const char *str,
> > bool metric_no_group,
> > bool metric_no_merge,
> > --
> > 2.37.2.672.g94769d06f0-goog
>
> --
>
> - Arnaldo
© 2016 - 2026 Red Hat, Inc.