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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fWS7dsqrjM2gP6fpsjbjW5CoFHwGLYf-WxzJ9JdhD-LzQ@mail.gmail.com>
Date:   Mon, 29 Jun 2020 09:35:12 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Jiri Olsa <jolsa@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Michael Petlan <mpetlan@...hat.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        John Garry <john.garry@...wei.com>,
        "Paul A. Clarke" <pc@...ibm.com>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 09/10] perf tools: Compute other metrics

On Sun, Jun 28, 2020 at 3:00 PM Jiri Olsa <jolsa@...hat.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
> > >
> >
>