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

[Jiri Olsa <jolsa@redhat.com>]

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@kernel.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@kernel.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