Re: [PATCH] perf record: add a shortcut for metrics

[Artem Savkov <asavkov@redhat.com>]

On Mon, May 27, 2024 at 02:28:29PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, May 27, 2024 at 02:04:54PM -0300, Arnaldo Carvalho de Melo wrote:
> > 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@kernel.org>
> > > > Signed-off-by: Artem Savkov <asavkov@redhat.com>
>
> > > 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.
>
> How did you test this?
>
> I'm trying:
>
> perf list metric
>
> pick a metric then:
>
> perf record -M tma_core_bound
>
> And it gets in a long loop doing perf_event_open() calls...

[snip]

> (gdb) bt
> #0  0x00007ffff6f21804 in close () from /lib64/libc.so.6
> #1  0x000000000061fbd2 in perf_evsel__close_fd_cpu (evsel=0xdab470, cpu_map_idx=6) at evsel.c:188
> #2  0x000000000061fc22 in perf_evsel__close_fd (evsel=0xdab470) at evsel.c:197
> #3  0x000000000061fc9b in perf_evsel__close (evsel=0xdab470) at evsel.c:211
> #4  0x00000000004e0b5f in evlist.reset_weak_group ()
> #5  0x0000000000423bb9 in __cmd_record.constprop.0 ()
> #6  0x00000000004276c5 in cmd_record ()
> #7  0x00000000004c4579 in run_builtin ()
> #8  0x00000000004c4889 in handle_internal_command ()
> #9  0x0000000000410e57 in main ()
> (gdb) c
> Continuing.
> ^C
> Program received signal SIGINT, Interrupt.
> 0x00007ffff6f21804 in close () from /lib64/libc.so.6
> (gdb)
>
> So you should investigate this further.

I tried a bunch of random metrics from perf list but didn't hit this.

It spins forever in evlist__for_each_entry() loop in record__open() with
the same error:

        Weak group for TOPDOWN.SLOTS/5 failed

Looks like the culprit is one of those unsupported metrics, will
investigate.

> The idea, from my notes, was to be able to have extra columns in 'perf
> report' with things like IPC and other metrics, probably not all metrics
> will apply. We need to find a way to find out which ones are OK for that
> purpose, for instance:
>
> Opening: cpu_core/topdown-bad-spec/
> ------------------------------------------------------------
> perf_event_attr:
>   type                             4 (cpu_core)
>   size                             136
>   config                           0x8100 (topdown-bad-spec)
>   { sample_period, sample_freq }   4000
>   sample_type                      IP|TID|TIME|CPU|PERIOD|IDENTIFIER
>   read_format                      ID|LOST
>   disabled                         1
>   inherit                          1
>   freq                             1
>   sample_id_all                    1
>   exclude_guest                    1
> ------------------------------------------------------------
> sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8
> sys_perf_event_open failed, error -22
> switching off PERF_FORMAT_LOST support
> Opening: cpu_core/topdown-bad-spec/

Is it just metrics containing unsupported events that need to be skipped
or there are other cases that wouldn't make much sense? If the latter
maybe it will be easier to just tag the ones that are supported (or not) in
pmu-events?

--
 Artem