Re: [PATCH v5 1/2] perf script: Fix perf script -F +metric

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 13 Jul 2024, at 9:24 PM, Andi Kleen <ak@linux.intel.com> wrote:
> 
> This fixes a regression with perf script -F +metric originally caused by :
> 
> commit 37cc8ad77cf81f3ffd226856c367b0e15333a738
> Author: Ian Rogers <irogers@google.com>
> Date:   Sun Feb 19 01:28:46 2023 -0800
> 
>    perf metric: Directly use counts rather than saved_value
> 
> In the perf script environment the evsel wouldn't allocate an aggr
> values array, which led to a -1 reference because the metric
> evaluation would try to reference NULL - 1 (for aggr_idx)
> 
> Give the perf script evsels a single CPU aggr setup. That's
> enough because the groups are always contiguous, so no need
> to store more than one CPU's worth of values.
> 
> Before
> 
> % perf record -e '{cycles,instructions}:S' perf bench  mem memcpy
> % perf script -F +metric
> Segmentation fault (core dumped)
> 
> After:
> 
> % perf record -e '{cycles,instructions}:S' perf bench  mem memcpy
> ...
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.028 MB perf.data (90 samples) ]
> % perf script -F +metric
>       perf-exec 1847557 264658.180789:       3009       cycles:  ffffffff990a579a native_write_msr+0xa ([kernel.kallsyms])
>       perf-exec 1847557 264658.180789:        382 instructions:  ffffffff990a579a native_write_msr+0xa ([kernel.kallsyms])
>       perf-exec 1847557 264658.180789:         metric:    0.13  insn per cycle
> ...
> 
> Fixes: 37cc8ad77cf8 ("perf metric: Directly use counts rather ...")
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Hi Andi,

I tested this on powerpc.  It fails.
Version 4 had worked for me. But version 5 fails with segfault

# ./perf record -e '{cycles,instructions}:S' perf bench mem memcpy
# Running 'mem/memcpy' benchmark:
# function 'default' (Default memcpy() provided by glibc)
# Copying 1MB bytes ...

      25.699013 GB/sec
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.012 MB perf.data (24 samples) ]

# ./perf script -F +metric
Segmentation fault (core dumped)

Adding results from perf test also:

# ./perf test -v "perf script tests”

script metric test
[ perf record: Woken up 2 times to write data ]
[ perf record: Captured and wrote 0.379 MB /tmp/perf-test-script.6I9EykwokH/perf.data (8050 samples) ]
/linux/tools/perf/tests/shell/script.sh: line 93: 2280279 Segmentation fault      (core dumped) perf script -i "${perfdatafile}" -F +metric > $scriptoutput
--- Cleaning up ---
---- end(-1) ----
 93: perf script tests                                               : FAILED!

I am trying on top of tmp.perf-tools-next acme tree

# git log --oneline -n 2
7f1c0c721699 (HEAD -> try) Add a test case for perf script -F +metric
cadd820159b3 perf script: Fix perf script -F +metric

Thanks
Athira

> 

> ----
> 
> v2: Reformat code
> v3: Work around bogus warning
> v4: Set up aggr map only for metrics case to keep perf stat record
> working
> ---
> tools/perf/builtin-script.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index c16224b1fef3..33b5c7af5071 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -2127,23 +2127,29 @@ static void perf_sample__fprint_metric(struct perf_script *script,
> };
> struct evsel *ev2;
> u64 val;
> + struct cpu_aggr_map *map;
> 
> if (!evsel->stats)
> evlist__alloc_stats(&stat_config, script->session->evlist, /*alloc_raw=*/false);
> if (evsel_script(leader)->gnum++ == 0)
> perf_stat__reset_shadow_stats();
> val = sample->period * evsel->scale;
> + map = stat_config.aggr_map;
> + stat_config.aggr_map = &(struct cpu_aggr_map){ .nr = 1 };
> + /* Always use CPU 0 storage because the groups are contiguous. */
> + evsel->stats->aggr[0].counts.val = val;
> evsel_script(evsel)->val = val;
> if (evsel_script(leader)->gnum == leader->core.nr_members) {
> for_each_group_member (ev2, leader) {
> perf_stat__print_shadow_stats(&stat_config, ev2,
>      evsel_script(ev2)->val,
> -      sample->cpu,
> +      0,
>      &ctx,
>      NULL);
> }
> evsel_script(leader)->gnum = 0;
> }
> + stat_config.aggr_map = map;
> }
> 
> static bool show_event(struct perf_sample *sample,
> -- 
> 2.45.2
> 
>