[PATCH 0/7] Share events between metrics

[PATCH 0/7] Share events between metrics

[Ian Rogers]

Metric groups contain metrics. Metrics create groups of events to
ideally be scheduled together. Often metrics refer to the same events,
for example, a cache hit and cache miss rate. Using separate event
groups means these metrics are multiplexed at different times and the
counts don't sum to 100%. More multiplexing also decreases the
accuracy of the measurement.

This change orders metrics from groups or the command line, so that
the ones with the most events are set up first. Later metrics see if
groups already provide their events, and reuse them if
possible. Unnecessary events and groups are eliminated.

The option --metric-no-group is added so that metrics aren't placed in
groups. This affects multiplexing and may increase sharing.

The option --metric-mo-merge is added and with this option the
existing grouping behavior is preserved.

Using skylakex metrics I ran the following shell code to count the
number of events for each metric group (this ignores metric groups
with a single metric, and one of the duplicated TopdownL1 and
TopDownL1 groups):

for i in all Branches BrMispredicts Cache_Misses FLOPS Instruction_Type Memory_BW Pipeline Power SMT Summary TopdownL1 TopdownL1_SMT
do
  echo Metric group: $i
  echo -n " - No merging (old default, now --metric-no-merge): "
  /tmp/perf/perf stat -a --metric-no-merge -M $i sleep 1 2>&1 | grep -v "^ *#" | egrep " +[0-9,.]+ [^s]" | wc -l
  echo -n " - Merging over metrics (new default)             : "
  /tmp/perf/perf stat -a -M $i sleep 1 2>&1 | grep -v "^ *#" | egrep " +[0-9,.]+ [^s]"|wc -l
  echo -n " - No event groups and merging (--metric-no-group): "
  /tmp/perf/perf stat -a --metric-no-group -M $i sleep 1 2>&1 | grep -v "^ *#" | egrep " +[0-9,.]+ [^s]"|wc -l
done

Metric group: all
 - No merging (old default, now --metric-no-merge): 193
 - Merging over metrics (new default)             : 142
 - No event groups and merging (--metric-no-group): 84
Metric group: Branches
 - No merging (old default, now --metric-no-merge): 8
 - Merging over metrics (new default)             : 8
 - No event groups and merging (--metric-no-group): 4
Metric group: BrMispredicts
 - No merging (old default, now --metric-no-merge): 11
 - Merging over metrics (new default)             : 11
 - No event groups and merging (--metric-no-group): 10
Metric group: Cache_Misses
 - No merging (old default, now --metric-no-merge): 11
 - Merging over metrics (new default)             : 9
 - No event groups and merging (--metric-no-group): 6
Metric group: FLOPS
 - No merging (old default, now --metric-no-merge): 18
 - Merging over metrics (new default)             : 10
 - No event groups and merging (--metric-no-group): 10
Metric group: Instruction_Type
 - No merging (old default, now --metric-no-merge): 6
 - Merging over metrics (new default)             : 6
 - No event groups and merging (--metric-no-group): 4
Metric group: Pipeline
 - No merging (old default, now --metric-no-merge): 6
 - Merging over metrics (new default)             : 6
 - No event groups and merging (--metric-no-group): 5
Metric group: Power
 - No merging (old default, now --metric-no-merge): 16
 - Merging over metrics (new default)             : 16
 - No event groups and merging (--metric-no-group): 10
Metric group: SMT
 - No merging (old default, now --metric-no-merge): 11
 - Merging over metrics (new default)             : 8
 - No event groups and merging (--metric-no-group): 7
Metric group: Summary
 - No merging (old default, now --metric-no-merge): 19
 - Merging over metrics (new default)             : 17
 - No event groups and merging (--metric-no-group): 17
Metric group: TopdownL1
 - No merging (old default, now --metric-no-merge): 16
 - Merging over metrics (new default)             : 7
 - No event groups and merging (--metric-no-group): 7
Metric group: TopdownL1_SMT
 - No merging (old default, now --metric-no-merge): 24
 - Merging over metrics (new default)             : 7
 - No event groups and merging (--metric-no-group): 7

There are 5 out of 12 metric groups where no events are shared, such
as Power, however, disabling grouping of events always reduces the
number of events.

The result for Memory_BW needs explanation:

Metric group: Memory_BW
 - No merging (old default, now --metric-no-merge): 9
 - Merging over metrics (new default)             : 5
 - No event groups and merging (--metric-no-group): 11

Both with and without merging the groups fail to be set up and so the
event counts here are for broken metrics. The --metric-no-group number
is accurate as all the events are scheduled. Ideally a constraint
would be added for these metrics in the json code to avoid grouping.

v1. was prepared on kernel/git/acme/linux.git branch tmp.perf/core

Compared to RFC v3: fix a bug where unnecessary commas were passed to
parse-events and were echoed. Fix a bug where the same event could be
matched more than once with --metric-no-group, causing there to be
events missing.
https://lore.kernel.org/lkml/20200508053629.210324-1-irogers@google.com/

Ian Rogers (7):
  perf metricgroup: Change evlist_used to a bitmap
  perf metricgroup: Always place duration_time last
  perf metricgroup: Delay events string creation
  perf metricgroup: Order event groups by size
  perf metricgroup: Remove duped metric group events
  perf metricgroup: Add options to not group or merge
  perf metricgroup: Remove unnecessary ',' from events

 tools/perf/Documentation/perf-stat.txt |  19 ++
 tools/perf/builtin-stat.c              |  11 +-
 tools/perf/util/metricgroup.c          | 241 ++++++++++++++++++-------
 tools/perf/util/metricgroup.h          |   6 +-
 tools/perf/util/stat.h                 |   2 +
 5 files changed, 206 insertions(+), 73 deletions(-)

-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 1/7] perf metricgroup: Change evlist_used to a bitmap

[Ian Rogers]

Use a bitmap rather than an array of bools.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 6772d256dfdf..a16f60da06ab 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -95,7 +95,7 @@ struct egroup {
 static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 				      struct expr_parse_ctx *pctx,
 				      struct evsel **metric_events,
-				      bool *evlist_used)
+				      unsigned long *evlist_used)
 {
 	struct evsel *ev;
 	bool leader_found;
@@ -105,7 +105,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 	double *val_ptr;
 
 	evlist__for_each_entry (perf_evlist, ev) {
-		if (evlist_used[j++])
+		if (test_bit(j++, evlist_used))
 			continue;
 		if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
 			if (!metric_events[i])
@@ -141,7 +141,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 			j++;
 		}
 		ev = metric_events[i];
-		evlist_used[ev->idx] = true;
+		set_bit(ev->idx, evlist_used);
 	}
 
 	return metric_events[0];
@@ -157,13 +157,11 @@ static int metricgroup__setup_events(struct list_head *groups,
 	int ret = 0;
 	struct egroup *eg;
 	struct evsel *evsel;
-	bool *evlist_used;
+	unsigned long *evlist_used;
 
-	evlist_used = calloc(perf_evlist->core.nr_entries, sizeof(bool));
-	if (!evlist_used) {
-		ret = -ENOMEM;
-		return ret;
-	}
+	evlist_used = bitmap_alloc(perf_evlist->core.nr_entries);
+	if (!evlist_used)
+		return -ENOMEM;
 
 	list_for_each_entry (eg, groups, nd) {
 		struct evsel **metric_events;
@@ -201,7 +199,7 @@ static int metricgroup__setup_events(struct list_head *groups,
 		list_add(&expr->nd, &me->head);
 	}
 
-	free(evlist_used);
+	bitmap_free(evlist_used);
 
 	return ret;
 }
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 2/7] perf metricgroup: Always place duration_time last

[Ian Rogers]

If a metric contains the duration_time event then the event is placed
outside of the metric's group of events. Rather than split the group,
make it so the duration_time is immediately after the group.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index a16f60da06ab..7a43ee0a2e40 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -410,8 +410,8 @@ static void metricgroup__add_metric_weak_group(struct strbuf *events,
 					       struct expr_parse_ctx *ctx)
 {
 	struct hashmap_entry *cur;
-	size_t bkt, i = 0;
-	bool no_group = false;
+	size_t bkt;
+	bool no_group = true, has_duration = false;
 
 	hashmap__for_each_entry((&ctx->ids), cur, bkt) {
 		pr_debug("found event %s\n", (const char *)cur->key);
@@ -421,20 +421,20 @@ static void metricgroup__add_metric_weak_group(struct strbuf *events,
 		 * group.
 		 */
 		if (!strcmp(cur->key, "duration_time")) {
-			if (i > 0)
-				strbuf_addf(events, "}:W,");
-			strbuf_addf(events, "duration_time");
-			no_group = true;
+			has_duration = true;
 			continue;
 		}
 		strbuf_addf(events, "%s%s",
-			i == 0 || no_group ? "{" : ",",
+			no_group ? "{" : ",",
 			(const char *)cur->key);
 		no_group = false;
-		i++;
 	}
-	if (!no_group)
+	if (!no_group) {
 		strbuf_addf(events, "}:W");
+		if (has_duration)
+			strbuf_addf(events, ",duration_time");
+	} else if (has_duration)
+		strbuf_addf(events, "duration_time");
 }
 
 static void metricgroup__add_metric_non_group(struct strbuf *events,
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 3/7] perf metricgroup: Delay events string creation

[Ian Rogers]

Currently event groups are placed into groups_list at the same time as
the events string containing the events is built. Separate these two
operations and build the groups_list first, then the event string from
the groups_list. This adds an ability to reorder the groups_list that
will be used in a later patch.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 7a43ee0a2e40..afd960d03a77 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -90,6 +90,7 @@ struct egroup {
 	const char *metric_expr;
 	const char *metric_unit;
 	int runtime;
+	bool has_constraint;
 };
 
 static struct evsel *find_evsel_group(struct evlist *perf_evlist,
@@ -485,8 +486,8 @@ int __weak arch_get_runtimeparam(void)
 	return 1;
 }
 
-static int __metricgroup__add_metric(struct strbuf *events,
-		struct list_head *group_list, struct pmu_event *pe, int runtime)
+static int __metricgroup__add_metric(struct list_head *group_list,
+				     struct pmu_event *pe, int runtime)
 {
 	struct egroup *eg;
 
@@ -499,6 +500,7 @@ static int __metricgroup__add_metric(struct strbuf *events,
 	eg->metric_expr = pe->metric_expr;
 	eg->metric_unit = pe->unit;
 	eg->runtime = runtime;
+	eg->has_constraint = metricgroup__has_constraint(pe);
 
 	if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
 		expr__ctx_clear(&eg->pctx);
@@ -506,14 +508,6 @@ static int __metricgroup__add_metric(struct strbuf *events,
 		return -EINVAL;
 	}
 
-	if (events->len > 0)
-		strbuf_addf(events, ",");
-
-	if (metricgroup__has_constraint(pe))
-		metricgroup__add_metric_non_group(events, &eg->pctx);
-	else
-		metricgroup__add_metric_weak_group(events, &eg->pctx);
-
 	list_add_tail(&eg->nd, group_list);
 
 	return 0;
@@ -524,6 +518,7 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 {
 	struct pmu_events_map *map = perf_pmu__find_map(NULL);
 	struct pmu_event *pe;
+	struct egroup *eg;
 	int i, ret = -EINVAL;
 
 	if (!map)
@@ -542,7 +537,8 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 			pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
 
 			if (!strstr(pe->metric_expr, "?")) {
-				ret = __metricgroup__add_metric(events, group_list, pe, 1);
+				ret = __metricgroup__add_metric(group_list,
+								pe, 1);
 			} else {
 				int j, count;
 
@@ -553,13 +549,29 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 				 * those events to group_list.
 				 */
 
-				for (j = 0; j < count; j++)
-					ret = __metricgroup__add_metric(events, group_list, pe, j);
+				for (j = 0; j < count; j++) {
+					ret = __metricgroup__add_metric(
+						group_list, pe, j);
+				}
 			}
 			if (ret == -ENOMEM)
 				break;
 		}
 	}
+	if (!ret) {
+		list_for_each_entry(eg, group_list, nd) {
+			if (events->len > 0)
+				strbuf_addf(events, ",");
+
+			if (eg->has_constraint) {
+				metricgroup__add_metric_non_group(events,
+								  &eg->pctx);
+			} else {
+				metricgroup__add_metric_weak_group(events,
+								   &eg->pctx);
+			}
+		}
+	}
 	return ret;
 }
 
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 4/7] perf metricgroup: Order event groups by size

[Ian Rogers]

When adding event groups to the group list, insert them in size order.
This performs an insertion sort on the group list. By placing the
largest groups at the front of the group list it is possible to see if a
larger group contains the same events as a later group. This can make
the later group redundant - it can reuse the events from the large group.
A later patch will add this sharing.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index afd960d03a77..52e4c3e4748a 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -508,7 +508,21 @@ static int __metricgroup__add_metric(struct list_head *group_list,
 		return -EINVAL;
 	}
 
-	list_add_tail(&eg->nd, group_list);
+	if (list_empty(group_list))
+		list_add(&eg->nd, group_list);
+	else {
+		struct list_head *pos;
+
+		/* Place the largest groups at the front. */
+		list_for_each_prev(pos, group_list) {
+			struct egroup *old = list_entry(pos, struct egroup, nd);
+
+			if (hashmap__size(&eg->pctx.ids) <=
+			    hashmap__size(&old->pctx.ids))
+				break;
+		}
+		list_add(&eg->nd, pos);
+	}
 
 	return 0;
 }
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 5/7] perf metricgroup: Remove duped metric group events

[Ian Rogers]

A metric group contains multiple metrics. These metrics may use the same
events. If metrics use separate events then it leads to more
multiplexing and overall metric counts fail to sum to 100%.
Modify how metrics are associated with events so that if the events in
an earlier group satisfy the current metric, the same events are used.
A record of used events is kept and at the end of processing unnecessary
events are eliminated.

Before:
$ perf stat -a -M TopDownL1 sleep 1

 Performance counter stats for 'system wide':

       920,211,343      uops_issued.any           #      0.5 Backend_Bound            (16.56%)
     1,977,733,128      idq_uops_not_delivered.core                                     (16.56%)
        51,668,510      int_misc.recovery_cycles                                      (16.56%)
       732,305,692      uops_retired.retire_slots                                     (16.56%)
     1,497,621,849      cycles                                                        (16.56%)
       721,098,274      uops_issued.any           #      0.1 Bad_Speculation          (16.79%)
     1,332,681,791      cycles                                                        (16.79%)
       552,475,482      uops_retired.retire_slots                                     (16.79%)
        47,708,340      int_misc.recovery_cycles                                      (16.79%)
     1,383,713,292      cycles
                                                  #      0.4 Frontend_Bound           (16.76%)
     2,013,757,701      idq_uops_not_delivered.core                                     (16.76%)
     1,373,363,790      cycles
                                                  #      0.1 Retiring                 (33.54%)
       577,302,589      uops_retired.retire_slots                                     (33.54%)
       392,766,987      inst_retired.any          #      0.3 IPC                      (50.24%)
     1,351,873,350      cpu_clk_unhalted.thread                                       (50.24%)
     1,332,510,318      cycles
                                                  # 5330041272.0 SLOTS                (49.90%)

       1.006336145 seconds time elapsed

After:
$ perf stat -a -M TopDownL1 sleep 1

 Performance counter stats for 'system wide':

       765,949,145      uops_issued.any           #      0.1 Bad_Speculation
                                                  #      0.5 Backend_Bound            (50.09%)
     1,883,830,591      idq_uops_not_delivered.core #      0.3 Frontend_Bound           (50.09%)
        48,237,080      int_misc.recovery_cycles                                      (50.09%)
       581,798,385      uops_retired.retire_slots #      0.1 Retiring                 (50.09%)
     1,361,628,527      cycles
                                                  # 5446514108.0 SLOTS                (50.09%)
       391,415,714      inst_retired.any          #      0.3 IPC                      (49.91%)
     1,336,486,781      cpu_clk_unhalted.thread                                       (49.91%)

       1.005469298 seconds time elapsed

Note: Bad_Speculation + Backend_Bound + Frontend_Bound + Retiring = 100%
after, where as before it is 110%. After there are 2 groups, whereas
before there are 6. After the cycles event appears once, before it
appeared 5 times.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 52e4c3e4748a..7ed32baeb767 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -93,36 +93,72 @@ struct egroup {
 	bool has_constraint;
 };
 
+/**
+ * Find a group of events in perf_evlist that correpond to those from a parsed
+ * metric expression.
+ * @perf_evlist: a list of events something like: {metric1 leader, metric1
+ * sibling, metric1 sibling}:W,duration_time,{metric2 leader, metric2 sibling,
+ * metric2 sibling}:W,duration_time
+ * @pctx: the parse context for the metric expression.
+ * @has_constraint: is there a contraint on the group of events? In which case
+ * the events won't be grouped.
+ * @metric_events: out argument, null terminated array of evsel's associated
+ * with the metric.
+ * @evlist_used: in/out argument, bitmap tracking which evlist events are used.
+ * @return the first metric event or NULL on failure.
+ */
 static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 				      struct expr_parse_ctx *pctx,
+				      bool has_constraint,
 				      struct evsel **metric_events,
 				      unsigned long *evlist_used)
 {
-	struct evsel *ev;
-	bool leader_found;
-	const size_t idnum = hashmap__size(&pctx->ids);
-	size_t i = 0;
-	int j = 0;
+	struct evsel *ev, *current_leader = NULL;
 	double *val_ptr;
+	int i = 0, matched_events = 0, events_to_match;
+	const int idnum = (int)hashmap__size(&pctx->ids);
+
+	/* duration_time is grouped separately. */
+	if (!has_constraint &&
+	    hashmap__find(&pctx->ids, "duration_time", (void **)&val_ptr))
+		events_to_match = idnum - 1;
+	else
+		events_to_match = idnum;
 
 	evlist__for_each_entry (perf_evlist, ev) {
-		if (test_bit(j++, evlist_used))
+		/*
+		 * Events with a constraint aren't grouped and match the first
+		 * events available.
+		 */
+		if (has_constraint && ev->weak_group)
 			continue;
-		if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
-			if (!metric_events[i])
-				metric_events[i] = ev;
-			i++;
-			if (i == idnum)
-				break;
-		} else {
-			/* Discard the whole match and start again */
-			i = 0;
+		if (!has_constraint && ev->leader != current_leader) {
+			/*
+			 * Start of a new group, discard the whole match and
+			 * start again.
+			 */
+			matched_events = 0;
 			memset(metric_events, 0,
 				sizeof(struct evsel *) * idnum);
+			current_leader = ev->leader;
+		}
+		if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr))
+			metric_events[matched_events++] = ev;
+		if (matched_events == events_to_match)
+			break;
+	}
+
+	if (events_to_match != idnum) {
+		/* Add the first duration_time. */
+		evlist__for_each_entry(perf_evlist, ev) {
+			if (!strcmp(ev->name, "duration_time")) {
+				metric_events[matched_events++] = ev;
+				break;
+			}
 		}
 	}
 
-	if (i != idnum) {
+	if (matched_events != idnum) {
 		/* Not whole match */
 		return NULL;
 	}
@@ -130,18 +166,8 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 	metric_events[idnum] = NULL;
 
 	for (i = 0; i < idnum; i++) {
-		leader_found = false;
-		evlist__for_each_entry(perf_evlist, ev) {
-			if (!leader_found && (ev == metric_events[i]))
-				leader_found = true;
-
-			if (leader_found &&
-			    !strcmp(ev->name, metric_events[i]->name)) {
-				ev->metric_leader = metric_events[i];
-			}
-			j++;
-		}
 		ev = metric_events[i];
+		ev->metric_leader = ev;
 		set_bit(ev->idx, evlist_used);
 	}
 
@@ -157,7 +183,7 @@ static int metricgroup__setup_events(struct list_head *groups,
 	int i = 0;
 	int ret = 0;
 	struct egroup *eg;
-	struct evsel *evsel;
+	struct evsel *evsel, *tmp;
 	unsigned long *evlist_used;
 
 	evlist_used = bitmap_alloc(perf_evlist->core.nr_entries);
@@ -173,7 +199,8 @@ static int metricgroup__setup_events(struct list_head *groups,
 			ret = -ENOMEM;
 			break;
 		}
-		evsel = find_evsel_group(perf_evlist, &eg->pctx, metric_events,
+		evsel = find_evsel_group(perf_evlist, &eg->pctx,
+					eg->has_constraint, metric_events,
 					evlist_used);
 		if (!evsel) {
 			pr_debug("Cannot resolve %s: %s\n",
@@ -200,6 +227,12 @@ static int metricgroup__setup_events(struct list_head *groups,
 		list_add(&expr->nd, &me->head);
 	}
 
+	evlist__for_each_entry_safe(perf_evlist, tmp, evsel) {
+		if (!test_bit(evsel->idx, evlist_used)) {
+			evlist__remove(perf_evlist, evsel);
+			evsel__delete(evsel);
+		}
+	}
 	bitmap_free(evlist_used);
 
 	return ret;
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 6/7] perf metricgroup: Add options to not group or merge

[Ian Rogers]

Add --metric-no-group that causes all events within metrics to not be
grouped. This can allow the event to get more time when multiplexed, but
may also lower accuracy.
Add --metric-no-merge option. By default events in different metrics may
be shared if the group of events for one metric is the same or larger
than that of the second. Sharing may increase or lower accuracy and so
is now configurable.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/Documentation/perf-stat.txt | 19 +++++++
 tools/perf/builtin-stat.c              | 11 ++++-
 tools/perf/util/metricgroup.c          | 68 ++++++++++++++++++++------
 tools/perf/util/metricgroup.h          |  6 ++-
 tools/perf/util/stat.h                 |  2 +
 5 files changed, 87 insertions(+), 19 deletions(-)

diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
index 3fb5028aef08..cc1e4c62bc91 100644
--- a/tools/perf/Documentation/perf-stat.txt
+++ b/tools/perf/Documentation/perf-stat.txt
@@ -234,6 +234,25 @@ filter out the startup phase of the program, which is often very different.
 
 Print statistics of transactional execution if supported.
 
+--metric-no-group::
+By default, events to compute a metric are placed in weak groups. The
+group tries to enforce scheduling all or none of the events. The
+--metric-no-group option places events outside of groups and may
+increase the chance of the event being scheduled - leading to more
+accuracy. However, as events may not be scheduled together accuracy
+for metrics like instructions per cycle can be lower - as both metrics
+may no longer be being measured at the same time.
+
+--metric-no-merge::
+By default metric events in different weak groups can be shared if one
+group contains all the events needed by another. In such cases one
+group will be eliminated reducing event multiplexing and making it so
+that certain groups of metrics sum to 100%. A downside to sharing a
+group is that the group may require multiplexing and so accuracy for a
+small group that need not have multiplexing is lowered. This option
+forbids the event merging logic from sharing events between groups and
+may be used to increase accuracy in this case.
+
 STAT RECORD
 -----------
 Stores stat data into perf data file.
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 4deb2d46a343..b013fea29e0b 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -840,7 +840,10 @@ static int parse_metric_groups(const struct option *opt,
 			       const char *str,
 			       int unset __maybe_unused)
 {
-	return metricgroup__parse_groups(opt, str, &stat_config.metric_events);
+	return metricgroup__parse_groups(opt, str,
+					 stat_config.metric_no_group,
+					 stat_config.metric_no_merge,
+					 &stat_config.metric_events);
 }
 
 static struct option stat_options[] = {
@@ -918,6 +921,10 @@ static struct option stat_options[] = {
 		     "ms to wait before starting measurement after program start"),
 	OPT_CALLBACK_NOOPT(0, "metric-only", &stat_config.metric_only, NULL,
 			"Only print computed metrics. No raw values", enable_metric_only),
+	OPT_BOOLEAN(0, "metric-no-group", &stat_config.metric_no_group,
+		       "don't group metric events, impacts multiplexing"),
+	OPT_BOOLEAN(0, "metric-no-merge", &stat_config.metric_no_merge,
+		       "don't try to share events between metrics in a group"),
 	OPT_BOOLEAN(0, "topdown", &topdown_run,
 			"measure topdown level 1 statistics"),
 	OPT_BOOLEAN(0, "smi-cost", &smi_cost,
@@ -1442,6 +1449,8 @@ static int add_default_attributes(void)
 			struct option opt = { .value = &evsel_list };
 
 			return metricgroup__parse_groups(&opt, "transaction",
+							 stat_config.metric_no_group,
+							stat_config.metric_no_merge,
 							 &stat_config.metric_events);
 		}
 
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 7ed32baeb767..432ae2e4c7b1 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -95,11 +95,15 @@ struct egroup {
 
 /**
  * Find a group of events in perf_evlist that correpond to those from a parsed
- * metric expression.
+ * metric expression. Note, as find_evsel_group is called in the same order as
+ * perf_evlist was constructed, metric_no_merge doesn't need to test for
+ * underfilling a group.
  * @perf_evlist: a list of events something like: {metric1 leader, metric1
  * sibling, metric1 sibling}:W,duration_time,{metric2 leader, metric2 sibling,
  * metric2 sibling}:W,duration_time
  * @pctx: the parse context for the metric expression.
+ * @metric_no_merge: don't attempt to share events for the metric with other
+ * metrics.
  * @has_constraint: is there a contraint on the group of events? In which case
  * the events won't be grouped.
  * @metric_events: out argument, null terminated array of evsel's associated
@@ -109,6 +113,7 @@ struct egroup {
  */
 static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 				      struct expr_parse_ctx *pctx,
+				      bool metric_no_merge,
 				      bool has_constraint,
 				      struct evsel **metric_events,
 				      unsigned long *evlist_used)
@@ -132,6 +137,9 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 		 */
 		if (has_constraint && ev->weak_group)
 			continue;
+		/* Ignore event if already used and merging is disabled. */
+		if (metric_no_merge && test_bit(ev->idx, evlist_used))
+			continue;
 		if (!has_constraint && ev->leader != current_leader) {
 			/*
 			 * Start of a new group, discard the whole match and
@@ -142,8 +150,23 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 				sizeof(struct evsel *) * idnum);
 			current_leader = ev->leader;
 		}
-		if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr))
+		if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
+			if (has_constraint) {
+				/*
+				 * Events aren't grouped, ensure the same event
+				 * isn't matched from two groups.
+				 */
+				for (i = 0; i < matched_events; i++) {
+					if (!strcmp(ev->name,
+						    metric_events[i]->name)) {
+						break;
+					}
+				}
+				if (i != matched_events)
+					continue;
+			}
 			metric_events[matched_events++] = ev;
+		}
 		if (matched_events == events_to_match)
 			break;
 	}
@@ -175,6 +198,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 }
 
 static int metricgroup__setup_events(struct list_head *groups,
+				     bool metric_no_merge,
 				     struct evlist *perf_evlist,
 				     struct rblist *metric_events_list)
 {
@@ -200,8 +224,9 @@ static int metricgroup__setup_events(struct list_head *groups,
 			break;
 		}
 		evsel = find_evsel_group(perf_evlist, &eg->pctx,
-					eg->has_constraint, metric_events,
-					evlist_used);
+					 metric_no_merge,
+					 eg->has_constraint, metric_events,
+					 evlist_used);
 		if (!evsel) {
 			pr_debug("Cannot resolve %s: %s\n",
 					eg->metric_name, eg->metric_expr);
@@ -520,7 +545,9 @@ int __weak arch_get_runtimeparam(void)
 }
 
 static int __metricgroup__add_metric(struct list_head *group_list,
-				     struct pmu_event *pe, int runtime)
+				     struct pmu_event *pe,
+				     bool metric_no_group,
+				     int runtime)
 {
 	struct egroup *eg;
 
@@ -533,7 +560,7 @@ static int __metricgroup__add_metric(struct list_head *group_list,
 	eg->metric_expr = pe->metric_expr;
 	eg->metric_unit = pe->unit;
 	eg->runtime = runtime;
-	eg->has_constraint = metricgroup__has_constraint(pe);
+	eg->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
 
 	if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
 		expr__ctx_clear(&eg->pctx);
@@ -560,7 +587,8 @@ static int __metricgroup__add_metric(struct list_head *group_list,
 	return 0;
 }
 
-static int metricgroup__add_metric(const char *metric, struct strbuf *events,
+static int metricgroup__add_metric(const char *metric, bool metric_no_group,
+				   struct strbuf *events,
 				   struct list_head *group_list)
 {
 	struct pmu_events_map *map = perf_pmu__find_map(NULL);
@@ -585,7 +613,9 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 
 			if (!strstr(pe->metric_expr, "?")) {
 				ret = __metricgroup__add_metric(group_list,
-								pe, 1);
+								pe,
+								metric_no_group,
+								1);
 			} else {
 				int j, count;
 
@@ -598,7 +628,8 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 
 				for (j = 0; j < count; j++) {
 					ret = __metricgroup__add_metric(
-						group_list, pe, j);
+						group_list, pe,
+						metric_no_group, j);
 				}
 			}
 			if (ret == -ENOMEM)
@@ -622,7 +653,8 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 	return ret;
 }
 
-static int metricgroup__add_metric_list(const char *list, struct strbuf *events,
+static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
+					struct strbuf *events,
 				        struct list_head *group_list)
 {
 	char *llist, *nlist, *p;
@@ -637,7 +669,8 @@ static int metricgroup__add_metric_list(const char *list, struct strbuf *events,
 	strbuf_addf(events, "%s", "");
 
 	while ((p = strsep(&llist, ",")) != NULL) {
-		ret = metricgroup__add_metric(p, events, group_list);
+		ret = metricgroup__add_metric(p, metric_no_group, events,
+					      group_list);
 		if (ret == -EINVAL) {
 			fprintf(stderr, "Cannot find metric or group `%s'\n",
 					p);
@@ -664,8 +697,10 @@ static void metricgroup__free_egroups(struct list_head *group_list)
 }
 
 int metricgroup__parse_groups(const struct option *opt,
-			   const char *str,
-			   struct rblist *metric_events)
+			      const char *str,
+			      bool metric_no_group,
+			      bool metric_no_merge,
+			      struct rblist *metric_events)
 {
 	struct parse_events_error parse_error;
 	struct evlist *perf_evlist = *(struct evlist **)opt->value;
@@ -675,7 +710,8 @@ int metricgroup__parse_groups(const struct option *opt,
 
 	if (metric_events->nr_entries == 0)
 		metricgroup__rblist_init(metric_events);
-	ret = metricgroup__add_metric_list(str, &extra_events, &group_list);
+	ret = metricgroup__add_metric_list(str, metric_no_group,
+					   &extra_events, &group_list);
 	if (ret)
 		return ret;
 	pr_debug("adding %s\n", extra_events.buf);
@@ -686,8 +722,8 @@ int metricgroup__parse_groups(const struct option *opt,
 		goto out;
 	}
 	strbuf_release(&extra_events);
-	ret = metricgroup__setup_events(&group_list, perf_evlist,
-					metric_events);
+	ret = metricgroup__setup_events(&group_list, metric_no_merge,
+					perf_evlist, metric_events);
 out:
 	metricgroup__free_egroups(&group_list);
 	return ret;
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index 6b09eb30b4ec..287850bcdeca 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -29,8 +29,10 @@ struct metric_event *metricgroup__lookup(struct rblist *metric_events,
 					 struct evsel *evsel,
 					 bool create);
 int metricgroup__parse_groups(const struct option *opt,
-			const char *str,
-			struct rblist *metric_events);
+			      const char *str,
+			      bool metric_no_group,
+			      bool metric_no_merge,
+			      struct rblist *metric_events);
 
 void metricgroup__print(bool metrics, bool groups, char *filter,
 			bool raw, bool details);
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index b4fdfaa7f2c0..ea054d27ce1d 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -110,6 +110,8 @@ struct perf_stat_config {
 	bool			 all_kernel;
 	bool			 all_user;
 	bool			 percore_show_thread;
+	bool			 metric_no_group;
+	bool			 metric_no_merge;
 	FILE			*output;
 	unsigned int		 interval;
 	unsigned int		 timeout;
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 7/7] perf metricgroup: Remove unnecessary ',' from events

[Ian Rogers]

Remove unnecessary commas from events before they are parsed. This
avoids ',' being echoed by parse-events.l.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 432ae2e4c7b1..570285132cf6 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -501,9 +501,14 @@ static void metricgroup__add_metric_non_group(struct strbuf *events,
 {
 	struct hashmap_entry *cur;
 	size_t bkt;
+	bool first = true;
 
-	hashmap__for_each_entry((&ctx->ids), cur, bkt)
-		strbuf_addf(events, ",%s", (const char *)cur->key);
+	hashmap__for_each_entry((&ctx->ids), cur, bkt) {
+		if (!first)
+			strbuf_addf(events, ",");
+		strbuf_addf(events, "%s", (const char *)cur->key);
+		first = false;
+	}
 }
 
 static void metricgroup___watchdog_constraint_hint(const char *name, bool foot)
@@ -637,8 +642,10 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 		}
 	}
 	if (!ret) {
+		bool first = true;
+
 		list_for_each_entry(eg, group_list, nd) {
-			if (events->len > 0)
+			if (events->len > 0 && !first)
 				strbuf_addf(events, ",");
 
 			if (eg->has_constraint) {
@@ -648,6 +655,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 				metricgroup__add_metric_weak_group(events,
 								   &eg->pctx);
 			}
+			first = false;
 		}
 	}
 	return ret;
-- 
2.26.2.761.g0e0b3e54be-goog

Re: [PATCH 0/7] Share events between metrics

[Jiri Olsa]

On Wed, May 20, 2020 at 12:28:07AM -0700, Ian Rogers wrote:
> Metric groups contain metrics. Metrics create groups of events to
> ideally be scheduled together. Often metrics refer to the same events,
> for example, a cache hit and cache miss rate. Using separate event
> groups means these metrics are multiplexed at different times and the
> counts don't sum to 100%. More multiplexing also decreases the
> accuracy of the measurement.
> 
> This change orders metrics from groups or the command line, so that
> the ones with the most events are set up first. Later metrics see if
> groups already provide their events, and reuse them if
> possible. Unnecessary events and groups are eliminated.
> 
> The option --metric-no-group is added so that metrics aren't placed in
> groups. This affects multiplexing and may increase sharing.
> 
> The option --metric-mo-merge is added and with this option the
> existing grouping behavior is preserved.
> 
> Using skylakex metrics I ran the following shell code to count the
> number of events for each metric group (this ignores metric groups
> with a single metric, and one of the duplicated TopdownL1 and
> TopDownL1 groups):

hi,
I'm getting parser error with:

[jolsa@krava perf]$ sudo ./perf stat -M IPC,CPI -a -I 1000
event syntax error: '..ed.thread}:W{inst_retired.any,cpu_clk_unhalted.thread}:W,{inst_retired.any,cycles}:W'
                                  \___ parser error


jirka

Re: [PATCH 3/7] perf metricgroup: Delay events string creation

[Jiri Olsa]

On Wed, May 20, 2020 at 12:28:10AM -0700, Ian Rogers wrote:
> Currently event groups are placed into groups_list at the same time as
> the events string containing the events is built. Separate these two
> operations and build the groups_list first, then the event string from
> the groups_list. This adds an ability to reorder the groups_list that
> will be used in a later patch.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/metricgroup.c | 38 +++++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 7a43ee0a2e40..afd960d03a77 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -90,6 +90,7 @@ struct egroup {
>  	const char *metric_expr;
>  	const char *metric_unit;
>  	int runtime;
> +	bool has_constraint;
>  };
>  
>  static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> @@ -485,8 +486,8 @@ int __weak arch_get_runtimeparam(void)
>  	return 1;
>  }
>  
> -static int __metricgroup__add_metric(struct strbuf *events,
> -		struct list_head *group_list, struct pmu_event *pe, int runtime)
> +static int __metricgroup__add_metric(struct list_head *group_list,
> +				     struct pmu_event *pe, int runtime)
>  {
>  	struct egroup *eg;
>  
> @@ -499,6 +500,7 @@ static int __metricgroup__add_metric(struct strbuf *events,
>  	eg->metric_expr = pe->metric_expr;
>  	eg->metric_unit = pe->unit;
>  	eg->runtime = runtime;
> +	eg->has_constraint = metricgroup__has_constraint(pe);
>  
>  	if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
>  		expr__ctx_clear(&eg->pctx);
> @@ -506,14 +508,6 @@ static int __metricgroup__add_metric(struct strbuf *events,
>  		return -EINVAL;
>  	}
>  
> -	if (events->len > 0)
> -		strbuf_addf(events, ",");
> -
> -	if (metricgroup__has_constraint(pe))
> -		metricgroup__add_metric_non_group(events, &eg->pctx);
> -	else
> -		metricgroup__add_metric_weak_group(events, &eg->pctx);
> -
>  	list_add_tail(&eg->nd, group_list);
>  
>  	return 0;
> @@ -524,6 +518,7 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  {
>  	struct pmu_events_map *map = perf_pmu__find_map(NULL);
>  	struct pmu_event *pe;
> +	struct egroup *eg;
>  	int i, ret = -EINVAL;
>  
>  	if (!map)
> @@ -542,7 +537,8 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  			pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>  
>  			if (!strstr(pe->metric_expr, "?")) {
> -				ret = __metricgroup__add_metric(events, group_list, pe, 1);
> +				ret = __metricgroup__add_metric(group_list,
> +								pe, 1);
>  			} else {
>  				int j, count;
>  
> @@ -553,13 +549,29 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  				 * those events to group_list.
>  				 */
>  
> -				for (j = 0; j < count; j++)
> -					ret = __metricgroup__add_metric(events, group_list, pe, j);
> +				for (j = 0; j < count; j++) {
> +					ret = __metricgroup__add_metric(
> +						group_list, pe, j);
> +				}
>  			}
>  			if (ret == -ENOMEM)
>  				break;
>  		}
>  	}
> +	if (!ret) {

could you please do instead:

	if (ret)
		return ret;

so the code below cuts down one indent level and you
don't need to split up the lines

thanks,
jirka

> +		list_for_each_entry(eg, group_list, nd) {
> +			if (events->len > 0)
> +				strbuf_addf(events, ",");
> +
> +			if (eg->has_constraint) {
> +				metricgroup__add_metric_non_group(events,
> +								  &eg->pctx);
> +			} else {
> +				metricgroup__add_metric_weak_group(events,
> +								   &eg->pctx);
> +			}
> +		}
> +	}
>  	return ret;
>  }
>  
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

Re: [PATCH 1/7] perf metricgroup: Change evlist_used to a bitmap

[Jiri Olsa]

On Wed, May 20, 2020 at 12:28:08AM -0700, Ian Rogers wrote:
> Use a bitmap rather than an array of bools.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

> ---
>  tools/perf/util/metricgroup.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 6772d256dfdf..a16f60da06ab 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -95,7 +95,7 @@ struct egroup {
>  static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>  				      struct expr_parse_ctx *pctx,
>  				      struct evsel **metric_events,
> -				      bool *evlist_used)
> +				      unsigned long *evlist_used)
>  {
>  	struct evsel *ev;
>  	bool leader_found;
> @@ -105,7 +105,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>  	double *val_ptr;
>  
>  	evlist__for_each_entry (perf_evlist, ev) {
> -		if (evlist_used[j++])
> +		if (test_bit(j++, evlist_used))
>  			continue;
>  		if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
>  			if (!metric_events[i])
> @@ -141,7 +141,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>  			j++;
>  		}
>  		ev = metric_events[i];
> -		evlist_used[ev->idx] = true;
> +		set_bit(ev->idx, evlist_used);
>  	}
>  
>  	return metric_events[0];
> @@ -157,13 +157,11 @@ static int metricgroup__setup_events(struct list_head *groups,
>  	int ret = 0;
>  	struct egroup *eg;
>  	struct evsel *evsel;
> -	bool *evlist_used;
> +	unsigned long *evlist_used;
>  
> -	evlist_used = calloc(perf_evlist->core.nr_entries, sizeof(bool));
> -	if (!evlist_used) {
> -		ret = -ENOMEM;
> -		return ret;
> -	}
> +	evlist_used = bitmap_alloc(perf_evlist->core.nr_entries);
> +	if (!evlist_used)
> +		return -ENOMEM;
>  
>  	list_for_each_entry (eg, groups, nd) {
>  		struct evsel **metric_events;
> @@ -201,7 +199,7 @@ static int metricgroup__setup_events(struct list_head *groups,
>  		list_add(&expr->nd, &me->head);
>  	}
>  
> -	free(evlist_used);
> +	bitmap_free(evlist_used);
>  
>  	return ret;
>  }
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

Re: [PATCH 1/7] perf metricgroup: Change evlist_used to a bitmap

[Arnaldo Carvalho de Melo]

Em Wed, May 20, 2020 at 03:14:22PM +0200, Jiri Olsa escreveu:
> On Wed, May 20, 2020 at 12:28:08AM -0700, Ian Rogers wrote:
> > Use a bitmap rather than an array of bools.
> > 
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks, applied.
 
> thanks,
> jirka
> 
> > ---
> >  tools/perf/util/metricgroup.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 6772d256dfdf..a16f60da06ab 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -95,7 +95,7 @@ struct egroup {
> >  static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >  				      struct expr_parse_ctx *pctx,
> >  				      struct evsel **metric_events,
> > -				      bool *evlist_used)
> > +				      unsigned long *evlist_used)
> >  {
> >  	struct evsel *ev;
> >  	bool leader_found;
> > @@ -105,7 +105,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >  	double *val_ptr;
> >  
> >  	evlist__for_each_entry (perf_evlist, ev) {
> > -		if (evlist_used[j++])
> > +		if (test_bit(j++, evlist_used))
> >  			continue;
> >  		if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> >  			if (!metric_events[i])
> > @@ -141,7 +141,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >  			j++;
> >  		}
> >  		ev = metric_events[i];
> > -		evlist_used[ev->idx] = true;
> > +		set_bit(ev->idx, evlist_used);
> >  	}
> >  
> >  	return metric_events[0];
> > @@ -157,13 +157,11 @@ static int metricgroup__setup_events(struct list_head *groups,
> >  	int ret = 0;
> >  	struct egroup *eg;
> >  	struct evsel *evsel;
> > -	bool *evlist_used;
> > +	unsigned long *evlist_used;
> >  
> > -	evlist_used = calloc(perf_evlist->core.nr_entries, sizeof(bool));
> > -	if (!evlist_used) {
> > -		ret = -ENOMEM;
> > -		return ret;
> > -	}
> > +	evlist_used = bitmap_alloc(perf_evlist->core.nr_entries);
> > +	if (!evlist_used)
> > +		return -ENOMEM;
> >  
> >  	list_for_each_entry (eg, groups, nd) {
> >  		struct evsel **metric_events;
> > @@ -201,7 +199,7 @@ static int metricgroup__setup_events(struct list_head *groups,
> >  		list_add(&expr->nd, &me->head);
> >  	}
> >  
> > -	free(evlist_used);
> > +	bitmap_free(evlist_used);
> >  
> >  	return ret;
> >  }
> > -- 
> > 2.26.2.761.g0e0b3e54be-goog
> > 
> 

-- 

- Arnaldo

Re: [PATCH 5/7] perf metricgroup: Remove duped metric group events

[Jiri Olsa]

On Wed, May 20, 2020 at 12:28:12AM -0700, Ian Rogers wrote:

SNIP

>  
> @@ -157,7 +183,7 @@ static int metricgroup__setup_events(struct list_head *groups,
>  	int i = 0;
>  	int ret = 0;
>  	struct egroup *eg;
> -	struct evsel *evsel;
> +	struct evsel *evsel, *tmp;
>  	unsigned long *evlist_used;
>  
>  	evlist_used = bitmap_alloc(perf_evlist->core.nr_entries);
> @@ -173,7 +199,8 @@ static int metricgroup__setup_events(struct list_head *groups,
>  			ret = -ENOMEM;
>  			break;
>  		}
> -		evsel = find_evsel_group(perf_evlist, &eg->pctx, metric_events,
> +		evsel = find_evsel_group(perf_evlist, &eg->pctx,
> +					eg->has_constraint, metric_events,
>  					evlist_used);
>  		if (!evsel) {
>  			pr_debug("Cannot resolve %s: %s\n",
> @@ -200,6 +227,12 @@ static int metricgroup__setup_events(struct list_head *groups,
>  		list_add(&expr->nd, &me->head);
>  	}
>  
> +	evlist__for_each_entry_safe(perf_evlist, tmp, evsel) {
> +		if (!test_bit(evsel->idx, evlist_used)) {
> +			evlist__remove(perf_evlist, evsel);
> +			evsel__delete(evsel);
> +		}

is the groupping still enabled when we merge groups? could part
of the metric (events) be now computed in different groups?

I was wondering if we could merge all the hasmaps into single
one before the parse the evlist.. this way we won't need removing
later.. but I did not thought this through completely, so it
might not work at some point

jirka

Re: [PATCH 0/7] Share events between metrics

[Ian Rogers]

On Wed, May 20, 2020 at 6:14 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, May 20, 2020 at 12:28:07AM -0700, Ian Rogers wrote:
> > Metric groups contain metrics. Metrics create groups of events to
> > ideally be scheduled together. Often metrics refer to the same events,
> > for example, a cache hit and cache miss rate. Using separate event
> > groups means these metrics are multiplexed at different times and the
> > counts don't sum to 100%. More multiplexing also decreases the
> > accuracy of the measurement.
> >
> > This change orders metrics from groups or the command line, so that
> > the ones with the most events are set up first. Later metrics see if
> > groups already provide their events, and reuse them if
> > possible. Unnecessary events and groups are eliminated.
> >
> > The option --metric-no-group is added so that metrics aren't placed in
> > groups. This affects multiplexing and may increase sharing.
> >
> > The option --metric-mo-merge is added and with this option the
> > existing grouping behavior is preserved.
> >
> > Using skylakex metrics I ran the following shell code to count the
> > number of events for each metric group (this ignores metric groups
> > with a single metric, and one of the duplicated TopdownL1 and
> > TopDownL1 groups):
>
> hi,
> I'm getting parser error with:
>
> [jolsa@krava perf]$ sudo ./perf stat -M IPC,CPI -a -I 1000
> event syntax error: '..ed.thread}:W{inst_retired.any,cpu_clk_unhalted.thread}:W,{inst_retired.any,cycles}:W'
>                                   \___ parser error
>
> jirka

Ah, looks like an issue introduced by:
https://lore.kernel.org/lkml/20200520072814.128267-8-irogers@google.com/
as there is a missing comma. I'll investigate after some breakfast.

Thanks,
Ian

Re: [PATCH 5/7] perf metricgroup: Remove duped metric group events

[Ian Rogers]

On Wed, May 20, 2020 at 6:49 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, May 20, 2020 at 12:28:12AM -0700, Ian Rogers wrote:
>
> SNIP
>
> >
> > @@ -157,7 +183,7 @@ static int metricgroup__setup_events(struct list_head *groups,
> >       int i = 0;
> >       int ret = 0;
> >       struct egroup *eg;
> > -     struct evsel *evsel;
> > +     struct evsel *evsel, *tmp;
> >       unsigned long *evlist_used;
> >
> >       evlist_used = bitmap_alloc(perf_evlist->core.nr_entries);
> > @@ -173,7 +199,8 @@ static int metricgroup__setup_events(struct list_head *groups,
> >                       ret = -ENOMEM;
> >                       break;
> >               }
> > -             evsel = find_evsel_group(perf_evlist, &eg->pctx, metric_events,
> > +             evsel = find_evsel_group(perf_evlist, &eg->pctx,
> > +                                     eg->has_constraint, metric_events,
> >                                       evlist_used);
> >               if (!evsel) {
> >                       pr_debug("Cannot resolve %s: %s\n",
> > @@ -200,6 +227,12 @@ static int metricgroup__setup_events(struct list_head *groups,
> >               list_add(&expr->nd, &me->head);
> >       }
> >
> > +     evlist__for_each_entry_safe(perf_evlist, tmp, evsel) {
> > +             if (!test_bit(evsel->idx, evlist_used)) {
> > +                     evlist__remove(perf_evlist, evsel);
> > +                     evsel__delete(evsel);
> > +             }
>
> is the groupping still enabled when we merge groups? could part
> of the metric (events) be now computed in different groups?

By default the change will take two metrics and allow the shorter
metric (in terms of number of events) to share the events of the
longer metric. If the events for the shorter metric aren't in the
longer metric then the shorter metric must use its own group of
events. If sharing has occurred then the bitmap is used to work out
which events and groups are no longer in use.

With --metric-no-group then any event can be used for a metric as
there is no grouping. This is why more events can be eliminated.

With --metric-no-merge then the logic to share events between
different metrics is disabled and every metric is in a group. This
allows the current behavior to be had.

There are some corner cases, such as metrics with constraints (that
don't group) and duration_time that is never placed into a group.

Partial sharing, with one event in 1 weak event group and 1 in another
is never performed. Using --metric-no-group allows something similar.
Given multiplexing, I'd be concerned about accuracy problems if events
between groups were shared - say for IPC, were you measuring
instructions and cycles at the same moment?

> I was wondering if we could merge all the hasmaps into single
> one before the parse the evlist.. this way we won't need removing
> later.. but I did not thought this through completely, so it
> might not work at some point

This could be done in the --metric-no-group case reasonably easily
like the current hashmap. For groups you'd want something like a set
of sets of events, but then you'd only be able to share events if the
sets were the same. A directed acyclic graph could capture the events
and the sharing relationships, it may be possible to optimize cases
like {A,B,C}, {A,B,D}, {A,B} so that the small group on the end shares
events with both the {A,B,C} and {A,B,D} group. This may be good
follow up work. We could also solve this in the json, for example
create a "phony" group of {A,B,C,D} that all three metrics share from.
You could also use --metric-no-group to achieve that sharing now.

Thanks,
Ian

> jirka
>

Re: [PATCH 3/7] perf metricgroup: Delay events string creation

[Ian Rogers]

On Wed, May 20, 2020 at 6:14 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, May 20, 2020 at 12:28:10AM -0700, Ian Rogers wrote:
> > Currently event groups are placed into groups_list at the same time as
> > the events string containing the events is built. Separate these two
> > operations and build the groups_list first, then the event string from
> > the groups_list. This adds an ability to reorder the groups_list that
> > will be used in a later patch.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/metricgroup.c | 38 +++++++++++++++++++++++------------
> >  1 file changed, 25 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 7a43ee0a2e40..afd960d03a77 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -90,6 +90,7 @@ struct egroup {
> >       const char *metric_expr;
> >       const char *metric_unit;
> >       int runtime;
> > +     bool has_constraint;
> >  };
> >
> >  static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > @@ -485,8 +486,8 @@ int __weak arch_get_runtimeparam(void)
> >       return 1;
> >  }
> >
> > -static int __metricgroup__add_metric(struct strbuf *events,
> > -             struct list_head *group_list, struct pmu_event *pe, int runtime)
> > +static int __metricgroup__add_metric(struct list_head *group_list,
> > +                                  struct pmu_event *pe, int runtime)
> >  {
> >       struct egroup *eg;
> >
> > @@ -499,6 +500,7 @@ static int __metricgroup__add_metric(struct strbuf *events,
> >       eg->metric_expr = pe->metric_expr;
> >       eg->metric_unit = pe->unit;
> >       eg->runtime = runtime;
> > +     eg->has_constraint = metricgroup__has_constraint(pe);
> >
> >       if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
> >               expr__ctx_clear(&eg->pctx);
> > @@ -506,14 +508,6 @@ static int __metricgroup__add_metric(struct strbuf *events,
> >               return -EINVAL;
> >       }
> >
> > -     if (events->len > 0)
> > -             strbuf_addf(events, ",");
> > -
> > -     if (metricgroup__has_constraint(pe))
> > -             metricgroup__add_metric_non_group(events, &eg->pctx);
> > -     else
> > -             metricgroup__add_metric_weak_group(events, &eg->pctx);
> > -
> >       list_add_tail(&eg->nd, group_list);
> >
> >       return 0;
> > @@ -524,6 +518,7 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> >  {
> >       struct pmu_events_map *map = perf_pmu__find_map(NULL);
> >       struct pmu_event *pe;
> > +     struct egroup *eg;
> >       int i, ret = -EINVAL;
> >
> >       if (!map)
> > @@ -542,7 +537,8 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> >                       pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
> >
> >                       if (!strstr(pe->metric_expr, "?")) {
> > -                             ret = __metricgroup__add_metric(events, group_list, pe, 1);
> > +                             ret = __metricgroup__add_metric(group_list,
> > +                                                             pe, 1);
> >                       } else {
> >                               int j, count;
> >
> > @@ -553,13 +549,29 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> >                                * those events to group_list.
> >                                */
> >
> > -                             for (j = 0; j < count; j++)
> > -                                     ret = __metricgroup__add_metric(events, group_list, pe, j);
> > +                             for (j = 0; j < count; j++) {
> > +                                     ret = __metricgroup__add_metric(
> > +                                             group_list, pe, j);
> > +                             }
> >                       }
> >                       if (ret == -ENOMEM)
> >                               break;
> >               }
> >       }
> > +     if (!ret) {
>
> could you please do instead:
>
>         if (ret)
>                 return ret;
>
> so the code below cuts down one indent level and you
> don't need to split up the lines

Done, broken out as a separate patch in v2:
https://lore.kernel.org/lkml/20200520182011.32236-3-irogers@google.com/

Thanks,
Ian

> thanks,
> jirka
>
> > +             list_for_each_entry(eg, group_list, nd) {
> > +                     if (events->len > 0)
> > +                             strbuf_addf(events, ",");
> > +
> > +                     if (eg->has_constraint) {
> > +                             metricgroup__add_metric_non_group(events,
> > +                                                               &eg->pctx);
> > +                     } else {
> > +                             metricgroup__add_metric_weak_group(events,
> > +                                                                &eg->pctx);
> > +                     }
> > +             }
> > +     }
> >       return ret;
> >  }
> >
> > --
> > 2.26.2.761.g0e0b3e54be-goog
> >
>

Re: [PATCH 3/7] perf metricgroup: Delay events string creation

[Arnaldo Carvalho de Melo]

Em Wed, May 20, 2020 at 11:22:22AM -0700, Ian Rogers escreveu:
> On Wed, May 20, 2020 at 6:14 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >                               break;
> > >               }
> > >       }
> > > +     if (!ret) {
> >
> > could you please do instead:
> >
> >         if (ret)
> >                 return ret;
> >
> > so the code below cuts down one indent level and you
> > don't need to split up the lines
> 
> Done, broken out as a separate patch in v2:
> https://lore.kernel.org/lkml/20200520182011.32236-3-irogers@google.com/

Jiri, was this the only issue with this patchkit? I've merged already
the first one, that you acked.

- Arnaldo

Re: [PATCH 5/7] perf metricgroup: Remove duped metric group events

[Jiri Olsa]

On Wed, May 20, 2020 at 09:50:13AM -0700, Ian Rogers wrote:

SNIP

> > > +             }
> >
> > is the groupping still enabled when we merge groups? could part
> > of the metric (events) be now computed in different groups?
> 
> By default the change will take two metrics and allow the shorter
> metric (in terms of number of events) to share the events of the
> longer metric. If the events for the shorter metric aren't in the
> longer metric then the shorter metric must use its own group of
> events. If sharing has occurred then the bitmap is used to work out
> which events and groups are no longer in use.
> 
> With --metric-no-group then any event can be used for a metric as
> there is no grouping. This is why more events can be eliminated.
> 
> With --metric-no-merge then the logic to share events between
> different metrics is disabled and every metric is in a group. This
> allows the current behavior to be had.
> 
> There are some corner cases, such as metrics with constraints (that
> don't group) and duration_time that is never placed into a group.
> 
> Partial sharing, with one event in 1 weak event group and 1 in another
> is never performed. Using --metric-no-group allows something similar.
> Given multiplexing, I'd be concerned about accuracy problems if events
> between groups were shared - say for IPC, were you measuring
> instructions and cycles at the same moment?

hum, I think that's also concern if you are multiplexing 2 groups and one
metric getting events from both groups that were not meassured together 

it makes sense to me put all the merged events into single weak group
anything else will have the issue you described above, no?

and perhaps add command line option for merging that to make sure it's
what user actuly wants

thanks,
jirka


> 
> > I was wondering if we could merge all the hasmaps into single
> > one before the parse the evlist.. this way we won't need removing
> > later.. but I did not thought this through completely, so it
> > might not work at some point
> 
> This could be done in the --metric-no-group case reasonably easily
> like the current hashmap. For groups you'd want something like a set
> of sets of events, but then you'd only be able to share events if the
> sets were the same. A directed acyclic graph could capture the events
> and the sharing relationships, it may be possible to optimize cases
> like {A,B,C}, {A,B,D}, {A,B} so that the small group on the end shares
> events with both the {A,B,C} and {A,B,D} group. This may be good
> follow up work. We could also solve this in the json, for example
> create a "phony" group of {A,B,C,D} that all three metrics share from.
> You could also use --metric-no-group to achieve that sharing now.
> 
> Thanks,
> Ian
> 
> > jirka
> >
> 

Re: [PATCH 3/7] perf metricgroup: Delay events string creation

[Jiri Olsa]

On Wed, May 20, 2020 at 05:34:09PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, May 20, 2020 at 11:22:22AM -0700, Ian Rogers escreveu:
> > On Wed, May 20, 2020 at 6:14 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > > >                               break;
> > > >               }
> > > >       }
> > > > +     if (!ret) {
> > >
> > > could you please do instead:
> > >
> > >         if (ret)
> > >                 return ret;
> > >
> > > so the code below cuts down one indent level and you
> > > don't need to split up the lines
> > 
> > Done, broken out as a separate patch in v2:
> > https://lore.kernel.org/lkml/20200520182011.32236-3-irogers@google.com/
> 
> Jiri, was this the only issue with this patchkit? I've merged already
> the first one, that you acked.

I'm still wondering if we can keep groups after the merge,
it's in my other reply

jirka

Re: [PATCH 5/7] perf metricgroup: Remove duped metric group events

[Ian Rogers]

On Wed, May 20, 2020 at 3:09 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, May 20, 2020 at 09:50:13AM -0700, Ian Rogers wrote:
>
> SNIP
>
> > > > +             }
> > >
> > > is the groupping still enabled when we merge groups? could part
> > > of the metric (events) be now computed in different groups?
> >
> > By default the change will take two metrics and allow the shorter
> > metric (in terms of number of events) to share the events of the
> > longer metric. If the events for the shorter metric aren't in the
> > longer metric then the shorter metric must use its own group of
> > events. If sharing has occurred then the bitmap is used to work out
> > which events and groups are no longer in use.
> >
> > With --metric-no-group then any event can be used for a metric as
> > there is no grouping. This is why more events can be eliminated.
> >
> > With --metric-no-merge then the logic to share events between
> > different metrics is disabled and every metric is in a group. This
> > allows the current behavior to be had.
> >
> > There are some corner cases, such as metrics with constraints (that
> > don't group) and duration_time that is never placed into a group.
> >
> > Partial sharing, with one event in 1 weak event group and 1 in another
> > is never performed. Using --metric-no-group allows something similar.
> > Given multiplexing, I'd be concerned about accuracy problems if events
> > between groups were shared - say for IPC, were you measuring
> > instructions and cycles at the same moment?
>
> hum, I think that's also concern if you are multiplexing 2 groups and one
> metric getting events from both groups that were not meassured together
>
> it makes sense to me put all the merged events into single weak group
> anything else will have the issue you described above, no?
>
> and perhaps add command line option for merging that to make sure it's
> what user actuly wants

I'm not sure I'm following. With the patch set if we have 3 metrics
with the event groups shown:
M1: {A,B,C}:W
M2: {A,B}:W
M3: {A,B,D}:W

then what happens is we sort the metrics in to M1, M3, M2 then when we
come to match the events:

 - by default: match events allowing sharing if all events come from
the same group. So in the example M1 will first match with {A,B,C}
then M3 will fail to match the group {A,B,C} but match {A,B,D}; M2
will succeed with matching {A,B} from M1. The events/group for M2 can
be removed as they are no longer used. This kind of sharing is
opportunistic and respects existing groupings. While it may mean a
metric is computed from a group that now multiplexes, that group will
run for more of the time as there are fewer groups to multiplex with.
In this example we've gone from 3 groups down to 2, 8 events down to
6. An improvement would be to realize that A,B is in both M1 and M3,
so when we print the stat we could combine these values.

 - with --metric-no-merge: no events are shared by metrics M1, M2 and
M3 have their events and computation as things currently are. There
are 3 groups and 8 events.

 - with --metric-no-group: all groups are removed and so the evlist
has A,B,C,A,B,A,B,D in it. The matching will now match M1 to A,B,C at
the beginning of the list, M2 to the first A,B and M3 to the same A,B
and D at the end of the list. We've got no groups and the events have
gone from 8 down to 4.

It is difficult to reason about which grouping is most accurate. If we
have 4 counters (no NMI watchdog) then this example will fit with no
multiplexing. The default above should achieve less multiplexing, in
the same way merging PMU events currently does - this patch is trying
to mirror the --no-merge functionality to a degree. Considering
TopDownL1 then we go from metrics that never sum to 100%, to metrics
that do in either the default or --metric-no-group cases.

I'm not sure what user option is missing with these combinations? The
default is trying to strike a compromise and I think user interaction
is unnecessary, just as --no-merge doesn't cause interaction. If the
existing behavior is wanted using --metric-no-merge will give that.
The new default and --metric-no-group are hopefully going to reduce
the number of groups and events. I'm somewhat agnostic as to what the
flag functionality should be as what I'm working with needs either the
default or --metric-no-group, I can use whatever flag is agreed upon.

Thanks,
Ian

> thanks,
> jirka
>
>
> >
> > > I was wondering if we could merge all the hasmaps into single
> > > one before the parse the evlist.. this way we won't need removing
> > > later.. but I did not thought this through completely, so it
> > > might not work at some point
> >
> > This could be done in the --metric-no-group case reasonably easily
> > like the current hashmap. For groups you'd want something like a set
> > of sets of events, but then you'd only be able to share events if the
> > sets were the same. A directed acyclic graph could capture the events
> > and the sharing relationships, it may be possible to optimize cases
> > like {A,B,C}, {A,B,D}, {A,B} so that the small group on the end shares
> > events with both the {A,B,C} and {A,B,D} group. This may be good
> > follow up work. We could also solve this in the json, for example
> > create a "phony" group of {A,B,C,D} that all three metrics share from.
> > You could also use --metric-no-group to achieve that sharing now.
> >
> > Thanks,
> > Ian
> >
> > > jirka
> > >
> >
>

Re: [PATCH 5/7] perf metricgroup: Remove duped metric group events

[Jiri Olsa]

On Wed, May 20, 2020 at 03:42:02PM -0700, Ian Rogers wrote:

SNIP

> >
> > hum, I think that's also concern if you are multiplexing 2 groups and one
> > metric getting events from both groups that were not meassured together
> >
> > it makes sense to me put all the merged events into single weak group
> > anything else will have the issue you described above, no?
> >
> > and perhaps add command line option for merging that to make sure it's
> > what user actuly wants
> 
> I'm not sure I'm following. With the patch set if we have 3 metrics
> with the event groups shown:
> M1: {A,B,C}:W
> M2: {A,B}:W
> M3: {A,B,D}:W
> 
> then what happens is we sort the metrics in to M1, M3, M2 then when we
> come to match the events:
> 
>  - by default: match events allowing sharing if all events come from
> the same group. So in the example M1 will first match with {A,B,C}
> then M3 will fail to match the group {A,B,C} but match {A,B,D}; M2
> will succeed with matching {A,B} from M1. The events/group for M2 can
> be removed as they are no longer used. This kind of sharing is
> opportunistic and respects existing groupings. While it may mean a
> metric is computed from a group that now multiplexes, that group will
> run for more of the time as there are fewer groups to multiplex with.
> In this example we've gone from 3 groups down to 2, 8 events down to
> 6. An improvement would be to realize that A,B is in both M1 and M3,
> so when we print the stat we could combine these values.

ok, I misunderstood and thought you would colaps also M3 to
have A,B computed via M1 group and with separate D ...

thanks a lot for the explanation, it might be great to have it
in the comments/changelog or even man page

> 
>  - with --metric-no-merge: no events are shared by metrics M1, M2 and
> M3 have their events and computation as things currently are. There
> are 3 groups and 8 events.
> 
>  - with --metric-no-group: all groups are removed and so the evlist
> has A,B,C,A,B,A,B,D in it. The matching will now match M1 to A,B,C at
> the beginning of the list, M2 to the first A,B and M3 to the same A,B
> and D at the end of the list. We've got no groups and the events have
> gone from 8 down to 4.
> 
> It is difficult to reason about which grouping is most accurate. If we
> have 4 counters (no NMI watchdog) then this example will fit with no
> multiplexing. The default above should achieve less multiplexing, in
> the same way merging PMU events currently does - this patch is trying
> to mirror the --no-merge functionality to a degree. Considering
> TopDownL1 then we go from metrics that never sum to 100%, to metrics
> that do in either the default or --metric-no-group cases.
> 
> I'm not sure what user option is missing with these combinations? The
> default is trying to strike a compromise and I think user interaction
> is unnecessary, just as --no-merge doesn't cause interaction. If the
> existing behavior is wanted using --metric-no-merge will give that.
> The new default and --metric-no-group are hopefully going to reduce
> the number of groups and events. I'm somewhat agnostic as to what the
> flag functionality should be as what I'm working with needs either the
> default or --metric-no-group, I can use whatever flag is agreed upon.

no other option is needed then

thanks,
jirka

Re: [PATCH 5/7] perf metricgroup: Remove duped metric group events

[Ian Rogers]

On Thu, May 21, 2020 at 3:54 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, May 20, 2020 at 03:42:02PM -0700, Ian Rogers wrote:
>
> SNIP
>
> > >
> > > hum, I think that's also concern if you are multiplexing 2 groups and one
> > > metric getting events from both groups that were not meassured together
> > >
> > > it makes sense to me put all the merged events into single weak group
> > > anything else will have the issue you described above, no?
> > >
> > > and perhaps add command line option for merging that to make sure it's
> > > what user actuly wants
> >
> > I'm not sure I'm following. With the patch set if we have 3 metrics
> > with the event groups shown:
> > M1: {A,B,C}:W
> > M2: {A,B}:W
> > M3: {A,B,D}:W
> >
> > then what happens is we sort the metrics in to M1, M3, M2 then when we
> > come to match the events:
> >
> >  - by default: match events allowing sharing if all events come from
> > the same group. So in the example M1 will first match with {A,B,C}
> > then M3 will fail to match the group {A,B,C} but match {A,B,D}; M2
> > will succeed with matching {A,B} from M1. The events/group for M2 can
> > be removed as they are no longer used. This kind of sharing is
> > opportunistic and respects existing groupings. While it may mean a
> > metric is computed from a group that now multiplexes, that group will
> > run for more of the time as there are fewer groups to multiplex with.
> > In this example we've gone from 3 groups down to 2, 8 events down to
> > 6. An improvement would be to realize that A,B is in both M1 and M3,
> > so when we print the stat we could combine these values.
>
> ok, I misunderstood and thought you would colaps also M3 to
> have A,B computed via M1 group and with separate D ...
>
> thanks a lot for the explanation, it might be great to have it
> in the comments/changelog or even man page

Thanks Jiri! Arnaldo do you want me to copy the description above into
the commit message of this change and resend?
This patch adds some description to find_evsel_group, this is expanded
by the next patch that adds the two command line flags:
https://lore.kernel.org/lkml/20200520072814.128267-7-irogers@google.com/
When writing the patches it wasn't clear to me how much detail to
include in say the man pages.

Thanks,
Ian

> >
> >  - with --metric-no-merge: no events are shared by metrics M1, M2 and
> > M3 have their events and computation as things currently are. There
> > are 3 groups and 8 events.
> >
> >  - with --metric-no-group: all groups are removed and so the evlist
> > has A,B,C,A,B,A,B,D in it. The matching will now match M1 to A,B,C at
> > the beginning of the list, M2 to the first A,B and M3 to the same A,B
> > and D at the end of the list. We've got no groups and the events have
> > gone from 8 down to 4.
> >
> > It is difficult to reason about which grouping is most accurate. If we
> > have 4 counters (no NMI watchdog) then this example will fit with no
> > multiplexing. The default above should achieve less multiplexing, in
> > the same way merging PMU events currently does - this patch is trying
> > to mirror the --no-merge functionality to a degree. Considering
> > TopDownL1 then we go from metrics that never sum to 100%, to metrics
> > that do in either the default or --metric-no-group cases.
> >
> > I'm not sure what user option is missing with these combinations? The
> > default is trying to strike a compromise and I think user interaction
> > is unnecessary, just as --no-merge doesn't cause interaction. If the
> > existing behavior is wanted using --metric-no-merge will give that.
> > The new default and --metric-no-group are hopefully going to reduce
> > the number of groups and events. I'm somewhat agnostic as to what the
> > flag functionality should be as what I'm working with needs either the
> > default or --metric-no-group, I can use whatever flag is agreed upon.
>
> no other option is needed then
>
> thanks,
> jirka
>

Re: [PATCH 5/7] perf metricgroup: Remove duped metric group events

[Arnaldo Carvalho de Melo]

Em Thu, May 21, 2020 at 10:26:02AM -0700, Ian Rogers escreveu:
> On Thu, May 21, 2020 at 3:54 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > ok, I misunderstood and thought you would colaps also M3 to
> > have A,B computed via M1 group and with separate D ...
> >
> > thanks a lot for the explanation, it might be great to have it
> > in the comments/changelog or even man page
> 
> Thanks Jiri! Arnaldo do you want me to copy the description above into
> the commit message of this change and resend?

Send as a follow on patch, please.

- Arnaldo

> This patch adds some description to find_evsel_group, this is expanded
> by the next patch that adds the two command line flags:
> https://lore.kernel.org/lkml/20200520072814.128267-7-irogers@google.com/
> When writing the patches it wasn't clear to me how much detail to
> include in say the man pages.
> 
> Thanks,
> Ian