Messages in this thread |  | | Date | Wed, 15 Jul 2020 23:29:23 +0200 | From | Jiri Olsa <> | 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@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
|  |