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;
Date:   Wed, 13 May 2020 13:33:45 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     Ian Rogers <irogers@google.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>,
        Joe Mario <jmario@redhat.com>, Andi Kleen <ak@linux.intel.com>,
        Kajol Jain <kjain@linux.ibm.com>,
        John Garry <john.garry@huawei.com>
Subject: Re: [PATCH 3/4] perf stat: Add --metrics-file option
Message-ID: <20200513113345.GI3158213@krava>
References: <20200511205307.3107775-1-jolsa@kernel.org>
 <20200511205307.3107775-4-jolsa@kernel.org>
 <CAP-5=fU5bbWWyWXDMoMvsMY4BbUsRUqgrOtGUrdiYjSsJZ+t-A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAP-5=fU5bbWWyWXDMoMvsMY4BbUsRUqgrOtGUrdiYjSsJZ+t-A@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, May 13, 2020 at 12:04:55AM -0700, Ian Rogers wrote:

SNIP

> > +METRICS FILE
> > +------------
> > +The file with metrics has following syntax:
> > +
> > +  NAME = EXPRESSION ;
> > +  NAME = EXPRESSION ;
> > +  ...
> > +
> > +where NAME is unique identifier of the metric, which is later used in
> > +perf stat as -M option argument (see below).
> > +
> > +The EXPRESSION is the metric's formula with following grammar:
> > +
> > +  EXPR: EVENT
> > +  EXPR: EXPR if EXPR else EXPR
> 
> Not introduced by this patch, but this patch is exposing it as an API.

yea, I was thinking about this and I think we will put a disclaimer in
here that this is not an API and the interface can change.. it's really
mostly intended to help out with running a custom metric which is not
compiled in ... I don't want to be commited to support old API

> This notion of if-else is really weird. For one thing there are no
> comparison operators. The unit test doesn't really help:
>         ret |= test(&ctx, "1+1 if 3*4 else 0", 2);
> What kind of comparison is "3*4"? If 0.0 causes the else clause then will -0.0?
> A typical expression I see written in C is to give a ratio such:
>   value = denom == 0 ? 0 : nom / denom;
> I've worked around encoding this by extending expr.y locally.

AFAICS it's used only with #SMT_on in the condition, aybe we could limit
the condition only for #SMT_on term?


> 
> > +  EXPR: NUMBER
> > +  EXPR: EXPR | EXPR
> > +  EXPR: EXPR & EXPR
> > +  EXPR: EXPR ^ EXPR
> 
> Again, it's odd that these cast the double to a long and then assign
> the result back to a double.

is this even used anywhere? perhaps it was added just to be complete

SNIP

> > +       2.002460174                 0.86                23.37                 0.86
> > +       3.003969795                 1.03                23.93                 1.03
> > +  ...
> 
> A feature request would be to allow metrics in terms of other metrics,
> not just events :-) For example, it is common to sum all cache
> hit/miss events. It is laborious to copy that expression for hit rate,
> miss rate, etc.
> 
> Perhaps the expression parsing code should be folded into the event
> parsing code.

nice idea, but let's finish straighten up what we have first ;-)

I'll try to go through all the fixes/tests you posted and let's
get it in first

thanks,
jirka