Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200626194720.2915044-1-jolsa@kernel.org> <20200626194720.2915044-10-jolsa@kernel.org>
 <CAP-5=fUODuF+LY6cbU4BPEcxu_YXDYg42pgRYU=yY5e47z05=g@mail.gmail.com> <20200628215957.GO2988321@krava>
In-Reply-To: <20200628215957.GO2988321@krava>
From:   Ian Rogers <irogers@google.com>
Date:   Mon, 29 Jun 2020 09:35:12 -0700
Message-ID: <CAP-5=fWS7dsqrjM2gP6fpsjbjW5CoFHwGLYf-WxzJ9JdhD-LzQ@mail.gmail.com>
Subject: Re: [PATCH 09/10] perf tools: Compute other metrics
To:     Jiri Olsa <jolsa@redhat.com>
Cc:     Jiri Olsa <jolsa@kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Michael Petlan <mpetlan@redhat.com>,
        Andi Kleen <ak@linux.intel.com>,
        Kajol Jain <kjain@linux.ibm.com>,
        John Garry <john.garry@huawei.com>,
        "Paul A. Clarke" <pc@us.ibm.com>,
        Stephane Eranian <eranian@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
> > >
> >
>