On Tue, Apr 06, 2021 at 12:00:27PM +0100, John Garry wrote:
> On 01/04/2021 14:49, Jiri Olsa wrote:
> > On Thu, Mar 25, 2021 at 06:33:14PM +0800, John Garry wrote:
> >
> > SNIP
> >
> > > +struct metric {
> > > + struct list_head list;
> > > + struct metric_ref metric_ref;
> > > +};
> > > +
> > > +static int resolve_metric_simple(struct expr_parse_ctx *pctx,
> > > + struct list_head *compound_list,
> > > + struct pmu_events_map *map,
> > > + const char *metric_name)
> > > +{
> > > + struct hashmap_entry *cur, *cur_tmp;
> > > + struct metric *metric, *tmp;
> > > + size_t bkt;
> > > + bool all;
> > > + int rc;
> > > +
> > > + do {
> > > + all = true;
> > > + hashmap__for_each_entry_safe((&pctx->ids), cur, cur_tmp, bkt) {
> > > + struct metric_ref *ref;
> > > + struct pmu_event *pe;
> > > +
> > > + pe = metrcgroup_find_metric(cur->key, map);
>
> *
>
> > > + if (!pe)
> > > + continue;
> > > +
> > > + if (!strcmp(metric_name, (char *)cur->key)) {
> > > + pr_warning("Recursion detected for metric
> > > %s\n", metric_name);
> > > + rc = -1;
> > > + goto out_err;
> > > + }
> > > +
> > > + all = false;
> > > +
> > > + /* The metric key itself needs to go out.. */
> > > + expr__del_id(pctx, cur->key);
> > > +
> > > + metric = malloc(sizeof(*metric));
> > > + if (!metric) {
> > > + rc = -ENOMEM;
> > > + goto out_err;
> > > + }
> > > +
> > > + ref = &metric->metric_ref;
> > > + ref->metric_name = pe->metric_name;
> > > + ref->metric_expr = pe->metric_expr;
> > > + list_add_tail(&metric->list, compound_list);
> > > +
> > > + rc = expr__find_other(pe->metric_expr, NULL, pctx, 0);
> >
>
> Hi Jirka,
>
> > so this might add new items to pctx->ids, I think you need
> > to restart the iteration as we do it in __resolve_metric
> > otherwise you could miss some new keys
>
> I thought that I was doing this. Indeed, this code is very much like
> __resolve_metric() ;)
>
> So expr__find_other() may add a new item to pctx->ids, and we always iterate
> again, and try to lookup any pmu_events, *, above. If none exist, then we
hm, I don't see that.. so, what you do is:
hashmap__for_each_entry_safe((&pctx->ids) ....) {
rc = expr__find_other(pe->metric_expr, NULL, pctx, 0);
}
and what I think we need to do is:
hashmap__for_each_entry_safe((&pctx->ids) ....) {
rc = expr__find_other(pe->metric_expr, NULL, pctx, 0);
break;
}
each time you resolve another metric, you need to restart
the pctx->ids iteration, because there will be new items,
and we are in the middle of it
jirka
> have broken down pctx into primitive events aliases and unresolvable
> metrics, and stop iterating. And then unresolvable metrics would be found in
> check_parse_cpu().
>
> As an example, we can deal with metric test1, below, which references 2x
> other metrics:
>
> {
> "MetricExpr": "IDQ_UOPS_NOT_DELIVERED.CORE / (4 * (( (
> CPU_CLK_UNHALTED.THREAD / 2 ) * ( 1 + CPU_CLK_UNHALTED.ONE_THREAD_ACTIVE /
> CPU_CLK_UNHALTED.REF_XCLK ) )))",
> "MetricName": "Frontend_Bound",
> },
> {
> "MetricExpr": "( UOPS_ISSUED.ANY - UOPS_RETIRED.RETIRE_SLOTS + 4 *
> INT_MISC.RECOVERY_CYCLES ) / (4 * cycles)",
> "MetricName": "Bad_Speculation",
> },
> {
> "MetricExpr": "Bad_Speculation + Frontend_Bound",
> "MetricName": "test1",
> },
>
> Does that satisfy your concern, or have I missed something?
>
> Thanks,
> John
>
> >
> > jirka
> >
> > > + if (rc)
> > > + goto out_err;
> > > + }
> > > + } while (!all);
> > > +
> > > + return 0;
> > > +
> > > +out_err:
> > > + list_for_each_entry_safe(metric, tmp, compound_list, list)
> > > + free(metric);
> > > +
> > > + return rc;
> > > +
> > > +}
> >
> > SNIP
> >
> > .
> >
>
