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

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

> 
> Fwiw, not acked-by me. Repeating my comments below:

Let me just point out that you broke it in the first place.

A little more enthusiasm in handling regressions would be appreciated.

> > > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > > index c16224b1fef3..cb63507af839 100644
> > > --- a/tools/perf/builtin-script.c
> > > +++ b/tools/perf/builtin-script.c
> > > @@ -335,7 +335,6 @@ struct evsel_script {
> > >         FILE *fp;
> > >         u64  samples;
> > >         /* For metric output */
> > > -       u64  val;
> > >         int  gnum;
> 
> We're globally assuming properties of events assuming leader sampling.
> gnum shouldn't exist. The assumptions will be invalid when leader
> sampling isn't enabled.

This is checked for now, see the earlier discussion.

> > > +       if (!(leader->core.attr.read_format & PERF_FORMAT_GROUP) ||
> > > +           !(leader->core.attr.sample_type & PERF_SAMPLE_READ)) {
> > > +               if (!printed)
> > > +                       fprintf(stderr, "perf script: -F metric requires {}:S for groups leader sampling\n");
> > > +               printed = 1;
> > > +               return;
> > > +       }
> > > +       /*
> > > +        * Always use the first entry as storage because the leader sampling
> > > +        * groups are contiguous and there's no need to handle multiple indexes
> > > +        * for anything.
> > > +        */
> > > +       evsel->stats->aggr[0].counts.val = val;
> 
> Updating counts needs perf_stat_process_counter to update the
> aggregation. How does sample->cpu given below relate to aggr[0] here?
> This and the comment above are a hack liable to break in anything but
> straightforward circumstances. See:
> https://lore.kernel.org/lkml/20240720074552.1915993-2-irogers@google.com/
> on how to handle this.

Your patch is based on a fundamental misunderstanding on the perf script
metrics use model, see below.

> 
> > >         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,
> > > +                                                     evsel->stats->aggr[0].counts.val,
> > > +                                                     0,
> 
> Similarly this hard coded and unexplained constant.

There's a comment just on top of it explaining it:

        /*
         * Always use the first entry as storage because the leader sampling
         * groups are contiguous and there's no need to handle multiple indexes
         * for anything.
         */


> 
> > >                                                       &ctx,
> > >                                                       NULL);
> > >                 }
> > > @@ -2325,6 +2337,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.
> > > +        */
> 
> Why is it a conflict, why not support aggregation modes?

I thought I had explained that before.

perf script has two touch point with perf stat output code: one is this one
(for metrics on sampling group) and the other is for perf stat record / script

None of them actually support aggregation because perf script as a
general rule doesn't aggregate, it only handles single events.

But the later one forces AGGR_NONE which doesn't work with the mode the group
output uses.

This feature is only to report the metric state for a single sampling period
and group as explained in the original commit [1]

If you want to aggregate multiple sample periods you need a lot the
machinery from perf report, which doesn't make sense to duplicate
in perf script.

[1] https://www.uwsg.indiana.edu/hypermail/linux/kernel/1712.0/04705.html

-Andi