linux-kernel - Re: [PATCH 5/5] perf metric: Don't compute unused events.

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201113233415.GJ894261@tassilo.jf.intel.com>
Date:   Fri, 13 Nov 2020 15:34:15 -0800
From:   Andi Kleen <ak@...ux.intel.com>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        linux-kernel@...r.kernel.org, Jin Yao <yao.jin@...ux.intel.com>,
        John Garry <john.garry@...wei.com>,
        Paul Clarke <pc@...ibm.com>, kajoljain <kjain@...ux.ibm.com>,
        Stephane Eranian <eranian@...gle.com>,
        Sandeep Dasgupta <sdasgup@...gle.com>,
        linux-perf-users@...r.kernel.org
Subject: Re: [PATCH 5/5] perf metric: Don't compute unused events.

The patch does a lot of stuff and is hard to review.

The earlier patches all look good to me.

>  
>  static int
>  __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
> -	      int start, int runtime)
> +	      bool compute_ids, int runtime)
>  {
>  	struct expr_scanner_ctx scanner_ctx = {
> -		.start_token = start,

Okay so why do we not need that anymore?

This should probably be a separate change.

I'm not sure about the NaN handling. Why can't you use some 
other value (perhaps 0 with some special case for division) 

After all the computed values of the dead branch shouldn't 
be used anyways, so I don't think we need the extra NaN 
ceremony everywhere.

The only thing that really matters is to track that the event
is not added and not resolved.

I think it would be a lot simpler if we changed the IF syntax.
The problem right now is that the condition is after the first
expression, so you can't use global state with an early reduction
to track if the if branch is dead or not.

If we used the C style cond ? a : b  it could be

cond { set flag } '?' expr  { reset flag } : expr { reset flag }

scanner checks flag and ignores the event if false

This would need changing the JSON files but that should be easy enough
with a script.

Or maybe at some point need to bite the bullet and build
an AST, but for now we probably can get away of not doing it.

-Andi