Re: [PATCH v2 2/6] perf test: Handle metric reuse in pmu-events parsing test

[Jiri Olsa]

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