[PATCH 0/5] Revert metric hybrid events, fix tools events

[PATCH 0/5] Revert metric hybrid events, fix tools events

[Ian Rogers]

Hybrid metrics place a PMU at the end of the parse string. This is
also where tool events are placed. The behavior of the parse string
isn't clear and so revert the change for now.

commit 75eafc970bd9 ("perf list: Print all available tool events") had
an off-by-1 error. Try to separate the tool event enum logic from
evsels. Also add a for loop helper to simplify working with a variety
of tool events.

Add support to metrics for more tool events than just
duration_time. Make the sharing logic only look to add for sharing
potentially used tool events, found by scanning the list of metrics.

Ian Rogers (5):
  Revert "perf stat: Support metrics with hybrid events"
  perf evsel: Constify a few arrays
  perf evsel: Add tool event helpers
  perf metrics: Support all tool events
  perf metrics: Don't add all tool events for sharing

 tools/perf/tests/evsel-roundtrip-name.c |   2 +-
 tools/perf/util/evsel.c                 |  53 +++-
 tools/perf/util/evsel.h                 |  22 +-
 tools/perf/util/metricgroup.c           | 373 +++++++-----------------
 tools/perf/util/parse-events.c          |   2 +-
 tools/perf/util/stat-display.c          |   8 +-
 tools/perf/util/stat-shadow.c           |  27 +-
 7 files changed, 180 insertions(+), 307 deletions(-)

-- 
2.36.0.512.ge40c2bad7a-goog

[PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

[Ian Rogers]

This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c  | 263 +++------------------------------
 tools/perf/util/stat-display.c |   8 +-
 2 files changed, 22 insertions(+), 249 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 126a43a8917e..d8492e339521 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -141,11 +141,6 @@ struct metric {
 	 * output.
 	 */
 	const char *metric_unit;
-	/**
-	 * The name of the CPU such as "cpu_core" or "cpu_atom" in hybrid systems
-	 * and "NULL" in non-hybrid systems.
-	 */
-	const char *pmu_name;
 	/** Optional null terminated array of referenced metrics. */
 	struct metric_ref *metric_refs;
 	/**
@@ -220,7 +215,6 @@ static struct metric *metric__new(const struct pmu_event *pe,
 	}
 	m->metric_expr = pe->metric_expr;
 	m->metric_unit = pe->unit;
-	m->pmu_name = pe->pmu;
 	m->pctx->runtime = runtime;
 	m->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
 	m->metric_refs = NULL;
@@ -256,12 +250,10 @@ static bool contains_metric_id(struct evsel **metric_events, int num_events,
  * @ids: the metric IDs to match.
  * @metric_evlist: the list of perf events.
  * @out_metric_events: holds the created metric events array.
- * @pmu_name: the name of the CPU.
  */
 static int setup_metric_events(struct hashmap *ids,
 			       struct evlist *metric_evlist,
-			       struct evsel ***out_metric_events,
-			       const char *pmu_name)
+			       struct evsel ***out_metric_events)
 {
 	struct evsel **metric_events;
 	const char *metric_id;
@@ -294,10 +286,6 @@ static int setup_metric_events(struct hashmap *ids,
 		 * about this event.
 		 */
 		if (hashmap__find(ids, metric_id, (void **)&val_ptr)) {
-			if (evsel__is_hybrid(ev) && pmu_name &&
-			    strcmp(pmu_name, ev->pmu_name)) {
-				continue;
-			}
 			metric_events[matched_events++] = ev;
 
 			if (matched_events >= ids_size)
@@ -736,8 +724,7 @@ static int decode_all_metric_ids(struct evlist *perf_evlist, const char *modifie
 static int metricgroup__build_event_string(struct strbuf *events,
 					   const struct expr_parse_ctx *ctx,
 					   const char *modifier,
-					   bool has_constraint,
-					   const char *pmu_name)
+					   bool has_constraint)
 {
 	struct hashmap_entry *cur;
 	size_t bkt;
@@ -819,18 +806,12 @@ static int metricgroup__build_event_string(struct strbuf *events,
 		if (no_group) {
 			/* Strange case of a metric of just duration_time. */
 			ret = strbuf_addf(events, "duration_time");
-		} else if (!has_constraint) {
-			ret = strbuf_addf(events, "}:W");
-			if (pmu_name)
-				ret = strbuf_addf(events, "#%s", pmu_name);
-			ret = strbuf_addf(events, ",duration_time");
-		} else
+		} else if (!has_constraint)
+			ret = strbuf_addf(events, "}:W,duration_time");
+		else
 			ret = strbuf_addf(events, ",duration_time");
-	} else if (!no_group && !has_constraint) {
+	} else if (!no_group && !has_constraint)
 		ret = strbuf_addf(events, "}:W");
-		if (pmu_name)
-			ret = strbuf_addf(events, "#%s", pmu_name);
-	}
 
 	return ret;
 #undef RETURN_IF_NON_ZERO
@@ -1169,13 +1150,11 @@ static int metric_list_cmp(void *priv __maybe_unused, const struct list_head *l,
  * @metric_list: The list that the metric or metric group are added to.
  * @map: The map that is searched for metrics, most commonly the table for the
  *       architecture perf is running upon.
- * @pmu_name: the name of the CPU.
  */
-static int metricgroup__add_metric(const char *metric_name,
-				   const char *modifier, bool metric_no_group,
+static int metricgroup__add_metric(const char *metric_name, const char *modifier,
+				   bool metric_no_group,
 				   struct list_head *metric_list,
-				   const struct pmu_events_map *map,
-				   const char *pmu_name)
+				   const struct pmu_events_map *map)
 {
 	const struct pmu_event *pe;
 	LIST_HEAD(list);
@@ -1188,8 +1167,6 @@ static int metricgroup__add_metric(const char *metric_name,
 	 */
 	map_for_each_metric(pe, i, map, metric_name) {
 		has_match = true;
-		if (pmu_name && pe->pmu && strcmp(pmu_name, pe->pmu))
-			continue;
 		ret = add_metric(&list, pe, modifier, metric_no_group,
 				 /*root_metric=*/NULL,
 				 /*visited_metrics=*/NULL, map);
@@ -1238,12 +1215,10 @@ static int metricgroup__add_metric(const char *metric_name,
  * @metric_list: The list that metrics are added to.
  * @map: The map that is searched for metrics, most commonly the table for the
  *       architecture perf is running upon.
- * @pmu_name: the name of the CPU.
  */
 static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 					struct list_head *metric_list,
-					const struct pmu_events_map *map,
-					const char *pmu_name)
+					const struct pmu_events_map *map)
 {
 	char *list_itr, *list_copy, *metric_name, *modifier;
 	int ret, count = 0;
@@ -1260,7 +1235,7 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 
 		ret = metricgroup__add_metric(metric_name, modifier,
 					      metric_no_group, metric_list,
-					      map, pmu_name);
+					      map);
 		if (ret == -EINVAL)
 			pr_err("Cannot find metric or group `%s'\n", metric_name);
 
@@ -1335,183 +1310,6 @@ static int build_combined_expr_ctx(const struct list_head *metric_list,
 	return ret;
 }
 
-static char *get_metric_pmus(char *orig_str, struct strbuf *metric_pmus)
-{
-	char *llist, *nlist, *p1, *p2, *new_str = NULL;
-	int ret;
-	struct strbuf new_events;
-
-	if (!strchr(orig_str, '#')) {
-		/*
-		 * pmu name is added after '#'. If no '#' found,
-		 * don't need to process pmu.
-		 */
-		return strdup(orig_str);
-	}
-
-	nlist = strdup(orig_str);
-	if (!nlist)
-		return new_str;
-
-	ret = strbuf_init(&new_events, 100);
-	if (ret)
-		goto err_out;
-
-	ret = strbuf_grow(metric_pmus, 100);
-	if (ret)
-		goto err_out;
-
-	llist = nlist;
-	while ((p1 = strsep(&llist, ",")) != NULL) {
-		p2 = strchr(p1, '#');
-		if (p2) {
-			*p2 = 0;
-			ret = strbuf_addf(&new_events, "%s,", p1);
-			if (ret)
-				goto err_out;
-
-			ret = strbuf_addf(metric_pmus, "%s,", p2 + 1);
-			if (ret)
-				goto err_out;
-
-		} else {
-			ret = strbuf_addf(&new_events, "%s,", p1);
-			if (ret)
-				goto err_out;
-		}
-	}
-
-	new_str = strdup(new_events.buf);
-	if (new_str) {
-		/* Remove last ',' */
-		new_str[strlen(new_str) - 1] = 0;
-	}
-err_out:
-	free(nlist);
-	strbuf_release(&new_events);
-	return new_str;
-}
-
-static void set_pmu_unmatched_events(struct evlist *evlist, int group_idx,
-				     char *pmu_name,
-				     unsigned long *evlist_removed)
-{
-	struct evsel *evsel, *pos;
-	int i = 0, j = 0;
-
-	/*
-	 * Move to the first evsel of a given group
-	 */
-	evlist__for_each_entry(evlist, evsel) {
-		if (evsel__is_group_leader(evsel) &&
-		    evsel->core.nr_members >= 1) {
-			if (i < group_idx) {
-				j += evsel->core.nr_members;
-				i++;
-				continue;
-			}
-		}
-	}
-
-	i = 0;
-	evlist__for_each_entry(evlist, evsel) {
-		if (i < j) {
-			i++;
-			continue;
-		}
-
-		/*
-		 * Now we are at the first evsel in the group
-		 */
-		for_each_group_evsel(pos, evsel) {
-			if (evsel__is_hybrid(pos) &&
-			    strcmp(pos->pmu_name, pmu_name)) {
-				set_bit(pos->core.idx, evlist_removed);
-			}
-		}
-		break;
-	}
-}
-
-static void remove_pmu_umatched_events(struct evlist *evlist, char *metric_pmus)
-{
-	struct evsel *evsel, *tmp, *new_leader = NULL;
-	unsigned long *evlist_removed;
-	char *llist, *nlist, *p1;
-	bool need_new_leader = false;
-	int i = 0, new_nr_members = 0;
-
-	nlist = strdup(metric_pmus);
-	if (!nlist)
-		return;
-
-	evlist_removed = bitmap_zalloc(evlist->core.nr_entries);
-	if (!evlist_removed) {
-		free(nlist);
-		return;
-	}
-
-	llist = nlist;
-	while ((p1 = strsep(&llist, ",")) != NULL) {
-		if (strlen(p1) > 0) {
-			/*
-			 * p1 points to the string of pmu name, e.g. "cpu_atom".
-			 * The metric group string has pmu suffixes, e.g.
-			 * "{inst_retired.any,cpu_clk_unhalted.thread}:W#cpu_core,
-			 *  {cpu_clk_unhalted.core,inst_retired.any_p}:W#cpu_atom"
-			 * By counting the pmu name, we can know the index of
-			 * group.
-			 */
-			set_pmu_unmatched_events(evlist, i++, p1,
-						 evlist_removed);
-		}
-	}
-
-	evlist__for_each_entry_safe(evlist, tmp, evsel) {
-		if (test_bit(evsel->core.idx, evlist_removed)) {
-			if (!evsel__is_group_leader(evsel)) {
-				if (!need_new_leader) {
-					if (new_leader)
-						new_leader->core.leader->nr_members--;
-					else
-						evsel->core.leader->nr_members--;
-				} else
-					new_nr_members--;
-			} else {
-				/*
-				 * If group leader is to remove, we need to
-				 * prepare a new leader and adjust all group
-				 * members.
-				 */
-				need_new_leader = true;
-				new_nr_members =
-				    evsel->core.leader->nr_members - 1;
-			}
-
-			evlist__remove(evlist, evsel);
-			evsel__delete(evsel);
-		} else {
-			if (!evsel__is_group_leader(evsel)) {
-				if (need_new_leader) {
-					need_new_leader = false;
-					new_leader = evsel;
-					new_leader->core.leader =
-					    &new_leader->core;
-					new_leader->core.nr_members =
-					    new_nr_members;
-				} else if (new_leader)
-					evsel->core.leader = &new_leader->core;
-			} else {
-				need_new_leader = false;
-				new_leader = NULL;
-			}
-		}
-	}
-
-	bitmap_free(evlist_removed);
-	free(nlist);
-}
-
 /**
  * parse_ids - Build the event string for the ids and parse them creating an
  *             evlist. The encoded metric_ids are decoded.
@@ -1521,18 +1319,14 @@ static void remove_pmu_umatched_events(struct evlist *evlist, char *metric_pmus)
  * @modifier: any modifiers added to the events.
  * @has_constraint: false if events should be placed in a weak group.
  * @out_evlist: the created list of events.
- * @pmu_name: the name of the CPU.
  */
 static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
 		     struct expr_parse_ctx *ids, const char *modifier,
-		     bool has_constraint, struct evlist **out_evlist,
-		     const char *pmu_name)
+		     bool has_constraint, struct evlist **out_evlist)
 {
 	struct parse_events_error parse_error;
 	struct evlist *parsed_evlist;
 	struct strbuf events = STRBUF_INIT;
-	struct strbuf metric_pmus = STRBUF_INIT;
-	char *nlist = NULL;
 	int ret;
 
 	*out_evlist = NULL;
@@ -1559,7 +1353,7 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
 		ids__insert(ids->ids, tmp);
 	}
 	ret = metricgroup__build_event_string(&events, ids, modifier,
-					      has_constraint, pmu_name);
+					      has_constraint);
 	if (ret)
 		return ret;
 
@@ -1570,20 +1364,11 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
 	}
 	pr_debug("Parsing metric events '%s'\n", events.buf);
 	parse_events_error__init(&parse_error);
-	nlist = get_metric_pmus(events.buf, &metric_pmus);
-	if (!nlist) {
-		ret = -ENOMEM;
-		goto err_out;
-	}
-	ret = __parse_events(parsed_evlist, nlist, &parse_error, fake_pmu);
+	ret = __parse_events(parsed_evlist, events.buf, &parse_error, fake_pmu);
 	if (ret) {
 		parse_events_error__print(&parse_error, events.buf);
 		goto err_out;
 	}
-
-	if (metric_pmus.alloc)
-		remove_pmu_umatched_events(parsed_evlist, metric_pmus.buf);
-
 	ret = decode_all_metric_ids(parsed_evlist, modifier);
 	if (ret)
 		goto err_out;
@@ -1591,12 +1376,9 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
 	*out_evlist = parsed_evlist;
 	parsed_evlist = NULL;
 err_out:
-	if (nlist)
-		free(nlist);
 	parse_events_error__exit(&parse_error);
 	evlist__delete(parsed_evlist);
 	strbuf_release(&events);
-	strbuf_release(&metric_pmus);
 	return ret;
 }
 
@@ -1615,8 +1397,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 	if (metric_events_list->nr_entries == 0)
 		metricgroup__rblist_init(metric_events_list);
 	ret = metricgroup__add_metric_list(str, metric_no_group,
-					   &metric_list, map,
-					   perf_evlist->hybrid_pmu_name);
+					   &metric_list, map);
 	if (ret)
 		goto out;
 
@@ -1632,8 +1413,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 			ret = parse_ids(metric_no_merge, fake_pmu, combined,
 					/*modifier=*/NULL,
 					/*has_constraint=*/true,
-					&combined_evlist,
-					perf_evlist->hybrid_pmu_name);
+					&combined_evlist);
 		}
 		if (combined)
 			expr__ctx_free(combined);
@@ -1670,9 +1450,6 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 					continue;
 
 				if (expr__subset_of_ids(n->pctx, m->pctx)) {
-					if (m->pmu_name && n->pmu_name
-					    && strcmp(m->pmu_name, n->pmu_name))
-						continue;
 					pr_debug("Events in '%s' fully contained within '%s'\n",
 						 m->metric_name, n->metric_name);
 					metric_evlist = n->evlist;
@@ -1682,16 +1459,14 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 			}
 		}
 		if (!metric_evlist) {
-			ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
-				      m->modifier, m->has_constraint,
-				      &m->evlist, m->pmu_name);
+			ret = parse_ids(metric_no_merge, fake_pmu, m->pctx, m->modifier,
+					m->has_constraint, &m->evlist);
 			if (ret)
 				goto out;
 
 			metric_evlist = m->evlist;
 		}
-		ret = setup_metric_events(m->pctx->ids, metric_evlist,
-					  &metric_events, m->pmu_name);
+		ret = setup_metric_events(m->pctx->ids, metric_evlist, &metric_events);
 		if (ret) {
 			pr_debug("Cannot resolve IDs for %s: %s\n",
 				m->metric_name, m->metric_expr);
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 13f705737367..98669ca5a86b 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -539,8 +539,7 @@ static void aggr_update_shadow(struct perf_stat_config *config,
 	}
 }
 
-static void uniquify_event_name(struct evsel *counter,
-				struct perf_stat_config *stat_config)
+static void uniquify_event_name(struct evsel *counter)
 {
 	char *new_name;
 	char *config;
@@ -559,8 +558,7 @@ static void uniquify_event_name(struct evsel *counter,
 			counter->name = new_name;
 		}
 	} else {
-		if (perf_pmu__has_hybrid() &&
-		    stat_config->metric_events.nr_entries == 0) {
+		if (perf_pmu__has_hybrid()) {
 			ret = asprintf(&new_name, "%s/%s/",
 				       counter->pmu_name, counter->name);
 		} else {
@@ -634,7 +632,7 @@ static bool collect_data(struct perf_stat_config *config, struct evsel *counter,
 		return false;
 	cb(config, counter, data, true);
 	if (config->no_merge || hybrid_merge(counter, config, false))
-		uniquify_event_name(counter, config);
+		uniquify_event_name(counter);
 	else if (counter->auto_merge_stats || hybrid_merge(counter, config, true))
 		collect_all_aliases(config, counter, cb, data);
 	return true;
-- 
2.36.0.512.ge40c2bad7a-goog

Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

[Arnaldo Carvalho de Melo]

Em Fri, May 06, 2022 at 10:34:06PM -0700, Ian Rogers escreveu:
> This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.

I picked this from the cover letter and added to this revert, to justify
it:

"Hybrid metrics place a PMU at the end of the parse string. This is
also where tool events are placed. The behavior of the parse string
isn't clear and so revert the change for now."

I apologise for not having provided ample time for reviewing this patch,
sorry about that.

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/metricgroup.c  | 263 +++------------------------------
>  tools/perf/util/stat-display.c |   8 +-
>  2 files changed, 22 insertions(+), 249 deletions(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 126a43a8917e..d8492e339521 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -141,11 +141,6 @@ struct metric {
>  	 * output.
>  	 */
>  	const char *metric_unit;
> -	/**
> -	 * The name of the CPU such as "cpu_core" or "cpu_atom" in hybrid systems
> -	 * and "NULL" in non-hybrid systems.
> -	 */
> -	const char *pmu_name;
>  	/** Optional null terminated array of referenced metrics. */
>  	struct metric_ref *metric_refs;
>  	/**
> @@ -220,7 +215,6 @@ static struct metric *metric__new(const struct pmu_event *pe,
>  	}
>  	m->metric_expr = pe->metric_expr;
>  	m->metric_unit = pe->unit;
> -	m->pmu_name = pe->pmu;
>  	m->pctx->runtime = runtime;
>  	m->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
>  	m->metric_refs = NULL;
> @@ -256,12 +250,10 @@ static bool contains_metric_id(struct evsel **metric_events, int num_events,
>   * @ids: the metric IDs to match.
>   * @metric_evlist: the list of perf events.
>   * @out_metric_events: holds the created metric events array.
> - * @pmu_name: the name of the CPU.
>   */
>  static int setup_metric_events(struct hashmap *ids,
>  			       struct evlist *metric_evlist,
> -			       struct evsel ***out_metric_events,
> -			       const char *pmu_name)
> +			       struct evsel ***out_metric_events)
>  {
>  	struct evsel **metric_events;
>  	const char *metric_id;
> @@ -294,10 +286,6 @@ static int setup_metric_events(struct hashmap *ids,
>  		 * about this event.
>  		 */
>  		if (hashmap__find(ids, metric_id, (void **)&val_ptr)) {
> -			if (evsel__is_hybrid(ev) && pmu_name &&
> -			    strcmp(pmu_name, ev->pmu_name)) {
> -				continue;
> -			}
>  			metric_events[matched_events++] = ev;
>  
>  			if (matched_events >= ids_size)
> @@ -736,8 +724,7 @@ static int decode_all_metric_ids(struct evlist *perf_evlist, const char *modifie
>  static int metricgroup__build_event_string(struct strbuf *events,
>  					   const struct expr_parse_ctx *ctx,
>  					   const char *modifier,
> -					   bool has_constraint,
> -					   const char *pmu_name)
> +					   bool has_constraint)
>  {
>  	struct hashmap_entry *cur;
>  	size_t bkt;
> @@ -819,18 +806,12 @@ static int metricgroup__build_event_string(struct strbuf *events,
>  		if (no_group) {
>  			/* Strange case of a metric of just duration_time. */
>  			ret = strbuf_addf(events, "duration_time");
> -		} else if (!has_constraint) {
> -			ret = strbuf_addf(events, "}:W");
> -			if (pmu_name)
> -				ret = strbuf_addf(events, "#%s", pmu_name);
> -			ret = strbuf_addf(events, ",duration_time");
> -		} else
> +		} else if (!has_constraint)
> +			ret = strbuf_addf(events, "}:W,duration_time");
> +		else
>  			ret = strbuf_addf(events, ",duration_time");
> -	} else if (!no_group && !has_constraint) {
> +	} else if (!no_group && !has_constraint)
>  		ret = strbuf_addf(events, "}:W");
> -		if (pmu_name)
> -			ret = strbuf_addf(events, "#%s", pmu_name);
> -	}
>  
>  	return ret;
>  #undef RETURN_IF_NON_ZERO
> @@ -1169,13 +1150,11 @@ static int metric_list_cmp(void *priv __maybe_unused, const struct list_head *l,
>   * @metric_list: The list that the metric or metric group are added to.
>   * @map: The map that is searched for metrics, most commonly the table for the
>   *       architecture perf is running upon.
> - * @pmu_name: the name of the CPU.
>   */
> -static int metricgroup__add_metric(const char *metric_name,
> -				   const char *modifier, bool metric_no_group,
> +static int metricgroup__add_metric(const char *metric_name, const char *modifier,
> +				   bool metric_no_group,
>  				   struct list_head *metric_list,
> -				   const struct pmu_events_map *map,
> -				   const char *pmu_name)
> +				   const struct pmu_events_map *map)
>  {
>  	const struct pmu_event *pe;
>  	LIST_HEAD(list);
> @@ -1188,8 +1167,6 @@ static int metricgroup__add_metric(const char *metric_name,
>  	 */
>  	map_for_each_metric(pe, i, map, metric_name) {
>  		has_match = true;
> -		if (pmu_name && pe->pmu && strcmp(pmu_name, pe->pmu))
> -			continue;
>  		ret = add_metric(&list, pe, modifier, metric_no_group,
>  				 /*root_metric=*/NULL,
>  				 /*visited_metrics=*/NULL, map);
> @@ -1238,12 +1215,10 @@ static int metricgroup__add_metric(const char *metric_name,
>   * @metric_list: The list that metrics are added to.
>   * @map: The map that is searched for metrics, most commonly the table for the
>   *       architecture perf is running upon.
> - * @pmu_name: the name of the CPU.
>   */
>  static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
>  					struct list_head *metric_list,
> -					const struct pmu_events_map *map,
> -					const char *pmu_name)
> +					const struct pmu_events_map *map)
>  {
>  	char *list_itr, *list_copy, *metric_name, *modifier;
>  	int ret, count = 0;
> @@ -1260,7 +1235,7 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
>  
>  		ret = metricgroup__add_metric(metric_name, modifier,
>  					      metric_no_group, metric_list,
> -					      map, pmu_name);
> +					      map);
>  		if (ret == -EINVAL)
>  			pr_err("Cannot find metric or group `%s'\n", metric_name);
>  
> @@ -1335,183 +1310,6 @@ static int build_combined_expr_ctx(const struct list_head *metric_list,
>  	return ret;
>  }
>  
> -static char *get_metric_pmus(char *orig_str, struct strbuf *metric_pmus)
> -{
> -	char *llist, *nlist, *p1, *p2, *new_str = NULL;
> -	int ret;
> -	struct strbuf new_events;
> -
> -	if (!strchr(orig_str, '#')) {
> -		/*
> -		 * pmu name is added after '#'. If no '#' found,
> -		 * don't need to process pmu.
> -		 */
> -		return strdup(orig_str);
> -	}
> -
> -	nlist = strdup(orig_str);
> -	if (!nlist)
> -		return new_str;
> -
> -	ret = strbuf_init(&new_events, 100);
> -	if (ret)
> -		goto err_out;
> -
> -	ret = strbuf_grow(metric_pmus, 100);
> -	if (ret)
> -		goto err_out;
> -
> -	llist = nlist;
> -	while ((p1 = strsep(&llist, ",")) != NULL) {
> -		p2 = strchr(p1, '#');
> -		if (p2) {
> -			*p2 = 0;
> -			ret = strbuf_addf(&new_events, "%s,", p1);
> -			if (ret)
> -				goto err_out;
> -
> -			ret = strbuf_addf(metric_pmus, "%s,", p2 + 1);
> -			if (ret)
> -				goto err_out;
> -
> -		} else {
> -			ret = strbuf_addf(&new_events, "%s,", p1);
> -			if (ret)
> -				goto err_out;
> -		}
> -	}
> -
> -	new_str = strdup(new_events.buf);
> -	if (new_str) {
> -		/* Remove last ',' */
> -		new_str[strlen(new_str) - 1] = 0;
> -	}
> -err_out:
> -	free(nlist);
> -	strbuf_release(&new_events);
> -	return new_str;
> -}
> -
> -static void set_pmu_unmatched_events(struct evlist *evlist, int group_idx,
> -				     char *pmu_name,
> -				     unsigned long *evlist_removed)
> -{
> -	struct evsel *evsel, *pos;
> -	int i = 0, j = 0;
> -
> -	/*
> -	 * Move to the first evsel of a given group
> -	 */
> -	evlist__for_each_entry(evlist, evsel) {
> -		if (evsel__is_group_leader(evsel) &&
> -		    evsel->core.nr_members >= 1) {
> -			if (i < group_idx) {
> -				j += evsel->core.nr_members;
> -				i++;
> -				continue;
> -			}
> -		}
> -	}
> -
> -	i = 0;
> -	evlist__for_each_entry(evlist, evsel) {
> -		if (i < j) {
> -			i++;
> -			continue;
> -		}
> -
> -		/*
> -		 * Now we are at the first evsel in the group
> -		 */
> -		for_each_group_evsel(pos, evsel) {
> -			if (evsel__is_hybrid(pos) &&
> -			    strcmp(pos->pmu_name, pmu_name)) {
> -				set_bit(pos->core.idx, evlist_removed);
> -			}
> -		}
> -		break;
> -	}
> -}
> -
> -static void remove_pmu_umatched_events(struct evlist *evlist, char *metric_pmus)
> -{
> -	struct evsel *evsel, *tmp, *new_leader = NULL;
> -	unsigned long *evlist_removed;
> -	char *llist, *nlist, *p1;
> -	bool need_new_leader = false;
> -	int i = 0, new_nr_members = 0;
> -
> -	nlist = strdup(metric_pmus);
> -	if (!nlist)
> -		return;
> -
> -	evlist_removed = bitmap_zalloc(evlist->core.nr_entries);
> -	if (!evlist_removed) {
> -		free(nlist);
> -		return;
> -	}
> -
> -	llist = nlist;
> -	while ((p1 = strsep(&llist, ",")) != NULL) {
> -		if (strlen(p1) > 0) {
> -			/*
> -			 * p1 points to the string of pmu name, e.g. "cpu_atom".
> -			 * The metric group string has pmu suffixes, e.g.
> -			 * "{inst_retired.any,cpu_clk_unhalted.thread}:W#cpu_core,
> -			 *  {cpu_clk_unhalted.core,inst_retired.any_p}:W#cpu_atom"
> -			 * By counting the pmu name, we can know the index of
> -			 * group.
> -			 */
> -			set_pmu_unmatched_events(evlist, i++, p1,
> -						 evlist_removed);
> -		}
> -	}
> -
> -	evlist__for_each_entry_safe(evlist, tmp, evsel) {
> -		if (test_bit(evsel->core.idx, evlist_removed)) {
> -			if (!evsel__is_group_leader(evsel)) {
> -				if (!need_new_leader) {
> -					if (new_leader)
> -						new_leader->core.leader->nr_members--;
> -					else
> -						evsel->core.leader->nr_members--;
> -				} else
> -					new_nr_members--;
> -			} else {
> -				/*
> -				 * If group leader is to remove, we need to
> -				 * prepare a new leader and adjust all group
> -				 * members.
> -				 */
> -				need_new_leader = true;
> -				new_nr_members =
> -				    evsel->core.leader->nr_members - 1;
> -			}
> -
> -			evlist__remove(evlist, evsel);
> -			evsel__delete(evsel);
> -		} else {
> -			if (!evsel__is_group_leader(evsel)) {
> -				if (need_new_leader) {
> -					need_new_leader = false;
> -					new_leader = evsel;
> -					new_leader->core.leader =
> -					    &new_leader->core;
> -					new_leader->core.nr_members =
> -					    new_nr_members;
> -				} else if (new_leader)
> -					evsel->core.leader = &new_leader->core;
> -			} else {
> -				need_new_leader = false;
> -				new_leader = NULL;
> -			}
> -		}
> -	}
> -
> -	bitmap_free(evlist_removed);
> -	free(nlist);
> -}
> -
>  /**
>   * parse_ids - Build the event string for the ids and parse them creating an
>   *             evlist. The encoded metric_ids are decoded.
> @@ -1521,18 +1319,14 @@ static void remove_pmu_umatched_events(struct evlist *evlist, char *metric_pmus)
>   * @modifier: any modifiers added to the events.
>   * @has_constraint: false if events should be placed in a weak group.
>   * @out_evlist: the created list of events.
> - * @pmu_name: the name of the CPU.
>   */
>  static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>  		     struct expr_parse_ctx *ids, const char *modifier,
> -		     bool has_constraint, struct evlist **out_evlist,
> -		     const char *pmu_name)
> +		     bool has_constraint, struct evlist **out_evlist)
>  {
>  	struct parse_events_error parse_error;
>  	struct evlist *parsed_evlist;
>  	struct strbuf events = STRBUF_INIT;
> -	struct strbuf metric_pmus = STRBUF_INIT;
> -	char *nlist = NULL;
>  	int ret;
>  
>  	*out_evlist = NULL;
> @@ -1559,7 +1353,7 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>  		ids__insert(ids->ids, tmp);
>  	}
>  	ret = metricgroup__build_event_string(&events, ids, modifier,
> -					      has_constraint, pmu_name);
> +					      has_constraint);
>  	if (ret)
>  		return ret;
>  
> @@ -1570,20 +1364,11 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>  	}
>  	pr_debug("Parsing metric events '%s'\n", events.buf);
>  	parse_events_error__init(&parse_error);
> -	nlist = get_metric_pmus(events.buf, &metric_pmus);
> -	if (!nlist) {
> -		ret = -ENOMEM;
> -		goto err_out;
> -	}
> -	ret = __parse_events(parsed_evlist, nlist, &parse_error, fake_pmu);
> +	ret = __parse_events(parsed_evlist, events.buf, &parse_error, fake_pmu);
>  	if (ret) {
>  		parse_events_error__print(&parse_error, events.buf);
>  		goto err_out;
>  	}
> -
> -	if (metric_pmus.alloc)
> -		remove_pmu_umatched_events(parsed_evlist, metric_pmus.buf);
> -
>  	ret = decode_all_metric_ids(parsed_evlist, modifier);
>  	if (ret)
>  		goto err_out;
> @@ -1591,12 +1376,9 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>  	*out_evlist = parsed_evlist;
>  	parsed_evlist = NULL;
>  err_out:
> -	if (nlist)
> -		free(nlist);
>  	parse_events_error__exit(&parse_error);
>  	evlist__delete(parsed_evlist);
>  	strbuf_release(&events);
> -	strbuf_release(&metric_pmus);
>  	return ret;
>  }
>  
> @@ -1615,8 +1397,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>  	if (metric_events_list->nr_entries == 0)
>  		metricgroup__rblist_init(metric_events_list);
>  	ret = metricgroup__add_metric_list(str, metric_no_group,
> -					   &metric_list, map,
> -					   perf_evlist->hybrid_pmu_name);
> +					   &metric_list, map);
>  	if (ret)
>  		goto out;
>  
> @@ -1632,8 +1413,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>  			ret = parse_ids(metric_no_merge, fake_pmu, combined,
>  					/*modifier=*/NULL,
>  					/*has_constraint=*/true,
> -					&combined_evlist,
> -					perf_evlist->hybrid_pmu_name);
> +					&combined_evlist);
>  		}
>  		if (combined)
>  			expr__ctx_free(combined);
> @@ -1670,9 +1450,6 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>  					continue;
>  
>  				if (expr__subset_of_ids(n->pctx, m->pctx)) {
> -					if (m->pmu_name && n->pmu_name
> -					    && strcmp(m->pmu_name, n->pmu_name))
> -						continue;
>  					pr_debug("Events in '%s' fully contained within '%s'\n",
>  						 m->metric_name, n->metric_name);
>  					metric_evlist = n->evlist;
> @@ -1682,16 +1459,14 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>  			}
>  		}
>  		if (!metric_evlist) {
> -			ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
> -				      m->modifier, m->has_constraint,
> -				      &m->evlist, m->pmu_name);
> +			ret = parse_ids(metric_no_merge, fake_pmu, m->pctx, m->modifier,
> +					m->has_constraint, &m->evlist);
>  			if (ret)
>  				goto out;
>  
>  			metric_evlist = m->evlist;
>  		}
> -		ret = setup_metric_events(m->pctx->ids, metric_evlist,
> -					  &metric_events, m->pmu_name);
> +		ret = setup_metric_events(m->pctx->ids, metric_evlist, &metric_events);
>  		if (ret) {
>  			pr_debug("Cannot resolve IDs for %s: %s\n",
>  				m->metric_name, m->metric_expr);
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 13f705737367..98669ca5a86b 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -539,8 +539,7 @@ static void aggr_update_shadow(struct perf_stat_config *config,
>  	}
>  }
>  
> -static void uniquify_event_name(struct evsel *counter,
> -				struct perf_stat_config *stat_config)
> +static void uniquify_event_name(struct evsel *counter)
>  {
>  	char *new_name;
>  	char *config;
> @@ -559,8 +558,7 @@ static void uniquify_event_name(struct evsel *counter,
>  			counter->name = new_name;
>  		}
>  	} else {
> -		if (perf_pmu__has_hybrid() &&
> -		    stat_config->metric_events.nr_entries == 0) {
> +		if (perf_pmu__has_hybrid()) {
>  			ret = asprintf(&new_name, "%s/%s/",
>  				       counter->pmu_name, counter->name);
>  		} else {
> @@ -634,7 +632,7 @@ static bool collect_data(struct perf_stat_config *config, struct evsel *counter,
>  		return false;
>  	cb(config, counter, data, true);
>  	if (config->no_merge || hybrid_merge(counter, config, false))
> -		uniquify_event_name(counter, config);
> +		uniquify_event_name(counter);
>  	else if (counter->auto_merge_stats || hybrid_merge(counter, config, true))
>  		collect_all_aliases(config, counter, cb, data);
>  	return true;
> -- 
> 2.36.0.512.ge40c2bad7a-goog

-- 

- Arnaldo

Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

[Liang, Kan]

On 5/9/2022 9:12 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, May 06, 2022 at 10:34:06PM -0700, Ian Rogers escreveu:
>> This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.
> 
> I picked this from the cover letter and added to this revert, to justify
> it:
> 
> "Hybrid metrics place a PMU at the end of the parse string. This is
> also where tool events are placed. The behavior of the parse string
> isn't clear and so revert the change for now."
>

I think the original patch used a "#" to indicate the PMU name, which 
can be used to distinguish between the tool events and the PMU name.
To be honest, I'm not sure what's unclear here. Could you please clarify?

With this revert, the issue mentioned in the original patch must be 
broken on ADL. I don't see a replacement fix in this patch series.
Could you please propose a solution for the issue to replace the #PMU 
name solution?

Thanks,
Kan

> I apologise for not having provided ample time for reviewing this patch,
> sorry about that.
> 
> - Arnaldo
>   
>> Signed-off-by: Ian Rogers <irogers@google.com>
>> ---
>>   tools/perf/util/metricgroup.c  | 263 +++------------------------------
>>   tools/perf/util/stat-display.c |   8 +-
>>   2 files changed, 22 insertions(+), 249 deletions(-)
>>
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index 126a43a8917e..d8492e339521 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -141,11 +141,6 @@ struct metric {
>>   	 * output.
>>   	 */
>>   	const char *metric_unit;
>> -	/**
>> -	 * The name of the CPU such as "cpu_core" or "cpu_atom" in hybrid systems
>> -	 * and "NULL" in non-hybrid systems.
>> -	 */
>> -	const char *pmu_name;
>>   	/** Optional null terminated array of referenced metrics. */
>>   	struct metric_ref *metric_refs;
>>   	/**
>> @@ -220,7 +215,6 @@ static struct metric *metric__new(const struct pmu_event *pe,
>>   	}
>>   	m->metric_expr = pe->metric_expr;
>>   	m->metric_unit = pe->unit;
>> -	m->pmu_name = pe->pmu;
>>   	m->pctx->runtime = runtime;
>>   	m->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
>>   	m->metric_refs = NULL;
>> @@ -256,12 +250,10 @@ static bool contains_metric_id(struct evsel **metric_events, int num_events,
>>    * @ids: the metric IDs to match.
>>    * @metric_evlist: the list of perf events.
>>    * @out_metric_events: holds the created metric events array.
>> - * @pmu_name: the name of the CPU.
>>    */
>>   static int setup_metric_events(struct hashmap *ids,
>>   			       struct evlist *metric_evlist,
>> -			       struct evsel ***out_metric_events,
>> -			       const char *pmu_name)
>> +			       struct evsel ***out_metric_events)
>>   {
>>   	struct evsel **metric_events;
>>   	const char *metric_id;
>> @@ -294,10 +286,6 @@ static int setup_metric_events(struct hashmap *ids,
>>   		 * about this event.
>>   		 */
>>   		if (hashmap__find(ids, metric_id, (void **)&val_ptr)) {
>> -			if (evsel__is_hybrid(ev) && pmu_name &&
>> -			    strcmp(pmu_name, ev->pmu_name)) {
>> -				continue;
>> -			}
>>   			metric_events[matched_events++] = ev;
>>   
>>   			if (matched_events >= ids_size)
>> @@ -736,8 +724,7 @@ static int decode_all_metric_ids(struct evlist *perf_evlist, const char *modifie
>>   static int metricgroup__build_event_string(struct strbuf *events,
>>   					   const struct expr_parse_ctx *ctx,
>>   					   const char *modifier,
>> -					   bool has_constraint,
>> -					   const char *pmu_name)
>> +					   bool has_constraint)
>>   {
>>   	struct hashmap_entry *cur;
>>   	size_t bkt;
>> @@ -819,18 +806,12 @@ static int metricgroup__build_event_string(struct strbuf *events,
>>   		if (no_group) {
>>   			/* Strange case of a metric of just duration_time. */
>>   			ret = strbuf_addf(events, "duration_time");
>> -		} else if (!has_constraint) {
>> -			ret = strbuf_addf(events, "}:W");
>> -			if (pmu_name)
>> -				ret = strbuf_addf(events, "#%s", pmu_name);
>> -			ret = strbuf_addf(events, ",duration_time");
>> -		} else
>> +		} else if (!has_constraint)
>> +			ret = strbuf_addf(events, "}:W,duration_time");
>> +		else
>>   			ret = strbuf_addf(events, ",duration_time");
>> -	} else if (!no_group && !has_constraint) {
>> +	} else if (!no_group && !has_constraint)
>>   		ret = strbuf_addf(events, "}:W");
>> -		if (pmu_name)
>> -			ret = strbuf_addf(events, "#%s", pmu_name);
>> -	}
>>   
>>   	return ret;
>>   #undef RETURN_IF_NON_ZERO
>> @@ -1169,13 +1150,11 @@ static int metric_list_cmp(void *priv __maybe_unused, const struct list_head *l,
>>    * @metric_list: The list that the metric or metric group are added to.
>>    * @map: The map that is searched for metrics, most commonly the table for the
>>    *       architecture perf is running upon.
>> - * @pmu_name: the name of the CPU.
>>    */
>> -static int metricgroup__add_metric(const char *metric_name,
>> -				   const char *modifier, bool metric_no_group,
>> +static int metricgroup__add_metric(const char *metric_name, const char *modifier,
>> +				   bool metric_no_group,
>>   				   struct list_head *metric_list,
>> -				   const struct pmu_events_map *map,
>> -				   const char *pmu_name)
>> +				   const struct pmu_events_map *map)
>>   {
>>   	const struct pmu_event *pe;
>>   	LIST_HEAD(list);
>> @@ -1188,8 +1167,6 @@ static int metricgroup__add_metric(const char *metric_name,
>>   	 */
>>   	map_for_each_metric(pe, i, map, metric_name) {
>>   		has_match = true;
>> -		if (pmu_name && pe->pmu && strcmp(pmu_name, pe->pmu))
>> -			continue;
>>   		ret = add_metric(&list, pe, modifier, metric_no_group,
>>   				 /*root_metric=*/NULL,
>>   				 /*visited_metrics=*/NULL, map);
>> @@ -1238,12 +1215,10 @@ static int metricgroup__add_metric(const char *metric_name,
>>    * @metric_list: The list that metrics are added to.
>>    * @map: The map that is searched for metrics, most commonly the table for the
>>    *       architecture perf is running upon.
>> - * @pmu_name: the name of the CPU.
>>    */
>>   static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
>>   					struct list_head *metric_list,
>> -					const struct pmu_events_map *map,
>> -					const char *pmu_name)
>> +					const struct pmu_events_map *map)
>>   {
>>   	char *list_itr, *list_copy, *metric_name, *modifier;
>>   	int ret, count = 0;
>> @@ -1260,7 +1235,7 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
>>   
>>   		ret = metricgroup__add_metric(metric_name, modifier,
>>   					      metric_no_group, metric_list,
>> -					      map, pmu_name);
>> +					      map);
>>   		if (ret == -EINVAL)
>>   			pr_err("Cannot find metric or group `%s'\n", metric_name);
>>   
>> @@ -1335,183 +1310,6 @@ static int build_combined_expr_ctx(const struct list_head *metric_list,
>>   	return ret;
>>   }
>>   
>> -static char *get_metric_pmus(char *orig_str, struct strbuf *metric_pmus)
>> -{
>> -	char *llist, *nlist, *p1, *p2, *new_str = NULL;
>> -	int ret;
>> -	struct strbuf new_events;
>> -
>> -	if (!strchr(orig_str, '#')) {
>> -		/*
>> -		 * pmu name is added after '#'. If no '#' found,
>> -		 * don't need to process pmu.
>> -		 */
>> -		return strdup(orig_str);
>> -	}
>> -
>> -	nlist = strdup(orig_str);
>> -	if (!nlist)
>> -		return new_str;
>> -
>> -	ret = strbuf_init(&new_events, 100);
>> -	if (ret)
>> -		goto err_out;
>> -
>> -	ret = strbuf_grow(metric_pmus, 100);
>> -	if (ret)
>> -		goto err_out;
>> -
>> -	llist = nlist;
>> -	while ((p1 = strsep(&llist, ",")) != NULL) {
>> -		p2 = strchr(p1, '#');
>> -		if (p2) {
>> -			*p2 = 0;
>> -			ret = strbuf_addf(&new_events, "%s,", p1);
>> -			if (ret)
>> -				goto err_out;
>> -
>> -			ret = strbuf_addf(metric_pmus, "%s,", p2 + 1);
>> -			if (ret)
>> -				goto err_out;
>> -
>> -		} else {
>> -			ret = strbuf_addf(&new_events, "%s,", p1);
>> -			if (ret)
>> -				goto err_out;
>> -		}
>> -	}
>> -
>> -	new_str = strdup(new_events.buf);
>> -	if (new_str) {
>> -		/* Remove last ',' */
>> -		new_str[strlen(new_str) - 1] = 0;
>> -	}
>> -err_out:
>> -	free(nlist);
>> -	strbuf_release(&new_events);
>> -	return new_str;
>> -}
>> -
>> -static void set_pmu_unmatched_events(struct evlist *evlist, int group_idx,
>> -				     char *pmu_name,
>> -				     unsigned long *evlist_removed)
>> -{
>> -	struct evsel *evsel, *pos;
>> -	int i = 0, j = 0;
>> -
>> -	/*
>> -	 * Move to the first evsel of a given group
>> -	 */
>> -	evlist__for_each_entry(evlist, evsel) {
>> -		if (evsel__is_group_leader(evsel) &&
>> -		    evsel->core.nr_members >= 1) {
>> -			if (i < group_idx) {
>> -				j += evsel->core.nr_members;
>> -				i++;
>> -				continue;
>> -			}
>> -		}
>> -	}
>> -
>> -	i = 0;
>> -	evlist__for_each_entry(evlist, evsel) {
>> -		if (i < j) {
>> -			i++;
>> -			continue;
>> -		}
>> -
>> -		/*
>> -		 * Now we are at the first evsel in the group
>> -		 */
>> -		for_each_group_evsel(pos, evsel) {
>> -			if (evsel__is_hybrid(pos) &&
>> -			    strcmp(pos->pmu_name, pmu_name)) {
>> -				set_bit(pos->core.idx, evlist_removed);
>> -			}
>> -		}
>> -		break;
>> -	}
>> -}
>> -
>> -static void remove_pmu_umatched_events(struct evlist *evlist, char *metric_pmus)
>> -{
>> -	struct evsel *evsel, *tmp, *new_leader = NULL;
>> -	unsigned long *evlist_removed;
>> -	char *llist, *nlist, *p1;
>> -	bool need_new_leader = false;
>> -	int i = 0, new_nr_members = 0;
>> -
>> -	nlist = strdup(metric_pmus);
>> -	if (!nlist)
>> -		return;
>> -
>> -	evlist_removed = bitmap_zalloc(evlist->core.nr_entries);
>> -	if (!evlist_removed) {
>> -		free(nlist);
>> -		return;
>> -	}
>> -
>> -	llist = nlist;
>> -	while ((p1 = strsep(&llist, ",")) != NULL) {
>> -		if (strlen(p1) > 0) {
>> -			/*
>> -			 * p1 points to the string of pmu name, e.g. "cpu_atom".
>> -			 * The metric group string has pmu suffixes, e.g.
>> -			 * "{inst_retired.any,cpu_clk_unhalted.thread}:W#cpu_core,
>> -			 *  {cpu_clk_unhalted.core,inst_retired.any_p}:W#cpu_atom"
>> -			 * By counting the pmu name, we can know the index of
>> -			 * group.
>> -			 */
>> -			set_pmu_unmatched_events(evlist, i++, p1,
>> -						 evlist_removed);
>> -		}
>> -	}
>> -
>> -	evlist__for_each_entry_safe(evlist, tmp, evsel) {
>> -		if (test_bit(evsel->core.idx, evlist_removed)) {
>> -			if (!evsel__is_group_leader(evsel)) {
>> -				if (!need_new_leader) {
>> -					if (new_leader)
>> -						new_leader->core.leader->nr_members--;
>> -					else
>> -						evsel->core.leader->nr_members--;
>> -				} else
>> -					new_nr_members--;
>> -			} else {
>> -				/*
>> -				 * If group leader is to remove, we need to
>> -				 * prepare a new leader and adjust all group
>> -				 * members.
>> -				 */
>> -				need_new_leader = true;
>> -				new_nr_members =
>> -				    evsel->core.leader->nr_members - 1;
>> -			}
>> -
>> -			evlist__remove(evlist, evsel);
>> -			evsel__delete(evsel);
>> -		} else {
>> -			if (!evsel__is_group_leader(evsel)) {
>> -				if (need_new_leader) {
>> -					need_new_leader = false;
>> -					new_leader = evsel;
>> -					new_leader->core.leader =
>> -					    &new_leader->core;
>> -					new_leader->core.nr_members =
>> -					    new_nr_members;
>> -				} else if (new_leader)
>> -					evsel->core.leader = &new_leader->core;
>> -			} else {
>> -				need_new_leader = false;
>> -				new_leader = NULL;
>> -			}
>> -		}
>> -	}
>> -
>> -	bitmap_free(evlist_removed);
>> -	free(nlist);
>> -}
>> -
>>   /**
>>    * parse_ids - Build the event string for the ids and parse them creating an
>>    *             evlist. The encoded metric_ids are decoded.
>> @@ -1521,18 +1319,14 @@ static void remove_pmu_umatched_events(struct evlist *evlist, char *metric_pmus)
>>    * @modifier: any modifiers added to the events.
>>    * @has_constraint: false if events should be placed in a weak group.
>>    * @out_evlist: the created list of events.
>> - * @pmu_name: the name of the CPU.
>>    */
>>   static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>>   		     struct expr_parse_ctx *ids, const char *modifier,
>> -		     bool has_constraint, struct evlist **out_evlist,
>> -		     const char *pmu_name)
>> +		     bool has_constraint, struct evlist **out_evlist)
>>   {
>>   	struct parse_events_error parse_error;
>>   	struct evlist *parsed_evlist;
>>   	struct strbuf events = STRBUF_INIT;
>> -	struct strbuf metric_pmus = STRBUF_INIT;
>> -	char *nlist = NULL;
>>   	int ret;
>>   
>>   	*out_evlist = NULL;
>> @@ -1559,7 +1353,7 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>>   		ids__insert(ids->ids, tmp);
>>   	}
>>   	ret = metricgroup__build_event_string(&events, ids, modifier,
>> -					      has_constraint, pmu_name);
>> +					      has_constraint);
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -1570,20 +1364,11 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>>   	}
>>   	pr_debug("Parsing metric events '%s'\n", events.buf);
>>   	parse_events_error__init(&parse_error);
>> -	nlist = get_metric_pmus(events.buf, &metric_pmus);
>> -	if (!nlist) {
>> -		ret = -ENOMEM;
>> -		goto err_out;
>> -	}
>> -	ret = __parse_events(parsed_evlist, nlist, &parse_error, fake_pmu);
>> +	ret = __parse_events(parsed_evlist, events.buf, &parse_error, fake_pmu);
>>   	if (ret) {
>>   		parse_events_error__print(&parse_error, events.buf);
>>   		goto err_out;
>>   	}
>> -
>> -	if (metric_pmus.alloc)
>> -		remove_pmu_umatched_events(parsed_evlist, metric_pmus.buf);
>> -
>>   	ret = decode_all_metric_ids(parsed_evlist, modifier);
>>   	if (ret)
>>   		goto err_out;
>> @@ -1591,12 +1376,9 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>>   	*out_evlist = parsed_evlist;
>>   	parsed_evlist = NULL;
>>   err_out:
>> -	if (nlist)
>> -		free(nlist);
>>   	parse_events_error__exit(&parse_error);
>>   	evlist__delete(parsed_evlist);
>>   	strbuf_release(&events);
>> -	strbuf_release(&metric_pmus);
>>   	return ret;
>>   }
>>   
>> @@ -1615,8 +1397,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>>   	if (metric_events_list->nr_entries == 0)
>>   		metricgroup__rblist_init(metric_events_list);
>>   	ret = metricgroup__add_metric_list(str, metric_no_group,
>> -					   &metric_list, map,
>> -					   perf_evlist->hybrid_pmu_name);
>> +					   &metric_list, map);
>>   	if (ret)
>>   		goto out;
>>   
>> @@ -1632,8 +1413,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>>   			ret = parse_ids(metric_no_merge, fake_pmu, combined,
>>   					/*modifier=*/NULL,
>>   					/*has_constraint=*/true,
>> -					&combined_evlist,
>> -					perf_evlist->hybrid_pmu_name);
>> +					&combined_evlist);
>>   		}
>>   		if (combined)
>>   			expr__ctx_free(combined);
>> @@ -1670,9 +1450,6 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>>   					continue;
>>   
>>   				if (expr__subset_of_ids(n->pctx, m->pctx)) {
>> -					if (m->pmu_name && n->pmu_name
>> -					    && strcmp(m->pmu_name, n->pmu_name))
>> -						continue;
>>   					pr_debug("Events in '%s' fully contained within '%s'\n",
>>   						 m->metric_name, n->metric_name);
>>   					metric_evlist = n->evlist;
>> @@ -1682,16 +1459,14 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>>   			}
>>   		}
>>   		if (!metric_evlist) {
>> -			ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
>> -				      m->modifier, m->has_constraint,
>> -				      &m->evlist, m->pmu_name);
>> +			ret = parse_ids(metric_no_merge, fake_pmu, m->pctx, m->modifier,
>> +					m->has_constraint, &m->evlist);
>>   			if (ret)
>>   				goto out;
>>   
>>   			metric_evlist = m->evlist;
>>   		}
>> -		ret = setup_metric_events(m->pctx->ids, metric_evlist,
>> -					  &metric_events, m->pmu_name);
>> +		ret = setup_metric_events(m->pctx->ids, metric_evlist, &metric_events);
>>   		if (ret) {
>>   			pr_debug("Cannot resolve IDs for %s: %s\n",
>>   				m->metric_name, m->metric_expr);
>> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
>> index 13f705737367..98669ca5a86b 100644
>> --- a/tools/perf/util/stat-display.c
>> +++ b/tools/perf/util/stat-display.c
>> @@ -539,8 +539,7 @@ static void aggr_update_shadow(struct perf_stat_config *config,
>>   	}
>>   }
>>   
>> -static void uniquify_event_name(struct evsel *counter,
>> -				struct perf_stat_config *stat_config)
>> +static void uniquify_event_name(struct evsel *counter)
>>   {
>>   	char *new_name;
>>   	char *config;
>> @@ -559,8 +558,7 @@ static void uniquify_event_name(struct evsel *counter,
>>   			counter->name = new_name;
>>   		}
>>   	} else {
>> -		if (perf_pmu__has_hybrid() &&
>> -		    stat_config->metric_events.nr_entries == 0) {
>> +		if (perf_pmu__has_hybrid()) {
>>   			ret = asprintf(&new_name, "%s/%s/",
>>   				       counter->pmu_name, counter->name);
>>   		} else {
>> @@ -634,7 +632,7 @@ static bool collect_data(struct perf_stat_config *config, struct evsel *counter,
>>   		return false;
>>   	cb(config, counter, data, true);
>>   	if (config->no_merge || hybrid_merge(counter, config, false))
>> -		uniquify_event_name(counter, config);
>> +		uniquify_event_name(counter);
>>   	else if (counter->auto_merge_stats || hybrid_merge(counter, config, true))
>>   		collect_all_aliases(config, counter, cb, data);
>>   	return true;
>> -- 
>> 2.36.0.512.ge40c2bad7a-goog
> 

Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

[Xing Zhengjun]

On 5/10/2022 5:55 AM, Liang, Kan wrote:
> 
> 
> On 5/9/2022 9:12 AM, Arnaldo Carvalho de Melo wrote:
>> Em Fri, May 06, 2022 at 10:34:06PM -0700, Ian Rogers escreveu:
>>> This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.
>>
>> I picked this from the cover letter and added to this revert, to justify
>> it:
>>
>> "Hybrid metrics place a PMU at the end of the parse string. This is
>> also where tool events are placed. The behavior of the parse string
>> isn't clear and so revert the change for now."
>>
> 
> I think the original patch used a "#" to indicate the PMU name, which 
> can be used to distinguish between the tool events and the PMU name.
> To be honest, I'm not sure what's unclear here. Could you please clarify?
> 
> With this revert, the issue mentioned in the original patch must be 
> broken on ADL. I don't see a replacement fix in this patch series.
> Could you please propose a solution for the issue to replace the #PMU 
> name solution?
> 
> Thanks,
> Kan

I am surprised the origin patch is reverted during discussion and though 
the discussion still has no conclusion.
Let me introduce the purpose of the origin patch.
For a hybrid system such as ADL, if both the metrics and the formula are 
different for the different PMUs, without this patch, the metric and 
event parser should work ok, nothing should be special for the hybrid. 
In fact, both "cpu_core" and "cpu_atom" may have the same metrics--the 
same metric name, even the same formula for the metrics. For example, 
both "cpu_core" and "cpu_atom" have metrics "IpBranch" and "IPC", For 
"IpBranch", both "cpu_core" and "cpu_atom" has the same metric name and 
their formula also is the same, the event name is the same though they 
belong to different PMUs. The old metric and event parser can not handle 
this kind of metric, that's why we need this patch.

     "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
     "MetricName": "IpBranch",
     "Unit": "cpu_core"

     "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
     "MetricName": "IpBranch",
     "Unit": "cpu_atom"


    "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
    "MetricName": "IPC",
    "Unit": "cpu_core"

    "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.CORE",
    "MetricName": "IPC",
    "Unit": "cpu_atom"

Except for the above two metrics, there are still a lot of similar 
metrics, "CPU_Utilization"...

The original patch expanded the metric group string by adding 
"#PMU_name" to indicate the PMU name, which can be used to distinguish 
between the tool events and the PMU name, then the metric and event 
parser can parse the right PMU for the events.

With the patch all the ADL metrics can pass, without the patch, a lot of 
metrics will fail. I don't think it's a good idea to revert it before 
the new solution is proposed.

> 
>> I apologise for not having provided ample time for reviewing this patch,
>> sorry about that.
>>
>> - Arnaldo
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>>   tools/perf/util/metricgroup.c  | 263 +++------------------------------
>>>   tools/perf/util/stat-display.c |   8 +-
>>>   2 files changed, 22 insertions(+), 249 deletions(-)
>>>
>>> diff --git a/tools/perf/util/metricgroup.c 
>>> b/tools/perf/util/metricgroup.c
>>> index 126a43a8917e..d8492e339521 100644
>>> --- a/tools/perf/util/metricgroup.c
>>> +++ b/tools/perf/util/metricgroup.c
>>> @@ -141,11 +141,6 @@ struct metric {
>>>        * output.
>>>        */
>>>       const char *metric_unit;
>>> -    /**
>>> -     * The name of the CPU such as "cpu_core" or "cpu_atom" in 
>>> hybrid systems
>>> -     * and "NULL" in non-hybrid systems.
>>> -     */
>>> -    const char *pmu_name;
>>>       /** Optional null terminated array of referenced metrics. */
>>>       struct metric_ref *metric_refs;
>>>       /**
>>> @@ -220,7 +215,6 @@ static struct metric *metric__new(const struct 
>>> pmu_event *pe,
>>>       }
>>>       m->metric_expr = pe->metric_expr;
>>>       m->metric_unit = pe->unit;
>>> -    m->pmu_name = pe->pmu;
>>>       m->pctx->runtime = runtime;
>>>       m->has_constraint = metric_no_group || 
>>> metricgroup__has_constraint(pe);
>>>       m->metric_refs = NULL;
>>> @@ -256,12 +250,10 @@ static bool contains_metric_id(struct evsel 
>>> **metric_events, int num_events,
>>>    * @ids: the metric IDs to match.
>>>    * @metric_evlist: the list of perf events.
>>>    * @out_metric_events: holds the created metric events array.
>>> - * @pmu_name: the name of the CPU.
>>>    */
>>>   static int setup_metric_events(struct hashmap *ids,
>>>                      struct evlist *metric_evlist,
>>> -                   struct evsel ***out_metric_events,
>>> -                   const char *pmu_name)
>>> +                   struct evsel ***out_metric_events)
>>>   {
>>>       struct evsel **metric_events;
>>>       const char *metric_id;
>>> @@ -294,10 +286,6 @@ static int setup_metric_events(struct hashmap *ids,
>>>            * about this event.
>>>            */
>>>           if (hashmap__find(ids, metric_id, (void **)&val_ptr)) {
>>> -            if (evsel__is_hybrid(ev) && pmu_name &&
>>> -                strcmp(pmu_name, ev->pmu_name)) {
>>> -                continue;
>>> -            }
>>>               metric_events[matched_events++] = ev;
>>>               if (matched_events >= ids_size)
>>> @@ -736,8 +724,7 @@ static int decode_all_metric_ids(struct evlist 
>>> *perf_evlist, const char *modifie
>>>   static int metricgroup__build_event_string(struct strbuf *events,
>>>                          const struct expr_parse_ctx *ctx,
>>>                          const char *modifier,
>>> -                       bool has_constraint,
>>> -                       const char *pmu_name)
>>> +                       bool has_constraint)
>>>   {
>>>       struct hashmap_entry *cur;
>>>       size_t bkt;
>>> @@ -819,18 +806,12 @@ static int 
>>> metricgroup__build_event_string(struct strbuf *events,
>>>           if (no_group) {
>>>               /* Strange case of a metric of just duration_time. */
>>>               ret = strbuf_addf(events, "duration_time");
>>> -        } else if (!has_constraint) {
>>> -            ret = strbuf_addf(events, "}:W");
>>> -            if (pmu_name)
>>> -                ret = strbuf_addf(events, "#%s", pmu_name);
>>> -            ret = strbuf_addf(events, ",duration_time");
>>> -        } else
>>> +        } else if (!has_constraint)
>>> +            ret = strbuf_addf(events, "}:W,duration_time");
>>> +        else
>>>               ret = strbuf_addf(events, ",duration_time");
>>> -    } else if (!no_group && !has_constraint) {
>>> +    } else if (!no_group && !has_constraint)
>>>           ret = strbuf_addf(events, "}:W");
>>> -        if (pmu_name)
>>> -            ret = strbuf_addf(events, "#%s", pmu_name);
>>> -    }
>>>       return ret;
>>>   #undef RETURN_IF_NON_ZERO
>>> @@ -1169,13 +1150,11 @@ static int metric_list_cmp(void *priv 
>>> __maybe_unused, const struct list_head *l,
>>>    * @metric_list: The list that the metric or metric group are added 
>>> to.
>>>    * @map: The map that is searched for metrics, most commonly the 
>>> table for the
>>>    *       architecture perf is running upon.
>>> - * @pmu_name: the name of the CPU.
>>>    */
>>> -static int metricgroup__add_metric(const char *metric_name,
>>> -                   const char *modifier, bool metric_no_group,
>>> +static int metricgroup__add_metric(const char *metric_name, const 
>>> char *modifier,
>>> +                   bool metric_no_group,
>>>                      struct list_head *metric_list,
>>> -                   const struct pmu_events_map *map,
>>> -                   const char *pmu_name)
>>> +                   const struct pmu_events_map *map)
>>>   {
>>>       const struct pmu_event *pe;
>>>       LIST_HEAD(list);
>>> @@ -1188,8 +1167,6 @@ static int metricgroup__add_metric(const char 
>>> *metric_name,
>>>        */
>>>       map_for_each_metric(pe, i, map, metric_name) {
>>>           has_match = true;
>>> -        if (pmu_name && pe->pmu && strcmp(pmu_name, pe->pmu))
>>> -            continue;
>>>           ret = add_metric(&list, pe, modifier, metric_no_group,
>>>                    /*root_metric=*/NULL,
>>>                    /*visited_metrics=*/NULL, map);
>>> @@ -1238,12 +1215,10 @@ static int metricgroup__add_metric(const char 
>>> *metric_name,
>>>    * @metric_list: The list that metrics are added to.
>>>    * @map: The map that is searched for metrics, most commonly the 
>>> table for the
>>>    *       architecture perf is running upon.
>>> - * @pmu_name: the name of the CPU.
>>>    */
>>>   static int metricgroup__add_metric_list(const char *list, bool 
>>> metric_no_group,
>>>                       struct list_head *metric_list,
>>> -                    const struct pmu_events_map *map,
>>> -                    const char *pmu_name)
>>> +                    const struct pmu_events_map *map)
>>>   {
>>>       char *list_itr, *list_copy, *metric_name, *modifier;
>>>       int ret, count = 0;
>>> @@ -1260,7 +1235,7 @@ static int metricgroup__add_metric_list(const 
>>> char *list, bool metric_no_group,
>>>           ret = metricgroup__add_metric(metric_name, modifier,
>>>                             metric_no_group, metric_list,
>>> -                          map, pmu_name);
>>> +                          map);
>>>           if (ret == -EINVAL)
>>>               pr_err("Cannot find metric or group `%s'\n", metric_name);
>>> @@ -1335,183 +1310,6 @@ static int build_combined_expr_ctx(const 
>>> struct list_head *metric_list,
>>>       return ret;
>>>   }
>>> -static char *get_metric_pmus(char *orig_str, struct strbuf 
>>> *metric_pmus)
>>> -{
>>> -    char *llist, *nlist, *p1, *p2, *new_str = NULL;
>>> -    int ret;
>>> -    struct strbuf new_events;
>>> -
>>> -    if (!strchr(orig_str, '#')) {
>>> -        /*
>>> -         * pmu name is added after '#'. If no '#' found,
>>> -         * don't need to process pmu.
>>> -         */
>>> -        return strdup(orig_str);
>>> -    }
>>> -
>>> -    nlist = strdup(orig_str);
>>> -    if (!nlist)
>>> -        return new_str;
>>> -
>>> -    ret = strbuf_init(&new_events, 100);
>>> -    if (ret)
>>> -        goto err_out;
>>> -
>>> -    ret = strbuf_grow(metric_pmus, 100);
>>> -    if (ret)
>>> -        goto err_out;
>>> -
>>> -    llist = nlist;
>>> -    while ((p1 = strsep(&llist, ",")) != NULL) {
>>> -        p2 = strchr(p1, '#');
>>> -        if (p2) {
>>> -            *p2 = 0;
>>> -            ret = strbuf_addf(&new_events, "%s,", p1);
>>> -            if (ret)
>>> -                goto err_out;
>>> -
>>> -            ret = strbuf_addf(metric_pmus, "%s,", p2 + 1);
>>> -            if (ret)
>>> -                goto err_out;
>>> -
>>> -        } else {
>>> -            ret = strbuf_addf(&new_events, "%s,", p1);
>>> -            if (ret)
>>> -                goto err_out;
>>> -        }
>>> -    }
>>> -
>>> -    new_str = strdup(new_events.buf);
>>> -    if (new_str) {
>>> -        /* Remove last ',' */
>>> -        new_str[strlen(new_str) - 1] = 0;
>>> -    }
>>> -err_out:
>>> -    free(nlist);
>>> -    strbuf_release(&new_events);
>>> -    return new_str;
>>> -}
>>> -
>>> -static void set_pmu_unmatched_events(struct evlist *evlist, int 
>>> group_idx,
>>> -                     char *pmu_name,
>>> -                     unsigned long *evlist_removed)
>>> -{
>>> -    struct evsel *evsel, *pos;
>>> -    int i = 0, j = 0;
>>> -
>>> -    /*
>>> -     * Move to the first evsel of a given group
>>> -     */
>>> -    evlist__for_each_entry(evlist, evsel) {
>>> -        if (evsel__is_group_leader(evsel) &&
>>> -            evsel->core.nr_members >= 1) {
>>> -            if (i < group_idx) {
>>> -                j += evsel->core.nr_members;
>>> -                i++;
>>> -                continue;
>>> -            }
>>> -        }
>>> -    }
>>> -
>>> -    i = 0;
>>> -    evlist__for_each_entry(evlist, evsel) {
>>> -        if (i < j) {
>>> -            i++;
>>> -            continue;
>>> -        }
>>> -
>>> -        /*
>>> -         * Now we are at the first evsel in the group
>>> -         */
>>> -        for_each_group_evsel(pos, evsel) {
>>> -            if (evsel__is_hybrid(pos) &&
>>> -                strcmp(pos->pmu_name, pmu_name)) {
>>> -                set_bit(pos->core.idx, evlist_removed);
>>> -            }
>>> -        }
>>> -        break;
>>> -    }
>>> -}
>>> -
>>> -static void remove_pmu_umatched_events(struct evlist *evlist, char 
>>> *metric_pmus)
>>> -{
>>> -    struct evsel *evsel, *tmp, *new_leader = NULL;
>>> -    unsigned long *evlist_removed;
>>> -    char *llist, *nlist, *p1;
>>> -    bool need_new_leader = false;
>>> -    int i = 0, new_nr_members = 0;
>>> -
>>> -    nlist = strdup(metric_pmus);
>>> -    if (!nlist)
>>> -        return;
>>> -
>>> -    evlist_removed = bitmap_zalloc(evlist->core.nr_entries);
>>> -    if (!evlist_removed) {
>>> -        free(nlist);
>>> -        return;
>>> -    }
>>> -
>>> -    llist = nlist;
>>> -    while ((p1 = strsep(&llist, ",")) != NULL) {
>>> -        if (strlen(p1) > 0) {
>>> -            /*
>>> -             * p1 points to the string of pmu name, e.g. "cpu_atom".
>>> -             * The metric group string has pmu suffixes, e.g.
>>> -             * "{inst_retired.any,cpu_clk_unhalted.thread}:W#cpu_core,
>>> -             *  {cpu_clk_unhalted.core,inst_retired.any_p}:W#cpu_atom"
>>> -             * By counting the pmu name, we can know the index of
>>> -             * group.
>>> -             */
>>> -            set_pmu_unmatched_events(evlist, i++, p1,
>>> -                         evlist_removed);
>>> -        }
>>> -    }
>>> -
>>> -    evlist__for_each_entry_safe(evlist, tmp, evsel) {
>>> -        if (test_bit(evsel->core.idx, evlist_removed)) {
>>> -            if (!evsel__is_group_leader(evsel)) {
>>> -                if (!need_new_leader) {
>>> -                    if (new_leader)
>>> -                        new_leader->core.leader->nr_members--;
>>> -                    else
>>> -                        evsel->core.leader->nr_members--;
>>> -                } else
>>> -                    new_nr_members--;
>>> -            } else {
>>> -                /*
>>> -                 * If group leader is to remove, we need to
>>> -                 * prepare a new leader and adjust all group
>>> -                 * members.
>>> -                 */
>>> -                need_new_leader = true;
>>> -                new_nr_members =
>>> -                    evsel->core.leader->nr_members - 1;
>>> -            }
>>> -
>>> -            evlist__remove(evlist, evsel);
>>> -            evsel__delete(evsel);
>>> -        } else {
>>> -            if (!evsel__is_group_leader(evsel)) {
>>> -                if (need_new_leader) {
>>> -                    need_new_leader = false;
>>> -                    new_leader = evsel;
>>> -                    new_leader->core.leader =
>>> -                        &new_leader->core;
>>> -                    new_leader->core.nr_members =
>>> -                        new_nr_members;
>>> -                } else if (new_leader)
>>> -                    evsel->core.leader = &new_leader->core;
>>> -            } else {
>>> -                need_new_leader = false;
>>> -                new_leader = NULL;
>>> -            }
>>> -        }
>>> -    }
>>> -
>>> -    bitmap_free(evlist_removed);
>>> -    free(nlist);
>>> -}
>>> -
>>>   /**
>>>    * parse_ids - Build the event string for the ids and parse them 
>>> creating an
>>>    *             evlist. The encoded metric_ids are decoded.
>>> @@ -1521,18 +1319,14 @@ static void remove_pmu_umatched_events(struct 
>>> evlist *evlist, char *metric_pmus)
>>>    * @modifier: any modifiers added to the events.
>>>    * @has_constraint: false if events should be placed in a weak group.
>>>    * @out_evlist: the created list of events.
>>> - * @pmu_name: the name of the CPU.
>>>    */
>>>   static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>>>                struct expr_parse_ctx *ids, const char *modifier,
>>> -             bool has_constraint, struct evlist **out_evlist,
>>> -             const char *pmu_name)
>>> +             bool has_constraint, struct evlist **out_evlist)
>>>   {
>>>       struct parse_events_error parse_error;
>>>       struct evlist *parsed_evlist;
>>>       struct strbuf events = STRBUF_INIT;
>>> -    struct strbuf metric_pmus = STRBUF_INIT;
>>> -    char *nlist = NULL;
>>>       int ret;
>>>       *out_evlist = NULL;
>>> @@ -1559,7 +1353,7 @@ static int parse_ids(bool metric_no_merge, 
>>> struct perf_pmu *fake_pmu,
>>>           ids__insert(ids->ids, tmp);
>>>       }
>>>       ret = metricgroup__build_event_string(&events, ids, modifier,
>>> -                          has_constraint, pmu_name);
>>> +                          has_constraint);
>>>       if (ret)
>>>           return ret;
>>> @@ -1570,20 +1364,11 @@ static int parse_ids(bool metric_no_merge, 
>>> struct perf_pmu *fake_pmu,
>>>       }
>>>       pr_debug("Parsing metric events '%s'\n", events.buf);
>>>       parse_events_error__init(&parse_error);
>>> -    nlist = get_metric_pmus(events.buf, &metric_pmus);
>>> -    if (!nlist) {
>>> -        ret = -ENOMEM;
>>> -        goto err_out;
>>> -    }
>>> -    ret = __parse_events(parsed_evlist, nlist, &parse_error, fake_pmu);
>>> +    ret = __parse_events(parsed_evlist, events.buf, &parse_error, 
>>> fake_pmu);
>>>       if (ret) {
>>>           parse_events_error__print(&parse_error, events.buf);
>>>           goto err_out;
>>>       }
>>> -
>>> -    if (metric_pmus.alloc)
>>> -        remove_pmu_umatched_events(parsed_evlist, metric_pmus.buf);
>>> -
>>>       ret = decode_all_metric_ids(parsed_evlist, modifier);
>>>       if (ret)
>>>           goto err_out;
>>> @@ -1591,12 +1376,9 @@ static int parse_ids(bool metric_no_merge, 
>>> struct perf_pmu *fake_pmu,
>>>       *out_evlist = parsed_evlist;
>>>       parsed_evlist = NULL;
>>>   err_out:
>>> -    if (nlist)
>>> -        free(nlist);
>>>       parse_events_error__exit(&parse_error);
>>>       evlist__delete(parsed_evlist);
>>>       strbuf_release(&events);
>>> -    strbuf_release(&metric_pmus);
>>>       return ret;
>>>   }
>>> @@ -1615,8 +1397,7 @@ static int parse_groups(struct evlist 
>>> *perf_evlist, const char *str,
>>>       if (metric_events_list->nr_entries == 0)
>>>           metricgroup__rblist_init(metric_events_list);
>>>       ret = metricgroup__add_metric_list(str, metric_no_group,
>>> -                       &metric_list, map,
>>> -                       perf_evlist->hybrid_pmu_name);
>>> +                       &metric_list, map);
>>>       if (ret)
>>>           goto out;
>>> @@ -1632,8 +1413,7 @@ static int parse_groups(struct evlist 
>>> *perf_evlist, const char *str,
>>>               ret = parse_ids(metric_no_merge, fake_pmu, combined,
>>>                       /*modifier=*/NULL,
>>>                       /*has_constraint=*/true,
>>> -                    &combined_evlist,
>>> -                    perf_evlist->hybrid_pmu_name);
>>> +                    &combined_evlist);
>>>           }
>>>           if (combined)
>>>               expr__ctx_free(combined);
>>> @@ -1670,9 +1450,6 @@ static int parse_groups(struct evlist 
>>> *perf_evlist, const char *str,
>>>                       continue;
>>>                   if (expr__subset_of_ids(n->pctx, m->pctx)) {
>>> -                    if (m->pmu_name && n->pmu_name
>>> -                        && strcmp(m->pmu_name, n->pmu_name))
>>> -                        continue;
>>>                       pr_debug("Events in '%s' fully contained within 
>>> '%s'\n",
>>>                            m->metric_name, n->metric_name);
>>>                       metric_evlist = n->evlist;
>>> @@ -1682,16 +1459,14 @@ static int parse_groups(struct evlist 
>>> *perf_evlist, const char *str,
>>>               }
>>>           }
>>>           if (!metric_evlist) {
>>> -            ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
>>> -                      m->modifier, m->has_constraint,
>>> -                      &m->evlist, m->pmu_name);
>>> +            ret = parse_ids(metric_no_merge, fake_pmu, m->pctx, 
>>> m->modifier,
>>> +                    m->has_constraint, &m->evlist);
>>>               if (ret)
>>>                   goto out;
>>>               metric_evlist = m->evlist;
>>>           }
>>> -        ret = setup_metric_events(m->pctx->ids, metric_evlist,
>>> -                      &metric_events, m->pmu_name);
>>> +        ret = setup_metric_events(m->pctx->ids, metric_evlist, 
>>> &metric_events);
>>>           if (ret) {
>>>               pr_debug("Cannot resolve IDs for %s: %s\n",
>>>                   m->metric_name, m->metric_expr);
>>> diff --git a/tools/perf/util/stat-display.c 
>>> b/tools/perf/util/stat-display.c
>>> index 13f705737367..98669ca5a86b 100644
>>> --- a/tools/perf/util/stat-display.c
>>> +++ b/tools/perf/util/stat-display.c
>>> @@ -539,8 +539,7 @@ static void aggr_update_shadow(struct 
>>> perf_stat_config *config,
>>>       }
>>>   }
>>> -static void uniquify_event_name(struct evsel *counter,
>>> -                struct perf_stat_config *stat_config)
>>> +static void uniquify_event_name(struct evsel *counter)
>>>   {
>>>       char *new_name;
>>>       char *config;
>>> @@ -559,8 +558,7 @@ static void uniquify_event_name(struct evsel 
>>> *counter,
>>>               counter->name = new_name;
>>>           }
>>>       } else {
>>> -        if (perf_pmu__has_hybrid() &&
>>> -            stat_config->metric_events.nr_entries == 0) {
>>> +        if (perf_pmu__has_hybrid()) {
>>>               ret = asprintf(&new_name, "%s/%s/",
>>>                          counter->pmu_name, counter->name);
>>>           } else {
>>> @@ -634,7 +632,7 @@ static bool collect_data(struct perf_stat_config 
>>> *config, struct evsel *counter,
>>>           return false;
>>>       cb(config, counter, data, true);
>>>       if (config->no_merge || hybrid_merge(counter, config, false))
>>> -        uniquify_event_name(counter, config);
>>> +        uniquify_event_name(counter);
>>>       else if (counter->auto_merge_stats || hybrid_merge(counter, 
>>> config, true))
>>>           collect_all_aliases(config, counter, cb, data);
>>>       return true;
>>> -- 
>>> 2.36.0.512.ge40c2bad7a-goog
>>

-- 
Zhengjun Xing

Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

[Arnaldo Carvalho de Melo]

Em Tue, May 10, 2022 at 05:31:37PM +0800, Xing Zhengjun escreveu:
> On 5/10/2022 5:55 AM, Liang, Kan wrote:
> > On 5/9/2022 9:12 AM, Arnaldo Carvalho de Melo wrote:
> > > Em Fri, May 06, 2022 at 10:34:06PM -0700, Ian Rogers escreveu:
> > > > This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.

> > > I picked this from the cover letter and added to this revert, to justify
> > > it:

> > > "Hybrid metrics place a PMU at the end of the parse string. This is
> > > also where tool events are placed. The behavior of the parse string
> > > isn't clear and so revert the change for now."

> > I think the original patch used a "#" to indicate the PMU name, which
> > can be used to distinguish between the tool events and the PMU name.
> > To be honest, I'm not sure what's unclear here. Could you please clarify?

> > With this revert, the issue mentioned in the original patch must be
> > broken on ADL. I don't see a replacement fix in this patch series.
> > Could you please propose a solution for the issue to replace the #PMU
> > name solution?

> > Thanks,
> > Kan
 
> I am surprised the origin patch is reverted during discussion and though the
> discussion still has no conclusion.

Yeah, the situation is not a good one, it was my mistake to merge the
patch in the first place without giving time for people like Ian, that
works a lot in the metrics code to voice his concerns, to review it.

So I decided to revert to give more time for interested parties to
discuss it further, again, I'm sorry about that.

- Arnaldo

> Let me introduce the purpose of the origin patch.
> For a hybrid system such as ADL, if both the metrics and the formula are
> different for the different PMUs, without this patch, the metric and event
> parser should work ok, nothing should be special for the hybrid. In fact,
> both "cpu_core" and "cpu_atom" may have the same metrics--the same metric
> name, even the same formula for the metrics. For example, both "cpu_core"
> and "cpu_atom" have metrics "IpBranch" and "IPC", For "IpBranch", both
> "cpu_core" and "cpu_atom" has the same metric name and their formula also is
> the same, the event name is the same though they belong to different PMUs.
> The old metric and event parser can not handle this kind of metric, that's
> why we need this patch.
> 
>     "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
>     "MetricName": "IpBranch",
>     "Unit": "cpu_core"
> 
>     "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
>     "MetricName": "IpBranch",
>     "Unit": "cpu_atom"
> 
> 
>    "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
>    "MetricName": "IPC",
>    "Unit": "cpu_core"
> 
>    "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.CORE",
>    "MetricName": "IPC",
>    "Unit": "cpu_atom"
> 
> Except for the above two metrics, there are still a lot of similar metrics,
> "CPU_Utilization"...
> 
> The original patch expanded the metric group string by adding "#PMU_name" to
> indicate the PMU name, which can be used to distinguish between the tool
> events and the PMU name, then the metric and event parser can parse the
> right PMU for the events.
> 
> With the patch all the ADL metrics can pass, without the patch, a lot of
> metrics will fail. I don't think it's a good idea to revert it before the
> new solution is proposed.
> 
> > 
> > > I apologise for not having provided ample time for reviewing this patch,
> > > sorry about that.
> > > 
> > > - Arnaldo
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >   tools/perf/util/metricgroup.c  | 263 +++------------------------------
> > > >   tools/perf/util/stat-display.c |   8 +-
> > > >   2 files changed, 22 insertions(+), 249 deletions(-)
> > > > 
> > > > diff --git a/tools/perf/util/metricgroup.c
> > > > b/tools/perf/util/metricgroup.c
> > > > index 126a43a8917e..d8492e339521 100644
> > > > --- a/tools/perf/util/metricgroup.c
> > > > +++ b/tools/perf/util/metricgroup.c
> > > > @@ -141,11 +141,6 @@ struct metric {
> > > >        * output.
> > > >        */
> > > >       const char *metric_unit;
> > > > -    /**
> > > > -     * The name of the CPU such as "cpu_core" or "cpu_atom" in
> > > > hybrid systems
> > > > -     * and "NULL" in non-hybrid systems.
> > > > -     */
> > > > -    const char *pmu_name;
> > > >       /** Optional null terminated array of referenced metrics. */
> > > >       struct metric_ref *metric_refs;
> > > >       /**
> > > > @@ -220,7 +215,6 @@ static struct metric *metric__new(const
> > > > struct pmu_event *pe,
> > > >       }
> > > >       m->metric_expr = pe->metric_expr;
> > > >       m->metric_unit = pe->unit;
> > > > -    m->pmu_name = pe->pmu;
> > > >       m->pctx->runtime = runtime;
> > > >       m->has_constraint = metric_no_group ||
> > > > metricgroup__has_constraint(pe);
> > > >       m->metric_refs = NULL;
> > > > @@ -256,12 +250,10 @@ static bool contains_metric_id(struct
> > > > evsel **metric_events, int num_events,
> > > >    * @ids: the metric IDs to match.
> > > >    * @metric_evlist: the list of perf events.
> > > >    * @out_metric_events: holds the created metric events array.
> > > > - * @pmu_name: the name of the CPU.
> > > >    */
> > > >   static int setup_metric_events(struct hashmap *ids,
> > > >                      struct evlist *metric_evlist,
> > > > -                   struct evsel ***out_metric_events,
> > > > -                   const char *pmu_name)
> > > > +                   struct evsel ***out_metric_events)
> > > >   {
> > > >       struct evsel **metric_events;
> > > >       const char *metric_id;
> > > > @@ -294,10 +286,6 @@ static int setup_metric_events(struct hashmap *ids,
> > > >            * about this event.
> > > >            */
> > > >           if (hashmap__find(ids, metric_id, (void **)&val_ptr)) {
> > > > -            if (evsel__is_hybrid(ev) && pmu_name &&
> > > > -                strcmp(pmu_name, ev->pmu_name)) {
> > > > -                continue;
> > > > -            }
> > > >               metric_events[matched_events++] = ev;
> > > >               if (matched_events >= ids_size)
> > > > @@ -736,8 +724,7 @@ static int decode_all_metric_ids(struct
> > > > evlist *perf_evlist, const char *modifie
> > > >   static int metricgroup__build_event_string(struct strbuf *events,
> > > >                          const struct expr_parse_ctx *ctx,
> > > >                          const char *modifier,
> > > > -                       bool has_constraint,
> > > > -                       const char *pmu_name)
> > > > +                       bool has_constraint)
> > > >   {
> > > >       struct hashmap_entry *cur;
> > > >       size_t bkt;
> > > > @@ -819,18 +806,12 @@ static int
> > > > metricgroup__build_event_string(struct strbuf *events,
> > > >           if (no_group) {
> > > >               /* Strange case of a metric of just duration_time. */
> > > >               ret = strbuf_addf(events, "duration_time");
> > > > -        } else if (!has_constraint) {
> > > > -            ret = strbuf_addf(events, "}:W");
> > > > -            if (pmu_name)
> > > > -                ret = strbuf_addf(events, "#%s", pmu_name);
> > > > -            ret = strbuf_addf(events, ",duration_time");
> > > > -        } else
> > > > +        } else if (!has_constraint)
> > > > +            ret = strbuf_addf(events, "}:W,duration_time");
> > > > +        else
> > > >               ret = strbuf_addf(events, ",duration_time");
> > > > -    } else if (!no_group && !has_constraint) {
> > > > +    } else if (!no_group && !has_constraint)
> > > >           ret = strbuf_addf(events, "}:W");
> > > > -        if (pmu_name)
> > > > -            ret = strbuf_addf(events, "#%s", pmu_name);
> > > > -    }
> > > >       return ret;
> > > >   #undef RETURN_IF_NON_ZERO
> > > > @@ -1169,13 +1150,11 @@ static int metric_list_cmp(void *priv
> > > > __maybe_unused, const struct list_head *l,
> > > >    * @metric_list: The list that the metric or metric group are
> > > > added to.
> > > >    * @map: The map that is searched for metrics, most commonly
> > > > the table for the
> > > >    *       architecture perf is running upon.
> > > > - * @pmu_name: the name of the CPU.
> > > >    */
> > > > -static int metricgroup__add_metric(const char *metric_name,
> > > > -                   const char *modifier, bool metric_no_group,
> > > > +static int metricgroup__add_metric(const char *metric_name,
> > > > const char *modifier,
> > > > +                   bool metric_no_group,
> > > >                      struct list_head *metric_list,
> > > > -                   const struct pmu_events_map *map,
> > > > -                   const char *pmu_name)
> > > > +                   const struct pmu_events_map *map)
> > > >   {
> > > >       const struct pmu_event *pe;
> > > >       LIST_HEAD(list);
> > > > @@ -1188,8 +1167,6 @@ static int metricgroup__add_metric(const
> > > > char *metric_name,
> > > >        */
> > > >       map_for_each_metric(pe, i, map, metric_name) {
> > > >           has_match = true;
> > > > -        if (pmu_name && pe->pmu && strcmp(pmu_name, pe->pmu))
> > > > -            continue;
> > > >           ret = add_metric(&list, pe, modifier, metric_no_group,
> > > >                    /*root_metric=*/NULL,
> > > >                    /*visited_metrics=*/NULL, map);
> > > > @@ -1238,12 +1215,10 @@ static int metricgroup__add_metric(const
> > > > char *metric_name,
> > > >    * @metric_list: The list that metrics are added to.
> > > >    * @map: The map that is searched for metrics, most commonly
> > > > the table for the
> > > >    *       architecture perf is running upon.
> > > > - * @pmu_name: the name of the CPU.
> > > >    */
> > > >   static int metricgroup__add_metric_list(const char *list, bool
> > > > metric_no_group,
> > > >                       struct list_head *metric_list,
> > > > -                    const struct pmu_events_map *map,
> > > > -                    const char *pmu_name)
> > > > +                    const struct pmu_events_map *map)
> > > >   {
> > > >       char *list_itr, *list_copy, *metric_name, *modifier;
> > > >       int ret, count = 0;
> > > > @@ -1260,7 +1235,7 @@ static int
> > > > metricgroup__add_metric_list(const char *list, bool
> > > > metric_no_group,
> > > >           ret = metricgroup__add_metric(metric_name, modifier,
> > > >                             metric_no_group, metric_list,
> > > > -                          map, pmu_name);
> > > > +                          map);
> > > >           if (ret == -EINVAL)
> > > >               pr_err("Cannot find metric or group `%s'\n", metric_name);
> > > > @@ -1335,183 +1310,6 @@ static int build_combined_expr_ctx(const
> > > > struct list_head *metric_list,
> > > >       return ret;
> > > >   }
> > > > -static char *get_metric_pmus(char *orig_str, struct strbuf
> > > > *metric_pmus)
> > > > -{
> > > > -    char *llist, *nlist, *p1, *p2, *new_str = NULL;
> > > > -    int ret;
> > > > -    struct strbuf new_events;
> > > > -
> > > > -    if (!strchr(orig_str, '#')) {
> > > > -        /*
> > > > -         * pmu name is added after '#'. If no '#' found,
> > > > -         * don't need to process pmu.
> > > > -         */
> > > > -        return strdup(orig_str);
> > > > -    }
> > > > -
> > > > -    nlist = strdup(orig_str);
> > > > -    if (!nlist)
> > > > -        return new_str;
> > > > -
> > > > -    ret = strbuf_init(&new_events, 100);
> > > > -    if (ret)
> > > > -        goto err_out;
> > > > -
> > > > -    ret = strbuf_grow(metric_pmus, 100);
> > > > -    if (ret)
> > > > -        goto err_out;
> > > > -
> > > > -    llist = nlist;
> > > > -    while ((p1 = strsep(&llist, ",")) != NULL) {
> > > > -        p2 = strchr(p1, '#');
> > > > -        if (p2) {
> > > > -            *p2 = 0;
> > > > -            ret = strbuf_addf(&new_events, "%s,", p1);
> > > > -            if (ret)
> > > > -                goto err_out;
> > > > -
> > > > -            ret = strbuf_addf(metric_pmus, "%s,", p2 + 1);
> > > > -            if (ret)
> > > > -                goto err_out;
> > > > -
> > > > -        } else {
> > > > -            ret = strbuf_addf(&new_events, "%s,", p1);
> > > > -            if (ret)
> > > > -                goto err_out;
> > > > -        }
> > > > -    }
> > > > -
> > > > -    new_str = strdup(new_events.buf);
> > > > -    if (new_str) {
> > > > -        /* Remove last ',' */
> > > > -        new_str[strlen(new_str) - 1] = 0;
> > > > -    }
> > > > -err_out:
> > > > -    free(nlist);
> > > > -    strbuf_release(&new_events);
> > > > -    return new_str;
> > > > -}
> > > > -
> > > > -static void set_pmu_unmatched_events(struct evlist *evlist, int
> > > > group_idx,
> > > > -                     char *pmu_name,
> > > > -                     unsigned long *evlist_removed)
> > > > -{
> > > > -    struct evsel *evsel, *pos;
> > > > -    int i = 0, j = 0;
> > > > -
> > > > -    /*
> > > > -     * Move to the first evsel of a given group
> > > > -     */
> > > > -    evlist__for_each_entry(evlist, evsel) {
> > > > -        if (evsel__is_group_leader(evsel) &&
> > > > -            evsel->core.nr_members >= 1) {
> > > > -            if (i < group_idx) {
> > > > -                j += evsel->core.nr_members;
> > > > -                i++;
> > > > -                continue;
> > > > -            }
> > > > -        }
> > > > -    }
> > > > -
> > > > -    i = 0;
> > > > -    evlist__for_each_entry(evlist, evsel) {
> > > > -        if (i < j) {
> > > > -            i++;
> > > > -            continue;
> > > > -        }
> > > > -
> > > > -        /*
> > > > -         * Now we are at the first evsel in the group
> > > > -         */
> > > > -        for_each_group_evsel(pos, evsel) {
> > > > -            if (evsel__is_hybrid(pos) &&
> > > > -                strcmp(pos->pmu_name, pmu_name)) {
> > > > -                set_bit(pos->core.idx, evlist_removed);
> > > > -            }
> > > > -        }
> > > > -        break;
> > > > -    }
> > > > -}
> > > > -
> > > > -static void remove_pmu_umatched_events(struct evlist *evlist,
> > > > char *metric_pmus)
> > > > -{
> > > > -    struct evsel *evsel, *tmp, *new_leader = NULL;
> > > > -    unsigned long *evlist_removed;
> > > > -    char *llist, *nlist, *p1;
> > > > -    bool need_new_leader = false;
> > > > -    int i = 0, new_nr_members = 0;
> > > > -
> > > > -    nlist = strdup(metric_pmus);
> > > > -    if (!nlist)
> > > > -        return;
> > > > -
> > > > -    evlist_removed = bitmap_zalloc(evlist->core.nr_entries);
> > > > -    if (!evlist_removed) {
> > > > -        free(nlist);
> > > > -        return;
> > > > -    }
> > > > -
> > > > -    llist = nlist;
> > > > -    while ((p1 = strsep(&llist, ",")) != NULL) {
> > > > -        if (strlen(p1) > 0) {
> > > > -            /*
> > > > -             * p1 points to the string of pmu name, e.g. "cpu_atom".
> > > > -             * The metric group string has pmu suffixes, e.g.
> > > > -             * "{inst_retired.any,cpu_clk_unhalted.thread}:W#cpu_core,
> > > > -             *  {cpu_clk_unhalted.core,inst_retired.any_p}:W#cpu_atom"
> > > > -             * By counting the pmu name, we can know the index of
> > > > -             * group.
> > > > -             */
> > > > -            set_pmu_unmatched_events(evlist, i++, p1,
> > > > -                         evlist_removed);
> > > > -        }
> > > > -    }
> > > > -
> > > > -    evlist__for_each_entry_safe(evlist, tmp, evsel) {
> > > > -        if (test_bit(evsel->core.idx, evlist_removed)) {
> > > > -            if (!evsel__is_group_leader(evsel)) {
> > > > -                if (!need_new_leader) {
> > > > -                    if (new_leader)
> > > > -                        new_leader->core.leader->nr_members--;
> > > > -                    else
> > > > -                        evsel->core.leader->nr_members--;
> > > > -                } else
> > > > -                    new_nr_members--;
> > > > -            } else {
> > > > -                /*
> > > > -                 * If group leader is to remove, we need to
> > > > -                 * prepare a new leader and adjust all group
> > > > -                 * members.
> > > > -                 */
> > > > -                need_new_leader = true;
> > > > -                new_nr_members =
> > > > -                    evsel->core.leader->nr_members - 1;
> > > > -            }
> > > > -
> > > > -            evlist__remove(evlist, evsel);
> > > > -            evsel__delete(evsel);
> > > > -        } else {
> > > > -            if (!evsel__is_group_leader(evsel)) {
> > > > -                if (need_new_leader) {
> > > > -                    need_new_leader = false;
> > > > -                    new_leader = evsel;
> > > > -                    new_leader->core.leader =
> > > > -                        &new_leader->core;
> > > > -                    new_leader->core.nr_members =
> > > > -                        new_nr_members;
> > > > -                } else if (new_leader)
> > > > -                    evsel->core.leader = &new_leader->core;
> > > > -            } else {
> > > > -                need_new_leader = false;
> > > > -                new_leader = NULL;
> > > > -            }
> > > > -        }
> > > > -    }
> > > > -
> > > > -    bitmap_free(evlist_removed);
> > > > -    free(nlist);
> > > > -}
> > > > -
> > > >   /**
> > > >    * parse_ids - Build the event string for the ids and parse
> > > > them creating an
> > > >    *             evlist. The encoded metric_ids are decoded.
> > > > @@ -1521,18 +1319,14 @@ static void
> > > > remove_pmu_umatched_events(struct evlist *evlist, char
> > > > *metric_pmus)
> > > >    * @modifier: any modifiers added to the events.
> > > >    * @has_constraint: false if events should be placed in a weak group.
> > > >    * @out_evlist: the created list of events.
> > > > - * @pmu_name: the name of the CPU.
> > > >    */
> > > >   static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
> > > >                struct expr_parse_ctx *ids, const char *modifier,
> > > > -             bool has_constraint, struct evlist **out_evlist,
> > > > -             const char *pmu_name)
> > > > +             bool has_constraint, struct evlist **out_evlist)
> > > >   {
> > > >       struct parse_events_error parse_error;
> > > >       struct evlist *parsed_evlist;
> > > >       struct strbuf events = STRBUF_INIT;
> > > > -    struct strbuf metric_pmus = STRBUF_INIT;
> > > > -    char *nlist = NULL;
> > > >       int ret;
> > > >       *out_evlist = NULL;
> > > > @@ -1559,7 +1353,7 @@ static int parse_ids(bool metric_no_merge,
> > > > struct perf_pmu *fake_pmu,
> > > >           ids__insert(ids->ids, tmp);
> > > >       }
> > > >       ret = metricgroup__build_event_string(&events, ids, modifier,
> > > > -                          has_constraint, pmu_name);
> > > > +                          has_constraint);
> > > >       if (ret)
> > > >           return ret;
> > > > @@ -1570,20 +1364,11 @@ static int parse_ids(bool
> > > > metric_no_merge, struct perf_pmu *fake_pmu,
> > > >       }
> > > >       pr_debug("Parsing metric events '%s'\n", events.buf);
> > > >       parse_events_error__init(&parse_error);
> > > > -    nlist = get_metric_pmus(events.buf, &metric_pmus);
> > > > -    if (!nlist) {
> > > > -        ret = -ENOMEM;
> > > > -        goto err_out;
> > > > -    }
> > > > -    ret = __parse_events(parsed_evlist, nlist, &parse_error, fake_pmu);
> > > > +    ret = __parse_events(parsed_evlist, events.buf,
> > > > &parse_error, fake_pmu);
> > > >       if (ret) {
> > > >           parse_events_error__print(&parse_error, events.buf);
> > > >           goto err_out;
> > > >       }
> > > > -
> > > > -    if (metric_pmus.alloc)
> > > > -        remove_pmu_umatched_events(parsed_evlist, metric_pmus.buf);
> > > > -
> > > >       ret = decode_all_metric_ids(parsed_evlist, modifier);
> > > >       if (ret)
> > > >           goto err_out;
> > > > @@ -1591,12 +1376,9 @@ static int parse_ids(bool
> > > > metric_no_merge, struct perf_pmu *fake_pmu,
> > > >       *out_evlist = parsed_evlist;
> > > >       parsed_evlist = NULL;
> > > >   err_out:
> > > > -    if (nlist)
> > > > -        free(nlist);
> > > >       parse_events_error__exit(&parse_error);
> > > >       evlist__delete(parsed_evlist);
> > > >       strbuf_release(&events);
> > > > -    strbuf_release(&metric_pmus);
> > > >       return ret;
> > > >   }
> > > > @@ -1615,8 +1397,7 @@ static int parse_groups(struct evlist
> > > > *perf_evlist, const char *str,
> > > >       if (metric_events_list->nr_entries == 0)
> > > >           metricgroup__rblist_init(metric_events_list);
> > > >       ret = metricgroup__add_metric_list(str, metric_no_group,
> > > > -                       &metric_list, map,
> > > > -                       perf_evlist->hybrid_pmu_name);
> > > > +                       &metric_list, map);
> > > >       if (ret)
> > > >           goto out;
> > > > @@ -1632,8 +1413,7 @@ static int parse_groups(struct evlist
> > > > *perf_evlist, const char *str,
> > > >               ret = parse_ids(metric_no_merge, fake_pmu, combined,
> > > >                       /*modifier=*/NULL,
> > > >                       /*has_constraint=*/true,
> > > > -                    &combined_evlist,
> > > > -                    perf_evlist->hybrid_pmu_name);
> > > > +                    &combined_evlist);
> > > >           }
> > > >           if (combined)
> > > >               expr__ctx_free(combined);
> > > > @@ -1670,9 +1450,6 @@ static int parse_groups(struct evlist
> > > > *perf_evlist, const char *str,
> > > >                       continue;
> > > >                   if (expr__subset_of_ids(n->pctx, m->pctx)) {
> > > > -                    if (m->pmu_name && n->pmu_name
> > > > -                        && strcmp(m->pmu_name, n->pmu_name))
> > > > -                        continue;
> > > >                       pr_debug("Events in '%s' fully contained
> > > > within '%s'\n",
> > > >                            m->metric_name, n->metric_name);
> > > >                       metric_evlist = n->evlist;
> > > > @@ -1682,16 +1459,14 @@ static int parse_groups(struct evlist
> > > > *perf_evlist, const char *str,
> > > >               }
> > > >           }
> > > >           if (!metric_evlist) {
> > > > -            ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
> > > > -                      m->modifier, m->has_constraint,
> > > > -                      &m->evlist, m->pmu_name);
> > > > +            ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
> > > > m->modifier,
> > > > +                    m->has_constraint, &m->evlist);
> > > >               if (ret)
> > > >                   goto out;
> > > >               metric_evlist = m->evlist;
> > > >           }
> > > > -        ret = setup_metric_events(m->pctx->ids, metric_evlist,
> > > > -                      &metric_events, m->pmu_name);
> > > > +        ret = setup_metric_events(m->pctx->ids, metric_evlist,
> > > > &metric_events);
> > > >           if (ret) {
> > > >               pr_debug("Cannot resolve IDs for %s: %s\n",
> > > >                   m->metric_name, m->metric_expr);
> > > > diff --git a/tools/perf/util/stat-display.c
> > > > b/tools/perf/util/stat-display.c
> > > > index 13f705737367..98669ca5a86b 100644
> > > > --- a/tools/perf/util/stat-display.c
> > > > +++ b/tools/perf/util/stat-display.c
> > > > @@ -539,8 +539,7 @@ static void aggr_update_shadow(struct
> > > > perf_stat_config *config,
> > > >       }
> > > >   }
> > > > -static void uniquify_event_name(struct evsel *counter,
> > > > -                struct perf_stat_config *stat_config)
> > > > +static void uniquify_event_name(struct evsel *counter)
> > > >   {
> > > >       char *new_name;
> > > >       char *config;
> > > > @@ -559,8 +558,7 @@ static void uniquify_event_name(struct evsel
> > > > *counter,
> > > >               counter->name = new_name;
> > > >           }
> > > >       } else {
> > > > -        if (perf_pmu__has_hybrid() &&
> > > > -            stat_config->metric_events.nr_entries == 0) {
> > > > +        if (perf_pmu__has_hybrid()) {
> > > >               ret = asprintf(&new_name, "%s/%s/",
> > > >                          counter->pmu_name, counter->name);
> > > >           } else {
> > > > @@ -634,7 +632,7 @@ static bool collect_data(struct
> > > > perf_stat_config *config, struct evsel *counter,
> > > >           return false;
> > > >       cb(config, counter, data, true);
> > > >       if (config->no_merge || hybrid_merge(counter, config, false))
> > > > -        uniquify_event_name(counter, config);
> > > > +        uniquify_event_name(counter);
> > > >       else if (counter->auto_merge_stats ||
> > > > hybrid_merge(counter, config, true))
> > > >           collect_all_aliases(config, counter, cb, data);
> > > >       return true;
> > > > -- 
> > > > 2.36.0.512.ge40c2bad7a-goog
> > > 
> 
> -- 
> Zhengjun Xing

-- 

- Arnaldo

Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

[Ian Rogers]

On Tue, May 10, 2022 at 8:49 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Tue, May 10, 2022 at 05:31:37PM +0800, Xing Zhengjun escreveu:
> > On 5/10/2022 5:55 AM, Liang, Kan wrote:
> > > On 5/9/2022 9:12 AM, Arnaldo Carvalho de Melo wrote:
> > > > Em Fri, May 06, 2022 at 10:34:06PM -0700, Ian Rogers escreveu:
> > > > > This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.
>
> > > > I picked this from the cover letter and added to this revert, to justify
> > > > it:
>
> > > > "Hybrid metrics place a PMU at the end of the parse string. This is
> > > > also where tool events are placed. The behavior of the parse string
> > > > isn't clear and so revert the change for now."
>
> > > I think the original patch used a "#" to indicate the PMU name, which
> > > can be used to distinguish between the tool events and the PMU name.
> > > To be honest, I'm not sure what's unclear here. Could you please clarify?
>
> > > With this revert, the issue mentioned in the original patch must be
> > > broken on ADL. I don't see a replacement fix in this patch series.
> > > Could you please propose a solution for the issue to replace the #PMU
> > > name solution?
>
> > > Thanks,
> > > Kan
>
> > I am surprised the origin patch is reverted during discussion and though the
> > discussion still has no conclusion.
>
> Yeah, the situation is not a good one, it was my mistake to merge the
> patch in the first place without giving time for people like Ian, that
> works a lot in the metrics code to voice his concerns, to review it.
>
> So I decided to revert to give more time for interested parties to
> discuss it further, again, I'm sorry about that.
>
> - Arnaldo

Thanks, I appreciate the time to discuss and get this done in a more proper way.

> > Let me introduce the purpose of the origin patch.
> > For a hybrid system such as ADL, if both the metrics and the formula are
> > different for the different PMUs, without this patch, the metric and event
> > parser should work ok, nothing should be special for the hybrid. In fact,
> > both "cpu_core" and "cpu_atom" may have the same metrics--the same metric
> > name, even the same formula for the metrics. For example, both "cpu_core"
> > and "cpu_atom" have metrics "IpBranch" and "IPC", For "IpBranch", both
> > "cpu_core" and "cpu_atom" has the same metric name and their formula also is
> > the same, the event name is the same though they belong to different PMUs.
> > The old metric and event parser can not handle this kind of metric, that's
> > why we need this patch.
> >
> >     "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
> >     "MetricName": "IpBranch",
> >     "Unit": "cpu_core"
> >
> >     "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
> >     "MetricName": "IpBranch",
> >     "Unit": "cpu_atom"
> >
> >
> >    "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> >    "MetricName": "IPC",
> >    "Unit": "cpu_core"
> >
> >    "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.CORE",
> >    "MetricName": "IPC",
> >    "Unit": "cpu_atom"
> >
> > Except for the above two metrics, there are still a lot of similar metrics,
> > "CPU_Utilization"...
> >
> > The original patch expanded the metric group string by adding "#PMU_name" to
> > indicate the PMU name, which can be used to distinguish between the tool
> > events and the PMU name, then the metric and event parser can parse the
> > right PMU for the events.
> >
> > With the patch all the ADL metrics can pass, without the patch, a lot of
> > metrics will fail. I don't think it's a good idea to revert it before the
> > new solution is proposed.
> >
> > >
> > > > I apologise for not having provided ample time for reviewing this patch,
> > > > sorry about that.
> > > >
> > > > - Arnaldo
> > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > > ---
> > > > >   tools/perf/util/metricgroup.c  | 263 +++------------------------------
> > > > >   tools/perf/util/stat-display.c |   8 +-
> > > > >   2 files changed, 22 insertions(+), 249 deletions(-)
> > > > >
> > > > > diff --git a/tools/perf/util/metricgroup.c
> > > > > b/tools/perf/util/metricgroup.c
> > > > > index 126a43a8917e..d8492e339521 100644
> > > > > --- a/tools/perf/util/metricgroup.c
> > > > > +++ b/tools/perf/util/metricgroup.c
> > > > > @@ -141,11 +141,6 @@ struct metric {
> > > > >        * output.
> > > > >        */
> > > > >       const char *metric_unit;
> > > > > -    /**
> > > > > -     * The name of the CPU such as "cpu_core" or "cpu_atom" in
> > > > > hybrid systems
> > > > > -     * and "NULL" in non-hybrid systems.
> > > > > -     */
> > > > > -    const char *pmu_name;
> > > > >       /** Optional null terminated array of referenced metrics. */
> > > > >       struct metric_ref *metric_refs;
> > > > >       /**
> > > > > @@ -220,7 +215,6 @@ static struct metric *metric__new(const
> > > > > struct pmu_event *pe,
> > > > >       }
> > > > >       m->metric_expr = pe->metric_expr;
> > > > >       m->metric_unit = pe->unit;
> > > > > -    m->pmu_name = pe->pmu;
> > > > >       m->pctx->runtime = runtime;
> > > > >       m->has_constraint = metric_no_group ||
> > > > > metricgroup__has_constraint(pe);
> > > > >       m->metric_refs = NULL;
> > > > > @@ -256,12 +250,10 @@ static bool contains_metric_id(struct
> > > > > evsel **metric_events, int num_events,
> > > > >    * @ids: the metric IDs to match.
> > > > >    * @metric_evlist: the list of perf events.
> > > > >    * @out_metric_events: holds the created metric events array.
> > > > > - * @pmu_name: the name of the CPU.
> > > > >    */
> > > > >   static int setup_metric_events(struct hashmap *ids,
> > > > >                      struct evlist *metric_evlist,
> > > > > -                   struct evsel ***out_metric_events,
> > > > > -                   const char *pmu_name)
> > > > > +                   struct evsel ***out_metric_events)
> > > > >   {
> > > > >       struct evsel **metric_events;
> > > > >       const char *metric_id;
> > > > > @@ -294,10 +286,6 @@ static int setup_metric_events(struct hashmap *ids,
> > > > >            * about this event.
> > > > >            */
> > > > >           if (hashmap__find(ids, metric_id, (void **)&val_ptr)) {
> > > > > -            if (evsel__is_hybrid(ev) && pmu_name &&
> > > > > -                strcmp(pmu_name, ev->pmu_name)) {
> > > > > -                continue;
> > > > > -            }
> > > > >               metric_events[matched_events++] = ev;
> > > > >               if (matched_events >= ids_size)
> > > > > @@ -736,8 +724,7 @@ static int decode_all_metric_ids(struct
> > > > > evlist *perf_evlist, const char *modifie
> > > > >   static int metricgroup__build_event_string(struct strbuf *events,
> > > > >                          const struct expr_parse_ctx *ctx,
> > > > >                          const char *modifier,
> > > > > -                       bool has_constraint,
> > > > > -                       const char *pmu_name)
> > > > > +                       bool has_constraint)
> > > > >   {
> > > > >       struct hashmap_entry *cur;
> > > > >       size_t bkt;
> > > > > @@ -819,18 +806,12 @@ static int
> > > > > metricgroup__build_event_string(struct strbuf *events,
> > > > >           if (no_group) {
> > > > >               /* Strange case of a metric of just duration_time. */
> > > > >               ret = strbuf_addf(events, "duration_time");
> > > > > -        } else if (!has_constraint) {
> > > > > -            ret = strbuf_addf(events, "}:W");
> > > > > -            if (pmu_name)
> > > > > -                ret = strbuf_addf(events, "#%s", pmu_name);
> > > > > -            ret = strbuf_addf(events, ",duration_time");
> > > > > -        } else
> > > > > +        } else if (!has_constraint)
> > > > > +            ret = strbuf_addf(events, "}:W,duration_time");
> > > > > +        else
> > > > >               ret = strbuf_addf(events, ",duration_time");
> > > > > -    } else if (!no_group && !has_constraint) {
> > > > > +    } else if (!no_group && !has_constraint)
> > > > >           ret = strbuf_addf(events, "}:W");
> > > > > -        if (pmu_name)
> > > > > -            ret = strbuf_addf(events, "#%s", pmu_name);
> > > > > -    }

To Zhengjun's point about adding the #pmu_name done above: the code is
conditionally adding the #pmu_name but only when events are grouped,
this means that the behavior with the command line option
--metric-no-group is different as in that case no #pmu_name will be
added (there's no group). In that case how do the events choose the
correct PMU? While this may work for existing Alderlake metrics I
don't expect it to work in the future or with command line options.
The problem reminds me of modifiers on metrics where we rewrite events
to add in the modifiers:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/metricgroup.c?h=perf/core#n703
The logic here is also used in event deduplication for things like the
combined event list and I don't understand how #pmu_name is expected
to work in that context. It is only partially working here.

Thanks,
Ian

> > > > >       return ret;
> > > > >   #undef RETURN_IF_NON_ZERO
> > > > > @@ -1169,13 +1150,11 @@ static int metric_list_cmp(void *priv
> > > > > __maybe_unused, const struct list_head *l,
> > > > >    * @metric_list: The list that the metric or metric group are
> > > > > added to.
> > > > >    * @map: The map that is searched for metrics, most commonly
> > > > > the table for the
> > > > >    *       architecture perf is running upon.
> > > > > - * @pmu_name: the name of the CPU.
> > > > >    */
> > > > > -static int metricgroup__add_metric(const char *metric_name,
> > > > > -                   const char *modifier, bool metric_no_group,
> > > > > +static int metricgroup__add_metric(const char *metric_name,
> > > > > const char *modifier,
> > > > > +                   bool metric_no_group,
> > > > >                      struct list_head *metric_list,
> > > > > -                   const struct pmu_events_map *map,
> > > > > -                   const char *pmu_name)
> > > > > +                   const struct pmu_events_map *map)
> > > > >   {
> > > > >       const struct pmu_event *pe;
> > > > >       LIST_HEAD(list);
> > > > > @@ -1188,8 +1167,6 @@ static int metricgroup__add_metric(const
> > > > > char *metric_name,
> > > > >        */
> > > > >       map_for_each_metric(pe, i, map, metric_name) {
> > > > >           has_match = true;
> > > > > -        if (pmu_name && pe->pmu && strcmp(pmu_name, pe->pmu))
> > > > > -            continue;
> > > > >           ret = add_metric(&list, pe, modifier, metric_no_group,
> > > > >                    /*root_metric=*/NULL,
> > > > >                    /*visited_metrics=*/NULL, map);
> > > > > @@ -1238,12 +1215,10 @@ static int metricgroup__add_metric(const
> > > > > char *metric_name,
> > > > >    * @metric_list: The list that metrics are added to.
> > > > >    * @map: The map that is searched for metrics, most commonly
> > > > > the table for the
> > > > >    *       architecture perf is running upon.
> > > > > - * @pmu_name: the name of the CPU.
> > > > >    */
> > > > >   static int metricgroup__add_metric_list(const char *list, bool
> > > > > metric_no_group,
> > > > >                       struct list_head *metric_list,
> > > > > -                    const struct pmu_events_map *map,
> > > > > -                    const char *pmu_name)
> > > > > +                    const struct pmu_events_map *map)
> > > > >   {
> > > > >       char *list_itr, *list_copy, *metric_name, *modifier;
> > > > >       int ret, count = 0;
> > > > > @@ -1260,7 +1235,7 @@ static int
> > > > > metricgroup__add_metric_list(const char *list, bool
> > > > > metric_no_group,
> > > > >           ret = metricgroup__add_metric(metric_name, modifier,
> > > > >                             metric_no_group, metric_list,
> > > > > -                          map, pmu_name);
> > > > > +                          map);
> > > > >           if (ret == -EINVAL)
> > > > >               pr_err("Cannot find metric or group `%s'\n", metric_name);
> > > > > @@ -1335,183 +1310,6 @@ static int build_combined_expr_ctx(const
> > > > > struct list_head *metric_list,
> > > > >       return ret;
> > > > >   }
> > > > > -static char *get_metric_pmus(char *orig_str, struct strbuf
> > > > > *metric_pmus)
> > > > > -{
> > > > > -    char *llist, *nlist, *p1, *p2, *new_str = NULL;
> > > > > -    int ret;
> > > > > -    struct strbuf new_events;
> > > > > -
> > > > > -    if (!strchr(orig_str, '#')) {
> > > > > -        /*
> > > > > -         * pmu name is added after '#'. If no '#' found,
> > > > > -         * don't need to process pmu.
> > > > > -         */
> > > > > -        return strdup(orig_str);
> > > > > -    }
> > > > > -
> > > > > -    nlist = strdup(orig_str);
> > > > > -    if (!nlist)
> > > > > -        return new_str;
> > > > > -
> > > > > -    ret = strbuf_init(&new_events, 100);
> > > > > -    if (ret)
> > > > > -        goto err_out;
> > > > > -
> > > > > -    ret = strbuf_grow(metric_pmus, 100);
> > > > > -    if (ret)
> > > > > -        goto err_out;
> > > > > -
> > > > > -    llist = nlist;
> > > > > -    while ((p1 = strsep(&llist, ",")) != NULL) {
> > > > > -        p2 = strchr(p1, '#');
> > > > > -        if (p2) {
> > > > > -            *p2 = 0;
> > > > > -            ret = strbuf_addf(&new_events, "%s,", p1);
> > > > > -            if (ret)
> > > > > -                goto err_out;
> > > > > -
> > > > > -            ret = strbuf_addf(metric_pmus, "%s,", p2 + 1);
> > > > > -            if (ret)
> > > > > -                goto err_out;
> > > > > -
> > > > > -        } else {
> > > > > -            ret = strbuf_addf(&new_events, "%s,", p1);
> > > > > -            if (ret)
> > > > > -                goto err_out;
> > > > > -        }
> > > > > -    }
> > > > > -
> > > > > -    new_str = strdup(new_events.buf);
> > > > > -    if (new_str) {
> > > > > -        /* Remove last ',' */
> > > > > -        new_str[strlen(new_str) - 1] = 0;
> > > > > -    }
> > > > > -err_out:
> > > > > -    free(nlist);
> > > > > -    strbuf_release(&new_events);
> > > > > -    return new_str;
> > > > > -}
> > > > > -
> > > > > -static void set_pmu_unmatched_events(struct evlist *evlist, int
> > > > > group_idx,
> > > > > -                     char *pmu_name,
> > > > > -                     unsigned long *evlist_removed)
> > > > > -{
> > > > > -    struct evsel *evsel, *pos;
> > > > > -    int i = 0, j = 0;
> > > > > -
> > > > > -    /*
> > > > > -     * Move to the first evsel of a given group
> > > > > -     */
> > > > > -    evlist__for_each_entry(evlist, evsel) {
> > > > > -        if (evsel__is_group_leader(evsel) &&
> > > > > -            evsel->core.nr_members >= 1) {
> > > > > -            if (i < group_idx) {
> > > > > -                j += evsel->core.nr_members;
> > > > > -                i++;
> > > > > -                continue;
> > > > > -            }
> > > > > -        }
> > > > > -    }
> > > > > -
> > > > > -    i = 0;
> > > > > -    evlist__for_each_entry(evlist, evsel) {
> > > > > -        if (i < j) {
> > > > > -            i++;
> > > > > -            continue;
> > > > > -        }
> > > > > -
> > > > > -        /*
> > > > > -         * Now we are at the first evsel in the group
> > > > > -         */
> > > > > -        for_each_group_evsel(pos, evsel) {
> > > > > -            if (evsel__is_hybrid(pos) &&
> > > > > -                strcmp(pos->pmu_name, pmu_name)) {
> > > > > -                set_bit(pos->core.idx, evlist_removed);
> > > > > -            }
> > > > > -        }
> > > > > -        break;
> > > > > -    }
> > > > > -}
> > > > > -
> > > > > -static void remove_pmu_umatched_events(struct evlist *evlist,
> > > > > char *metric_pmus)
> > > > > -{
> > > > > -    struct evsel *evsel, *tmp, *new_leader = NULL;
> > > > > -    unsigned long *evlist_removed;
> > > > > -    char *llist, *nlist, *p1;
> > > > > -    bool need_new_leader = false;
> > > > > -    int i = 0, new_nr_members = 0;
> > > > > -
> > > > > -    nlist = strdup(metric_pmus);
> > > > > -    if (!nlist)
> > > > > -        return;
> > > > > -
> > > > > -    evlist_removed = bitmap_zalloc(evlist->core.nr_entries);
> > > > > -    if (!evlist_removed) {
> > > > > -        free(nlist);
> > > > > -        return;
> > > > > -    }
> > > > > -
> > > > > -    llist = nlist;
> > > > > -    while ((p1 = strsep(&llist, ",")) != NULL) {
> > > > > -        if (strlen(p1) > 0) {
> > > > > -            /*
> > > > > -             * p1 points to the string of pmu name, e.g. "cpu_atom".
> > > > > -             * The metric group string has pmu suffixes, e.g.
> > > > > -             * "{inst_retired.any,cpu_clk_unhalted.thread}:W#cpu_core,
> > > > > -             *  {cpu_clk_unhalted.core,inst_retired.any_p}:W#cpu_atom"
> > > > > -             * By counting the pmu name, we can know the index of
> > > > > -             * group.
> > > > > -             */
> > > > > -            set_pmu_unmatched_events(evlist, i++, p1,
> > > > > -                         evlist_removed);
> > > > > -        }
> > > > > -    }
> > > > > -
> > > > > -    evlist__for_each_entry_safe(evlist, tmp, evsel) {
> > > > > -        if (test_bit(evsel->core.idx, evlist_removed)) {
> > > > > -            if (!evsel__is_group_leader(evsel)) {
> > > > > -                if (!need_new_leader) {
> > > > > -                    if (new_leader)
> > > > > -                        new_leader->core.leader->nr_members--;
> > > > > -                    else
> > > > > -                        evsel->core.leader->nr_members--;
> > > > > -                } else
> > > > > -                    new_nr_members--;
> > > > > -            } else {
> > > > > -                /*
> > > > > -                 * If group leader is to remove, we need to
> > > > > -                 * prepare a new leader and adjust all group
> > > > > -                 * members.
> > > > > -                 */
> > > > > -                need_new_leader = true;
> > > > > -                new_nr_members =
> > > > > -                    evsel->core.leader->nr_members - 1;
> > > > > -            }
> > > > > -
> > > > > -            evlist__remove(evlist, evsel);
> > > > > -            evsel__delete(evsel);
> > > > > -        } else {
> > > > > -            if (!evsel__is_group_leader(evsel)) {
> > > > > -                if (need_new_leader) {
> > > > > -                    need_new_leader = false;
> > > > > -                    new_leader = evsel;
> > > > > -                    new_leader->core.leader =
> > > > > -                        &new_leader->core;
> > > > > -                    new_leader->core.nr_members =
> > > > > -                        new_nr_members;
> > > > > -                } else if (new_leader)
> > > > > -                    evsel->core.leader = &new_leader->core;
> > > > > -            } else {
> > > > > -                need_new_leader = false;
> > > > > -                new_leader = NULL;
> > > > > -            }
> > > > > -        }
> > > > > -    }
> > > > > -
> > > > > -    bitmap_free(evlist_removed);
> > > > > -    free(nlist);
> > > > > -}
> > > > > -
> > > > >   /**
> > > > >    * parse_ids - Build the event string for the ids and parse
> > > > > them creating an
> > > > >    *             evlist. The encoded metric_ids are decoded.
> > > > > @@ -1521,18 +1319,14 @@ static void
> > > > > remove_pmu_umatched_events(struct evlist *evlist, char
> > > > > *metric_pmus)
> > > > >    * @modifier: any modifiers added to the events.
> > > > >    * @has_constraint: false if events should be placed in a weak group.
> > > > >    * @out_evlist: the created list of events.
> > > > > - * @pmu_name: the name of the CPU.
> > > > >    */
> > > > >   static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
> > > > >                struct expr_parse_ctx *ids, const char *modifier,
> > > > > -             bool has_constraint, struct evlist **out_evlist,
> > > > > -             const char *pmu_name)
> > > > > +             bool has_constraint, struct evlist **out_evlist)
> > > > >   {
> > > > >       struct parse_events_error parse_error;
> > > > >       struct evlist *parsed_evlist;
> > > > >       struct strbuf events = STRBUF_INIT;
> > > > > -    struct strbuf metric_pmus = STRBUF_INIT;
> > > > > -    char *nlist = NULL;
> > > > >       int ret;
> > > > >       *out_evlist = NULL;
> > > > > @@ -1559,7 +1353,7 @@ static int parse_ids(bool metric_no_merge,
> > > > > struct perf_pmu *fake_pmu,
> > > > >           ids__insert(ids->ids, tmp);
> > > > >       }
> > > > >       ret = metricgroup__build_event_string(&events, ids, modifier,
> > > > > -                          has_constraint, pmu_name);
> > > > > +                          has_constraint);
> > > > >       if (ret)
> > > > >           return ret;
> > > > > @@ -1570,20 +1364,11 @@ static int parse_ids(bool
> > > > > metric_no_merge, struct perf_pmu *fake_pmu,
> > > > >       }
> > > > >       pr_debug("Parsing metric events '%s'\n", events.buf);
> > > > >       parse_events_error__init(&parse_error);
> > > > > -    nlist = get_metric_pmus(events.buf, &metric_pmus);
> > > > > -    if (!nlist) {
> > > > > -        ret = -ENOMEM;
> > > > > -        goto err_out;
> > > > > -    }
> > > > > -    ret = __parse_events(parsed_evlist, nlist, &parse_error, fake_pmu);
> > > > > +    ret = __parse_events(parsed_evlist, events.buf,
> > > > > &parse_error, fake_pmu);
> > > > >       if (ret) {
> > > > >           parse_events_error__print(&parse_error, events.buf);
> > > > >           goto err_out;
> > > > >       }
> > > > > -
> > > > > -    if (metric_pmus.alloc)
> > > > > -        remove_pmu_umatched_events(parsed_evlist, metric_pmus.buf);
> > > > > -
> > > > >       ret = decode_all_metric_ids(parsed_evlist, modifier);
> > > > >       if (ret)
> > > > >           goto err_out;
> > > > > @@ -1591,12 +1376,9 @@ static int parse_ids(bool
> > > > > metric_no_merge, struct perf_pmu *fake_pmu,
> > > > >       *out_evlist = parsed_evlist;
> > > > >       parsed_evlist = NULL;
> > > > >   err_out:
> > > > > -    if (nlist)
> > > > > -        free(nlist);
> > > > >       parse_events_error__exit(&parse_error);
> > > > >       evlist__delete(parsed_evlist);
> > > > >       strbuf_release(&events);
> > > > > -    strbuf_release(&metric_pmus);
> > > > >       return ret;
> > > > >   }
> > > > > @@ -1615,8 +1397,7 @@ static int parse_groups(struct evlist
> > > > > *perf_evlist, const char *str,
> > > > >       if (metric_events_list->nr_entries == 0)
> > > > >           metricgroup__rblist_init(metric_events_list);
> > > > >       ret = metricgroup__add_metric_list(str, metric_no_group,
> > > > > -                       &metric_list, map,
> > > > > -                       perf_evlist->hybrid_pmu_name);
> > > > > +                       &metric_list, map);
> > > > >       if (ret)
> > > > >           goto out;
> > > > > @@ -1632,8 +1413,7 @@ static int parse_groups(struct evlist
> > > > > *perf_evlist, const char *str,
> > > > >               ret = parse_ids(metric_no_merge, fake_pmu, combined,
> > > > >                       /*modifier=*/NULL,
> > > > >                       /*has_constraint=*/true,
> > > > > -                    &combined_evlist,
> > > > > -                    perf_evlist->hybrid_pmu_name);
> > > > > +                    &combined_evlist);
> > > > >           }
> > > > >           if (combined)
> > > > >               expr__ctx_free(combined);
> > > > > @@ -1670,9 +1450,6 @@ static int parse_groups(struct evlist
> > > > > *perf_evlist, const char *str,
> > > > >                       continue;
> > > > >                   if (expr__subset_of_ids(n->pctx, m->pctx)) {
> > > > > -                    if (m->pmu_name && n->pmu_name
> > > > > -                        && strcmp(m->pmu_name, n->pmu_name))
> > > > > -                        continue;
> > > > >                       pr_debug("Events in '%s' fully contained
> > > > > within '%s'\n",
> > > > >                            m->metric_name, n->metric_name);
> > > > >                       metric_evlist = n->evlist;
> > > > > @@ -1682,16 +1459,14 @@ static int parse_groups(struct evlist
> > > > > *perf_evlist, const char *str,
> > > > >               }
> > > > >           }
> > > > >           if (!metric_evlist) {
> > > > > -            ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
> > > > > -                      m->modifier, m->has_constraint,
> > > > > -                      &m->evlist, m->pmu_name);
> > > > > +            ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
> > > > > m->modifier,
> > > > > +                    m->has_constraint, &m->evlist);
> > > > >               if (ret)
> > > > >                   goto out;
> > > > >               metric_evlist = m->evlist;
> > > > >           }
> > > > > -        ret = setup_metric_events(m->pctx->ids, metric_evlist,
> > > > > -                      &metric_events, m->pmu_name);
> > > > > +        ret = setup_metric_events(m->pctx->ids, metric_evlist,
> > > > > &metric_events);
> > > > >           if (ret) {
> > > > >               pr_debug("Cannot resolve IDs for %s: %s\n",
> > > > >                   m->metric_name, m->metric_expr);
> > > > > diff --git a/tools/perf/util/stat-display.c
> > > > > b/tools/perf/util/stat-display.c
> > > > > index 13f705737367..98669ca5a86b 100644
> > > > > --- a/tools/perf/util/stat-display.c
> > > > > +++ b/tools/perf/util/stat-display.c
> > > > > @@ -539,8 +539,7 @@ static void aggr_update_shadow(struct
> > > > > perf_stat_config *config,
> > > > >       }
> > > > >   }
> > > > > -static void uniquify_event_name(struct evsel *counter,
> > > > > -                struct perf_stat_config *stat_config)
> > > > > +static void uniquify_event_name(struct evsel *counter)
> > > > >   {
> > > > >       char *new_name;
> > > > >       char *config;
> > > > > @@ -559,8 +558,7 @@ static void uniquify_event_name(struct evsel
> > > > > *counter,
> > > > >               counter->name = new_name;
> > > > >           }
> > > > >       } else {
> > > > > -        if (perf_pmu__has_hybrid() &&
> > > > > -            stat_config->metric_events.nr_entries == 0) {
> > > > > +        if (perf_pmu__has_hybrid()) {
> > > > >               ret = asprintf(&new_name, "%s/%s/",
> > > > >                          counter->pmu_name, counter->name);
> > > > >           } else {
> > > > > @@ -634,7 +632,7 @@ static bool collect_data(struct
> > > > > perf_stat_config *config, struct evsel *counter,
> > > > >           return false;
> > > > >       cb(config, counter, data, true);
> > > > >       if (config->no_merge || hybrid_merge(counter, config, false))
> > > > > -        uniquify_event_name(counter, config);
> > > > > +        uniquify_event_name(counter);
> > > > >       else if (counter->auto_merge_stats ||
> > > > > hybrid_merge(counter, config, true))
> > > > >           collect_all_aliases(config, counter, cb, data);
> > > > >       return true;
> > > > > --
> > > > > 2.36.0.512.ge40c2bad7a-goog
> > > >
> >
> > --
> > Zhengjun Xing
>
> --
>
> - Arnaldo

Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

[Xing Zhengjun]

On 5/11/2022 12:45 AM, Ian Rogers wrote:
> On Tue, May 10, 2022 at 8:49 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
>>
>> Em Tue, May 10, 2022 at 05:31:37PM +0800, Xing Zhengjun escreveu:
>>> On 5/10/2022 5:55 AM, Liang, Kan wrote:
>>>> On 5/9/2022 9:12 AM, Arnaldo Carvalho de Melo wrote:
>>>>> Em Fri, May 06, 2022 at 10:34:06PM -0700, Ian Rogers escreveu:
>>>>>> This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.
>>
>>>>> I picked this from the cover letter and added to this revert, to justify
>>>>> it:
>>
>>>>> "Hybrid metrics place a PMU at the end of the parse string. This is
>>>>> also where tool events are placed. The behavior of the parse string
>>>>> isn't clear and so revert the change for now."
>>
>>>> I think the original patch used a "#" to indicate the PMU name, which
>>>> can be used to distinguish between the tool events and the PMU name.
>>>> To be honest, I'm not sure what's unclear here. Could you please clarify?
>>
>>>> With this revert, the issue mentioned in the original patch must be
>>>> broken on ADL. I don't see a replacement fix in this patch series.
>>>> Could you please propose a solution for the issue to replace the #PMU
>>>> name solution?
>>
>>>> Thanks,
>>>> Kan
>>
>>> I am surprised the origin patch is reverted during discussion and though the
>>> discussion still has no conclusion.
>>
>> Yeah, the situation is not a good one, it was my mistake to merge the
>> patch in the first place without giving time for people like Ian, that
>> works a lot in the metrics code to voice his concerns, to review it.
>>
>> So I decided to revert to give more time for interested parties to
>> discuss it further, again, I'm sorry about that.
>>
>> - Arnaldo
> 
> Thanks, I appreciate the time to discuss and get this done in a more proper way.
> 
>>> Let me introduce the purpose of the origin patch.
>>> For a hybrid system such as ADL, if both the metrics and the formula are
>>> different for the different PMUs, without this patch, the metric and event
>>> parser should work ok, nothing should be special for the hybrid. In fact,
>>> both "cpu_core" and "cpu_atom" may have the same metrics--the same metric
>>> name, even the same formula for the metrics. For example, both "cpu_core"
>>> and "cpu_atom" have metrics "IpBranch" and "IPC", For "IpBranch", both
>>> "cpu_core" and "cpu_atom" has the same metric name and their formula also is
>>> the same, the event name is the same though they belong to different PMUs.
>>> The old metric and event parser can not handle this kind of metric, that's
>>> why we need this patch.
>>>
>>>      "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
>>>      "MetricName": "IpBranch",
>>>      "Unit": "cpu_core"
>>>
>>>      "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
>>>      "MetricName": "IpBranch",
>>>      "Unit": "cpu_atom"
>>>
>>>
>>>     "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
>>>     "MetricName": "IPC",
>>>     "Unit": "cpu_core"
>>>
>>>     "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.CORE",
>>>     "MetricName": "IPC",
>>>     "Unit": "cpu_atom"
>>>
>>> Except for the above two metrics, there are still a lot of similar metrics,
>>> "CPU_Utilization"...
>>>
>>> The original patch expanded the metric group string by adding "#PMU_name" to
>>> indicate the PMU name, which can be used to distinguish between the tool
>>> events and the PMU name, then the metric and event parser can parse the
>>> right PMU for the events.
>>>
>>> With the patch all the ADL metrics can pass, without the patch, a lot of
>>> metrics will fail. I don't think it's a good idea to revert it before the
>>> new solution is proposed.
>>>
>>>>
>>>>> I apologise for not having provided ample time for reviewing this patch,
>>>>> sorry about that.
>>>>>
>>>>> - Arnaldo
>>>>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>>>>> ---
>>>>>>    tools/perf/util/metricgroup.c  | 263 +++------------------------------
>>>>>>    tools/perf/util/stat-display.c |   8 +-
>>>>>>    2 files changed, 22 insertions(+), 249 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/perf/util/metricgroup.c
>>>>>> b/tools/perf/util/metricgroup.c
>>>>>> index 126a43a8917e..d8492e339521 100644
>>>>>> --- a/tools/perf/util/metricgroup.c
>>>>>> +++ b/tools/perf/util/metricgroup.c
>>>>>> @@ -141,11 +141,6 @@ struct metric {
>>>>>>         * output.
>>>>>>         */
>>>>>>        const char *metric_unit;
>>>>>> -    /**
>>>>>> -     * The name of the CPU such as "cpu_core" or "cpu_atom" in
>>>>>> hybrid systems
>>>>>> -     * and "NULL" in non-hybrid systems.
>>>>>> -     */
>>>>>> -    const char *pmu_name;
>>>>>>        /** Optional null terminated array of referenced metrics. */
>>>>>>        struct metric_ref *metric_refs;
>>>>>>        /**
>>>>>> @@ -220,7 +215,6 @@ static struct metric *metric__new(const
>>>>>> struct pmu_event *pe,
>>>>>>        }
>>>>>>        m->metric_expr = pe->metric_expr;
>>>>>>        m->metric_unit = pe->unit;
>>>>>> -    m->pmu_name = pe->pmu;
>>>>>>        m->pctx->runtime = runtime;
>>>>>>        m->has_constraint = metric_no_group ||
>>>>>> metricgroup__has_constraint(pe);
>>>>>>        m->metric_refs = NULL;
>>>>>> @@ -256,12 +250,10 @@ static bool contains_metric_id(struct
>>>>>> evsel **metric_events, int num_events,
>>>>>>     * @ids: the metric IDs to match.
>>>>>>     * @metric_evlist: the list of perf events.
>>>>>>     * @out_metric_events: holds the created metric events array.
>>>>>> - * @pmu_name: the name of the CPU.
>>>>>>     */
>>>>>>    static int setup_metric_events(struct hashmap *ids,
>>>>>>                       struct evlist *metric_evlist,
>>>>>> -                   struct evsel ***out_metric_events,
>>>>>> -                   const char *pmu_name)
>>>>>> +                   struct evsel ***out_metric_events)
>>>>>>    {
>>>>>>        struct evsel **metric_events;
>>>>>>        const char *metric_id;
>>>>>> @@ -294,10 +286,6 @@ static int setup_metric_events(struct hashmap *ids,
>>>>>>             * about this event.
>>>>>>             */
>>>>>>            if (hashmap__find(ids, metric_id, (void **)&val_ptr)) {
>>>>>> -            if (evsel__is_hybrid(ev) && pmu_name &&
>>>>>> -                strcmp(pmu_name, ev->pmu_name)) {
>>>>>> -                continue;
>>>>>> -            }
>>>>>>                metric_events[matched_events++] = ev;
>>>>>>                if (matched_events >= ids_size)
>>>>>> @@ -736,8 +724,7 @@ static int decode_all_metric_ids(struct
>>>>>> evlist *perf_evlist, const char *modifie
>>>>>>    static int metricgroup__build_event_string(struct strbuf *events,
>>>>>>                           const struct expr_parse_ctx *ctx,
>>>>>>                           const char *modifier,
>>>>>> -                       bool has_constraint,
>>>>>> -                       const char *pmu_name)
>>>>>> +                       bool has_constraint)
>>>>>>    {
>>>>>>        struct hashmap_entry *cur;
>>>>>>        size_t bkt;
>>>>>> @@ -819,18 +806,12 @@ static int
>>>>>> metricgroup__build_event_string(struct strbuf *events,
>>>>>>            if (no_group) {
>>>>>>                /* Strange case of a metric of just duration_time. */
>>>>>>                ret = strbuf_addf(events, "duration_time");
>>>>>> -        } else if (!has_constraint) {
>>>>>> -            ret = strbuf_addf(events, "}:W");
>>>>>> -            if (pmu_name)
>>>>>> -                ret = strbuf_addf(events, "#%s", pmu_name);
>>>>>> -            ret = strbuf_addf(events, ",duration_time");
>>>>>> -        } else
>>>>>> +        } else if (!has_constraint)
>>>>>> +            ret = strbuf_addf(events, "}:W,duration_time");
>>>>>> +        else
>>>>>>                ret = strbuf_addf(events, ",duration_time");
>>>>>> -    } else if (!no_group && !has_constraint) {
>>>>>> +    } else if (!no_group && !has_constraint)
>>>>>>            ret = strbuf_addf(events, "}:W");
>>>>>> -        if (pmu_name)
>>>>>> -            ret = strbuf_addf(events, "#%s", pmu_name);
>>>>>> -    }
> 
> To Zhengjun's point about adding the #pmu_name done above: the code is
> conditionally adding the #pmu_name but only when events are grouped,
> this means that the behavior with the command line option
> --metric-no-group is different as in that case no #pmu_name will be
> added (there's no group). In that case how do the events choose the
> correct PMU? While this may work for existing Alderlake metrics I
> don't expect it to work in the future or with command line options.
> The problem reminds me of modifiers on metrics where we rewrite events
> to add in the modifiers:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/metricgroup.c?h=perf/core#n703
> The logic here is also used in event deduplication for things like the
> combined event list and I don't understand how #pmu_name is expected
> to work in that context. It is only partially working here.

I just build perf based on perf/core branch, the head commit is 
280c36d26eb8 (perf test: Add skip to --per-thread test), it is just 
before the reverted commit. I test with the "--metric-no-group" option, 
and it still works ok.

./perf -v
perf version 5.18.rc4.g280c36d26eb8

./perf stat -v -M "IpBranch" --metric-no-group -a sleep 1
Using CPUID GenuineIntel-6-97-2
metric expr INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES for IpBranch
metric expr INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES for IpBranch
found event duration_time
found event BR_INST_RETIRED.ALL_BRANCHES
found event INST_RETIRED.ANY
Parsing metric events 
'{BR_INST_RETIRED.ALL_BRANCHES/metric-id=BR_INST_RETIRED.ALL_BRANCHES/,INST_RETIRED.ANY/metric-id=INST_RETIRED.ANY/}:W#cpu_atom,duration_time'
BR_INST_RETIRED.ALL_BRANCHES -> cpu_core/period=0x61a89,event=0xc4/
BR_INST_RETIRED.ALL_BRANCHES -> cpu_atom/period=0x30d43,event=0xc4/
INST_RETIRED.ANY -> cpu_core/event=0xc0,period=0x1e8483/
INST_RETIRED.ANY -> cpu_atom/event=0xc0,period=0x1e8483/
found event duration_time
found event BR_INST_RETIRED.ALL_BRANCHES
found event INST_RETIRED.ANY
Parsing metric events 
'{BR_INST_RETIRED.ALL_BRANCHES/metric-id=BR_INST_RETIRED.ALL_BRANCHES/,INST_RETIRED.ANY/metric-id=INST_RETIRED.ANY/}:W#cpu_core,duration_time'
BR_INST_RETIRED.ALL_BRANCHES -> cpu_core/period=0x61a89,event=0xc4/
BR_INST_RETIRED.ALL_BRANCHES -> cpu_atom/period=0x30d43,event=0xc4/
INST_RETIRED.ANY -> cpu_core/event=0xc0,period=0x1e8483/
INST_RETIRED.ANY -> cpu_atom/event=0xc0,period=0x1e8483/
Control descriptor is not initialized
BR_INST_RETIRED.ALL_BRANCHES: 118542 8028259379 8028259379
INST_RETIRED.ANY: 625378 8028259379 8028259379
duration_time: 1003654656 1003654656 1003654656
BR_INST_RETIRED.ALL_BRANCHES: 1740268 16050148595 16050148595
INST_RETIRED.ANY: 8463993 16050148595 16050148595
duration_time: 1003654656 1003654656 1003654656

  Performance counter stats for 'system wide':

            118,542      BR_INST_RETIRED.ALL_BRANCHES [cpu_atom] # 
5.28 IpBranch
            625,378      INST_RETIRED.ANY [cpu_atom]
      1,003,654,656 ns   duration_time
          1,740,268      BR_INST_RETIRED.ALL_BRANCHES [cpu_core] # 
4.86 IpBranch
          8,463,993      INST_RETIRED.ANY [cpu_core]
      1,003,654,656 ns   duration_time

        1.003654656 seconds time elapsed

> 
> Thanks,
> Ian
> 
>>>>>>        return ret;
>>>>>>    #undef RETURN_IF_NON_ZERO
>>>>>> @@ -1169,13 +1150,11 @@ static int metric_list_cmp(void *priv
>>>>>> __maybe_unused, const struct list_head *l,
>>>>>>     * @metric_list: The list that the metric or metric group are
>>>>>> added to.
>>>>>>     * @map: The map that is searched for metrics, most commonly
>>>>>> the table for the
>>>>>>     *       architecture perf is running upon.
>>>>>> - * @pmu_name: the name of the CPU.
>>>>>>     */
>>>>>> -static int metricgroup__add_metric(const char *metric_name,
>>>>>> -                   const char *modifier, bool metric_no_group,
>>>>>> +static int metricgroup__add_metric(const char *metric_name,
>>>>>> const char *modifier,
>>>>>> +                   bool metric_no_group,
>>>>>>                       struct list_head *metric_list,
>>>>>> -                   const struct pmu_events_map *map,
>>>>>> -                   const char *pmu_name)
>>>>>> +                   const struct pmu_events_map *map)
>>>>>>    {
>>>>>>        const struct pmu_event *pe;
>>>>>>        LIST_HEAD(list);
>>>>>> @@ -1188,8 +1167,6 @@ static int metricgroup__add_metric(const
>>>>>> char *metric_name,
>>>>>>         */
>>>>>>        map_for_each_metric(pe, i, map, metric_name) {
>>>>>>            has_match = true;
>>>>>> -        if (pmu_name && pe->pmu && strcmp(pmu_name, pe->pmu))
>>>>>> -            continue;
>>>>>>            ret = add_metric(&list, pe, modifier, metric_no_group,
>>>>>>                     /*root_metric=*/NULL,
>>>>>>                     /*visited_metrics=*/NULL, map);
>>>>>> @@ -1238,12 +1215,10 @@ static int metricgroup__add_metric(const
>>>>>> char *metric_name,
>>>>>>     * @metric_list: The list that metrics are added to.
>>>>>>     * @map: The map that is searched for metrics, most commonly
>>>>>> the table for the
>>>>>>     *       architecture perf is running upon.
>>>>>> - * @pmu_name: the name of the CPU.
>>>>>>     */
>>>>>>    static int metricgroup__add_metric_list(const char *list, bool
>>>>>> metric_no_group,
>>>>>>                        struct list_head *metric_list,
>>>>>> -                    const struct pmu_events_map *map,
>>>>>> -                    const char *pmu_name)
>>>>>> +                    const struct pmu_events_map *map)
>>>>>>    {
>>>>>>        char *list_itr, *list_copy, *metric_name, *modifier;
>>>>>>        int ret, count = 0;
>>>>>> @@ -1260,7 +1235,7 @@ static int
>>>>>> metricgroup__add_metric_list(const char *list, bool
>>>>>> metric_no_group,
>>>>>>            ret = metricgroup__add_metric(metric_name, modifier,
>>>>>>                              metric_no_group, metric_list,
>>>>>> -                          map, pmu_name);
>>>>>> +                          map);
>>>>>>            if (ret == -EINVAL)
>>>>>>                pr_err("Cannot find metric or group `%s'\n", metric_name);
>>>>>> @@ -1335,183 +1310,6 @@ static int build_combined_expr_ctx(const
>>>>>> struct list_head *metric_list,
>>>>>>        return ret;
>>>>>>    }
>>>>>> -static char *get_metric_pmus(char *orig_str, struct strbuf
>>>>>> *metric_pmus)
>>>>>> -{
>>>>>> -    char *llist, *nlist, *p1, *p2, *new_str = NULL;
>>>>>> -    int ret;
>>>>>> -    struct strbuf new_events;
>>>>>> -
>>>>>> -    if (!strchr(orig_str, '#')) {
>>>>>> -        /*
>>>>>> -         * pmu name is added after '#'. If no '#' found,
>>>>>> -         * don't need to process pmu.
>>>>>> -         */
>>>>>> -        return strdup(orig_str);
>>>>>> -    }
>>>>>> -
>>>>>> -    nlist = strdup(orig_str);
>>>>>> -    if (!nlist)
>>>>>> -        return new_str;
>>>>>> -
>>>>>> -    ret = strbuf_init(&new_events, 100);
>>>>>> -    if (ret)
>>>>>> -        goto err_out;
>>>>>> -
>>>>>> -    ret = strbuf_grow(metric_pmus, 100);
>>>>>> -    if (ret)
>>>>>> -        goto err_out;
>>>>>> -
>>>>>> -    llist = nlist;
>>>>>> -    while ((p1 = strsep(&llist, ",")) != NULL) {
>>>>>> -        p2 = strchr(p1, '#');
>>>>>> -        if (p2) {
>>>>>> -            *p2 = 0;
>>>>>> -            ret = strbuf_addf(&new_events, "%s,", p1);
>>>>>> -            if (ret)
>>>>>> -                goto err_out;
>>>>>> -
>>>>>> -            ret = strbuf_addf(metric_pmus, "%s,", p2 + 1);
>>>>>> -            if (ret)
>>>>>> -                goto err_out;
>>>>>> -
>>>>>> -        } else {
>>>>>> -            ret = strbuf_addf(&new_events, "%s,", p1);
>>>>>> -            if (ret)
>>>>>> -                goto err_out;
>>>>>> -        }
>>>>>> -    }
>>>>>> -
>>>>>> -    new_str = strdup(new_events.buf);
>>>>>> -    if (new_str) {
>>>>>> -        /* Remove last ',' */
>>>>>> -        new_str[strlen(new_str) - 1] = 0;
>>>>>> -    }
>>>>>> -err_out:
>>>>>> -    free(nlist);
>>>>>> -    strbuf_release(&new_events);
>>>>>> -    return new_str;
>>>>>> -}
>>>>>> -
>>>>>> -static void set_pmu_unmatched_events(struct evlist *evlist, int
>>>>>> group_idx,
>>>>>> -                     char *pmu_name,
>>>>>> -                     unsigned long *evlist_removed)
>>>>>> -{
>>>>>> -    struct evsel *evsel, *pos;
>>>>>> -    int i = 0, j = 0;
>>>>>> -
>>>>>> -    /*
>>>>>> -     * Move to the first evsel of a given group
>>>>>> -     */
>>>>>> -    evlist__for_each_entry(evlist, evsel) {
>>>>>> -        if (evsel__is_group_leader(evsel) &&
>>>>>> -            evsel->core.nr_members >= 1) {
>>>>>> -            if (i < group_idx) {
>>>>>> -                j += evsel->core.nr_members;
>>>>>> -                i++;
>>>>>> -                continue;
>>>>>> -            }
>>>>>> -        }
>>>>>> -    }
>>>>>> -
>>>>>> -    i = 0;
>>>>>> -    evlist__for_each_entry(evlist, evsel) {
>>>>>> -        if (i < j) {
>>>>>> -            i++;
>>>>>> -            continue;
>>>>>> -        }
>>>>>> -
>>>>>> -        /*
>>>>>> -         * Now we are at the first evsel in the group
>>>>>> -         */
>>>>>> -        for_each_group_evsel(pos, evsel) {
>>>>>> -            if (evsel__is_hybrid(pos) &&
>>>>>> -                strcmp(pos->pmu_name, pmu_name)) {
>>>>>> -                set_bit(pos->core.idx, evlist_removed);
>>>>>> -            }
>>>>>> -        }
>>>>>> -        break;
>>>>>> -    }
>>>>>> -}
>>>>>> -
>>>>>> -static void remove_pmu_umatched_events(struct evlist *evlist,
>>>>>> char *metric_pmus)
>>>>>> -{
>>>>>> -    struct evsel *evsel, *tmp, *new_leader = NULL;
>>>>>> -    unsigned long *evlist_removed;
>>>>>> -    char *llist, *nlist, *p1;
>>>>>> -    bool need_new_leader = false;
>>>>>> -    int i = 0, new_nr_members = 0;
>>>>>> -
>>>>>> -    nlist = strdup(metric_pmus);
>>>>>> -    if (!nlist)
>>>>>> -        return;
>>>>>> -
>>>>>> -    evlist_removed = bitmap_zalloc(evlist->core.nr_entries);
>>>>>> -    if (!evlist_removed) {
>>>>>> -        free(nlist);
>>>>>> -        return;
>>>>>> -    }
>>>>>> -
>>>>>> -    llist = nlist;
>>>>>> -    while ((p1 = strsep(&llist, ",")) != NULL) {
>>>>>> -        if (strlen(p1) > 0) {
>>>>>> -            /*
>>>>>> -             * p1 points to the string of pmu name, e.g. "cpu_atom".
>>>>>> -             * The metric group string has pmu suffixes, e.g.
>>>>>> -             * "{inst_retired.any,cpu_clk_unhalted.thread}:W#cpu_core,
>>>>>> -             *  {cpu_clk_unhalted.core,inst_retired.any_p}:W#cpu_atom"
>>>>>> -             * By counting the pmu name, we can know the index of
>>>>>> -             * group.
>>>>>> -             */
>>>>>> -            set_pmu_unmatched_events(evlist, i++, p1,
>>>>>> -                         evlist_removed);
>>>>>> -        }
>>>>>> -    }
>>>>>> -
>>>>>> -    evlist__for_each_entry_safe(evlist, tmp, evsel) {
>>>>>> -        if (test_bit(evsel->core.idx, evlist_removed)) {
>>>>>> -            if (!evsel__is_group_leader(evsel)) {
>>>>>> -                if (!need_new_leader) {
>>>>>> -                    if (new_leader)
>>>>>> -                        new_leader->core.leader->nr_members--;
>>>>>> -                    else
>>>>>> -                        evsel->core.leader->nr_members--;
>>>>>> -                } else
>>>>>> -                    new_nr_members--;
>>>>>> -            } else {
>>>>>> -                /*
>>>>>> -                 * If group leader is to remove, we need to
>>>>>> -                 * prepare a new leader and adjust all group
>>>>>> -                 * members.
>>>>>> -                 */
>>>>>> -                need_new_leader = true;
>>>>>> -                new_nr_members =
>>>>>> -                    evsel->core.leader->nr_members - 1;
>>>>>> -            }
>>>>>> -
>>>>>> -            evlist__remove(evlist, evsel);
>>>>>> -            evsel__delete(evsel);
>>>>>> -        } else {
>>>>>> -            if (!evsel__is_group_leader(evsel)) {
>>>>>> -                if (need_new_leader) {
>>>>>> -                    need_new_leader = false;
>>>>>> -                    new_leader = evsel;
>>>>>> -                    new_leader->core.leader =
>>>>>> -                        &new_leader->core;
>>>>>> -                    new_leader->core.nr_members =
>>>>>> -                        new_nr_members;
>>>>>> -                } else if (new_leader)
>>>>>> -                    evsel->core.leader = &new_leader->core;
>>>>>> -            } else {
>>>>>> -                need_new_leader = false;
>>>>>> -                new_leader = NULL;
>>>>>> -            }
>>>>>> -        }
>>>>>> -    }
>>>>>> -
>>>>>> -    bitmap_free(evlist_removed);
>>>>>> -    free(nlist);
>>>>>> -}
>>>>>> -
>>>>>>    /**
>>>>>>     * parse_ids - Build the event string for the ids and parse
>>>>>> them creating an
>>>>>>     *             evlist. The encoded metric_ids are decoded.
>>>>>> @@ -1521,18 +1319,14 @@ static void
>>>>>> remove_pmu_umatched_events(struct evlist *evlist, char
>>>>>> *metric_pmus)
>>>>>>     * @modifier: any modifiers added to the events.
>>>>>>     * @has_constraint: false if events should be placed in a weak group.
>>>>>>     * @out_evlist: the created list of events.
>>>>>> - * @pmu_name: the name of the CPU.
>>>>>>     */
>>>>>>    static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>>>>>>                 struct expr_parse_ctx *ids, const char *modifier,
>>>>>> -             bool has_constraint, struct evlist **out_evlist,
>>>>>> -             const char *pmu_name)
>>>>>> +             bool has_constraint, struct evlist **out_evlist)
>>>>>>    {
>>>>>>        struct parse_events_error parse_error;
>>>>>>        struct evlist *parsed_evlist;
>>>>>>        struct strbuf events = STRBUF_INIT;
>>>>>> -    struct strbuf metric_pmus = STRBUF_INIT;
>>>>>> -    char *nlist = NULL;
>>>>>>        int ret;
>>>>>>        *out_evlist = NULL;
>>>>>> @@ -1559,7 +1353,7 @@ static int parse_ids(bool metric_no_merge,
>>>>>> struct perf_pmu *fake_pmu,
>>>>>>            ids__insert(ids->ids, tmp);
>>>>>>        }
>>>>>>        ret = metricgroup__build_event_string(&events, ids, modifier,
>>>>>> -                          has_constraint, pmu_name);
>>>>>> +                          has_constraint);
>>>>>>        if (ret)
>>>>>>            return ret;
>>>>>> @@ -1570,20 +1364,11 @@ static int parse_ids(bool
>>>>>> metric_no_merge, struct perf_pmu *fake_pmu,
>>>>>>        }
>>>>>>        pr_debug("Parsing metric events '%s'\n", events.buf);
>>>>>>        parse_events_error__init(&parse_error);
>>>>>> -    nlist = get_metric_pmus(events.buf, &metric_pmus);
>>>>>> -    if (!nlist) {
>>>>>> -        ret = -ENOMEM;
>>>>>> -        goto err_out;
>>>>>> -    }
>>>>>> -    ret = __parse_events(parsed_evlist, nlist, &parse_error, fake_pmu);
>>>>>> +    ret = __parse_events(parsed_evlist, events.buf,
>>>>>> &parse_error, fake_pmu);
>>>>>>        if (ret) {
>>>>>>            parse_events_error__print(&parse_error, events.buf);
>>>>>>            goto err_out;
>>>>>>        }
>>>>>> -
>>>>>> -    if (metric_pmus.alloc)
>>>>>> -        remove_pmu_umatched_events(parsed_evlist, metric_pmus.buf);
>>>>>> -
>>>>>>        ret = decode_all_metric_ids(parsed_evlist, modifier);
>>>>>>        if (ret)
>>>>>>            goto err_out;
>>>>>> @@ -1591,12 +1376,9 @@ static int parse_ids(bool
>>>>>> metric_no_merge, struct perf_pmu *fake_pmu,
>>>>>>        *out_evlist = parsed_evlist;
>>>>>>        parsed_evlist = NULL;
>>>>>>    err_out:
>>>>>> -    if (nlist)
>>>>>> -        free(nlist);
>>>>>>        parse_events_error__exit(&parse_error);
>>>>>>        evlist__delete(parsed_evlist);
>>>>>>        strbuf_release(&events);
>>>>>> -    strbuf_release(&metric_pmus);
>>>>>>        return ret;
>>>>>>    }
>>>>>> @@ -1615,8 +1397,7 @@ static int parse_groups(struct evlist
>>>>>> *perf_evlist, const char *str,
>>>>>>        if (metric_events_list->nr_entries == 0)
>>>>>>            metricgroup__rblist_init(metric_events_list);
>>>>>>        ret = metricgroup__add_metric_list(str, metric_no_group,
>>>>>> -                       &metric_list, map,
>>>>>> -                       perf_evlist->hybrid_pmu_name);
>>>>>> +                       &metric_list, map);
>>>>>>        if (ret)
>>>>>>            goto out;
>>>>>> @@ -1632,8 +1413,7 @@ static int parse_groups(struct evlist
>>>>>> *perf_evlist, const char *str,
>>>>>>                ret = parse_ids(metric_no_merge, fake_pmu, combined,
>>>>>>                        /*modifier=*/NULL,
>>>>>>                        /*has_constraint=*/true,
>>>>>> -                    &combined_evlist,
>>>>>> -                    perf_evlist->hybrid_pmu_name);
>>>>>> +                    &combined_evlist);
>>>>>>            }
>>>>>>            if (combined)
>>>>>>                expr__ctx_free(combined);
>>>>>> @@ -1670,9 +1450,6 @@ static int parse_groups(struct evlist
>>>>>> *perf_evlist, const char *str,
>>>>>>                        continue;
>>>>>>                    if (expr__subset_of_ids(n->pctx, m->pctx)) {
>>>>>> -                    if (m->pmu_name && n->pmu_name
>>>>>> -                        && strcmp(m->pmu_name, n->pmu_name))
>>>>>> -                        continue;
>>>>>>                        pr_debug("Events in '%s' fully contained
>>>>>> within '%s'\n",
>>>>>>                             m->metric_name, n->metric_name);
>>>>>>                        metric_evlist = n->evlist;
>>>>>> @@ -1682,16 +1459,14 @@ static int parse_groups(struct evlist
>>>>>> *perf_evlist, const char *str,
>>>>>>                }
>>>>>>            }
>>>>>>            if (!metric_evlist) {
>>>>>> -            ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
>>>>>> -                      m->modifier, m->has_constraint,
>>>>>> -                      &m->evlist, m->pmu_name);
>>>>>> +            ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
>>>>>> m->modifier,
>>>>>> +                    m->has_constraint, &m->evlist);
>>>>>>                if (ret)
>>>>>>                    goto out;
>>>>>>                metric_evlist = m->evlist;
>>>>>>            }
>>>>>> -        ret = setup_metric_events(m->pctx->ids, metric_evlist,
>>>>>> -                      &metric_events, m->pmu_name);
>>>>>> +        ret = setup_metric_events(m->pctx->ids, metric_evlist,
>>>>>> &metric_events);
>>>>>>            if (ret) {
>>>>>>                pr_debug("Cannot resolve IDs for %s: %s\n",
>>>>>>                    m->metric_name, m->metric_expr);
>>>>>> diff --git a/tools/perf/util/stat-display.c
>>>>>> b/tools/perf/util/stat-display.c
>>>>>> index 13f705737367..98669ca5a86b 100644
>>>>>> --- a/tools/perf/util/stat-display.c
>>>>>> +++ b/tools/perf/util/stat-display.c
>>>>>> @@ -539,8 +539,7 @@ static void aggr_update_shadow(struct
>>>>>> perf_stat_config *config,
>>>>>>        }
>>>>>>    }
>>>>>> -static void uniquify_event_name(struct evsel *counter,
>>>>>> -                struct perf_stat_config *stat_config)
>>>>>> +static void uniquify_event_name(struct evsel *counter)
>>>>>>    {
>>>>>>        char *new_name;
>>>>>>        char *config;
>>>>>> @@ -559,8 +558,7 @@ static void uniquify_event_name(struct evsel
>>>>>> *counter,
>>>>>>                counter->name = new_name;
>>>>>>            }
>>>>>>        } else {
>>>>>> -        if (perf_pmu__has_hybrid() &&
>>>>>> -            stat_config->metric_events.nr_entries == 0) {
>>>>>> +        if (perf_pmu__has_hybrid()) {
>>>>>>                ret = asprintf(&new_name, "%s/%s/",
>>>>>>                           counter->pmu_name, counter->name);
>>>>>>            } else {
>>>>>> @@ -634,7 +632,7 @@ static bool collect_data(struct
>>>>>> perf_stat_config *config, struct evsel *counter,
>>>>>>            return false;
>>>>>>        cb(config, counter, data, true);
>>>>>>        if (config->no_merge || hybrid_merge(counter, config, false))
>>>>>> -        uniquify_event_name(counter, config);
>>>>>> +        uniquify_event_name(counter);
>>>>>>        else if (counter->auto_merge_stats ||
>>>>>> hybrid_merge(counter, config, true))
>>>>>>            collect_all_aliases(config, counter, cb, data);
>>>>>>        return true;
>>>>>> --
>>>>>> 2.36.0.512.ge40c2bad7a-goog
>>>>>
>>>
>>> --
>>> Zhengjun Xing
>>
>> --
>>
>> - Arnaldo

-- 
Zhengjun Xing

Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

[Ian Rogers]

On Tue, May 10, 2022 at 7:14 PM Xing Zhengjun
<zhengjun.xing@linux.intel.com> wrote:
>
> On 5/11/2022 12:45 AM, Ian Rogers wrote:
> > On Tue, May 10, 2022 at 8:49 AM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> >>
> >> Em Tue, May 10, 2022 at 05:31:37PM +0800, Xing Zhengjun escreveu:
> >>> On 5/10/2022 5:55 AM, Liang, Kan wrote:
> >>>> On 5/9/2022 9:12 AM, Arnaldo Carvalho de Melo wrote:
> >>>>> Em Fri, May 06, 2022 at 10:34:06PM -0700, Ian Rogers escreveu:
> >>>>>> This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.
> >>
> >>>>> I picked this from the cover letter and added to this revert, to justify
> >>>>> it:
> >>
> >>>>> "Hybrid metrics place a PMU at the end of the parse string. This is
> >>>>> also where tool events are placed. The behavior of the parse string
> >>>>> isn't clear and so revert the change for now."
> >>
> >>>> I think the original patch used a "#" to indicate the PMU name, which
> >>>> can be used to distinguish between the tool events and the PMU name.
> >>>> To be honest, I'm not sure what's unclear here. Could you please clarify?
> >>
> >>>> With this revert, the issue mentioned in the original patch must be
> >>>> broken on ADL. I don't see a replacement fix in this patch series.
> >>>> Could you please propose a solution for the issue to replace the #PMU
> >>>> name solution?
> >>
> >>>> Thanks,
> >>>> Kan
> >>
> >>> I am surprised the origin patch is reverted during discussion and though the
> >>> discussion still has no conclusion.
> >>
> >> Yeah, the situation is not a good one, it was my mistake to merge the
> >> patch in the first place without giving time for people like Ian, that
> >> works a lot in the metrics code to voice his concerns, to review it.
> >>
> >> So I decided to revert to give more time for interested parties to
> >> discuss it further, again, I'm sorry about that.
> >>
> >> - Arnaldo
> >
> > Thanks, I appreciate the time to discuss and get this done in a more proper way.
> >
> >>> Let me introduce the purpose of the origin patch.
> >>> For a hybrid system such as ADL, if both the metrics and the formula are
> >>> different for the different PMUs, without this patch, the metric and event
> >>> parser should work ok, nothing should be special for the hybrid. In fact,
> >>> both "cpu_core" and "cpu_atom" may have the same metrics--the same metric
> >>> name, even the same formula for the metrics. For example, both "cpu_core"
> >>> and "cpu_atom" have metrics "IpBranch" and "IPC", For "IpBranch", both
> >>> "cpu_core" and "cpu_atom" has the same metric name and their formula also is
> >>> the same, the event name is the same though they belong to different PMUs.
> >>> The old metric and event parser can not handle this kind of metric, that's
> >>> why we need this patch.
> >>>
> >>>      "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
> >>>      "MetricName": "IpBranch",
> >>>      "Unit": "cpu_core"
> >>>
> >>>      "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
> >>>      "MetricName": "IpBranch",
> >>>      "Unit": "cpu_atom"
> >>>
> >>>
> >>>     "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> >>>     "MetricName": "IPC",
> >>>     "Unit": "cpu_core"
> >>>
> >>>     "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.CORE",
> >>>     "MetricName": "IPC",
> >>>     "Unit": "cpu_atom"
> >>>
> >>> Except for the above two metrics, there are still a lot of similar metrics,
> >>> "CPU_Utilization"...
> >>>
> >>> The original patch expanded the metric group string by adding "#PMU_name" to
> >>> indicate the PMU name, which can be used to distinguish between the tool
> >>> events and the PMU name, then the metric and event parser can parse the
> >>> right PMU for the events.
> >>>
> >>> With the patch all the ADL metrics can pass, without the patch, a lot of
> >>> metrics will fail. I don't think it's a good idea to revert it before the
> >>> new solution is proposed.
> >>>
> >>>>
> >>>>> I apologise for not having provided ample time for reviewing this patch,
> >>>>> sorry about that.
> >>>>>
> >>>>> - Arnaldo
> >>>>>> Signed-off-by: Ian Rogers <irogers@google.com>
> >>>>>> ---
> >>>>>>    tools/perf/util/metricgroup.c  | 263 +++------------------------------
> >>>>>>    tools/perf/util/stat-display.c |   8 +-
> >>>>>>    2 files changed, 22 insertions(+), 249 deletions(-)
> >>>>>>
> >>>>>> diff --git a/tools/perf/util/metricgroup.c
> >>>>>> b/tools/perf/util/metricgroup.c
> >>>>>> index 126a43a8917e..d8492e339521 100644
> >>>>>> --- a/tools/perf/util/metricgroup.c
> >>>>>> +++ b/tools/perf/util/metricgroup.c
> >>>>>> @@ -141,11 +141,6 @@ struct metric {
> >>>>>>         * output.
> >>>>>>         */
> >>>>>>        const char *metric_unit;
> >>>>>> -    /**
> >>>>>> -     * The name of the CPU such as "cpu_core" or "cpu_atom" in
> >>>>>> hybrid systems
> >>>>>> -     * and "NULL" in non-hybrid systems.
> >>>>>> -     */
> >>>>>> -    const char *pmu_name;
> >>>>>>        /** Optional null terminated array of referenced metrics. */
> >>>>>>        struct metric_ref *metric_refs;
> >>>>>>        /**
> >>>>>> @@ -220,7 +215,6 @@ static struct metric *metric__new(const
> >>>>>> struct pmu_event *pe,
> >>>>>>        }
> >>>>>>        m->metric_expr = pe->metric_expr;
> >>>>>>        m->metric_unit = pe->unit;
> >>>>>> -    m->pmu_name = pe->pmu;
> >>>>>>        m->pctx->runtime = runtime;
> >>>>>>        m->has_constraint = metric_no_group ||
> >>>>>> metricgroup__has_constraint(pe);
> >>>>>>        m->metric_refs = NULL;
> >>>>>> @@ -256,12 +250,10 @@ static bool contains_metric_id(struct
> >>>>>> evsel **metric_events, int num_events,
> >>>>>>     * @ids: the metric IDs to match.
> >>>>>>     * @metric_evlist: the list of perf events.
> >>>>>>     * @out_metric_events: holds the created metric events array.
> >>>>>> - * @pmu_name: the name of the CPU.
> >>>>>>     */
> >>>>>>    static int setup_metric_events(struct hashmap *ids,
> >>>>>>                       struct evlist *metric_evlist,
> >>>>>> -                   struct evsel ***out_metric_events,
> >>>>>> -                   const char *pmu_name)
> >>>>>> +                   struct evsel ***out_metric_events)
> >>>>>>    {
> >>>>>>        struct evsel **metric_events;
> >>>>>>        const char *metric_id;
> >>>>>> @@ -294,10 +286,6 @@ static int setup_metric_events(struct hashmap *ids,
> >>>>>>             * about this event.
> >>>>>>             */
> >>>>>>            if (hashmap__find(ids, metric_id, (void **)&val_ptr)) {
> >>>>>> -            if (evsel__is_hybrid(ev) && pmu_name &&
> >>>>>> -                strcmp(pmu_name, ev->pmu_name)) {
> >>>>>> -                continue;
> >>>>>> -            }
> >>>>>>                metric_events[matched_events++] = ev;
> >>>>>>                if (matched_events >= ids_size)
> >>>>>> @@ -736,8 +724,7 @@ static int decode_all_metric_ids(struct
> >>>>>> evlist *perf_evlist, const char *modifie
> >>>>>>    static int metricgroup__build_event_string(struct strbuf *events,
> >>>>>>                           const struct expr_parse_ctx *ctx,
> >>>>>>                           const char *modifier,
> >>>>>> -                       bool has_constraint,
> >>>>>> -                       const char *pmu_name)
> >>>>>> +                       bool has_constraint)
> >>>>>>    {
> >>>>>>        struct hashmap_entry *cur;
> >>>>>>        size_t bkt;
> >>>>>> @@ -819,18 +806,12 @@ static int
> >>>>>> metricgroup__build_event_string(struct strbuf *events,
> >>>>>>            if (no_group) {
> >>>>>>                /* Strange case of a metric of just duration_time. */
> >>>>>>                ret = strbuf_addf(events, "duration_time");
> >>>>>> -        } else if (!has_constraint) {
> >>>>>> -            ret = strbuf_addf(events, "}:W");
> >>>>>> -            if (pmu_name)
> >>>>>> -                ret = strbuf_addf(events, "#%s", pmu_name);
> >>>>>> -            ret = strbuf_addf(events, ",duration_time");
> >>>>>> -        } else
> >>>>>> +        } else if (!has_constraint)
> >>>>>> +            ret = strbuf_addf(events, "}:W,duration_time");
> >>>>>> +        else
> >>>>>>                ret = strbuf_addf(events, ",duration_time");
> >>>>>> -    } else if (!no_group && !has_constraint) {
> >>>>>> +    } else if (!no_group && !has_constraint)
> >>>>>>            ret = strbuf_addf(events, "}:W");
> >>>>>> -        if (pmu_name)
> >>>>>> -            ret = strbuf_addf(events, "#%s", pmu_name);
> >>>>>> -    }
> >
> > To Zhengjun's point about adding the #pmu_name done above: the code is
> > conditionally adding the #pmu_name but only when events are grouped,
> > this means that the behavior with the command line option
> > --metric-no-group is different as in that case no #pmu_name will be
> > added (there's no group). In that case how do the events choose the
> > correct PMU? While this may work for existing Alderlake metrics I
> > don't expect it to work in the future or with command line options.
> > The problem reminds me of modifiers on metrics where we rewrite events
> > to add in the modifiers:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/metricgroup.c?h=perf/core#n703
> > The logic here is also used in event deduplication for things like the
> > combined event list and I don't understand how #pmu_name is expected
> > to work in that context. It is only partially working here.
>
> I just build perf based on perf/core branch, the head commit is
> 280c36d26eb8 (perf test: Add skip to --per-thread test), it is just
> before the reverted commit. I test with the "--metric-no-group" option,
> and it still works ok.
>
> ./perf -v
> perf version 5.18.rc4.g280c36d26eb8
>
> ./perf stat -v -M "IpBranch" --metric-no-group -a sleep 1
> Using CPUID GenuineIntel-6-97-2
> metric expr INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES for IpBranch
> metric expr INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES for IpBranch
> found event duration_time
> found event BR_INST_RETIRED.ALL_BRANCHES
> found event INST_RETIRED.ANY
> Parsing metric events
> '{BR_INST_RETIRED.ALL_BRANCHES/metric-id=BR_INST_RETIRED.ALL_BRANCHES/,INST_RETIRED.ANY/metric-id=INST_RETIRED.ANY/}:W#cpu_atom,duration_time'

You specified --metric-no-group but the events here are grouped. I
wouldn't call this a success, just evidence of more things being
broken. Note, this is the only logical conclusion of how this could
work as you don't add a #pmu_type for ungrouped events when building
the event string to parse.

Thanks,
Ian

> BR_INST_RETIRED.ALL_BRANCHES -> cpu_core/period=0x61a89,event=0xc4/
> BR_INST_RETIRED.ALL_BRANCHES -> cpu_atom/period=0x30d43,event=0xc4/
> INST_RETIRED.ANY -> cpu_core/event=0xc0,period=0x1e8483/
> INST_RETIRED.ANY -> cpu_atom/event=0xc0,period=0x1e8483/
> found event duration_time
> found event BR_INST_RETIRED.ALL_BRANCHES
> found event INST_RETIRED.ANY
> Parsing metric events
> '{BR_INST_RETIRED.ALL_BRANCHES/metric-id=BR_INST_RETIRED.ALL_BRANCHES/,INST_RETIRED.ANY/metric-id=INST_RETIRED.ANY/}:W#cpu_core,duration_time'
> BR_INST_RETIRED.ALL_BRANCHES -> cpu_core/period=0x61a89,event=0xc4/
> BR_INST_RETIRED.ALL_BRANCHES -> cpu_atom/period=0x30d43,event=0xc4/
> INST_RETIRED.ANY -> cpu_core/event=0xc0,period=0x1e8483/
> INST_RETIRED.ANY -> cpu_atom/event=0xc0,period=0x1e8483/
> Control descriptor is not initialized
> BR_INST_RETIRED.ALL_BRANCHES: 118542 8028259379 8028259379
> INST_RETIRED.ANY: 625378 8028259379 8028259379
> duration_time: 1003654656 1003654656 1003654656
> BR_INST_RETIRED.ALL_BRANCHES: 1740268 16050148595 16050148595
> INST_RETIRED.ANY: 8463993 16050148595 16050148595
> duration_time: 1003654656 1003654656 1003654656
>
>   Performance counter stats for 'system wide':
>
>             118,542      BR_INST_RETIRED.ALL_BRANCHES [cpu_atom] #
> 5.28 IpBranch
>             625,378      INST_RETIRED.ANY [cpu_atom]
>       1,003,654,656 ns   duration_time
>           1,740,268      BR_INST_RETIRED.ALL_BRANCHES [cpu_core] #
> 4.86 IpBranch
>           8,463,993      INST_RETIRED.ANY [cpu_core]
>       1,003,654,656 ns   duration_time
>
>         1.003654656 seconds time elapsed
>
> >
> > Thanks,
> > Ian
> >
> >>>>>>        return ret;
> >>>>>>    #undef RETURN_IF_NON_ZERO
> >>>>>> @@ -1169,13 +1150,11 @@ static int metric_list_cmp(void *priv
> >>>>>> __maybe_unused, const struct list_head *l,
> >>>>>>     * @metric_list: The list that the metric or metric group are
> >>>>>> added to.
> >>>>>>     * @map: The map that is searched for metrics, most commonly
> >>>>>> the table for the
> >>>>>>     *       architecture perf is running upon.
> >>>>>> - * @pmu_name: the name of the CPU.
> >>>>>>     */
> >>>>>> -static int metricgroup__add_metric(const char *metric_name,
> >>>>>> -                   const char *modifier, bool metric_no_group,
> >>>>>> +static int metricgroup__add_metric(const char *metric_name,
> >>>>>> const char *modifier,
> >>>>>> +                   bool metric_no_group,
> >>>>>>                       struct list_head *metric_list,
> >>>>>> -                   const struct pmu_events_map *map,
> >>>>>> -                   const char *pmu_name)
> >>>>>> +                   const struct pmu_events_map *map)
> >>>>>>    {
> >>>>>>        const struct pmu_event *pe;
> >>>>>>        LIST_HEAD(list);
> >>>>>> @@ -1188,8 +1167,6 @@ static int metricgroup__add_metric(const
> >>>>>> char *metric_name,
> >>>>>>         */
> >>>>>>        map_for_each_metric(pe, i, map, metric_name) {
> >>>>>>            has_match = true;
> >>>>>> -        if (pmu_name && pe->pmu && strcmp(pmu_name, pe->pmu))
> >>>>>> -            continue;
> >>>>>>            ret = add_metric(&list, pe, modifier, metric_no_group,
> >>>>>>                     /*root_metric=*/NULL,
> >>>>>>                     /*visited_metrics=*/NULL, map);
> >>>>>> @@ -1238,12 +1215,10 @@ static int metricgroup__add_metric(const
> >>>>>> char *metric_name,
> >>>>>>     * @metric_list: The list that metrics are added to.
> >>>>>>     * @map: The map that is searched for metrics, most commonly
> >>>>>> the table for the
> >>>>>>     *       architecture perf is running upon.
> >>>>>> - * @pmu_name: the name of the CPU.
> >>>>>>     */
> >>>>>>    static int metricgroup__add_metric_list(const char *list, bool
> >>>>>> metric_no_group,
> >>>>>>                        struct list_head *metric_list,
> >>>>>> -                    const struct pmu_events_map *map,
> >>>>>> -                    const char *pmu_name)
> >>>>>> +                    const struct pmu_events_map *map)
> >>>>>>    {
> >>>>>>        char *list_itr, *list_copy, *metric_name, *modifier;
> >>>>>>        int ret, count = 0;
> >>>>>> @@ -1260,7 +1235,7 @@ static int
> >>>>>> metricgroup__add_metric_list(const char *list, bool
> >>>>>> metric_no_group,
> >>>>>>            ret = metricgroup__add_metric(metric_name, modifier,
> >>>>>>                              metric_no_group, metric_list,
> >>>>>> -                          map, pmu_name);
> >>>>>> +                          map);
> >>>>>>            if (ret == -EINVAL)
> >>>>>>                pr_err("Cannot find metric or group `%s'\n", metric_name);
> >>>>>> @@ -1335,183 +1310,6 @@ static int build_combined_expr_ctx(const
> >>>>>> struct list_head *metric_list,
> >>>>>>        return ret;
> >>>>>>    }
> >>>>>> -static char *get_metric_pmus(char *orig_str, struct strbuf
> >>>>>> *metric_pmus)
> >>>>>> -{
> >>>>>> -    char *llist, *nlist, *p1, *p2, *new_str = NULL;
> >>>>>> -    int ret;
> >>>>>> -    struct strbuf new_events;
> >>>>>> -
> >>>>>> -    if (!strchr(orig_str, '#')) {
> >>>>>> -        /*
> >>>>>> -         * pmu name is added after '#'. If no '#' found,
> >>>>>> -         * don't need to process pmu.
> >>>>>> -         */
> >>>>>> -        return strdup(orig_str);
> >>>>>> -    }
> >>>>>> -
> >>>>>> -    nlist = strdup(orig_str);
> >>>>>> -    if (!nlist)
> >>>>>> -        return new_str;
> >>>>>> -
> >>>>>> -    ret = strbuf_init(&new_events, 100);
> >>>>>> -    if (ret)
> >>>>>> -        goto err_out;
> >>>>>> -
> >>>>>> -    ret = strbuf_grow(metric_pmus, 100);
> >>>>>> -    if (ret)
> >>>>>> -        goto err_out;
> >>>>>> -
> >>>>>> -    llist = nlist;
> >>>>>> -    while ((p1 = strsep(&llist, ",")) != NULL) {
> >>>>>> -        p2 = strchr(p1, '#');
> >>>>>> -        if (p2) {
> >>>>>> -            *p2 = 0;
> >>>>>> -            ret = strbuf_addf(&new_events, "%s,", p1);
> >>>>>> -            if (ret)
> >>>>>> -                goto err_out;
> >>>>>> -
> >>>>>> -            ret = strbuf_addf(metric_pmus, "%s,", p2 + 1);
> >>>>>> -            if (ret)
> >>>>>> -                goto err_out;
> >>>>>> -
> >>>>>> -        } else {
> >>>>>> -            ret = strbuf_addf(&new_events, "%s,", p1);
> >>>>>> -            if (ret)
> >>>>>> -                goto err_out;
> >>>>>> -        }
> >>>>>> -    }
> >>>>>> -
> >>>>>> -    new_str = strdup(new_events.buf);
> >>>>>> -    if (new_str) {
> >>>>>> -        /* Remove last ',' */
> >>>>>> -        new_str[strlen(new_str) - 1] = 0;
> >>>>>> -    }
> >>>>>> -err_out:
> >>>>>> -    free(nlist);
> >>>>>> -    strbuf_release(&new_events);
> >>>>>> -    return new_str;
> >>>>>> -}
> >>>>>> -
> >>>>>> -static void set_pmu_unmatched_events(struct evlist *evlist, int
> >>>>>> group_idx,
> >>>>>> -                     char *pmu_name,
> >>>>>> -                     unsigned long *evlist_removed)
> >>>>>> -{
> >>>>>> -    struct evsel *evsel, *pos;
> >>>>>> -    int i = 0, j = 0;
> >>>>>> -
> >>>>>> -    /*
> >>>>>> -     * Move to the first evsel of a given group
> >>>>>> -     */
> >>>>>> -    evlist__for_each_entry(evlist, evsel) {
> >>>>>> -        if (evsel__is_group_leader(evsel) &&
> >>>>>> -            evsel->core.nr_members >= 1) {
> >>>>>> -            if (i < group_idx) {
> >>>>>> -                j += evsel->core.nr_members;
> >>>>>> -                i++;
> >>>>>> -                continue;
> >>>>>> -            }
> >>>>>> -        }
> >>>>>> -    }
> >>>>>> -
> >>>>>> -    i = 0;
> >>>>>> -    evlist__for_each_entry(evlist, evsel) {
> >>>>>> -        if (i < j) {
> >>>>>> -            i++;
> >>>>>> -            continue;
> >>>>>> -        }
> >>>>>> -
> >>>>>> -        /*
> >>>>>> -         * Now we are at the first evsel in the group
> >>>>>> -         */
> >>>>>> -        for_each_group_evsel(pos, evsel) {
> >>>>>> -            if (evsel__is_hybrid(pos) &&
> >>>>>> -                strcmp(pos->pmu_name, pmu_name)) {
> >>>>>> -                set_bit(pos->core.idx, evlist_removed);
> >>>>>> -            }
> >>>>>> -        }
> >>>>>> -        break;
> >>>>>> -    }
> >>>>>> -}
> >>>>>> -
> >>>>>> -static void remove_pmu_umatched_events(struct evlist *evlist,
> >>>>>> char *metric_pmus)
> >>>>>> -{
> >>>>>> -    struct evsel *evsel, *tmp, *new_leader = NULL;
> >>>>>> -    unsigned long *evlist_removed;
> >>>>>> -    char *llist, *nlist, *p1;
> >>>>>> -    bool need_new_leader = false;
> >>>>>> -    int i = 0, new_nr_members = 0;
> >>>>>> -
> >>>>>> -    nlist = strdup(metric_pmus);
> >>>>>> -    if (!nlist)
> >>>>>> -        return;
> >>>>>> -
> >>>>>> -    evlist_removed = bitmap_zalloc(evlist->core.nr_entries);
> >>>>>> -    if (!evlist_removed) {
> >>>>>> -        free(nlist);
> >>>>>> -        return;
> >>>>>> -    }
> >>>>>> -
> >>>>>> -    llist = nlist;
> >>>>>> -    while ((p1 = strsep(&llist, ",")) != NULL) {
> >>>>>> -        if (strlen(p1) > 0) {
> >>>>>> -            /*
> >>>>>> -             * p1 points to the string of pmu name, e.g. "cpu_atom".
> >>>>>> -             * The metric group string has pmu suffixes, e.g.
> >>>>>> -             * "{inst_retired.any,cpu_clk_unhalted.thread}:W#cpu_core,
> >>>>>> -             *  {cpu_clk_unhalted.core,inst_retired.any_p}:W#cpu_atom"
> >>>>>> -             * By counting the pmu name, we can know the index of
> >>>>>> -             * group.
> >>>>>> -             */
> >>>>>> -            set_pmu_unmatched_events(evlist, i++, p1,
> >>>>>> -                         evlist_removed);
> >>>>>> -        }
> >>>>>> -    }
> >>>>>> -
> >>>>>> -    evlist__for_each_entry_safe(evlist, tmp, evsel) {
> >>>>>> -        if (test_bit(evsel->core.idx, evlist_removed)) {
> >>>>>> -            if (!evsel__is_group_leader(evsel)) {
> >>>>>> -                if (!need_new_leader) {
> >>>>>> -                    if (new_leader)
> >>>>>> -                        new_leader->core.leader->nr_members--;
> >>>>>> -                    else
> >>>>>> -                        evsel->core.leader->nr_members--;
> >>>>>> -                } else
> >>>>>> -                    new_nr_members--;
> >>>>>> -            } else {
> >>>>>> -                /*
> >>>>>> -                 * If group leader is to remove, we need to
> >>>>>> -                 * prepare a new leader and adjust all group
> >>>>>> -                 * members.
> >>>>>> -                 */
> >>>>>> -                need_new_leader = true;
> >>>>>> -                new_nr_members =
> >>>>>> -                    evsel->core.leader->nr_members - 1;
> >>>>>> -            }
> >>>>>> -
> >>>>>> -            evlist__remove(evlist, evsel);
> >>>>>> -            evsel__delete(evsel);
> >>>>>> -        } else {
> >>>>>> -            if (!evsel__is_group_leader(evsel)) {
> >>>>>> -                if (need_new_leader) {
> >>>>>> -                    need_new_leader = false;
> >>>>>> -                    new_leader = evsel;
> >>>>>> -                    new_leader->core.leader =
> >>>>>> -                        &new_leader->core;
> >>>>>> -                    new_leader->core.nr_members =
> >>>>>> -                        new_nr_members;
> >>>>>> -                } else if (new_leader)
> >>>>>> -                    evsel->core.leader = &new_leader->core;
> >>>>>> -            } else {
> >>>>>> -                need_new_leader = false;
> >>>>>> -                new_leader = NULL;
> >>>>>> -            }
> >>>>>> -        }
> >>>>>> -    }
> >>>>>> -
> >>>>>> -    bitmap_free(evlist_removed);
> >>>>>> -    free(nlist);
> >>>>>> -}
> >>>>>> -
> >>>>>>    /**
> >>>>>>     * parse_ids - Build the event string for the ids and parse
> >>>>>> them creating an
> >>>>>>     *             evlist. The encoded metric_ids are decoded.
> >>>>>> @@ -1521,18 +1319,14 @@ static void
> >>>>>> remove_pmu_umatched_events(struct evlist *evlist, char
> >>>>>> *metric_pmus)
> >>>>>>     * @modifier: any modifiers added to the events.
> >>>>>>     * @has_constraint: false if events should be placed in a weak group.
> >>>>>>     * @out_evlist: the created list of events.
> >>>>>> - * @pmu_name: the name of the CPU.
> >>>>>>     */
> >>>>>>    static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
> >>>>>>                 struct expr_parse_ctx *ids, const char *modifier,
> >>>>>> -             bool has_constraint, struct evlist **out_evlist,
> >>>>>> -             const char *pmu_name)
> >>>>>> +             bool has_constraint, struct evlist **out_evlist)
> >>>>>>    {
> >>>>>>        struct parse_events_error parse_error;
> >>>>>>        struct evlist *parsed_evlist;
> >>>>>>        struct strbuf events = STRBUF_INIT;
> >>>>>> -    struct strbuf metric_pmus = STRBUF_INIT;
> >>>>>> -    char *nlist = NULL;
> >>>>>>        int ret;
> >>>>>>        *out_evlist = NULL;
> >>>>>> @@ -1559,7 +1353,7 @@ static int parse_ids(bool metric_no_merge,
> >>>>>> struct perf_pmu *fake_pmu,
> >>>>>>            ids__insert(ids->ids, tmp);
> >>>>>>        }
> >>>>>>        ret = metricgroup__build_event_string(&events, ids, modifier,
> >>>>>> -                          has_constraint, pmu_name);
> >>>>>> +                          has_constraint);
> >>>>>>        if (ret)
> >>>>>>            return ret;
> >>>>>> @@ -1570,20 +1364,11 @@ static int parse_ids(bool
> >>>>>> metric_no_merge, struct perf_pmu *fake_pmu,
> >>>>>>        }
> >>>>>>        pr_debug("Parsing metric events '%s'\n", events.buf);
> >>>>>>        parse_events_error__init(&parse_error);
> >>>>>> -    nlist = get_metric_pmus(events.buf, &metric_pmus);
> >>>>>> -    if (!nlist) {
> >>>>>> -        ret = -ENOMEM;
> >>>>>> -        goto err_out;
> >>>>>> -    }
> >>>>>> -    ret = __parse_events(parsed_evlist, nlist, &parse_error, fake_pmu);
> >>>>>> +    ret = __parse_events(parsed_evlist, events.buf,
> >>>>>> &parse_error, fake_pmu);
> >>>>>>        if (ret) {
> >>>>>>            parse_events_error__print(&parse_error, events.buf);
> >>>>>>            goto err_out;
> >>>>>>        }
> >>>>>> -
> >>>>>> -    if (metric_pmus.alloc)
> >>>>>> -        remove_pmu_umatched_events(parsed_evlist, metric_pmus.buf);
> >>>>>> -
> >>>>>>        ret = decode_all_metric_ids(parsed_evlist, modifier);
> >>>>>>        if (ret)
> >>>>>>            goto err_out;
> >>>>>> @@ -1591,12 +1376,9 @@ static int parse_ids(bool
> >>>>>> metric_no_merge, struct perf_pmu *fake_pmu,
> >>>>>>        *out_evlist = parsed_evlist;
> >>>>>>        parsed_evlist = NULL;
> >>>>>>    err_out:
> >>>>>> -    if (nlist)
> >>>>>> -        free(nlist);
> >>>>>>        parse_events_error__exit(&parse_error);
> >>>>>>        evlist__delete(parsed_evlist);
> >>>>>>        strbuf_release(&events);
> >>>>>> -    strbuf_release(&metric_pmus);
> >>>>>>        return ret;
> >>>>>>    }
> >>>>>> @@ -1615,8 +1397,7 @@ static int parse_groups(struct evlist
> >>>>>> *perf_evlist, const char *str,
> >>>>>>        if (metric_events_list->nr_entries == 0)
> >>>>>>            metricgroup__rblist_init(metric_events_list);
> >>>>>>        ret = metricgroup__add_metric_list(str, metric_no_group,
> >>>>>> -                       &metric_list, map,
> >>>>>> -                       perf_evlist->hybrid_pmu_name);
> >>>>>> +                       &metric_list, map);
> >>>>>>        if (ret)
> >>>>>>            goto out;
> >>>>>> @@ -1632,8 +1413,7 @@ static int parse_groups(struct evlist
> >>>>>> *perf_evlist, const char *str,
> >>>>>>                ret = parse_ids(metric_no_merge, fake_pmu, combined,
> >>>>>>                        /*modifier=*/NULL,
> >>>>>>                        /*has_constraint=*/true,
> >>>>>> -                    &combined_evlist,
> >>>>>> -                    perf_evlist->hybrid_pmu_name);
> >>>>>> +                    &combined_evlist);
> >>>>>>            }
> >>>>>>            if (combined)
> >>>>>>                expr__ctx_free(combined);
> >>>>>> @@ -1670,9 +1450,6 @@ static int parse_groups(struct evlist
> >>>>>> *perf_evlist, const char *str,
> >>>>>>                        continue;
> >>>>>>                    if (expr__subset_of_ids(n->pctx, m->pctx)) {
> >>>>>> -                    if (m->pmu_name && n->pmu_name
> >>>>>> -                        && strcmp(m->pmu_name, n->pmu_name))
> >>>>>> -                        continue;
> >>>>>>                        pr_debug("Events in '%s' fully contained
> >>>>>> within '%s'\n",
> >>>>>>                             m->metric_name, n->metric_name);
> >>>>>>                        metric_evlist = n->evlist;
> >>>>>> @@ -1682,16 +1459,14 @@ static int parse_groups(struct evlist
> >>>>>> *perf_evlist, const char *str,
> >>>>>>                }
> >>>>>>            }
> >>>>>>            if (!metric_evlist) {
> >>>>>> -            ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
> >>>>>> -                      m->modifier, m->has_constraint,
> >>>>>> -                      &m->evlist, m->pmu_name);
> >>>>>> +            ret = parse_ids(metric_no_merge, fake_pmu, m->pctx,
> >>>>>> m->modifier,
> >>>>>> +                    m->has_constraint, &m->evlist);
> >>>>>>                if (ret)
> >>>>>>                    goto out;
> >>>>>>                metric_evlist = m->evlist;
> >>>>>>            }
> >>>>>> -        ret = setup_metric_events(m->pctx->ids, metric_evlist,
> >>>>>> -                      &metric_events, m->pmu_name);
> >>>>>> +        ret = setup_metric_events(m->pctx->ids, metric_evlist,
> >>>>>> &metric_events);
> >>>>>>            if (ret) {
> >>>>>>                pr_debug("Cannot resolve IDs for %s: %s\n",
> >>>>>>                    m->metric_name, m->metric_expr);
> >>>>>> diff --git a/tools/perf/util/stat-display.c
> >>>>>> b/tools/perf/util/stat-display.c
> >>>>>> index 13f705737367..98669ca5a86b 100644
> >>>>>> --- a/tools/perf/util/stat-display.c
> >>>>>> +++ b/tools/perf/util/stat-display.c
> >>>>>> @@ -539,8 +539,7 @@ static void aggr_update_shadow(struct
> >>>>>> perf_stat_config *config,
> >>>>>>        }
> >>>>>>    }
> >>>>>> -static void uniquify_event_name(struct evsel *counter,
> >>>>>> -                struct perf_stat_config *stat_config)
> >>>>>> +static void uniquify_event_name(struct evsel *counter)
> >>>>>>    {
> >>>>>>        char *new_name;
> >>>>>>        char *config;
> >>>>>> @@ -559,8 +558,7 @@ static void uniquify_event_name(struct evsel
> >>>>>> *counter,
> >>>>>>                counter->name = new_name;
> >>>>>>            }
> >>>>>>        } else {
> >>>>>> -        if (perf_pmu__has_hybrid() &&
> >>>>>> -            stat_config->metric_events.nr_entries == 0) {
> >>>>>> +        if (perf_pmu__has_hybrid()) {
> >>>>>>                ret = asprintf(&new_name, "%s/%s/",
> >>>>>>                           counter->pmu_name, counter->name);
> >>>>>>            } else {
> >>>>>> @@ -634,7 +632,7 @@ static bool collect_data(struct
> >>>>>> perf_stat_config *config, struct evsel *counter,
> >>>>>>            return false;
> >>>>>>        cb(config, counter, data, true);
> >>>>>>        if (config->no_merge || hybrid_merge(counter, config, false))
> >>>>>> -        uniquify_event_name(counter, config);
> >>>>>> +        uniquify_event_name(counter);
> >>>>>>        else if (counter->auto_merge_stats ||
> >>>>>> hybrid_merge(counter, config, true))
> >>>>>>            collect_all_aliases(config, counter, cb, data);
> >>>>>>        return true;
> >>>>>> --
> >>>>>> 2.36.0.512.ge40c2bad7a-goog
> >>>>>
> >>>
> >>> --
> >>> Zhengjun Xing
> >>
> >> --
> >>
> >> - Arnaldo
>
> --
> Zhengjun Xing

Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

[Namhyung Kim]

Hello,

On Tue, May 10, 2022 at 2:31 AM Xing Zhengjun
<zhengjun.xing@linux.intel.com> wrote:
>
>
>
> On 5/10/2022 5:55 AM, Liang, Kan wrote:
> >
> >
> > On 5/9/2022 9:12 AM, Arnaldo Carvalho de Melo wrote:
> >> Em Fri, May 06, 2022 at 10:34:06PM -0700, Ian Rogers escreveu:
> >>> This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.
> >>
> >> I picked this from the cover letter and added to this revert, to justify
> >> it:
> >>
> >> "Hybrid metrics place a PMU at the end of the parse string. This is
> >> also where tool events are placed. The behavior of the parse string
> >> isn't clear and so revert the change for now."
> >>
> >
> > I think the original patch used a "#" to indicate the PMU name, which
> > can be used to distinguish between the tool events and the PMU name.
> > To be honest, I'm not sure what's unclear here. Could you please clarify?
> >
> > With this revert, the issue mentioned in the original patch must be
> > broken on ADL. I don't see a replacement fix in this patch series.
> > Could you please propose a solution for the issue to replace the #PMU
> > name solution?
> >
> > Thanks,
> > Kan
>
> I am surprised the origin patch is reverted during discussion and though
> the discussion still has no conclusion.
> Let me introduce the purpose of the origin patch.
> For a hybrid system such as ADL, if both the metrics and the formula are
> different for the different PMUs, without this patch, the metric and
> event parser should work ok, nothing should be special for the hybrid.
> In fact, both "cpu_core" and "cpu_atom" may have the same metrics--the
> same metric name, even the same formula for the metrics. For example,
> both "cpu_core" and "cpu_atom" have metrics "IpBranch" and "IPC", For
> "IpBranch", both "cpu_core" and "cpu_atom" has the same metric name and
> their formula also is the same, the event name is the same though they
> belong to different PMUs. The old metric and event parser can not handle
> this kind of metric, that's why we need this patch.
>
>      "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
>      "MetricName": "IpBranch",
>      "Unit": "cpu_core"
>
>      "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
>      "MetricName": "IpBranch",
>      "Unit": "cpu_atom"
>
>
>     "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
>     "MetricName": "IPC",
>     "Unit": "cpu_core"
>
>     "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.CORE",
>     "MetricName": "IPC",
>     "Unit": "cpu_atom"
>
> Except for the above two metrics, there are still a lot of similar
> metrics, "CPU_Utilization"...
>
> The original patch expanded the metric group string by adding
> "#PMU_name" to indicate the PMU name, which can be used to distinguish
> between the tool events and the PMU name, then the metric and event
> parser can parse the right PMU for the events.
>
> With the patch all the ADL metrics can pass, without the patch, a lot of
> metrics will fail. I don't think it's a good idea to revert it before
> the new solution is proposed.

Just an idea.  Can we add a pmu prefix when it resolves the event
for a metric if it has the "Unit"?  It seems we can support something
like "cpu_core@INST_RETIRED.ANY@" already..

Or could it be done when creating JSON files?

Thanks,
Namhyung

Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

[Ian Rogers]

On Tue, May 17, 2022 at 3:58 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> On Tue, May 10, 2022 at 2:31 AM Xing Zhengjun
> <zhengjun.xing@linux.intel.com> wrote:
> >
> >
> >
> > On 5/10/2022 5:55 AM, Liang, Kan wrote:
> > >
> > >
> > > On 5/9/2022 9:12 AM, Arnaldo Carvalho de Melo wrote:
> > >> Em Fri, May 06, 2022 at 10:34:06PM -0700, Ian Rogers escreveu:
> > >>> This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.
> > >>
> > >> I picked this from the cover letter and added to this revert, to justify
> > >> it:
> > >>
> > >> "Hybrid metrics place a PMU at the end of the parse string. This is
> > >> also where tool events are placed. The behavior of the parse string
> > >> isn't clear and so revert the change for now."
> > >>
> > >
> > > I think the original patch used a "#" to indicate the PMU name, which
> > > can be used to distinguish between the tool events and the PMU name.
> > > To be honest, I'm not sure what's unclear here. Could you please clarify?
> > >
> > > With this revert, the issue mentioned in the original patch must be
> > > broken on ADL. I don't see a replacement fix in this patch series.
> > > Could you please propose a solution for the issue to replace the #PMU
> > > name solution?
> > >
> > > Thanks,
> > > Kan
> >
> > I am surprised the origin patch is reverted during discussion and though
> > the discussion still has no conclusion.
> > Let me introduce the purpose of the origin patch.
> > For a hybrid system such as ADL, if both the metrics and the formula are
> > different for the different PMUs, without this patch, the metric and
> > event parser should work ok, nothing should be special for the hybrid.
> > In fact, both "cpu_core" and "cpu_atom" may have the same metrics--the
> > same metric name, even the same formula for the metrics. For example,
> > both "cpu_core" and "cpu_atom" have metrics "IpBranch" and "IPC", For
> > "IpBranch", both "cpu_core" and "cpu_atom" has the same metric name and
> > their formula also is the same, the event name is the same though they
> > belong to different PMUs. The old metric and event parser can not handle
> > this kind of metric, that's why we need this patch.
> >
> >      "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
> >      "MetricName": "IpBranch",
> >      "Unit": "cpu_core"
> >
> >      "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
> >      "MetricName": "IpBranch",
> >      "Unit": "cpu_atom"
> >
> >
> >     "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> >     "MetricName": "IPC",
> >     "Unit": "cpu_core"
> >
> >     "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.CORE",
> >     "MetricName": "IPC",
> >     "Unit": "cpu_atom"
> >
> > Except for the above two metrics, there are still a lot of similar
> > metrics, "CPU_Utilization"...
> >
> > The original patch expanded the metric group string by adding
> > "#PMU_name" to indicate the PMU name, which can be used to distinguish
> > between the tool events and the PMU name, then the metric and event
> > parser can parse the right PMU for the events.
> >
> > With the patch all the ADL metrics can pass, without the patch, a lot of
> > metrics will fail. I don't think it's a good idea to revert it before
> > the new solution is proposed.
>
> Just an idea.  Can we add a pmu prefix when it resolves the event
> for a metric if it has the "Unit"?  It seems we can support something
> like "cpu_core@INST_RETIRED.ANY@" already..
>
> Or could it be done when creating JSON files?

Yep. The format for the events in the metric is the same as for
parse-events, we copy+paste the strings from one to the other. The @
in the json is used in place of slash (/) as slash is used in metrics
for division.

Thanks,
Ian

> Thanks,
> Namhyung

Re: [PATCH 1/5] Revert "perf stat: Support metrics with hybrid events"

[Xing Zhengjun]

On 5/18/2022 6:58 AM, Namhyung Kim wrote:
> Hello,
> 
> On Tue, May 10, 2022 at 2:31 AM Xing Zhengjun
> <zhengjun.xing@linux.intel.com> wrote:
>>
>>
>>
>> On 5/10/2022 5:55 AM, Liang, Kan wrote:
>>>
>>>
>>> On 5/9/2022 9:12 AM, Arnaldo Carvalho de Melo wrote:
>>>> Em Fri, May 06, 2022 at 10:34:06PM -0700, Ian Rogers escreveu:
>>>>> This reverts commit 60344f1a9a597f2e0efcd57df5dad0b42da15e21.
>>>>
>>>> I picked this from the cover letter and added to this revert, to justify
>>>> it:
>>>>
>>>> "Hybrid metrics place a PMU at the end of the parse string. This is
>>>> also where tool events are placed. The behavior of the parse string
>>>> isn't clear and so revert the change for now."
>>>>
>>>
>>> I think the original patch used a "#" to indicate the PMU name, which
>>> can be used to distinguish between the tool events and the PMU name.
>>> To be honest, I'm not sure what's unclear here. Could you please clarify?
>>>
>>> With this revert, the issue mentioned in the original patch must be
>>> broken on ADL. I don't see a replacement fix in this patch series.
>>> Could you please propose a solution for the issue to replace the #PMU
>>> name solution?
>>>
>>> Thanks,
>>> Kan
>>
>> I am surprised the origin patch is reverted during discussion and though
>> the discussion still has no conclusion.
>> Let me introduce the purpose of the origin patch.
>> For a hybrid system such as ADL, if both the metrics and the formula are
>> different for the different PMUs, without this patch, the metric and
>> event parser should work ok, nothing should be special for the hybrid.
>> In fact, both "cpu_core" and "cpu_atom" may have the same metrics--the
>> same metric name, even the same formula for the metrics. For example,
>> both "cpu_core" and "cpu_atom" have metrics "IpBranch" and "IPC", For
>> "IpBranch", both "cpu_core" and "cpu_atom" has the same metric name and
>> their formula also is the same, the event name is the same though they
>> belong to different PMUs. The old metric and event parser can not handle
>> this kind of metric, that's why we need this patch.
>>
>>       "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
>>       "MetricName": "IpBranch",
>>       "Unit": "cpu_core"
>>
>>       "MetricExpr": "INST_RETIRED.ANY / BR_INST_RETIRED.ALL_BRANCHES",
>>       "MetricName": "IpBranch",
>>       "Unit": "cpu_atom"
>>
>>
>>      "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
>>      "MetricName": "IPC",
>>      "Unit": "cpu_core"
>>
>>      "MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.CORE",
>>      "MetricName": "IPC",
>>      "Unit": "cpu_atom"
>>
>> Except for the above two metrics, there are still a lot of similar
>> metrics, "CPU_Utilization"...
>>
>> The original patch expanded the metric group string by adding
>> "#PMU_name" to indicate the PMU name, which can be used to distinguish
>> between the tool events and the PMU name, then the metric and event
>> parser can parse the right PMU for the events.
>>
>> With the patch all the ADL metrics can pass, without the patch, a lot of
>> metrics will fail. I don't think it's a good idea to revert it before
>> the new solution is proposed.
> 
> Just an idea.  Can we add a pmu prefix when it resolves the event
> for a metric if it has the "Unit"?  It seems we can support something
> like "cpu_core@INST_RETIRED.ANY@" already..
> 
> Or could it be done when creating JSON files?
> 
> Thanks,
> Namhyung

Yes, we have ever tested it, and it can work. we are changing the 
converter tools to implement it, but it still has some issues that need 
to fix.

-- 
Zhengjun Xing

[PATCH 2/5] perf evsel: Constify a few arrays

[Ian Rogers]

Remove public definition of evsel__tool_names.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/evsel-roundtrip-name.c |  2 +-
 tools/perf/util/evsel.c                 | 14 +++++++-------
 tools/perf/util/evsel.h                 | 11 +++++------
 tools/perf/util/parse-events.c          |  2 +-
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/tools/perf/tests/evsel-roundtrip-name.c b/tools/perf/tests/evsel-roundtrip-name.c
index fdbf17642e45..9d3c64974f77 100644
--- a/tools/perf/tests/evsel-roundtrip-name.c
+++ b/tools/perf/tests/evsel-roundtrip-name.c
@@ -64,7 +64,7 @@ static int perf_evsel__roundtrip_cache_name_test(void)
 	return ret;
 }
 
-static int __perf_evsel__name_array_test(const char *names[], int nr_names,
+static int __perf_evsel__name_array_test(const char *const names[], int nr_names,
 					 int distance)
 {
 	int i, err;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index d38722560e80..cdeace24d9be 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -486,7 +486,7 @@ struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx)
 	return ERR_PTR(err);
 }
 
-const char *evsel__hw_names[PERF_COUNT_HW_MAX] = {
+const char *const evsel__hw_names[PERF_COUNT_HW_MAX] = {
 	"cycles",
 	"instructions",
 	"cache-references",
@@ -571,7 +571,7 @@ static int evsel__hw_name(struct evsel *evsel, char *bf, size_t size)
 	return r + evsel__add_modifiers(evsel, bf + r, size - r);
 }
 
-const char *evsel__sw_names[PERF_COUNT_SW_MAX] = {
+const char *const evsel__sw_names[PERF_COUNT_SW_MAX] = {
 	"cpu-clock",
 	"task-clock",
 	"page-faults",
@@ -597,7 +597,7 @@ static int evsel__sw_name(struct evsel *evsel, char *bf, size_t size)
 	return r + evsel__add_modifiers(evsel, bf + r, size - r);
 }
 
-const char *evsel__tool_names[PERF_TOOL_MAX] = {
+static const char *const evsel__tool_names[PERF_TOOL_MAX] = {
 	"duration_time",
 	"user_time",
 	"system_time",
@@ -633,7 +633,7 @@ static int evsel__bp_name(struct evsel *evsel, char *bf, size_t size)
 	return r + evsel__add_modifiers(evsel, bf + r, size - r);
 }
 
-const char *evsel__hw_cache[PERF_COUNT_HW_CACHE_MAX][EVSEL__MAX_ALIASES] = {
+const char *const evsel__hw_cache[PERF_COUNT_HW_CACHE_MAX][EVSEL__MAX_ALIASES] = {
  { "L1-dcache",	"l1-d",		"l1d",		"L1-data",		},
  { "L1-icache",	"l1-i",		"l1i",		"L1-instruction",	},
  { "LLC",	"L2",							},
@@ -643,13 +643,13 @@ const char *evsel__hw_cache[PERF_COUNT_HW_CACHE_MAX][EVSEL__MAX_ALIASES] = {
  { "node",								},
 };
 
-const char *evsel__hw_cache_op[PERF_COUNT_HW_CACHE_OP_MAX][EVSEL__MAX_ALIASES] = {
+const char *const evsel__hw_cache_op[PERF_COUNT_HW_CACHE_OP_MAX][EVSEL__MAX_ALIASES] = {
  { "load",	"loads",	"read",					},
  { "store",	"stores",	"write",				},
  { "prefetch",	"prefetches",	"speculative-read", "speculative-load",	},
 };
 
-const char *evsel__hw_cache_result[PERF_COUNT_HW_CACHE_RESULT_MAX][EVSEL__MAX_ALIASES] = {
+const char *const evsel__hw_cache_result[PERF_COUNT_HW_CACHE_RESULT_MAX][EVSEL__MAX_ALIASES] = {
  { "refs",	"Reference",	"ops",		"access",		},
  { "misses",	"miss",							},
 };
@@ -665,7 +665,7 @@ const char *evsel__hw_cache_result[PERF_COUNT_HW_CACHE_RESULT_MAX][EVSEL__MAX_AL
  * L1I : Read and prefetch only
  * ITLB and BPU : Read-only
  */
-static unsigned long evsel__hw_cache_stat[C(MAX)] = {
+static const unsigned long evsel__hw_cache_stat[C(MAX)] = {
  [C(L1D)]	= (CACHE_READ | CACHE_WRITE | CACHE_PREFETCH),
  [C(L1I)]	= (CACHE_READ | CACHE_PREFETCH),
  [C(LL)]	= (CACHE_READ | CACHE_WRITE | CACHE_PREFETCH),
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 45d674812239..a017781cdd47 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -257,12 +257,11 @@ static inline bool evsel__is_bpf(struct evsel *evsel)
 
 #define EVSEL__MAX_ALIASES 8
 
-extern const char *evsel__hw_cache[PERF_COUNT_HW_CACHE_MAX][EVSEL__MAX_ALIASES];
-extern const char *evsel__hw_cache_op[PERF_COUNT_HW_CACHE_OP_MAX][EVSEL__MAX_ALIASES];
-extern const char *evsel__hw_cache_result[PERF_COUNT_HW_CACHE_RESULT_MAX][EVSEL__MAX_ALIASES];
-extern const char *evsel__hw_names[PERF_COUNT_HW_MAX];
-extern const char *evsel__sw_names[PERF_COUNT_SW_MAX];
-extern const char *evsel__tool_names[PERF_TOOL_MAX];
+extern const char *const evsel__hw_cache[PERF_COUNT_HW_CACHE_MAX][EVSEL__MAX_ALIASES];
+extern const char *const evsel__hw_cache_op[PERF_COUNT_HW_CACHE_OP_MAX][EVSEL__MAX_ALIASES];
+extern const char *const evsel__hw_cache_result[PERF_COUNT_HW_CACHE_RESULT_MAX][EVSEL__MAX_ALIASES];
+extern const char *const evsel__hw_names[PERF_COUNT_HW_MAX];
+extern const char *const evsel__sw_names[PERF_COUNT_SW_MAX];
 extern char *evsel__bpf_counter_events;
 bool evsel__match_bpf_counter_events(const char *name);
 
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 937f6c9434a2..30a9d915853d 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -426,7 +426,7 @@ static int add_event_tool(struct list_head *list, int *idx,
 	return 0;
 }
 
-static int parse_aliases(char *str, const char *names[][EVSEL__MAX_ALIASES], int size)
+static int parse_aliases(char *str, const char *const names[][EVSEL__MAX_ALIASES], int size)
 {
 	int i, j;
 	int n, longest = -1;
-- 
2.36.0.512.ge40c2bad7a-goog

[PATCH 3/5] perf evsel: Add tool event helpers

[Ian Rogers]

Convert to and from a string. Fix evsel__tool_name as array is off-by-1.
Support more than just duration_time as a metric-id.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c | 41 +++++++++++++++++++++++++++++++----------
 tools/perf/util/evsel.h | 11 +++++++++++
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index cdeace24d9be..5fd7924f8eb3 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -59,6 +59,33 @@ struct perf_missing_features perf_missing_features;
 
 static clockid_t clockid;
 
+static const char *const perf_tool_event__tool_names[PERF_TOOL_MAX] = {
+	NULL,
+	"duration_time",
+	"user_time",
+	"system_time",
+};
+
+const char *perf_tool_event__to_str(enum perf_tool_event ev)
+{
+	if (ev > PERF_TOOL_NONE && ev < PERF_TOOL_MAX)
+		return perf_tool_event__tool_names[ev];
+
+	return NULL;
+}
+
+enum perf_tool_event perf_tool_event__from_str(const char *str)
+{
+	int i;
+
+	perf_tool_event__for_each_event(i) {
+		if (!strcmp(str, perf_tool_event__tool_names[i]))
+			return i;
+	}
+	return PERF_TOOL_NONE;
+}
+
+
 static int evsel__no_extra_init(struct evsel *evsel __maybe_unused)
 {
 	return 0;
@@ -597,15 +624,9 @@ static int evsel__sw_name(struct evsel *evsel, char *bf, size_t size)
 	return r + evsel__add_modifiers(evsel, bf + r, size - r);
 }
 
-static const char *const evsel__tool_names[PERF_TOOL_MAX] = {
-	"duration_time",
-	"user_time",
-	"system_time",
-};
-
 static int evsel__tool_name(enum perf_tool_event ev, char *bf, size_t size)
 {
-	return scnprintf(bf, size, "%s", evsel__tool_names[ev]);
+	return scnprintf(bf, size, "%s", perf_tool_event__to_str(ev));
 }
 
 static int __evsel__bp_name(char *bf, size_t size, u64 addr, u64 type)
@@ -758,7 +779,7 @@ const char *evsel__name(struct evsel *evsel)
 		break;
 
 	case PERF_TYPE_SOFTWARE:
-		if (evsel->tool_event)
+		if (evsel__is_tool(evsel))
 			evsel__tool_name(evsel->tool_event, bf, sizeof(bf));
 		else
 			evsel__sw_name(evsel, bf, sizeof(bf));
@@ -791,8 +812,8 @@ const char *evsel__metric_id(const struct evsel *evsel)
 	if (evsel->metric_id)
 		return evsel->metric_id;
 
-	if (evsel->core.attr.type == PERF_TYPE_SOFTWARE && evsel->tool_event)
-		return "duration_time";
+	if (evsel__is_tool(evsel))
+		return perf_tool_event__to_str(evsel->tool_event);
 
 	return "unknown";
 }
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index a017781cdd47..d4b04537ce6d 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -36,6 +36,12 @@ enum perf_tool_event {
 	PERF_TOOL_MAX,
 };
 
+const char *perf_tool_event__to_str(enum perf_tool_event ev);
+enum perf_tool_event perf_tool_event__from_str(const char *str);
+
+#define perf_tool_event__for_each_event(ev)		\
+	for ((ev) = PERF_TOOL_DURATION_TIME; (ev) < PERF_TOOL_MAX; ev++)
+
 /** struct evsel - event selector
  *
  * @evlist - evlist this evsel is in, if it is in one.
@@ -269,6 +275,11 @@ int __evsel__hw_cache_type_op_res_name(u8 type, u8 op, u8 result, char *bf, size
 const char *evsel__name(struct evsel *evsel);
 const char *evsel__metric_id(const struct evsel *evsel);
 
+static inline bool evsel__is_tool(const struct evsel *evsel)
+{
+	return evsel->tool_event != PERF_TOOL_NONE;
+}
+
 const char *evsel__group_name(struct evsel *evsel);
 int evsel__group_desc(struct evsel *evsel, char *buf, size_t size);
 
-- 
2.36.0.512.ge40c2bad7a-goog

Re: [PATCH 3/5] perf evsel: Add tool event helpers

[Arnaldo Carvalho de Melo]

Em Fri, May 06, 2022 at 10:34:08PM -0700, Ian Rogers escreveu:
> Convert to and from a string. Fix evsel__tool_name as array is off-by-1.
> Support more than just duration_time as a metric-id.

Good catch, I added this:

Fixes: 75eafc970bd9d36d ("perf list: Print all available tool events")

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c | 41 +++++++++++++++++++++++++++++++----------
>  tools/perf/util/evsel.h | 11 +++++++++++
>  2 files changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index cdeace24d9be..5fd7924f8eb3 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -59,6 +59,33 @@ struct perf_missing_features perf_missing_features;
>  
>  static clockid_t clockid;
>  
> +static const char *const perf_tool_event__tool_names[PERF_TOOL_MAX] = {
> +	NULL,
> +	"duration_time",
> +	"user_time",
> +	"system_time",
> +};
> +
> +const char *perf_tool_event__to_str(enum perf_tool_event ev)
> +{
> +	if (ev > PERF_TOOL_NONE && ev < PERF_TOOL_MAX)
> +		return perf_tool_event__tool_names[ev];
> +
> +	return NULL;
> +}
> +
> +enum perf_tool_event perf_tool_event__from_str(const char *str)
> +{
> +	int i;
> +
> +	perf_tool_event__for_each_event(i) {
> +		if (!strcmp(str, perf_tool_event__tool_names[i]))
> +			return i;
> +	}
> +	return PERF_TOOL_NONE;
> +}
> +
> +
>  static int evsel__no_extra_init(struct evsel *evsel __maybe_unused)
>  {
>  	return 0;
> @@ -597,15 +624,9 @@ static int evsel__sw_name(struct evsel *evsel, char *bf, size_t size)
>  	return r + evsel__add_modifiers(evsel, bf + r, size - r);
>  }
>  
> -static const char *const evsel__tool_names[PERF_TOOL_MAX] = {
> -	"duration_time",
> -	"user_time",
> -	"system_time",
> -};
> -
>  static int evsel__tool_name(enum perf_tool_event ev, char *bf, size_t size)
>  {
> -	return scnprintf(bf, size, "%s", evsel__tool_names[ev]);
> +	return scnprintf(bf, size, "%s", perf_tool_event__to_str(ev));
>  }
>  
>  static int __evsel__bp_name(char *bf, size_t size, u64 addr, u64 type)
> @@ -758,7 +779,7 @@ const char *evsel__name(struct evsel *evsel)
>  		break;
>  
>  	case PERF_TYPE_SOFTWARE:
> -		if (evsel->tool_event)
> +		if (evsel__is_tool(evsel))
>  			evsel__tool_name(evsel->tool_event, bf, sizeof(bf));
>  		else
>  			evsel__sw_name(evsel, bf, sizeof(bf));
> @@ -791,8 +812,8 @@ const char *evsel__metric_id(const struct evsel *evsel)
>  	if (evsel->metric_id)
>  		return evsel->metric_id;
>  
> -	if (evsel->core.attr.type == PERF_TYPE_SOFTWARE && evsel->tool_event)
> -		return "duration_time";
> +	if (evsel__is_tool(evsel))
> +		return perf_tool_event__to_str(evsel->tool_event);
>  
>  	return "unknown";
>  }
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index a017781cdd47..d4b04537ce6d 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -36,6 +36,12 @@ enum perf_tool_event {
>  	PERF_TOOL_MAX,
>  };
>  
> +const char *perf_tool_event__to_str(enum perf_tool_event ev);
> +enum perf_tool_event perf_tool_event__from_str(const char *str);
> +
> +#define perf_tool_event__for_each_event(ev)		\
> +	for ((ev) = PERF_TOOL_DURATION_TIME; (ev) < PERF_TOOL_MAX; ev++)
> +
>  /** struct evsel - event selector
>   *
>   * @evlist - evlist this evsel is in, if it is in one.
> @@ -269,6 +275,11 @@ int __evsel__hw_cache_type_op_res_name(u8 type, u8 op, u8 result, char *bf, size
>  const char *evsel__name(struct evsel *evsel);
>  const char *evsel__metric_id(const struct evsel *evsel);
>  
> +static inline bool evsel__is_tool(const struct evsel *evsel)
> +{
> +	return evsel->tool_event != PERF_TOOL_NONE;
> +}
> +
>  const char *evsel__group_name(struct evsel *evsel);
>  int evsel__group_desc(struct evsel *evsel, char *buf, size_t size);
>  
> -- 
> 2.36.0.512.ge40c2bad7a-goog

-- 

- Arnaldo

[PATCH 4/5] perf metrics: Support all tool events

[Ian Rogers]

Previously duration_time was hard coded, which was ok until
commit b03b89b35003 ("perf stat: Add user_time and system_time events")
added additional tool events. Do for all tool events what was previously
done just for duration_time.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 87 ++++++++++++++++++++---------------
 tools/perf/util/stat-shadow.c | 27 +++++++++--
 2 files changed, 75 insertions(+), 39 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index d8492e339521..7a5f488aef02 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -728,22 +728,23 @@ static int metricgroup__build_event_string(struct strbuf *events,
 {
 	struct hashmap_entry *cur;
 	size_t bkt;
-	bool no_group = true, has_duration = false;
+	bool no_group = true, has_tool_events = false;
+	bool tool_events[PERF_TOOL_MAX] = {false};
 	int ret = 0;
 
 #define RETURN_IF_NON_ZERO(x) do { if (x) return x; } while (0)
 
 	hashmap__for_each_entry(ctx->ids, cur, bkt) {
 		const char *sep, *rsep, *id = cur->key;
+		enum perf_tool_event ev;
 
 		pr_debug("found event %s\n", id);
-		/*
-		 * Duration time maps to a software event and can make
-		 * groups not count. Always use it outside a
-		 * group.
-		 */
-		if (!strcmp(id, "duration_time")) {
-			has_duration = true;
+
+		/* Always move tool events outside of the group. */
+		ev = perf_tool_event__from_str(id);
+		if (ev != PERF_TOOL_NONE) {
+			has_tool_events = true;
+			tool_events[ev] = true;
 			continue;
 		}
 		/* Separate events with commas and open the group if necessary. */
@@ -802,16 +803,25 @@ static int metricgroup__build_event_string(struct strbuf *events,
 			RETURN_IF_NON_ZERO(ret);
 		}
 	}
-	if (has_duration) {
-		if (no_group) {
-			/* Strange case of a metric of just duration_time. */
-			ret = strbuf_addf(events, "duration_time");
-		} else if (!has_constraint)
-			ret = strbuf_addf(events, "}:W,duration_time");
-		else
-			ret = strbuf_addf(events, ",duration_time");
-	} else if (!no_group && !has_constraint)
+	if (!no_group && !has_constraint) {
 		ret = strbuf_addf(events, "}:W");
+		RETURN_IF_NON_ZERO(ret);
+	}
+	if (has_tool_events) {
+		int i;
+
+		perf_tool_event__for_each_event(i) {
+			if (tool_events[i]) {
+				if (!no_group) {
+					ret = strbuf_addch(events, ',');
+					RETURN_IF_NON_ZERO(ret);
+				}
+				no_group = false;
+				ret = strbuf_addstr(events, perf_tool_event__to_str(i));
+				RETURN_IF_NON_ZERO(ret);
+			}
+		}
+	}
 
 	return ret;
 #undef RETURN_IF_NON_ZERO
@@ -1117,7 +1127,7 @@ static int metricgroup__add_metric_sys_event_iter(const struct pmu_event *pe,
 
 /**
  * metric_list_cmp - list_sort comparator that sorts metrics with more events to
- *                   the front. duration_time is excluded from the count.
+ *                   the front. tool events are excluded from the count.
  */
 static int metric_list_cmp(void *priv __maybe_unused, const struct list_head *l,
 			   const struct list_head *r)
@@ -1125,15 +1135,19 @@ static int metric_list_cmp(void *priv __maybe_unused, const struct list_head *l,
 	const struct metric *left = container_of(l, struct metric, nd);
 	const struct metric *right = container_of(r, struct metric, nd);
 	struct expr_id_data *data;
-	int left_count, right_count;
+	int i, left_count, right_count;
 
 	left_count = hashmap__size(left->pctx->ids);
-	if (!expr__get_id(left->pctx, "duration_time", &data))
-		left_count--;
+	perf_tool_event__for_each_event(i) {
+		if (!expr__get_id(left->pctx, perf_tool_event__to_str(i), &data))
+			left_count--;
+	}
 
 	right_count = hashmap__size(right->pctx->ids);
-	if (!expr__get_id(right->pctx, "duration_time", &data))
-		right_count--;
+	perf_tool_event__for_each_event(i) {
+		if (!expr__get_id(right->pctx, perf_tool_event__to_str(i), &data))
+			right_count--;
+	}
 
 	return right_count - left_count;
 }
@@ -1331,26 +1345,27 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
 
 	*out_evlist = NULL;
 	if (!metric_no_merge || hashmap__size(ids->ids) == 0) {
-		char *tmp;
+		int i;
 		/*
-		 * We may fail to share events between metrics because
-		 * duration_time isn't present in one metric. For example, a
-		 * ratio of cache misses doesn't need duration_time but the same
-		 * events may be used for a misses per second. Events without
-		 * sharing implies multiplexing, that is best avoided, so place
-		 * duration_time in every group.
+		 * We may fail to share events between metrics because a tool
+		 * event isn't present in one metric. For example, a ratio of
+		 * cache misses doesn't need duration_time but the same events
+		 * may be used for a misses per second. Events without sharing
+		 * implies multiplexing, that is best avoided, so place
+		 * all tool events in every group.
 		 *
 		 * Also, there may be no ids/events in the expression parsing
 		 * context because of constant evaluation, e.g.:
 		 *    event1 if #smt_on else 0
-		 * Add a duration_time event to avoid a parse error on an empty
-		 * string.
+		 * Add a tool event to avoid a parse error on an empty string.
 		 */
-		tmp = strdup("duration_time");
-		if (!tmp)
-			return -ENOMEM;
+		perf_tool_event__for_each_event(i) {
+			char *tmp = strdup(perf_tool_event__to_str(i));
 
-		ids__insert(ids->ids, tmp);
+			if (!tmp)
+				return -ENOMEM;
+			ids__insert(ids->ids, tmp);
+		}
 	}
 	ret = metricgroup__build_event_string(&events, ids, modifier,
 					      has_constraint);
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index ea4c35e4f1da..979c8cb918f7 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -833,10 +833,31 @@ static int prepare_metric(struct evsel **metric_events,
 		u64 metric_total = 0;
 		int source_count;
 
-		if (!strcmp(metric_events[i]->name, "duration_time")) {
-			stats = &walltime_nsecs_stats;
-			scale = 1e-9;
+		if (evsel__is_tool(metric_events[i])) {
 			source_count = 1;
+			switch (metric_events[i]->tool_event) {
+			case PERF_TOOL_DURATION_TIME:
+				stats = &walltime_nsecs_stats;
+				scale = 1e-9;
+				break;
+			case PERF_TOOL_USER_TIME:
+				stats = &ru_stats.ru_utime_usec_stat;
+				scale = 1e-6;
+				break;
+			case PERF_TOOL_SYSTEM_TIME:
+				stats = &ru_stats.ru_stime_usec_stat;
+				scale = 1e-6;
+				break;
+			case PERF_TOOL_NONE:
+				pr_err("Invalid tool event 'none'");
+				abort();
+			case PERF_TOOL_MAX:
+				pr_err("Invalid tool event 'max'");
+				abort();
+			default:
+				pr_err("Unknown tool event '%s'", evsel__name(metric_events[i]));
+				abort();
+			}
 		} else {
 			v = saved_value_lookup(metric_events[i], cpu_map_idx, false,
 					       STAT_NONE, 0, st,
-- 
2.36.0.512.ge40c2bad7a-goog

[PATCH 5/5] perf metrics: Don't add all tool events for sharing

[Ian Rogers]

Tool events are added to the set of events for parsing so that having a
tool event in a metric doesn't inhibit event sharing of events between
metrics. All tool events were added but this meant unused tool events
would be counted. Reduce this set of tool events to just those present
in the overall metric list.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 45 ++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 7a5f488aef02..ee8fcfa115e5 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -1283,6 +1283,30 @@ static void metricgroup__free_metrics(struct list_head *metric_list)
 	}
 }
 
+/**
+ * find_tool_events - Search for the pressence of tool events in metric_list.
+ * @metric_list: List to take metrics from.
+ * @tool_events: Array of false values, indices corresponding to tool events set
+ *               to true if tool event is found.
+ */
+static void find_tool_events(const struct list_head *metric_list,
+			     bool tool_events[PERF_TOOL_MAX])
+{
+	struct metric *m;
+
+	list_for_each_entry(m, metric_list, nd) {
+		int i;
+
+		perf_tool_event__for_each_event(i) {
+			struct expr_id_data *data;
+
+			if (!tool_events[i] &&
+			    !expr__get_id(m->pctx, perf_tool_event__to_str(i), &data))
+				tool_events[i] = true;
+		}
+	}
+}
+
 /**
  * build_combined_expr_ctx - Make an expr_parse_ctx with all has_constraint
  *                           metric IDs, as the IDs are held in a set,
@@ -1332,11 +1356,14 @@ static int build_combined_expr_ctx(const struct list_head *metric_list,
  * @ids: the event identifiers parsed from a metric.
  * @modifier: any modifiers added to the events.
  * @has_constraint: false if events should be placed in a weak group.
+ * @tool_events: entries set true if the tool event of index could be present in
+ *               the overall list of metrics.
  * @out_evlist: the created list of events.
  */
 static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
 		     struct expr_parse_ctx *ids, const char *modifier,
-		     bool has_constraint, struct evlist **out_evlist)
+		     bool has_constraint, const bool tool_events[PERF_TOOL_MAX],
+		     struct evlist **out_evlist)
 {
 	struct parse_events_error parse_error;
 	struct evlist *parsed_evlist;
@@ -1360,11 +1387,13 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
 		 * Add a tool event to avoid a parse error on an empty string.
 		 */
 		perf_tool_event__for_each_event(i) {
-			char *tmp = strdup(perf_tool_event__to_str(i));
+			if (tool_events[i]) {
+				char *tmp = strdup(perf_tool_event__to_str(i));
 
-			if (!tmp)
-				return -ENOMEM;
-			ids__insert(ids->ids, tmp);
+				if (!tmp)
+					return -ENOMEM;
+				ids__insert(ids->ids, tmp);
+			}
 		}
 	}
 	ret = metricgroup__build_event_string(&events, ids, modifier,
@@ -1407,6 +1436,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 	struct evlist *combined_evlist = NULL;
 	LIST_HEAD(metric_list);
 	struct metric *m;
+	bool tool_events[PERF_TOOL_MAX] = {false};
 	int ret;
 
 	if (metric_events_list->nr_entries == 0)
@@ -1422,12 +1452,15 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 	if (!metric_no_merge) {
 		struct expr_parse_ctx *combined = NULL;
 
+		find_tool_events(&metric_list, tool_events);
+
 		ret = build_combined_expr_ctx(&metric_list, &combined);
 
 		if (!ret && combined && hashmap__size(combined->ids)) {
 			ret = parse_ids(metric_no_merge, fake_pmu, combined,
 					/*modifier=*/NULL,
 					/*has_constraint=*/true,
+					tool_events,
 					&combined_evlist);
 		}
 		if (combined)
@@ -1475,7 +1508,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 		}
 		if (!metric_evlist) {
 			ret = parse_ids(metric_no_merge, fake_pmu, m->pctx, m->modifier,
-					m->has_constraint, &m->evlist);
+					m->has_constraint, tool_events, &m->evlist);
 			if (ret)
 				goto out;
 
-- 
2.36.0.512.ge40c2bad7a-goog