Re: [PATCH] perf record: add a shortcut for metrics
From: Arnaldo Carvalho de Melo
Date: Mon May 27 2024 - 13:11:30 EST
On Mon, May 27, 2024 at 02:02:33PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, May 27, 2024 at 12:15:19PM +0200, Artem Savkov wrote:
> > Add -M/--metrics option to perf-record providing a shortcut to record
> > metrics and metricgroups. This option mirrors the one in perf-stat.
> >
> > Suggested-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > Signed-off-by: Artem Savkov <asavkov@xxxxxxxxxx>
>
> Not building for me, I needed to add the rblist.h header and also I
> think we need to use metricgroup__rblist_init(&mevents), right?
Argh, that is a static function, it seems we trigger it by having
nr_entries = 0, so the following should do the trick:
struct rblist mevents = { .nr_entries = 0, }
So that we don't depend on the compiler zeroing that field, which for
local variables it should not.
- Arnaldo
> Testing it now.
>
> - Arnaldo
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 18da3ce380152ad1..5d67b0711c166fae 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -27,6 +27,7 @@
> #include "util/session.h"
> #include "util/tool.h"
> #include "util/symbol.h"
> +#include "util/rblist.h"
> #include "util/record.h"
> #include "util/cpumap.h"
> #include "util/thread_map.h"
> @@ -4017,6 +4018,7 @@ int cmd_record(int argc, const char **argv)
> set_nobuild('\0', "off-cpu", "no BUILD_BPF_SKEL=1", true);
> # undef set_nobuild
> #endif
> + metricgroup__rblist_init(&mevents);
>
> /* Disable eager loading of kernel symbols that adds overhead to perf record. */
> symbol_conf.lazy_load_kernel_maps = true;
>
> > ---
> > tools/perf/Documentation/perf-record.txt | 7 +++-
> > tools/perf/builtin-record.c | 43 ++++++++++++++++++++++++
> > 2 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> > index 6015fdd08fb63..ebb560d137e62 100644
> > --- a/tools/perf/Documentation/perf-record.txt
> > +++ b/tools/perf/Documentation/perf-record.txt
> > @@ -18,7 +18,6 @@ from it, into perf.data - without displaying anything.
> >
> > This file can then be inspected later on, using 'perf report'.
> >
> > -
> > OPTIONS
> > -------
> > <command>...::
> > @@ -216,6 +215,12 @@ OPTIONS
> > na, by_data, by_addr (for mem_blk)
> > hops0, hops1, hops2, hops3 (for mem_hops)
> >
> > +-M::
> > +--metrics::
> > +Record metrics or metricgroups specified in a comma separated list.
> > +For a group all metrics from the group are added.
> > +See perf list output for the possible metrics and metricgroups.
> > +
> > --exclude-perf::
> > Don't record events issued by perf itself. This option should follow
> > an event selector (-e) which selects tracepoint event(s). It adds a
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index 66a3de8ac6618..5828051ff2736 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -40,6 +40,7 @@
> > #include "util/trigger.h"
> > #include "util/perf-hooks.h"
> > #include "util/cpu-set-sched.h"
> > +#include "util/metricgroup.h"
> > #include "util/synthetic-events.h"
> > #include "util/time-utils.h"
> > #include "util/units.h"
> > @@ -188,6 +189,7 @@ static volatile int done;
> > static volatile int auxtrace_record__snapshot_started;
> > static DEFINE_TRIGGER(auxtrace_snapshot_trigger);
> > static DEFINE_TRIGGER(switch_output_trigger);
> > +static char *metrics;
> >
> > static const char *affinity_tags[PERF_AFFINITY_MAX] = {
> > "SYS", "NODE", "CPU"
> > @@ -200,6 +202,25 @@ static inline pid_t gettid(void)
> > }
> > #endif
> >
> > +static int append_metric_groups(const struct option *opt __maybe_unused,
> > + const char *str,
> > + int unset __maybe_unused)
> > +{
> > + if (metrics) {
> > + char *tmp;
> > +
> > + if (asprintf(&tmp, "%s,%s", metrics, str) < 0)
> > + return -ENOMEM;
> > + free(metrics);
> > + metrics = tmp;
> > + } else {
> > + metrics = strdup(str);
> > + if (!metrics)
> > + return -ENOMEM;
> > + }
> > + return 0;
> > +}
> > +
> > static int record__threads_enabled(struct record *rec)
> > {
> > return rec->opts.threads_spec;
> > @@ -3382,6 +3403,9 @@ static struct option __record_options[] = {
> > parse_events_option),
> > OPT_CALLBACK(0, "filter", &record.evlist, "filter",
> > "event filter", parse_filter),
> > + OPT_CALLBACK('M', "metrics", &record.evlist, "metric/metric group list",
> > + "monitor specified metrics or metric groups (separated by ,)",
> > + append_metric_groups),
> > OPT_CALLBACK_NOOPT(0, "exclude-perf", &record.evlist,
> > NULL, "don't record events from perf itself",
> > exclude_perf),
> > @@ -3984,6 +4008,7 @@ int cmd_record(int argc, const char **argv)
> > int err;
> > struct record *rec = &record;
> > char errbuf[BUFSIZ];
> > + struct rblist mevents;
> >
> > setlocale(LC_ALL, "");
> >
> > @@ -4153,6 +4178,23 @@ int cmd_record(int argc, const char **argv)
> > if (record.opts.overwrite)
> > record.opts.tail_synthesize = true;
> >
> > + if (metrics) {
> > + const char *pmu = parse_events_option_args.pmu_filter ?: "all";
> > + int ret = metricgroup__parse_groups(rec->evlist, pmu, metrics,
> > + false, /* metric_no_group */
> > + false, /* metric_no_merge */
> > + false, /* metric_no_threshold */
> > + rec->opts.target.cpu_list,
> > + rec->opts.target.system_wide,
> > + false, /* hardware_aware_grouping */
> > + &mevents);
> > + if (ret) {
> > + err = ret;
> > + goto out;
> > + }
> > + zfree(&metrics);
> > + }
> > +
> > if (rec->evlist->core.nr_entries == 0) {
> > bool can_profile_kernel = perf_event_paranoid_check(1);
> >
> > @@ -4264,6 +4306,7 @@ int cmd_record(int argc, const char **argv)
> > out_opts:
> > record__free_thread_masks(rec, rec->nr_threads);
> > rec->nr_threads = 0;
> > + metricgroup__rblist_exit(&mevents);
> > evlist__close_control(rec->opts.ctl_fd, rec->opts.ctl_fd_ack, &rec->opts.ctl_fd_close);
> > return err;
> > }
> > --
> > 2.45.1