On Wed, Jul 15, 2020 at 10:40:41AM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> > ---
> > 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
