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

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

> > @@ -2132,13 +2131,17 @@ static void perf_sample__fprint_metric(struct perf_script *script,
> >  		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;
> > -	evsel_script(evsel)->val = val;
> > +	val = sample->period;
> > +	/*
> > +	 * Always use the first storage because the groups are contiguous
> 
> Without leader sampling we cannot guarantee groups events fire
> together, right?

Yes. I'm adding an explicit check for this.

> 
> 
> > +	 * and there's no need to handle multiple indexes for anything
> 
> Actually I think this is a behavior change that you changed the
> aggregation mode from NONE to GLOBAL.

Not with leader sampling because the group is contiguous and output
after its end, with all the previous values forgotten then.
The other cases don't really work anyways as multiple people pointed
out.

> > +	 */
> > +	evsel->stats->aggr[0].counts.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,
> > +						      evsel->stats->aggr[0].counts.val,
> > +						      0,
> 
> Like I said to Ian, we should pass a proper aggr_idx here not just 0 to
> support correct aggregation.  For now I think only possible choice is
> AGGR_NONE (for cpu-wide record) or AGGR_THREAD (for per-task record).
> Then it should be an index to cpu or thread map.

Given the above I think that is not needed.

Full aggregation for metrics over longer period would belong into perf report,
not here.  perf script is only to get non aggregated metrics.

-Andi