Create source symlink in perf object dir

This fixes a regression with perf script -F +metric originally caused by :

commit 37cc8ad77cf81f3ffd226856c367b0e15333a738

Author: Ian Rogers <[email protected]>
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 <[email protected]>

----

v2: Reformat code
v3: Work around bogus warning
v4: Set up aggr map only for metrics case to keep perf stat record working
v5: Broken version
v6: Only set up limited aggregation mode with -F +metric. Add conflict checks with perf stat record files.

---
 tools/perf/builtin-script.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

tools/perf/builtin-script.c +0 -0

index c16224b1fef3..1a4b9b3d240d 100644

tools/perf/builtin-script.c +5 -1

        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);
                }

@@ -2325,6 +2330,20 @@ static void process_event(struct perf_script *script,
                fflush(fp);
 }
 
+static void check_metric_conflict(void)
+{
+       int i;
+       /*

tools/perf/builtin-script.c +8 -0

 static struct scripting_ops    *scripting_ops;
 
 static void __process_stat(struct evsel *counter, u64 tstamp)

@@ -2334,6 +2353,8 @@ static void __process_stat(struct evsel *counter, u64 tstamp)
        struct perf_cpu cpu;
        static int header_printed;
 
+       check_metric_conflict();
+
        if (!header_printed) {
                printf("%3s %8s %15s %15s %15s %15s %s\n",
                       "CPU", "THREAD", "VAL", "ENA", "RUN", "TIME", "EVENT");

@@ -3725,6 +3746,8 @@ static int process_stat_config_event(struct perf_session *session __maybe_unused
 {
        perf_event__read_stat_config(&stat_config, &event->stat_config);
 
+       check_metric_conflict();
+
        /*
         * Aggregation modes are not used since post-processing scripts are
         * supposed to take care of such requirements

@@ -3760,6 +3783,8 @@ int process_thread_map_event(struct perf_session *session,
        struct perf_tool *tool = session->tool;
        struct perf_script *script = container_of(tool, struct perf_script, tool);
 
+       check_metric_conflict();
+
        if (dump_trace)
                perf_event__fprintf_thread_map(event, stdout);
 

@@ -3785,6 +3810,8 @@ int process_cpu_map_event(struct perf_session *session,
        if (dump_trace)
                perf_event__fprintf_cpu_map(event, stdout);
 
+       check_metric_conflict();
+
        if (script->cpus) {
                pr_warning("Extra cpu map event, ignoring.\n");
                return 0;

tools/perf/builtin-script.c +1 -1

+       }
 
        if (symbol_conf.guestmount ||
            symbol_conf.default_guest_vmlinux_name ||
--

2.45.2

Quoted 13 lines · Andi Kleen

This fixes a regression with perf script -F +metric originally caused by :

commit 37cc8ad77cf81f3ffd226856c367b0e15333a738

Author: Ian Rogers <[email protected]>
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)

Quoted 3 lines

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.

I don't follow this. Samples have CPUs but you're associating all values with CPU0. Why not just use counts and aggregation properly?

Quoted 4 lines

tools/perf/builtin-script.c +0 -0

@@ -2325,6 +2330,20 @@ static void process_event(struct perf_script *script,
                fflush(fp);
 }
 

Quoted 4 lines

tools/perf/builtin-script.c +4 -0

+static void check_metric_conflict(void)
+{
+       int i;
+       /*

tools/perf/builtin-script.c static void process_event(struct perf_script *script, · line 2346

Quoted 10 lines

tools/perf/builtin-script.c +10 -0

+        * Avoid conflict with the aggregation mode used for the metric printing.
+        */
+       for (i = 0; i < OUTPUT_TYPE_MAX; i++) {
+               if (output[i].fields & PERF_OUTPUT_METRIC) {
+                       fprintf(stderr, "perf stat record files are not supported with -F metric\n");
+                       exit(1);
+               }
+       }
+}
+

No idea what this is doing. What's conflicting with what?

Quoted 12 lines

tools/perf/builtin-script.c +2 -0

@@ -3785,6 +3810,8 @@ int process_cpu_map_event(struct perf_session *session,
        if (dump_trace)
                perf_event__fprintf_cpu_map(event, stdout);
 
+       check_metric_conflict();
+
        if (script->cpus) {
                pr_warning("Extra cpu map event, ignoring.\n");
                return 0;

@@ -4088,6 +4115,10 @@ int cmd_script(int argc, const char **argv)
 

tools/perf/builtin-script.c int cmd_script(int argc, const char **argv) · line 4120

Quoted 5 lines

tools/perf/builtin-script.c +3 -0

        argc = parse_options_subcommand(argc, argv, options, script_subcommands, script_usage,
                             PARSE_OPT_STOP_AT_NON_OPTION);
+       for (i = 0; i < OUTPUT_TYPE_MAX; i++) {
+               if (output[i].fields & PERF_OUTPUT_METRIC)
+                       stat_config.aggr_map = &(struct cpu_aggr_map){ .nr = 1 };

Assigning the address a temporary rval to a global variable seems wrong to the point I'm surprised it compiles. Accessing

stat_config.aggr_map->map[0] will lead to reading beyond the end of the value and presumably read uninitialized memory. Compiling with EXTRA_CFLAGS="-fsanitize=address" should complain about all of this.

Quoted 14 lines · Ian Rogers

Quoted 13 lines · Andi Kleen

This fixes a regression with perf script -F +metric originally caused by :

commit 37cc8ad77cf81f3ffd226856c367b0e15333a738

Author: Ian Rogers <[email protected]>
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)

Quoted 6 lines

Quoted 3 lines

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.

I don't follow this. Samples have CPUs but you're associating all values with CPU0. Why not just use counts and aggregation properly?

Why use something that is not needed?

It's not needed because the CPUs are not interleaved because the code is just processing a single group which only has counts from a single CPU. And there is no need to output an extra index for the count because the sample already has all the context.

If it was extended to multiple groups it would be needed, but it's not clear how useful it is. The benefit of the feature is that you can get the metric at a very fine grained level -- only for the time interval since the last sample.

Doing it for multiple groups means you would do some level of aggregation over longer time periods.. You could handle more complex metrics, but would lose this very fine grain benefit. If you want that it's probably better use perf report's time slicing feature instead of perf script. That one currently doesn't support metrics though, but probably it should.

Quoted 4 lines

Quoted 4 lines

tools/perf/builtin-script.c +0 -0

@@ -2325,6 +2330,20 @@ static void process_event(struct perf_script *script,
                fflush(fp);
 }
 

Quoted 16 lines

Quoted 4 lines

tools/perf/builtin-script.c +4 -0

+static void check_metric_conflict(void)
+{
+       int i;
+       /*

tools/perf/builtin-script.c static void process_event(struct perf_script *script, · line 2346

Quoted 10 lines

tools/perf/builtin-script.c +10 -0

+        * Avoid conflict with the aggregation mode used for the metric printing.
+        */
+       for (i = 0; i < OUTPUT_TYPE_MAX; i++) {
+               if (output[i].fields & PERF_OUTPUT_METRIC) {
+                       fprintf(stderr, "perf stat record files are not supported with -F metric\n");
+                       exit(1);
+               }
+       }
+}
+

No idea what this is doing. What's conflicting with what?

The conflict is between the -F +metric setup and processing STAT* records in the perf.data (as generated by perf stat record) The later uses AGGR_NONE which is a conflict.

Quoted 12 lines

Quoted 12 lines

tools/perf/builtin-script.c +2 -0

@@ -3785,6 +3810,8 @@ int process_cpu_map_event(struct perf_session *session,
        if (dump_trace)
                perf_event__fprintf_cpu_map(event, stdout);
 
+       check_metric_conflict();
+
        if (script->cpus) {
                pr_warning("Extra cpu map event, ignoring.\n");
                return 0;

@@ -4088,6 +4115,10 @@ int cmd_script(int argc, const char **argv)
 

Quoted 8 lines

tools/perf/builtin-script.c int cmd_script(int argc, const char **argv) · line 4120

Quoted 5 lines

tools/perf/builtin-script.c +3 -0

        argc = parse_options_subcommand(argc, argv, options, script_subcommands, script_usage,
                             PARSE_OPT_STOP_AT_NON_OPTION);
+       for (i = 0; i < OUTPUT_TYPE_MAX; i++) {
+               if (output[i].fields & PERF_OUTPUT_METRIC)
+                       stat_config.aggr_map = &(struct cpu_aggr_map){ .nr = 1 };

Assigning the address a temporary rval to a global variable seems wrong to the point I'm surprised it compiles. Accessing

AFAIK gcc keeps the local around for the function, but you're right it's not good code, especially with the buffer overrun.

Quoted 3 lines

stat_config.aggr_map->map[0] will lead to reading beyond the end of the value and presumably read uninitialized memory. Compiling with EXTRA_CFLAGS="-fsanitize=address" should complain about all of this.

Good point, but I doubt the address sanitizer would have caught it because it doesn't really track the stack.

-Andi

The main reason would be so that perf_stat_process_counter could work and update the aggregation accordingly. Then you can dump out the aggregation, be it per CPU, per core, per socket, per cache-level, etc. as appropriate.

If you follow the perf stat convention you are
also much less likely to be broken in the future, as perf stat would also get broken. It is complicated spaghetti to work out how this stuff works, but that's why I fixed in the patch I sent out.

Or let it set the aggregation mode and just let the aggregation code handle it when computing metrics? I think getting the STAT events isn't typical, so this is an academic argument.