linux-kernel - Re: [PATCH 3/4] perf stat: Add --metrics-file option

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]

Date:   Wed, 13 May 2020 13:33:45 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Ian Rogers <irogers@...gle.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>,
        Joe Mario <jmario@...hat.com>, Andi Kleen <ak@...ux.intel.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        John Garry <john.garry@...wei.com>
Subject: Re: [PATCH 3/4] perf stat: Add --metrics-file option

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