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>
In-Reply-To: <20200626194720.2915044-10-jolsa@kernel.org>
From:   Ian Rogers <irogers@google.com>
Date:   Fri, 26 Jun 2020 14:24:38 -0700
Message-ID: <CAP-5=fUODuF+LY6cbU4BPEcxu_YXDYg42pgRYU=yY5e47z05=g@mail.gmail.com>
Subject: Re: [PATCH 09/10] perf tools: Compute other metrics
To:     Jiri Olsa <jolsa@kernel.org>
Cc:     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 Fri, Jun 26, 2020 at 12:48 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding computation (expr__parse call) of 'other' metric at
> the point when it needs to be resolved during the 'master'
> metric computation.
>
> Once the inner metric is computed, the result is stored and
> used if there's another usage of that metric.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/expr.c |  3 +++
>  tools/perf/util/expr.h |  1 +
>  tools/perf/util/expr.y | 20 +++++++++++++++++++-
>  3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 32f7acac7c19..1b6d550cec5f 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -91,6 +91,7 @@ int expr__add_other(struct expr_parse_ctx *ctx, struct metric_other *other)
>
>         data_ptr->other.metric_name = other->metric_name;
>         data_ptr->other.metric_expr = other->metric_expr;
> +       data_ptr->other.counted = false;
>         data_ptr->is_other = true;
>
>         ret = hashmap__set(&ctx->ids, name, data_ptr,
> @@ -150,6 +151,8 @@ __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
>         void *scanner;
>         int ret;
>
> +       pr_debug2("parsing metric: %s\n", expr);
> +
>         ret = expr_lex_init_extra(&scanner_ctx, &scanner);
>         if (ret)
>                 return ret;
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index ed60f9227b43..f85f3941eda5 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -25,6 +25,7 @@ struct expr_parse_data {
>                 struct {
>                         const char *metric_name;
>                         const char *metric_expr;
> +                       bool counted;
>                 } other;
>         };
>  };
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index 6252d9f6cfc8..cca423331f65 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -89,12 +89,30 @@ if_expr:
>  expr:    NUMBER
>         | ID                    {
>                                         struct expr_parse_data *data;
> +                                       char *lookup = $1;
> +                                       const char *name;
>
> -                                       if (expr__get_id(ctx, $1, &data) || !data) {
> +                                       if (expr__is_metric($1, &name))
> +                                               lookup = name;
> +
> +                                       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",

It seems unfortunate to have the MetricExpr calculated twice, but it
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
>