Commit cb94a02e authored by Ian Rogers's avatar Ian Rogers Committed by Arnaldo Carvalho de Melo
Browse files

perf metric: Restructure struct expr_parse_ctx. 



A later change to parsing the ids out (in expr__find_other) will
potentially drop hashmaps and so it is more convenient to move
expr_parse_ctx to have a hashmap pointer rather than a struct value.

As this pointer must be freed, rather than just going out of scope, add
expr__ctx_new and expr__ctx_free to manage expr_parse_ctx memory.
Adjust use of struct expr_parse_ctx accordingly.

Reviewed-by: default avatarAndi Kleen <ak@linux.intel.com>
Signed-off-by: default avatarIan Rogers <irogers@google.com>
Tested-by: default avatarJohn Garry <john.garry@huawei.com>
Acked-by: default avatarJiri Olsa <jolsa@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Clarke <pc@us.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sandeep Dasgupta <sdasgup@google.com>
Cc: Stephane Eranian <eranian@google.com>
Link: https://lore.kernel.org/r/20210923074616.674826-2-irogers@google.com


Signed-off-by: default avatarArnaldo Carvalho de Melo <acme@redhat.com>
parent c8016128

tools/perf/tests/expr.c

+42 −39
Original line number Diff line number Diff line
@@ -22,67 +22,70 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused) 
	const char *p;

	double val;

	int ret;

	struct expr_parse_ctx ctx;

	struct expr_parse_ctx *ctx;



	expr__ctx_init(&ctx);

	expr__add_id_val(&ctx, strdup("FOO"), 1);

	expr__add_id_val(&ctx, strdup("BAR"), 2);

	ctx = expr__ctx_new();

	TEST_ASSERT_VAL("expr__ctx_new", ctx);

	expr__add_id_val(ctx, strdup("FOO"), 1);

	expr__add_id_val(ctx, strdup("BAR"), 2);



	ret = test(&ctx, "1+1", 2);

	ret |= test(&ctx, "FOO+BAR", 3);

	ret |= test(&ctx, "(BAR/2)%2", 1);

	ret |= test(&ctx, "1 - -4",  5);

	ret |= test(&ctx, "(FOO-1)*2 + (BAR/2)%2 - -4",  5);

	ret |= test(&ctx, "1-1 | 1", 1);

	ret |= test(&ctx, "1-1 & 1", 0);

	ret |= test(&ctx, "min(1,2) + 1", 2);

	ret |= test(&ctx, "max(1,2) + 1", 3);

	ret |= test(&ctx, "1+1 if 3*4 else 0", 2);

	ret |= test(&ctx, "1.1 + 2.1", 3.2);

	ret |= test(&ctx, ".1 + 2.", 2.1);

	ret |= test(&ctx, "d_ratio(1, 2)", 0.5);

	ret |= test(&ctx, "d_ratio(2.5, 0)", 0);

	ret |= test(&ctx, "1.1 < 2.2", 1);

	ret |= test(&ctx, "2.2 > 1.1", 1);

	ret |= test(&ctx, "1.1 < 1.1", 0);

	ret |= test(&ctx, "2.2 > 2.2", 0);

	ret |= test(&ctx, "2.2 < 1.1", 0);

	ret |= test(&ctx, "1.1 > 2.2", 0);

	ret = test(ctx, "1+1", 2);

	ret |= test(ctx, "FOO+BAR", 3);

	ret |= test(ctx, "(BAR/2)%2", 1);

	ret |= test(ctx, "1 - -4",  5);

	ret |= test(ctx, "(FOO-1)*2 + (BAR/2)%2 - -4",  5);

	ret |= test(ctx, "1-1 | 1", 1);

	ret |= test(ctx, "1-1 & 1", 0);

	ret |= test(ctx, "min(1,2) + 1", 2);

	ret |= test(ctx, "max(1,2) + 1", 3);

	ret |= test(ctx, "1+1 if 3*4 else 0", 2);

	ret |= test(ctx, "1.1 + 2.1", 3.2);

	ret |= test(ctx, ".1 + 2.", 2.1);

	ret |= test(ctx, "d_ratio(1, 2)", 0.5);

	ret |= test(ctx, "d_ratio(2.5, 0)", 0);

	ret |= test(ctx, "1.1 < 2.2", 1);

	ret |= test(ctx, "2.2 > 1.1", 1);

	ret |= test(ctx, "1.1 < 1.1", 0);

	ret |= test(ctx, "2.2 > 2.2", 0);

	ret |= test(ctx, "2.2 < 1.1", 0);

	ret |= test(ctx, "1.1 > 2.2", 0);



	if (ret)

	if (ret) {

		expr__ctx_free(ctx);

		return ret;

	}



	p = "FOO/0";

	ret = expr__parse(&val, &ctx, p, 1);

	ret = expr__parse(&val, ctx, p, 1);

	TEST_ASSERT_VAL("division by zero", ret == -1);



	p = "BAR/";

	ret = expr__parse(&val, &ctx, p, 1);

	ret = expr__parse(&val, ctx, p, 1);

	TEST_ASSERT_VAL("missing operand", ret == -1);



	expr__ctx_clear(&ctx);

	expr__ctx_clear(ctx);

	TEST_ASSERT_VAL("find other",

			expr__find_other("FOO + BAR + BAZ + BOZO", "FOO",

					 &ctx, 1) == 0);

	TEST_ASSERT_VAL("find other", hashmap__size(&ctx.ids) == 3);

	TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "BAR",

					 ctx, 1) == 0);

	TEST_ASSERT_VAL("find other", hashmap__size(ctx->ids) == 3);

	TEST_ASSERT_VAL("find other", hashmap__find(ctx->ids, "BAR",

						    (void **)&val_ptr));

	TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "BAZ",

	TEST_ASSERT_VAL("find other", hashmap__find(ctx->ids, "BAZ",

						    (void **)&val_ptr));

	TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "BOZO",

	TEST_ASSERT_VAL("find other", hashmap__find(ctx->ids, "BOZO",

						    (void **)&val_ptr));



	expr__ctx_clear(&ctx);

	expr__ctx_clear(ctx);

	TEST_ASSERT_VAL("find other",

			expr__find_other("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@",

					 NULL, &ctx, 3) == 0);

	TEST_ASSERT_VAL("find other", hashmap__size(&ctx.ids) == 2);

	TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "EVENT1,param=3/",

					 NULL, ctx, 3) == 0);

	TEST_ASSERT_VAL("find other", hashmap__size(ctx->ids) == 2);

	TEST_ASSERT_VAL("find other", hashmap__find(ctx->ids, "EVENT1,param=3/",

						    (void **)&val_ptr));

	TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "EVENT2,param=3/",

	TEST_ASSERT_VAL("find other", hashmap__find(ctx->ids, "EVENT2,param=3/",

						    (void **)&val_ptr));



	expr__ctx_clear(&ctx);

	expr__ctx_free(ctx);



	return 0;

}

tools/perf/tests/pmu-events.c

+28 −19
Original line number Diff line number Diff line
@@ -836,7 +836,7 @@ static int resolve_metric_simple(struct expr_parse_ctx *pctx, 


	do {

		all = true;

		hashmap__for_each_entry_safe((&pctx->ids), cur, cur_tmp, bkt) {

		hashmap__for_each_entry_safe(pctx->ids, cur, cur_tmp, bkt) {

			struct metric_ref *ref;

			struct pmu_event *pe;
@@ -890,9 +890,14 @@ static int test_parsing(void) 
	struct pmu_event *pe;

	int i, j, k;

	int ret = 0;

	struct expr_parse_ctx ctx;

	struct expr_parse_ctx *ctx;

	double result;



	ctx = expr__ctx_new();

	if (!ctx) {

		pr_debug("expr__ctx_new failed");

		return TEST_FAIL;

	}

	i = 0;

	for (;;) {

		map = &pmu_events_map[i++];
@@ -910,15 +915,15 @@ static int test_parsing(void) 
				break;

			if (!pe->metric_expr)

				continue;

			expr__ctx_init(&ctx);

			if (expr__find_other(pe->metric_expr, NULL, &ctx, 0)

			expr__ctx_clear(ctx);

			if (expr__find_other(pe->metric_expr, NULL, ctx, 0)

				  < 0) {

				expr_failure("Parse other failed", map, pe);

				ret++;

				continue;

			}



			if (resolve_metric_simple(&ctx, &compound_list, map,

			if (resolve_metric_simple(ctx, &compound_list, map,

						  pe->metric_name)) {

				expr_failure("Could not resolve metrics", map, pe);

				ret++;
@@ -931,27 +936,27 @@ static int test_parsing(void) 
			 * make them unique.

			 */

			k = 1;

			hashmap__for_each_entry((&ctx.ids), cur, bkt)

				expr__add_id_val(&ctx, strdup(cur->key), k++);

			hashmap__for_each_entry(ctx->ids, cur, bkt)

				expr__add_id_val(ctx, strdup(cur->key), k++);



			hashmap__for_each_entry((&ctx.ids), cur, bkt) {

			hashmap__for_each_entry(ctx->ids, cur, bkt) {

				if (check_parse_cpu(cur->key, map == cpus_map,

						   pe))

					ret++;

			}



			list_for_each_entry_safe(metric, tmp, &compound_list, list) {

				expr__add_ref(&ctx, &metric->metric_ref);

				expr__add_ref(ctx, &metric->metric_ref);

				free(metric);

			}



			if (expr__parse(&result, &ctx, pe->metric_expr, 0)) {

			if (expr__parse(&result, ctx, pe->metric_expr, 0)) {

				expr_failure("Parse failed", map, pe);

				ret++;

			}

			expr__ctx_clear(&ctx);

		}

	}

	expr__ctx_free(ctx);

	/* TODO: fail when not ok */

exit:

	return ret == 0 ? TEST_OK : TEST_SKIP;
@@ -971,7 +976,7 @@ static struct test_metric metrics[] = { 


static int metric_parse_fake(const char *str)

{

	struct expr_parse_ctx ctx;

	struct expr_parse_ctx *ctx;

	struct hashmap_entry *cur;

	double result;

	int ret = -1;
@@ -980,8 +985,12 @@ static int metric_parse_fake(const char *str) 


	pr_debug("parsing '%s'\n", str);



	expr__ctx_init(&ctx);

	if (expr__find_other(str, NULL, &ctx, 0) < 0) {

	ctx = expr__ctx_new();

	if (!ctx) {

		pr_debug("expr__ctx_new failed");

		return TEST_FAIL;

	}

	if (expr__find_other(str, NULL, ctx, 0) < 0) {

		pr_err("expr__find_other failed\n");

		return -1;

	}
@@ -992,23 +1001,23 @@ static int metric_parse_fake(const char *str) 
	 * make them unique.

	 */

	i = 1;

	hashmap__for_each_entry((&ctx.ids), cur, bkt)

		expr__add_id_val(&ctx, strdup(cur->key), i++);

	hashmap__for_each_entry(ctx->ids, cur, bkt)

		expr__add_id_val(ctx, strdup(cur->key), i++);



	hashmap__for_each_entry((&ctx.ids), cur, bkt) {

	hashmap__for_each_entry(ctx->ids, cur, bkt) {

		if (check_parse_fake(cur->key)) {

			pr_err("check_parse_fake failed\n");

			goto out;

		}

	}



	if (expr__parse(&result, &ctx, str, 0))

	if (expr__parse(&result, ctx, str, 0))

		pr_err("expr__parse failed\n");

	else

		ret = 0;



out:

	expr__ctx_clear(&ctx);

	expr__ctx_free(ctx);

	return ret;

}

tools/perf/util/expr.c

+30 −9
Original line number Diff line number Diff line
@@ -73,7 +73,7 @@ int expr__add_id(struct expr_parse_ctx *ctx, const char *id) 
	data_ptr->parent = ctx->parent;

	data_ptr->kind = EXPR_ID_DATA__PARENT;



	ret = hashmap__set(&ctx->ids, id, data_ptr,

	ret = hashmap__set(ctx->ids, id, data_ptr,

			   (const void **)&old_key, (void **)&old_data);

	if (ret)

		free(data_ptr);
@@ -95,7 +95,7 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val) 
	data_ptr->val = val;

	data_ptr->kind = EXPR_ID_DATA__VALUE;



	ret = hashmap__set(&ctx->ids, id, data_ptr,

	ret = hashmap__set(ctx->ids, id, data_ptr,

			   (const void **)&old_key, (void **)&old_data);

	if (ret)

		free(data_ptr);
@@ -140,7 +140,7 @@ int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref) 
	data_ptr->ref.metric_expr = ref->metric_expr;

	data_ptr->kind = EXPR_ID_DATA__REF;



	ret = hashmap__set(&ctx->ids, name, data_ptr,

	ret = hashmap__set(ctx->ids, name, data_ptr,

			   (const void **)&old_key, (void **)&old_data);

	if (ret)

		free(data_ptr);
@@ -156,7 +156,7 @@ int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref) 
int expr__get_id(struct expr_parse_ctx *ctx, const char *id,

		 struct expr_id_data **data)

{

	return hashmap__find(&ctx->ids, id, (void **)data) ? 0 : -1;

	return hashmap__find(ctx->ids, id, (void **)data) ? 0 : -1;

}



int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id,
@@ -205,15 +205,23 @@ void expr__del_id(struct expr_parse_ctx *ctx, const char *id) 
	struct expr_id_data *old_val = NULL;

	char *old_key = NULL;



	hashmap__delete(&ctx->ids, id,

	hashmap__delete(ctx->ids, id,

			(const void **)&old_key, (void **)&old_val);

	free(old_key);

	free(old_val);

}



void expr__ctx_init(struct expr_parse_ctx *ctx)

struct expr_parse_ctx *expr__ctx_new(void)

{

	hashmap__init(&ctx->ids, key_hash, key_equal, NULL);

	struct expr_parse_ctx *ctx;



	ctx = malloc(sizeof(struct expr_parse_ctx));

	if (!ctx)

		return NULL;



	ctx->ids = hashmap__new(key_hash, key_equal, NULL);

	ctx->parent = NULL;

	return ctx;

}



void expr__ctx_clear(struct expr_parse_ctx *ctx)
@@ -221,11 +229,24 @@ void expr__ctx_clear(struct expr_parse_ctx *ctx) 
	struct hashmap_entry *cur;

	size_t bkt;



	hashmap__for_each_entry((&ctx->ids), cur, bkt) {

	hashmap__for_each_entry(ctx->ids, cur, bkt) {

		free((char *)cur->key);

		free(cur->value);

	}

	hashmap__clear(ctx->ids);

}



void expr__ctx_free(struct expr_parse_ctx *ctx)

{

	struct hashmap_entry *cur;

	size_t bkt;



	hashmap__for_each_entry(ctx->ids, cur, bkt) {

		free((char *)cur->key);

		free(cur->value);

	}

	hashmap__clear(&ctx->ids);

	hashmap__free(ctx->ids);

	free(ctx);

}



static int

tools/perf/util/expr.h

+3 −2
Original line number Diff line number Diff line
@@ -19,7 +19,7 @@ struct expr_id { 
};



struct expr_parse_ctx {

	struct hashmap	 ids;

	struct hashmap	*ids;

	struct expr_id	*parent;

};
@@ -30,8 +30,9 @@ struct expr_scanner_ctx { 
	int runtime;

};



void expr__ctx_init(struct expr_parse_ctx *ctx);

struct expr_parse_ctx *expr__ctx_new(void);

void expr__ctx_clear(struct expr_parse_ctx *ctx);

void expr__ctx_free(struct expr_parse_ctx *ctx);

void expr__del_id(struct expr_parse_ctx *ctx, const char *id);

int expr__add_id(struct expr_parse_ctx *ctx, const char *id);

int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);

tools/perf/util/metricgroup.c

+24 −20
Original line number Diff line number Diff line
@@ -118,7 +118,7 @@ struct metric_ref_node { 


struct metric {

	struct list_head nd;

	struct expr_parse_ctx pctx;

	struct expr_parse_ctx *pctx;

	const char *metric_name;

	const char *metric_expr;

	const char *metric_unit;
@@ -198,7 +198,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist, 
	struct evsel *ev, *current_leader = NULL;

	struct expr_id_data *val_ptr;

	int i = 0, matched_events = 0, events_to_match;

	const int idnum = (int)hashmap__size(&pctx->ids);

	const int idnum = (int)hashmap__size(pctx->ids);



	/*

	 * duration_time is always grouped separately, when events are grouped
@@ -206,7 +206,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist, 
	 * add it to metric_events at the end.

	 */

	if (!has_constraint &&

	    hashmap__find(&pctx->ids, "duration_time", (void **)&val_ptr))

	    hashmap__find(pctx->ids, "duration_time", (void **)&val_ptr))

		events_to_match = idnum - 1;

	else

		events_to_match = idnum;
@@ -242,7 +242,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist, 
		if (contains_event(metric_events, matched_events, ev->name))

			continue;

		/* Does this event belong to the parse context? */

		if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr))

		if (hashmap__find(pctx->ids, ev->name, (void **)&val_ptr))

			metric_events[matched_events++] = ev;



		if (matched_events == events_to_match)
@@ -322,12 +322,12 @@ static int metricgroup__setup_events(struct list_head *groups, 
		struct metric_ref *metric_refs = NULL;



		metric_events = calloc(sizeof(void *),

				hashmap__size(&m->pctx.ids) + 1);

				hashmap__size(m->pctx->ids) + 1);

		if (!metric_events) {

			ret = -ENOMEM;

			break;

		}

		evsel = find_evsel_group(perf_evlist, &m->pctx,

		evsel = find_evsel_group(perf_evlist, m->pctx,

					 metric_no_merge,

					 m->has_constraint, metric_events,

					 evlist_used);
@@ -693,7 +693,7 @@ static void metricgroup__add_metric_weak_group(struct strbuf *events, 
	size_t bkt;

	bool no_group = true, has_duration = false;



	hashmap__for_each_entry((&ctx->ids), cur, bkt) {

	hashmap__for_each_entry(ctx->ids, cur, bkt) {

		pr_debug("found event %s\n", (const char *)cur->key);

		/*

		 * Duration time maps to a software event and can make
@@ -724,7 +724,7 @@ static void metricgroup__add_metric_non_group(struct strbuf *events, 
	size_t bkt;

	bool first = true;



	hashmap__for_each_entry((&ctx->ids), cur, bkt) {

	hashmap__for_each_entry(ctx->ids, cur, bkt) {

		if (!first)

			strbuf_addf(events, ",");

		strbuf_addf(events, "%s", (const char *)cur->key);
@@ -799,7 +799,11 @@ static int __add_metric(struct list_head *metric_list, 
		if (!m)

			return -ENOMEM;



		expr__ctx_init(&m->pctx);

		m->pctx = expr__ctx_new();

		if (!m->pctx) {

			free(m);

			return -ENOMEM;

		}

		m->metric_name = pe->metric_name;

		m->metric_expr = pe->metric_expr;

		m->metric_unit = pe->unit;
@@ -847,15 +851,15 @@ static int __add_metric(struct list_head *metric_list, 


	/* Force all found IDs in metric to have us as parent ID. */

	WARN_ON_ONCE(!parent);

	m->pctx.parent = parent;

	m->pctx->parent = parent;



	/*

	 * For both the parent and referenced metrics, we parse

	 * all the metric's IDs and add it to the parent context.

	 */

	if (expr__find_other(pe->metric_expr, NULL, &m->pctx, runtime) < 0) {

	if (expr__find_other(pe->metric_expr, NULL, m->pctx, runtime) < 0) {

		if (m->metric_refs_cnt == 0) {

			expr__ctx_clear(&m->pctx);

			expr__ctx_free(m->pctx);

			free(m);

			*mp = NULL;

		}
@@ -878,8 +882,8 @@ static int __add_metric(struct list_head *metric_list, 
		list_for_each_prev(pos, metric_list) {

			struct metric *old = list_entry(pos, struct metric, nd);



			if (hashmap__size(&m->pctx.ids) <=

			    hashmap__size(&old->pctx.ids))

			if (hashmap__size(m->pctx->ids) <=

			    hashmap__size(old->pctx->ids))

				break;

		}

		list_add(&m->nd, pos);
@@ -927,7 +931,7 @@ static int recursion_check(struct metric *m, const char *id, struct expr_id **pa 
	 * if we already processed 'id', if we did, it's recursion

	 * and we fail.

	 */

	ret = expr__get_id(&m->pctx, id, &data);

	ret = expr__get_id(m->pctx, id, &data);

	if (ret)

		return ret;
@@ -982,7 +986,7 @@ static int __resolve_metric(struct metric *m, 
	 */

	do {

		all = true;

		hashmap__for_each_entry((&m->pctx.ids), cur, bkt) {

		hashmap__for_each_entry(m->pctx->ids, cur, bkt) {

			struct expr_id *parent;

			struct pmu_event *pe;
@@ -996,7 +1000,7 @@ static int __resolve_metric(struct metric *m, 


			all = false;

			/* The metric key itself needs to go out.. */

			expr__del_id(&m->pctx, cur->key);

			expr__del_id(m->pctx, cur->key);



			/* ... and it gets resolved to the parent context. */

			ret = add_metric(metric_list, pe, metric_no_group, &m, parent, ids);
@@ -1144,10 +1148,10 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group, 


		if (m->has_constraint) {

			metricgroup__add_metric_non_group(events,

							  &m->pctx);

							  m->pctx);

		} else {

			metricgroup__add_metric_weak_group(events,

							   &m->pctx);

							   m->pctx);

		}

	}
@@ -1210,7 +1214,7 @@ static void metricgroup__free_metrics(struct list_head *metric_list) 


	list_for_each_entry_safe (m, tmp, metric_list, nd) {

		metric__free_refs(m);

		expr__ctx_clear(&m->pctx);

		expr__ctx_free(m->pctx);

		list_del_init(&m->nd);

		free(m);

	}