linux-kernel - Re: [PATCH 16/18] perf metric: Add recursion check when processing nested metrics

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]

Message-ID: <20200715212923.GT183694@krava>
Date:   Wed, 15 Jul 2020 23:29:23 +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>,
        Andi Kleen <ak@...ux.intel.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        John Garry <john.garry@...wei.com>,
        "Paul A. Clarke" <pc@...ibm.com>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 16/18] perf metric: Add recursion check when processing
 nested metrics

On Wed, Jul 15, 2020 at 10:40:41AM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@...nel.org> wrote:
> >
> > Keeping the stack of nested metrics via 'struct expr_id' objecs
> 
> s/objecs/objects/
> 
> > and checking if we are in recursion via already processed metric.
> >
> > The stack is implemented as static array within the struct egroup
> > with 100 entries, which should be enough nesting depth for any
> > metric we have or plan to have at the moment.
> >
> > Adding test that simulates the recursion and checks we can
> > detect it.
> >
> > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > ---
> >  tools/perf/tests/parse-metric.c |  27 ++++++++-
> >  tools/perf/util/expr.c          |   2 +
> >  tools/perf/util/expr.h          |   9 ++-
> >  tools/perf/util/metricgroup.c   | 101 +++++++++++++++++++++++++++++---
> >  4 files changed, 128 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
> > index b50e2a3f3b73..238a805edd55 100644
> > --- a/tools/perf/tests/parse-metric.c
> > +++ b/tools/perf/tests/parse-metric.c
> > @@ -57,6 +57,14 @@ static struct pmu_event pme_test[] = {
> >         .metric_expr    = "d_ratio(DCache_L2_All_Miss, DCache_L2_All)",
> >         .metric_name    = "DCache_L2_Misses",
> >  },
> > +{
> > +       .metric_expr    = "IPC + M2",
> > +       .metric_name    = "M1",
> > +},
> > +{
> > +       .metric_expr    = "IPC + M1",
> > +       .metric_name    = "M2",
> > +},
> >  };
> 
> Perhaps add a test on simple recursion too:
> {
>   .metric_expr = "1/M1",
>   .metric_name = "M1",
> }

ok, will add

SNIP

> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 66f25362702d..69ec20dd737b 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -24,6 +24,7 @@
> >  #include <subcmd/parse-options.h>
> >  #include <api/fs/fs.h>
> >  #include "util.h"
> > +#include <asm/bug.h>
> >
> >  struct metric_event *metricgroup__lookup(struct rblist *metric_events,
> >                                          struct evsel *evsel,
> > @@ -109,6 +110,8 @@ struct metric_ref_node {
> >         struct list_head list;
> >  };
> >
> > +#define RECURSION_ID_MAX 100
> > +
> >  struct egroup {
> >         struct list_head nd;
> >         struct expr_parse_ctx pctx;
> > @@ -119,6 +122,11 @@ struct egroup {
> >         int refs_cnt;
> >         int runtime;
> >         bool has_constraint;
> > +
> > +       struct {
> > +               struct expr_id  id[RECURSION_ID_MAX];
> > +               int             cnt;
> > +       } recursion;
> >  };
> 
> Rather than place this in the egroup why not pass a "visited" array to
> add metric. This would be more in keeping with Tarjan's algorithm
> (although the SCC isn't needed in this case):
> https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm

it's true that it's just source of the 'struct expr_id' objects and
one global array could be used in multiple metrics not event directly
nested ... seems good, will check

thanks,
jirka