Message-ID: <20200716012504.GA27583@oc3272150783.ibm.com>
Date:   Wed, 15 Jul 2020 20:25:04 -0500
From:   "Paul A. Clarke" <pc@...ibm.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>,
        Stephane Eranian <eranian@...gle.com>,
        Ian Rogers <irogers@...gle.com>
Subject: RE: [PATCH 00/18] perf metric: Add support to reuse metric

On Wed, Jul 15, 2020 at 11:41:34PM +0200, Jiri Olsa wrote:
> On Wed, Jul 15, 2020 at 01:33:27PM -0500, Paul A. Clarke wrote:
> > On Sun, Jul 12, 2020 at 03:26:16PM +0200, Jiri Olsa wrote:
> > > hi,
> > > this patchset is adding the support to reused metric in another 
> > > metric. The metric needs to be referenced by 'metric:' prefix.
> > > 
> > > For example, to define IPC by using CPI with change like:
> > > 
> > >          "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
> > >  -       "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> > >  +       "MetricExpr": "1/metric:CPI",
> > >          "MetricGroup": "TopDownL1",
> > >          "MetricName": "IPC"
> > > 
> > > I won't be able to find all the possible places we could
> > > use this at, so I wonder you guys (who was asking for this)
> > > would try it and come up with comments if there's something
> > > missing or we could already use it at some places.
> > > 
> > > It's based on Arnaldo's tmp.perf/core.
> > > 
> > > v2 changes:
> > >   - collected Ian's acks for few patches [Ian]
> > >   - renamed expr__add_id to expr__add_id_val [Ian]
> > >   - renamed expr_parse_data to expr_id_data [Ian]
> > >   - added recursion check [Ian]
> > >   - added metric test for DCache_L2 metric [Ian]
> > >   - added some renames as discussed in review [Ian]
> > > 
> > > Also available in here:
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > >   perf/metric
> > 
> > I'm having trouble testing this.
> > 
> > I checked out this tree, and am able to build with a JSON metrics definition
> > file which uses other metrics.  I put this aside, though, because of the
> > following issue.
> > 
> > I built the kernel from this same tree and booted it successfully.
> > However, the metrics are not working correctly.  (I may very well be
> > doing something wrong.)
> 
> if you'll share the metric change I can help debugging that

It fails as described below without any changes from me.

> > The base system is RHEL8, but it's now booted with the new kernel.
> > ```
> > # uname -a
> > Linux system 5.8.0-rc4-g7dd02cf0b #1 SMP Wed Jul 15 12:31:45 EDT 2020 ppc64le ppc64le ppc64le GNU/Linux
> > # perf stat --metrics cpi_breakdown ./load
> > failed: way too many variables
> 
> hm, this ^^^ error string was removed in:
>   43fe337c86a9 perf expr: Migrate expr ids table to a hashmap
> 
> looks like you're not running the correct perf binary

I should've explained this better in my post, but there are two runs of
perf there.  The first is above, and is the perf that comes with the
distribution.  I'll repeat that here:
```
# perf --version
perf version 4.18.0-214.el8.ppc64le
# perf stat --metrics cpi_breakdown ./load >/dev/null             
failed: way too many variables                                                               
 Performance counter stats for './load':                                                     
                                                                                             
           818,130      pm_cmplu_stall_bru        #      0.0 bru_stall_cpi            (0.45%)
     5,013,082,026      pm_run_inst_cmpl                                              (0.45%)
                 0      pm_cmplu_stall_crypto     #      0.0 crypto_stall_cpi         (0.89%)
     6,580,655,094      pm_run_inst_cmpl                                              (0.89%)
        25,729,751      pm_cmplu_stall_dcache_miss #      0.0 dcache_miss_stall_cpi    (1.77%)
     6,690,035,175      pm_run_inst_cmpl                                              (1.77%)
                 0      pm_cmplu_stall_dflong     #      0.0 dflong_stall_cpi         (1.77%)
     6,769,854,632      pm_run_inst_cmpl                                              (1.77%)
                 0      pm_cmplu_stall_dfu        #      0.0 dfu_other_stall_cpi      (0.89%)
[...and LOTS more...]
```

The second example in my previous post was from
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
branch perf/metric
```
commit 7dd02cf0b9f04ca5339fa97f9a2280ebdd60b1db (grafted, HEAD -> perf/metric, origin/perf/metric)
Author: Jiri Olsa <jolsa@...nel.org>
Date:   Thu Jul 9 13:45:30 2020 +0200

    perf metric: Rename group_list to list
```

This build of `perf` fails:
```
# ~/install/bin/perf --version
perf version 5.8.rc4.g7dd02cf0b9f0
# ~/install/bin/perf stat --metrics cpi_breakdown ./load >/dev/null

 Performance counter stats for './load':

     6,729,400,541      pm_run_inst_cmpl          #     0.00 bru_stall_cpi          
            57,953      pm_cmplu_stall_bru                                          

       1.127319209 seconds time elapsed

       1.124906000 seconds user
       0.000890000 seconds sys
```

PC

Powered by blists - more mailing lists