Re: [PATCH v6 3/4] perf script: Fix perf script -F +metric

[Andi Kleen <ak@linux.intel.com>]

On Tue, Jul 23, 2024 at 02:32:33PM -0700, Ian Rogers wrote:
> On Tue, Jul 23, 2024 at 1:48 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.
> 
> 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.

> > @@ -2325,6 +2330,20 @@ static void process_event(struct perf_script *script,
> >                 fflush(fp);
> >  }
> >
> > +static void check_metric_conflict(void)
> > +{
> > +       int i;
> > +       /*
> > +        * 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.

> >
> > @@ -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)
> >
> >         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.


> 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