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

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

[Andi Kleen]

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>

----

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

[PATCH v5 2/2] Add a test case for perf script -F +metric

[Andi Kleen]

Just a simple test

Signed-off-by: Andi Kleen <ak@linux.intel.com>

----

v2: Avoid bashisms. Use noploop
v3: Avoid false positive in shellcheck
---
 tools/perf/tests/shell/script.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/perf/tests/shell/script.sh b/tools/perf/tests/shell/script.sh
index c1a603653662..5e080e40b390 100755
--- a/tools/perf/tests/shell/script.sh
+++ b/tools/perf/tests/shell/script.sh
@@ -7,6 +7,7 @@ set -e
 temp_dir=$(mktemp -d /tmp/perf-test-script.XXXXXXXXXX)
 
 perfdatafile="${temp_dir}/perf.data"
+scriptoutput="${temp_dir}/script"
 db_test="${temp_dir}/db_test.py"
 
 err=0
@@ -88,8 +89,21 @@ test_parallel_perf()
 	echo "parallel-perf test [Success]"
 }
 
+test_metric()
+{
+	echo "script metric test"
+	if ! perf list | grep -q cycles ; then return ; fi
+	if ! perf list | grep -q instructions ; then return ; fi
+	perf record -e '{cycles,instructions}' -o "${perfdatafile}" perf test -w noploop
+	perf script -i "${perfdatafile}" -F +metric  > $scriptoutput
+	test "`grep -c metric $scriptoutput`" -gt 5
+	grep metric $scriptoutput | head
+	echo "script metric test [Success]"
+}
+
 test_db
 test_parallel_perf
+test_metric
 
 cleanup
 
-- 
2.45.2

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

[Athira Rajeev]

> 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
> 
> 

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

[Andi Kleen]

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

Thanks for testing. Looks like this fix is cursed. And clearly my perf
test runs are still not working. I usually try to avoid installing perf,
but you have set some symlinks manually and use a special directory
for the shell test to work and that is not reliable here.

I guess we can exclude the two cases for now (perf stat record vs -F +metric), 
although it would be better to make metrics work in both cases.
But changing the stat code to support multiple aggregations at the same
time is too much. Maybe if the perf metric code could also use
AGGR_NONE, but I don't know how to make that work.

-Andi

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

[Ian Rogers]

On Mon, Jul 15, 2024 at 8:33 PM Andi Kleen <ak@linux.intel.com> wrote:
>
> > I tested this on powerpc.  It fails.
> > Version 4 had worked for me. But version 5 fails with segfault
>
> Thanks for testing. Looks like this fix is cursed. And clearly my perf
> test runs are still not working. I usually try to avoid installing perf,
> but you have set some symlinks manually and use a special directory
> for the shell test to work and that is not reliable here.
>
> I guess we can exclude the two cases for now (perf stat record vs -F +metric),
> although it would be better to make metrics work in both cases.
> But changing the stat code to support multiple aggregations at the same
> time is too much. Maybe if the perf metric code could also use
> AGGR_NONE, but I don't know how to make that work.

Sorry for the breakage, I'm taking a look. I think we need to set the
counts on the evsel and use perf_stat_process_counter to update the
aggregated amounts. The stat shadow printing code should work then. I
think passing aggr_idx as 0 isn't going to work.

Thanks,
Ian