Messages in this thread | | | From | Ian Rogers <> | Date | Mon, 29 Jun 2020 09:35:12 -0700 | Subject | Re: [PATCH 09/10] perf tools: Compute other metrics |
| |
On Sun, Jun 28, 2020 at 3:00 PM Jiri Olsa <jolsa@redhat.com> 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 > > > > > >
| |