Re: [PATCH 09/10] perf tools: Compute other metrics

[Ian Rogers]

On Sun, Jun 28, 2020 at 3:00 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> On Fri, Jun 26, 2020 at 02:24:38PM -0700, Ian Rogers wrote:
>
> SNIP
>
> > > +
> > > +                                       if (expr__get_id(ctx, lookup, &data) || !data) {
> > >                                                 pr_debug("%s not found\n", $1);
> > >                                                 free($1);
> > >                                                 YYABORT;
> > >                                         }
> > > +
> > > +                                       pr_debug2("lookup: is_other %d, counted %d: %s\n",
> > > +                                                 data->is_other, data->other.counted, lookup);
> > > +
> > > +                                       if (data->is_other && !data->other.counted) {
> > > +                                               data->other.counted = true;
> > > +                                               if (expr__parse(&data->val, ctx, data->other.metric_expr, 1)) {
> >
> > Ah, so this handles the problem the referenced metric isn't calculated
> > and calculates it - with the sharing of events this doesn't impose
> > extra pmu cost. Do we need to worry about detecting recursion? For
> > example:
> >
> > "MetricName": "Foo",
> > "MetricExpr": "1/metric:Foo",
>
> right, we should add some recursion check,
> I'll lcheck on it
>
> >
> > It seems unfortunate to have the MetricExpr calculated twice, but it
>
> hum, not sure what you mean by twice.. we do that just once for
> each involved metric and store the value.. the metric is also
> processed before for 'other' metrics

So I'm thinking out loud. Here is an example from Skylake:

{
 "BriefDescription": "All L2 hit counts",
 "MetricExpr": "L2_RQSTS.DEMAND_DATA_RD_HIT + L2_RQSTS.PF_HIT +
L2_RQSTS.RFO_HIT",
 "MetricName": "DCache_L2_All_Hits",
}
{
 "BriefDescription": "All L2 miss counts",
 "MetricExpr": "MAX(L2_RQSTS.ALL_DEMAND_DATA_RD -
L2_RQSTS.DEMAND_DATA_RD_HIT, 0) + L2_RQSTS.PF_MISS +
L2_RQSTS.RFO_MISS",
 "MetricName": "DCache_L2_All_Miss",
}
{
 "BriefDescription": "All L2 counts",
 "MetricExpr": "metric:DCache_L2_All_Hits + metric:DCache_L2_All_Miss",
 "MetricName": "DCache_L2_All",
}
{
 "BriefDescription": "DCache L2 hit rate",
 "MetricExpr": "d_ratio(metric:DCache_L2_All_Hits, metric:DCache_L2_All)",
 "MetricName": "DCache_L2_Hits",
 "MetricGroup": "DCache_L2",
 "ScaleUnit": "100%",
},
{
 "BriefDescription": "DCache L2 miss rate",
 "MetricExpr": "d_ratio(metric:DCache_L2_All_Miss, metric:DCache_L2_All)",
 "MetricName": "DCache_L2_Misses",
 "MetricGroup": "DCache_L2",
 "ScaleUnit": "100%",
},

Firstly, it should be clear that having this change makes the json far
more readable! The current approach is to copy and paste resulting in
100s of characters wide expressions. This is a great improvement!

With these metrics the hope would be that 'perf stat -M DCache_L2 ...'
is going to report just DCache_L2_Hits and DCache_L2_Misses. To
compute these two metrics, as an example, DCache_L2_All_Hits is needed
three times. My comment was meant to mean that it seems a little
unfortunate to keep repeatedly evaluating the expression rather than
to compute it once and reuse the result.

Thanks,
Ian

> jirka
>
> > is understandable. Is it also a property that referenced/other metrics
> > won't be reported individually? Perhaps these are sub-metrics?
>
> >
> > Thanks,
> > Ian
> >
> > > +                                                       pr_debug("%s failed to count\n", $1);
> > > +                                                       free($1);
> > > +                                                       YYABORT;
> > > +                                               }
> > > +                                       }
> > > +
> > >                                         $$ = data->val;
> > >                                         free($1);
> > >                                 }
> > > --
> > > 2.25.4
> > >
> >
>