bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/7] Share events between metrics
@ 2020-05-20 18:20 Ian Rogers
  2020-05-20 18:20 ` [PATCH v2 1/7] perf metricgroup: Always place duration_time last Ian Rogers
                   ` (8 more replies)
  0 siblings, 9 replies; 17+ messages in thread
From: Ian Rogers @ 2020-05-20 18:20 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, Andrii Nakryiko, Kajol Jain, Andi Kleen, John Garry,
	Jin Yao, Kan Liang, Cong Wang, Kim Phillips, Paul Clarke,
	Srikar Dronamraju, linux-kernel
  Cc: netdev, bpf, linux-perf-users, Vince Weaver, Stephane Eranian,
	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.

v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a
missing comma with metric lists (reported-by Jiri Olsa
<jolsa@redhat.com>) and adds early returns to metricgroup__add_metric
(suggested-by Jiri Olsa).

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: Always place duration_time last
  perf metricgroup: Use early return in add_metric
  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          | 239 ++++++++++++++++++-------
 tools/perf/util/metricgroup.h          |   6 +-
 tools/perf/util/stat.h                 |   2 +
 5 files changed, 207 insertions(+), 70 deletions(-)

-- 
2.26.2.761.g0e0b3e54be-goog


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/7] perf metricgroup: Always place duration_time last
  2020-05-20 18:20 [PATCH v2 0/7] Share events between metrics Ian Rogers
@ 2020-05-20 18:20 ` Ian Rogers
  2020-05-20 18:20 ` [PATCH v2 2/7] perf metricgroup: Use early return in add_metric Ian Rogers
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Ian Rogers @ 2020-05-20 18:20 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, Andrii Nakryiko, Kajol Jain, Andi Kleen, John Garry,
	Jin Yao, Kan Liang, Cong Wang, Kim Phillips, Paul Clarke,
	Srikar Dronamraju, linux-kernel
  Cc: netdev, bpf, linux-perf-users, Vince Weaver, Stephane Eranian,
	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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/7] perf metricgroup: Use early return in add_metric
  2020-05-20 18:20 [PATCH v2 0/7] Share events between metrics Ian Rogers
  2020-05-20 18:20 ` [PATCH v2 1/7] perf metricgroup: Always place duration_time last Ian Rogers
@ 2020-05-20 18:20 ` Ian Rogers
  2020-05-20 18:20 ` [PATCH v2 3/7] perf metricgroup: Delay events string creation Ian Rogers
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Ian Rogers @ 2020-05-20 18:20 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, Andrii Nakryiko, Kajol Jain, Andi Kleen, John Garry,
	Jin Yao, Kan Liang, Cong Wang, Kim Phillips, Paul Clarke,
	Srikar Dronamraju, linux-kernel
  Cc: netdev, bpf, linux-perf-users, Vince Weaver, Stephane Eranian,
	Ian Rogers

Use early return in metricgroup__add_metric and try to make the intent
of the returns more intention revealing.

Suggested-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 7a43ee0a2e40..5c0603ef4c75 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -524,7 +524,8 @@ 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;
-	int i, ret = -EINVAL;
+	int i, ret;
+	bool has_match = false;
 
 	if (!map)
 		return 0;
@@ -532,17 +533,23 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 	for (i = 0; ; i++) {
 		pe = &map->table[i];
 
-		if (!pe->name && !pe->metric_group && !pe->metric_name)
+		if (!pe->name && !pe->metric_group && !pe->metric_name) {
+			/* End of pmu events. */
+			if (!has_match)
+				return -EINVAL;
 			break;
+		}
 		if (!pe->metric_expr)
 			continue;
 		if (match_metric(pe->metric_group, metric) ||
 		    match_metric(pe->metric_name, metric)) {
-
+			has_match = true;
 			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);
+				if (ret)
+					return ret;
 			} else {
 				int j, count;
 
@@ -553,14 +560,15 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 				 * those events to group_list.
 				 */
 
-				for (j = 0; j < count; j++)
+				for (j = 0; j < count; j++) {
 					ret = __metricgroup__add_metric(events, group_list, pe, j);
+					if (ret)
+						return ret;
+				}
 			}
-			if (ret == -ENOMEM)
-				break;
 		}
 	}
-	return ret;
+	return 0;
 }
 
 static int metricgroup__add_metric_list(const char *list, struct strbuf *events,
-- 
2.26.2.761.g0e0b3e54be-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 3/7] perf metricgroup: Delay events string creation
  2020-05-20 18:20 [PATCH v2 0/7] Share events between metrics Ian Rogers
  2020-05-20 18:20 ` [PATCH v2 1/7] perf metricgroup: Always place duration_time last Ian Rogers
  2020-05-20 18:20 ` [PATCH v2 2/7] perf metricgroup: Use early return in add_metric Ian Rogers
@ 2020-05-20 18:20 ` Ian Rogers
  2020-05-20 18:20 ` [PATCH v2 4/7] perf metricgroup: Order event groups by size Ian Rogers
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Ian Rogers @ 2020-05-20 18:20 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, Andrii Nakryiko, Kajol Jain, Andi Kleen, John Garry,
	Jin Yao, Kan Liang, Cong Wang, Kim Phillips, Paul Clarke,
	Srikar Dronamraju, linux-kernel
  Cc: netdev, bpf, linux-perf-users, Vince Weaver, Stephane Eranian,
	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 | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 5c0603ef4c75..dca433520b92 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;
 	bool has_match = false;
 
@@ -547,7 +542,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);
 				if (ret)
 					return ret;
 			} else {
@@ -561,13 +557,26 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 				 */
 
 				for (j = 0; j < count; j++) {
-					ret = __metricgroup__add_metric(events, group_list, pe, j);
+					ret = __metricgroup__add_metric(
+						group_list, pe, j);
 					if (ret)
 						return 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 0;
 }
 
-- 
2.26.2.761.g0e0b3e54be-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 4/7] perf metricgroup: Order event groups by size
  2020-05-20 18:20 [PATCH v2 0/7] Share events between metrics Ian Rogers
                   ` (2 preceding siblings ...)
  2020-05-20 18:20 ` [PATCH v2 3/7] perf metricgroup: Delay events string creation Ian Rogers
@ 2020-05-20 18:20 ` Ian Rogers
  2020-05-20 18:20 ` [PATCH v2 5/7] perf metricgroup: Remove duped metric group events Ian Rogers
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Ian Rogers @ 2020-05-20 18:20 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, Andrii Nakryiko, Kajol Jain, Andi Kleen, John Garry,
	Jin Yao, Kan Liang, Cong Wang, Kim Phillips, Paul Clarke,
	Srikar Dronamraju, linux-kernel
  Cc: netdev, bpf, linux-perf-users, Vince Weaver, Stephane Eranian,
	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 dca433520b92..bac63524d12f 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 5/7] perf metricgroup: Remove duped metric group events
  2020-05-20 18:20 [PATCH v2 0/7] Share events between metrics Ian Rogers
                   ` (3 preceding siblings ...)
  2020-05-20 18:20 ` [PATCH v2 4/7] perf metricgroup: Order event groups by size Ian Rogers
@ 2020-05-20 18:20 ` Ian Rogers
  2020-05-20 18:20 ` [PATCH v2 6/7] perf metricgroup: Add options to not group or merge Ian Rogers
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Ian Rogers @ 2020-05-20 18:20 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, Andrii Nakryiko, Kajol Jain, Andi Kleen, John Garry,
	Jin Yao, Kan Liang, Cong Wang, Kim Phillips, Paul Clarke,
	Srikar Dronamraju, linux-kernel
  Cc: netdev, bpf, linux-perf-users, Vince Weaver, Stephane Eranian,
	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 bac63524d12f..dba19de2f9ea 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 (i != idnum) {
+	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 (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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 6/7] perf metricgroup: Add options to not group or merge
  2020-05-20 18:20 [PATCH v2 0/7] Share events between metrics Ian Rogers
                   ` (4 preceding siblings ...)
  2020-05-20 18:20 ` [PATCH v2 5/7] perf metricgroup: Remove duped metric group events Ian Rogers
@ 2020-05-20 18:20 ` Ian Rogers
  2020-05-20 18:20 ` [PATCH v2 7/7] perf metricgroup: Remove unnecessary ',' from events Ian Rogers
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Ian Rogers @ 2020-05-20 18:20 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, Andrii Nakryiko, Kajol Jain, Andi Kleen, John Garry,
	Jin Yao, Kan Liang, Cong Wang, Kim Phillips, Paul Clarke,
	Srikar Dronamraju, linux-kernel
  Cc: netdev, bpf, linux-perf-users, Vince Weaver, Stephane Eranian,
	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 c43ba6080691..25aafbc13d17 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -904,7 +904,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[] = {
@@ -982,6 +985,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,
@@ -1506,6 +1513,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 dba19de2f9ea..50d22f6b60c0 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);
@@ -590,7 +618,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);
 				if (ret)
 					return ret;
 			} else {
@@ -605,7 +635,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)
 						return ret;
 				}
@@ -627,7 +658,8 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 	return 0;
 }
 
-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;
@@ -642,7 +674,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);
@@ -669,8 +702,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;
@@ -680,7 +715,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);
@@ -691,8 +727,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 a5604a20bdca..5322339d889c 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -111,6 +111,8 @@ struct perf_stat_config {
 	bool			 all_user;
 	bool			 percore_show_thread;
 	bool			 summary;
+	bool			 metric_no_group;
+	bool			 metric_no_merge;
 	FILE			*output;
 	unsigned int		 interval;
 	unsigned int		 timeout;
-- 
2.26.2.761.g0e0b3e54be-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 7/7] perf metricgroup: Remove unnecessary ',' from events
  2020-05-20 18:20 [PATCH v2 0/7] Share events between metrics Ian Rogers
                   ` (5 preceding siblings ...)
  2020-05-20 18:20 ` [PATCH v2 6/7] perf metricgroup: Add options to not group or merge Ian Rogers
@ 2020-05-20 18:20 ` Ian Rogers
  2020-05-21 11:43 ` [PATCH v2 0/7] Share events between metrics Jiri Olsa
  2020-05-22  9:25 ` kajoljain
  8 siblings, 0 replies; 17+ messages in thread
From: Ian Rogers @ 2020-05-20 18:20 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Song Liu, Andrii Nakryiko, Kajol Jain, Andi Kleen, John Garry,
	Jin Yao, Kan Liang, Cong Wang, Kim Phillips, Paul Clarke,
	Srikar Dronamraju, linux-kernel
  Cc: netdev, bpf, linux-perf-users, Vince Weaver, Stephane Eranian,
	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 | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 50d22f6b60c0..f787161e54ba 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)
-- 
2.26.2.761.g0e0b3e54be-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/7] Share events between metrics
  2020-05-20 18:20 [PATCH v2 0/7] Share events between metrics Ian Rogers
                   ` (6 preceding siblings ...)
  2020-05-20 18:20 ` [PATCH v2 7/7] perf metricgroup: Remove unnecessary ',' from events Ian Rogers
@ 2020-05-21 11:43 ` Jiri Olsa
  2020-05-21 17:22   ` Arnaldo Carvalho de Melo
  2020-05-22  9:25 ` kajoljain
  8 siblings, 1 reply; 17+ messages in thread
From: Jiri Olsa @ 2020-05-21 11:43 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim, Song Liu,
	Andrii Nakryiko, Kajol Jain, Andi Kleen, John Garry, Jin Yao,
	Kan Liang, Cong Wang, Kim Phillips, Paul Clarke,
	Srikar Dronamraju, linux-kernel, netdev, bpf, linux-perf-users,
	Vince Weaver, Stephane Eranian

On Wed, May 20, 2020 at 11:20:04AM -0700, Ian Rogers wrote:

SNIP

> 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.
> 
> v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a
> missing comma with metric lists (reported-by Jiri Olsa
> <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric
> (suggested-by Jiri Olsa).

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

thanks,
jirka

> 
> 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: Always place duration_time last
>   perf metricgroup: Use early return in add_metric
>   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          | 239 ++++++++++++++++++-------
>  tools/perf/util/metricgroup.h          |   6 +-
>  tools/perf/util/stat.h                 |   2 +
>  5 files changed, 207 insertions(+), 70 deletions(-)
> 
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/7] Share events between metrics
  2020-05-21 11:43 ` [PATCH v2 0/7] Share events between metrics Jiri Olsa
@ 2020-05-21 17:22   ` Arnaldo Carvalho de Melo
  2020-05-22 10:13     ` Jiri Olsa
  0 siblings, 1 reply; 17+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-21 17:22 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Namhyung Kim, Song Liu, Andrii Nakryiko,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang,
	Cong Wang, Kim Phillips, Paul Clarke, Srikar Dronamraju,
	linux-kernel, netdev, bpf, linux-perf-users, Vince Weaver,
	Stephane Eranian

Em Thu, May 21, 2020 at 01:43:25PM +0200, Jiri Olsa escreveu:
> On Wed, May 20, 2020 at 11:20:04AM -0700, Ian Rogers wrote:
> 
> SNIP
> 
> > 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.
> > 
> > v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a
> > missing comma with metric lists (reported-by Jiri Olsa
> > <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric
> > (suggested-by Jiri Olsa).
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>

Applied and pushed to tmp.perf/core, will move to perf/core as soon as
testing finishes,

- Arnaldo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/7] Share events between metrics
  2020-05-20 18:20 [PATCH v2 0/7] Share events between metrics Ian Rogers
                   ` (7 preceding siblings ...)
  2020-05-21 11:43 ` [PATCH v2 0/7] Share events between metrics Jiri Olsa
@ 2020-05-22  9:25 ` kajoljain
  2020-05-22 14:31   ` Arnaldo Carvalho de Melo
  8 siblings, 1 reply; 17+ messages in thread
From: kajoljain @ 2020-05-22  9:25 UTC (permalink / raw)
  To: Ian Rogers, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Song Liu, Andrii Nakryiko, Andi Kleen,
	John Garry, Jin Yao, Kan Liang, Cong Wang, Kim Phillips,
	Paul Clarke, Srikar Dronamraju, linux-kernel
  Cc: netdev, bpf, linux-perf-users, Vince Weaver, Stephane Eranian



On 5/20/20 11:50 PM, 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):
> 
> 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.
> 
> v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a
> missing comma with metric lists (reported-by Jiri Olsa
> <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric
> (suggested-by Jiri Olsa).
> 
> 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: Always place duration_time last
>   perf metricgroup: Use early return in add_metric
>   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
> 

Reviewd-By: Kajol Jain <kjain@linux.ibm.com>
Tested-By: Kajol Jain <kjain@linux.ibm.com> ( Tested it to see behavior with some metric groups in both x86 and Power machine)

Thanks,
Kajol Jain

>  tools/perf/Documentation/perf-stat.txt |  19 ++
>  tools/perf/builtin-stat.c              |  11 +-
>  tools/perf/util/metricgroup.c          | 239 ++++++++++++++++++-------
>  tools/perf/util/metricgroup.h          |   6 +-
>  tools/perf/util/stat.h                 |   2 +
>  5 files changed, 207 insertions(+), 70 deletions(-)
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/7] Share events between metrics
  2020-05-21 17:22   ` Arnaldo Carvalho de Melo
@ 2020-05-22 10:13     ` Jiri Olsa
  2020-05-22 14:49       ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 17+ messages in thread
From: Jiri Olsa @ 2020-05-22 10:13 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Namhyung Kim, Song Liu, Andrii Nakryiko, Kajol Jain, Andi Kleen,
	John Garry, Jin Yao, Kan Liang, Cong Wang, Kim Phillips,
	Paul Clarke, Srikar Dronamraju, linux-kernel, netdev, bpf,
	linux-perf-users, Vince Weaver, Stephane Eranian

On Thu, May 21, 2020 at 02:22:35PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, May 21, 2020 at 01:43:25PM +0200, Jiri Olsa escreveu:
> > On Wed, May 20, 2020 at 11:20:04AM -0700, Ian Rogers wrote:
> > 
> > SNIP
> > 
> > > 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.
> > > 
> > > v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a
> > > missing comma with metric lists (reported-by Jiri Olsa
> > > <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric
> > > (suggested-by Jiri Olsa).
> > 
> > Acked-by: Jiri Olsa <jolsa@redhat.com>
> 
> Applied and pushed to tmp.perf/core, will move to perf/core as soon as
> testing finishes,

I checked tmp.perf/core and I'm getting segfault for 'perf test expr'

	 7: Simple expression parser                              :
	Program received signal SIGSEGV, Segmentation fault.
	0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131
	131             for (prev_ptr = &map->buckets[hash], cur = *prev_ptr;
	(gdb) bt
	#0  0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131
	#1  0x000000000067853a in hashmap__insert (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, strategy=HASHMAP_SET, old_key=0x7fffffffc718, 
	    old_value=0x7fffffffc710) at hashmap.c:160
	#2  0x00000000005d3209 in hashmap__set (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, old_key=0x7fffffffc718, old_value=0x7fffffffc710)
	    at /home/jolsa/kernel/linux-perf/tools/perf/util/hashmap.h:107
	#3  0x00000000005d3386 in expr__add_id (ctx=0x7fffffffd0c0, name=0xc83b30 "FOO", val=0) at util/expr.c:45
	#4  0x00000000005d27ee in expr_parse (final_val=0x0, ctx=0x7fffffffd0c0, scanner=0xc87990) at util/expr.y:63
	#5  0x00000000005d35b7 in __expr__parse (val=0x0, ctx=0x7fffffffd0c0, expr=0x75a84b "FOO + BAR + BAZ + BOZO", start=259, runtime=1) at util/expr.c:102
	#6  0x00000000005d36c6 in expr__find_other (expr=0x75a84b "FOO + BAR + BAZ + BOZO", one=0x75a791 "FOO", ctx=0x7fffffffd0c0, runtime=1) at util/expr.c:121
	#7  0x00000000004e3aaf in test__expr (t=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/expr.c:55
	#8  0x00000000004b5651 in run_test (test=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/builtin-test.c:393
	#9  0x00000000004b5787 in test_and_print (t=0xa7bd40 <generic_tests+384>, force_skip=false, subtest=-1) at tests/builtin-test.c:423
	#10 0x00000000004b61c4 in __cmd_test (argc=1, argv=0x7fffffffd7f0, skiplist=0x0) at tests/builtin-test.c:628
	#11 0x00000000004b6911 in cmd_test (argc=1, argv=0x7fffffffd7f0) at tests/builtin-test.c:772
	#12 0x00000000004e977b in run_builtin (p=0xa7eee8 <commands+552>, argc=3, argv=0x7fffffffd7f0) at perf.c:312
	#13 0x00000000004e99e8 in handle_internal_command (argc=3, argv=0x7fffffffd7f0) at perf.c:364
	#14 0x00000000004e9b2f in run_argv (argcp=0x7fffffffd64c, argv=0x7fffffffd640) at perf.c:408
	#15 0x00000000004e9efb in main (argc=3, argv=0x7fffffffd7f0) at perf.c:538

attached patch fixes it for me, but I'm not sure this
should be necessary

jirka


---
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 1cb02ca2b15f..21693fe516c1 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -52,6 +52,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 	TEST_ASSERT_VAL("missing operand", ret == -1);
 
 	expr__ctx_clear(&ctx);
+	expr__ctx_init(&ctx);
 	TEST_ASSERT_VAL("find other",
 			expr__find_other("FOO + BAR + BAZ + BOZO", "FOO",
 					 &ctx, 1) == 0);
@@ -64,6 +65,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 						    (void **)&val_ptr));
 
 	expr__ctx_clear(&ctx);
+	expr__ctx_init(&ctx);
 	TEST_ASSERT_VAL("find other",
 			expr__find_other("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@",
 					 NULL, &ctx, 3) == 0);



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/7] Share events between metrics
  2020-05-22  9:25 ` kajoljain
@ 2020-05-22 14:31   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 17+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-22 14:31 UTC (permalink / raw)
  To: kajoljain
  Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, Song Liu,
	Andrii Nakryiko, Andi Kleen, John Garry, Jin Yao, Kan Liang,
	Cong Wang, Kim Phillips, Paul Clarke, Srikar Dronamraju,
	linux-kernel, netdev, bpf, linux-perf-users, Vince Weaver,
	Stephane Eranian

Em Fri, May 22, 2020 at 02:55:46PM +0530, kajoljain escreveu:
> On 5/20/20 11:50 PM, 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.

<SNIP>
 
> > Ian Rogers (7):
> >   perf metricgroup: Always place duration_time last
> >   perf metricgroup: Use early return in add_metric
> >   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
 
> Reviewd-By: Kajol Jain <kjain@linux.ibm.com>
> Tested-By: Kajol Jain <kjain@linux.ibm.com> ( Tested it to see behavior with some metric groups in both x86 and Power machine)

Thanks, added to the patches,

- Arnaldo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/7] Share events between metrics
  2020-05-22 10:13     ` Jiri Olsa
@ 2020-05-22 14:49       ` Arnaldo Carvalho de Melo
       [not found]         ` <CAP-5=fUaaNpi3RZd9-Q-uCaudop0tU5NN8HFek5e2XLoBZqt6w@mail.gmail.com>
  0 siblings, 1 reply; 17+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-22 14:49 UTC (permalink / raw)
  To: Jiri Olsa, Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Namhyung Kim, Song Liu, Andrii Nakryiko, Kajol Jain, Andi Kleen,
	John Garry, Jin Yao, Kan Liang, Cong Wang, Kim Phillips,
	Paul Clarke, Srikar Dronamraju, linux-kernel, netdev, bpf,
	linux-perf-users, Vince Weaver, Stephane Eranian

Em Fri, May 22, 2020 at 12:13:11PM +0200, Jiri Olsa escreveu:
> On Thu, May 21, 2020 at 02:22:35PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, May 21, 2020 at 01:43:25PM +0200, Jiri Olsa escreveu:
> > > On Wed, May 20, 2020 at 11:20:04AM -0700, Ian Rogers wrote:
> > > 
> > > SNIP
> > > 
> > > > 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.
> > > > 
> > > > v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a
> > > > missing comma with metric lists (reported-by Jiri Olsa
> > > > <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric
> > > > (suggested-by Jiri Olsa).
> > > 
> > > Acked-by: Jiri Olsa <jolsa@redhat.com>
> > 
> > Applied and pushed to tmp.perf/core, will move to perf/core as soon as
> > testing finishes,
> 
> I checked tmp.perf/core and I'm getting segfault for 'perf test expr'

Right, reproduced here and...
 
> 	 7: Simple expression parser                              :
> 	Program received signal SIGSEGV, Segmentation fault.
> 	0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131
> 	131             for (prev_ptr = &map->buckets[hash], cur = *prev_ptr;
> 	(gdb) bt
> 	#0  0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131
> 	#1  0x000000000067853a in hashmap__insert (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, strategy=HASHMAP_SET, old_key=0x7fffffffc718, 
> 	    old_value=0x7fffffffc710) at hashmap.c:160
> 	#2  0x00000000005d3209 in hashmap__set (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, old_key=0x7fffffffc718, old_value=0x7fffffffc710)
> 	    at /home/jolsa/kernel/linux-perf/tools/perf/util/hashmap.h:107
> 	#3  0x00000000005d3386 in expr__add_id (ctx=0x7fffffffd0c0, name=0xc83b30 "FOO", val=0) at util/expr.c:45
> 	#4  0x00000000005d27ee in expr_parse (final_val=0x0, ctx=0x7fffffffd0c0, scanner=0xc87990) at util/expr.y:63
> 	#5  0x00000000005d35b7 in __expr__parse (val=0x0, ctx=0x7fffffffd0c0, expr=0x75a84b "FOO + BAR + BAZ + BOZO", start=259, runtime=1) at util/expr.c:102
> 	#6  0x00000000005d36c6 in expr__find_other (expr=0x75a84b "FOO + BAR + BAZ + BOZO", one=0x75a791 "FOO", ctx=0x7fffffffd0c0, runtime=1) at util/expr.c:121
> 	#7  0x00000000004e3aaf in test__expr (t=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/expr.c:55
> 	#8  0x00000000004b5651 in run_test (test=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/builtin-test.c:393
> 	#9  0x00000000004b5787 in test_and_print (t=0xa7bd40 <generic_tests+384>, force_skip=false, subtest=-1) at tests/builtin-test.c:423
> 	#10 0x00000000004b61c4 in __cmd_test (argc=1, argv=0x7fffffffd7f0, skiplist=0x0) at tests/builtin-test.c:628
> 	#11 0x00000000004b6911 in cmd_test (argc=1, argv=0x7fffffffd7f0) at tests/builtin-test.c:772
> 	#12 0x00000000004e977b in run_builtin (p=0xa7eee8 <commands+552>, argc=3, argv=0x7fffffffd7f0) at perf.c:312
> 	#13 0x00000000004e99e8 in handle_internal_command (argc=3, argv=0x7fffffffd7f0) at perf.c:364
> 	#14 0x00000000004e9b2f in run_argv (argcp=0x7fffffffd64c, argv=0x7fffffffd640) at perf.c:408
> 	#15 0x00000000004e9efb in main (argc=3, argv=0x7fffffffd7f0) at perf.c:538
> 
> attached patch fixes it for me, but I'm not sure this
> should be necessary

... applying the patch below makes the segfault go away. Ian, Ack? I can
fold it into the patch introducing the problem.

- Arnaldo
 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index 1cb02ca2b15f..21693fe516c1 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -52,6 +52,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
>  	TEST_ASSERT_VAL("missing operand", ret == -1);
>  
>  	expr__ctx_clear(&ctx);
> +	expr__ctx_init(&ctx);
>  	TEST_ASSERT_VAL("find other",
>  			expr__find_other("FOO + BAR + BAZ + BOZO", "FOO",
>  					 &ctx, 1) == 0);
> @@ -64,6 +65,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
>  						    (void **)&val_ptr));
>  
>  	expr__ctx_clear(&ctx);
> +	expr__ctx_init(&ctx);
>  	TEST_ASSERT_VAL("find other",
>  			expr__find_other("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@",
>  					 NULL, &ctx, 3) == 0);
> 
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/7] Share events between metrics
       [not found]         ` <CAP-5=fUaaNpi3RZd9-Q-uCaudop0tU5NN8HFek5e2XLoBZqt6w@mail.gmail.com>
@ 2020-05-22 17:56           ` Ian Rogers
  2020-05-23 22:19             ` Jiri Olsa
  0 siblings, 1 reply; 17+ messages in thread
From: Ian Rogers @ 2020-05-22 17:56 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Namhyung Kim, Song Liu, Andrii Nakryiko,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang,
	Cong Wang, Kim Phillips, Paul Clarke, Srikar Dronamraju, LKML,
	Networking, bpf, linux-perf-users, Vince Weaver,
	Stephane Eranian

On Fri, May 22, 2020 at 7:59 AM Ian Rogers <irogers@google.com> wrote:
>
>
>
> On Fri, May 22, 2020, 7:49 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>>
>> Em Fri, May 22, 2020 at 12:13:11PM +0200, Jiri Olsa escreveu:
>> > On Thu, May 21, 2020 at 02:22:35PM -0300, Arnaldo Carvalho de Melo wrote:
>> > > Em Thu, May 21, 2020 at 01:43:25PM +0200, Jiri Olsa escreveu:
>> > > > On Wed, May 20, 2020 at 11:20:04AM -0700, Ian Rogers wrote:
>> > > >
>> > > > SNIP
>> > > >
>> > > > > 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.
>> > > > >
>> > > > > v2. rebases on kernel/git/acme/linux.git branch tmp.perf/core, fixes a
>> > > > > missing comma with metric lists (reported-by Jiri Olsa
>> > > > > <jolsa@redhat.com>) and adds early returns to metricgroup__add_metric
>> > > > > (suggested-by Jiri Olsa).
>> > > >
>> > > > Acked-by: Jiri Olsa <jolsa@redhat.com>
>> > >
>> > > Applied and pushed to tmp.perf/core, will move to perf/core as soon as
>> > > testing finishes,
>> >
>> > I checked tmp.perf/core and I'm getting segfault for 'perf test expr'
>>
>> Right, reproduced here and...
>>
>> >        7: Simple expression parser                              :
>> >       Program received signal SIGSEGV, Segmentation fault.
>> >       0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131
>> >       131             for (prev_ptr = &map->buckets[hash], cur = *prev_ptr;
>> >       (gdb) bt
>> >       #0  0x000000000067841e in hashmap_find_entry (map=0x7fffffffd0c0, key=0xc83b30, hash=9893851511679796638, pprev=0x0, entry=0x7fffffffc658) at hashmap.c:131
>> >       #1  0x000000000067853a in hashmap__insert (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, strategy=HASHMAP_SET, old_key=0x7fffffffc718,
>> >           old_value=0x7fffffffc710) at hashmap.c:160
>> >       #2  0x00000000005d3209 in hashmap__set (map=0x7fffffffd0c0, key=0xc83b30, value=0x0, old_key=0x7fffffffc718, old_value=0x7fffffffc710)
>> >           at /home/jolsa/kernel/linux-perf/tools/perf/util/hashmap.h:107
>> >       #3  0x00000000005d3386 in expr__add_id (ctx=0x7fffffffd0c0, name=0xc83b30 "FOO", val=0) at util/expr.c:45
>> >       #4  0x00000000005d27ee in expr_parse (final_val=0x0, ctx=0x7fffffffd0c0, scanner=0xc87990) at util/expr.y:63
>> >       #5  0x00000000005d35b7 in __expr__parse (val=0x0, ctx=0x7fffffffd0c0, expr=0x75a84b "FOO + BAR + BAZ + BOZO", start=259, runtime=1) at util/expr.c:102
>> >       #6  0x00000000005d36c6 in expr__find_other (expr=0x75a84b "FOO + BAR + BAZ + BOZO", one=0x75a791 "FOO", ctx=0x7fffffffd0c0, runtime=1) at util/expr.c:121
>> >       #7  0x00000000004e3aaf in test__expr (t=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/expr.c:55
>> >       #8  0x00000000004b5651 in run_test (test=0xa7bd40 <generic_tests+384>, subtest=-1) at tests/builtin-test.c:393
>> >       #9  0x00000000004b5787 in test_and_print (t=0xa7bd40 <generic_tests+384>, force_skip=false, subtest=-1) at tests/builtin-test.c:423
>> >       #10 0x00000000004b61c4 in __cmd_test (argc=1, argv=0x7fffffffd7f0, skiplist=0x0) at tests/builtin-test.c:628
>> >       #11 0x00000000004b6911 in cmd_test (argc=1, argv=0x7fffffffd7f0) at tests/builtin-test.c:772
>> >       #12 0x00000000004e977b in run_builtin (p=0xa7eee8 <commands+552>, argc=3, argv=0x7fffffffd7f0) at perf.c:312
>> >       #13 0x00000000004e99e8 in handle_internal_command (argc=3, argv=0x7fffffffd7f0) at perf.c:364
>> >       #14 0x00000000004e9b2f in run_argv (argcp=0x7fffffffd64c, argv=0x7fffffffd640) at perf.c:408
>> >       #15 0x00000000004e9efb in main (argc=3, argv=0x7fffffffd7f0) at perf.c:538
>> >
>> > attached patch fixes it for me, but I'm not sure this
>> > should be necessary
>>
>> ... applying the patch below makes the segfault go away. Ian, Ack? I can
>> fold it into the patch introducing the problem.
>
>
> I suspect this patch is a memory leak. The underlying issue is likely the outstanding hashmap_clear fix in libbpf. Let me check.
>
> Thanks,
> Ian

Tested:
$ git checkout -b testing acme/tmp.perf/core
$ make ...
$ perf test 7
7: Simple expression parser                              : FAILED!
$ git cherry-pick 6bca339175bf
[acme-perf-expr-testing 4614bd252003] libbpf: Fix memory leak and
possible double-free in hashmap__c
lear
Author: Andrii Nakryiko <andriin@fb.com>
Date: Tue Apr 28 18:21:04 2020 -0700
1 file changed, 7 insertions(+)
$ make ...
$ perf test 7
7: Simple expression parser                              : Ok

I'd prefer we took the libbpf fix as initializing over the top of the
hashmap will leak. This fix is in the tools/perf/util/hashmap.c.

Thanks,
Ian

>> - Arnaldo
>>
>> > jirka
>> >
>> >
>> > ---
>> > diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
>> > index 1cb02ca2b15f..21693fe516c1 100644
>> > --- a/tools/perf/tests/expr.c
>> > +++ b/tools/perf/tests/expr.c
>> > @@ -52,6 +52,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
>> >       TEST_ASSERT_VAL("missing operand", ret == -1);
>> >
>> >       expr__ctx_clear(&ctx);
>> > +     expr__ctx_init(&ctx);
>> >       TEST_ASSERT_VAL("find other",
>> >                       expr__find_other("FOO + BAR + BAZ + BOZO", "FOO",
>> >                                        &ctx, 1) == 0);
>> > @@ -64,6 +65,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
>> >                                                   (void **)&val_ptr));
>> >
>> >       expr__ctx_clear(&ctx);
>> > +     expr__ctx_init(&ctx);
>> >       TEST_ASSERT_VAL("find other",
>> >                       expr__find_other("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@",
>> >                                        NULL, &ctx, 3) == 0);
>> >
>> >
>>
>> --
>>
>> - Arnaldo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/7] Share events between metrics
  2020-05-22 17:56           ` Ian Rogers
@ 2020-05-23 22:19             ` Jiri Olsa
  2020-05-25 13:34               ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 17+ messages in thread
From: Jiri Olsa @ 2020-05-23 22:19 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Namhyung Kim, Song Liu,
	Andrii Nakryiko, Kajol Jain, Andi Kleen, John Garry, Jin Yao,
	Kan Liang, Cong Wang, Kim Phillips, Paul Clarke,
	Srikar Dronamraju, LKML, Networking, bpf, linux-perf-users,
	Vince Weaver, Stephane Eranian

On Fri, May 22, 2020 at 10:56:59AM -0700, Ian Rogers wrote:

SNIP

> >> >       #11 0x00000000004b6911 in cmd_test (argc=1, argv=0x7fffffffd7f0) at tests/builtin-test.c:772
> >> >       #12 0x00000000004e977b in run_builtin (p=0xa7eee8 <commands+552>, argc=3, argv=0x7fffffffd7f0) at perf.c:312
> >> >       #13 0x00000000004e99e8 in handle_internal_command (argc=3, argv=0x7fffffffd7f0) at perf.c:364
> >> >       #14 0x00000000004e9b2f in run_argv (argcp=0x7fffffffd64c, argv=0x7fffffffd640) at perf.c:408
> >> >       #15 0x00000000004e9efb in main (argc=3, argv=0x7fffffffd7f0) at perf.c:538
> >> >
> >> > attached patch fixes it for me, but I'm not sure this
> >> > should be necessary
> >>
> >> ... applying the patch below makes the segfault go away. Ian, Ack? I can
> >> fold it into the patch introducing the problem.
> >
> >
> > I suspect this patch is a memory leak. The underlying issue is likely the outstanding hashmap_clear fix in libbpf. Let me check.
> >
> > Thanks,
> > Ian
> 
> Tested:
> $ git checkout -b testing acme/tmp.perf/core
> $ make ...
> $ perf test 7
> 7: Simple expression parser                              : FAILED!
> $ git cherry-pick 6bca339175bf
> [acme-perf-expr-testing 4614bd252003] libbpf: Fix memory leak and
> possible double-free in hashmap__c

yep, it fixes the issue for me, but I see that under different commit number:

  229bf8bf4d91 libbpf: Fix memory leak and possible double-free in hashmap__clear

jirka

> lear
> Author: Andrii Nakryiko <andriin@fb.com>
> Date: Tue Apr 28 18:21:04 2020 -0700
> 1 file changed, 7 insertions(+)
> $ make ...
> $ perf test 7
> 7: Simple expression parser                              : Ok
> 
> I'd prefer we took the libbpf fix as initializing over the top of the
> hashmap will leak. This fix is in the tools/perf/util/hashmap.c.
> 
> Thanks,
> Ian
> 
> >> - Arnaldo
> >>
> >> > jirka
> >> >
> >> >
> >> > ---

SNIP


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/7] Share events between metrics
  2020-05-23 22:19             ` Jiri Olsa
@ 2020-05-25 13:34               ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 17+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-25 13:34 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Namhyung Kim, Song Liu, Andrii Nakryiko,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang,
	Cong Wang, Kim Phillips, Paul Clarke, Srikar Dronamraju, LKML,
	Networking, bpf, linux-perf-users, Vince Weaver,
	Stephane Eranian

Em Sun, May 24, 2020 at 12:19:36AM +0200, Jiri Olsa escreveu:
> On Fri, May 22, 2020 at 10:56:59AM -0700, Ian Rogers wrote:
> 
> SNIP
> 
> > >> >       #11 0x00000000004b6911 in cmd_test (argc=1, argv=0x7fffffffd7f0) at tests/builtin-test.c:772
> > >> >       #12 0x00000000004e977b in run_builtin (p=0xa7eee8 <commands+552>, argc=3, argv=0x7fffffffd7f0) at perf.c:312
> > >> >       #13 0x00000000004e99e8 in handle_internal_command (argc=3, argv=0x7fffffffd7f0) at perf.c:364
> > >> >       #14 0x00000000004e9b2f in run_argv (argcp=0x7fffffffd64c, argv=0x7fffffffd640) at perf.c:408
> > >> >       #15 0x00000000004e9efb in main (argc=3, argv=0x7fffffffd7f0) at perf.c:538
> > >> >
> > >> > attached patch fixes it for me, but I'm not sure this
> > >> > should be necessary
> > >>
> > >> ... applying the patch below makes the segfault go away. Ian, Ack? I can
> > >> fold it into the patch introducing the problem.
> > >
> > >
> > > I suspect this patch is a memory leak. The underlying issue is likely the outstanding hashmap_clear fix in libbpf. Let me check.
> > >
> > > Thanks,
> > > Ian
> > 
> > Tested:
> > $ git checkout -b testing acme/tmp.perf/core
> > $ make ...
> > $ perf test 7
> > 7: Simple expression parser                              : FAILED!
> > $ git cherry-pick 6bca339175bf
> > [acme-perf-expr-testing 4614bd252003] libbpf: Fix memory leak and
> > possible double-free in hashmap__c
> 
> yep, it fixes the issue for me, but I see that under different commit number:
> 
>   229bf8bf4d91 libbpf: Fix memory leak and possible double-free in hashmap__clear

Ok, so I'll just add a note mentioning that this test will pass as soon
as the libbpf fix gets upstream.

Thanks,

- Arnaldo

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2020-05-25 13:34 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 18:20 [PATCH v2 0/7] Share events between metrics Ian Rogers
2020-05-20 18:20 ` [PATCH v2 1/7] perf metricgroup: Always place duration_time last Ian Rogers
2020-05-20 18:20 ` [PATCH v2 2/7] perf metricgroup: Use early return in add_metric Ian Rogers
2020-05-20 18:20 ` [PATCH v2 3/7] perf metricgroup: Delay events string creation Ian Rogers
2020-05-20 18:20 ` [PATCH v2 4/7] perf metricgroup: Order event groups by size Ian Rogers
2020-05-20 18:20 ` [PATCH v2 5/7] perf metricgroup: Remove duped metric group events Ian Rogers
2020-05-20 18:20 ` [PATCH v2 6/7] perf metricgroup: Add options to not group or merge Ian Rogers
2020-05-20 18:20 ` [PATCH v2 7/7] perf metricgroup: Remove unnecessary ',' from events Ian Rogers
2020-05-21 11:43 ` [PATCH v2 0/7] Share events between metrics Jiri Olsa
2020-05-21 17:22   ` Arnaldo Carvalho de Melo
2020-05-22 10:13     ` Jiri Olsa
2020-05-22 14:49       ` Arnaldo Carvalho de Melo
     [not found]         ` <CAP-5=fUaaNpi3RZd9-Q-uCaudop0tU5NN8HFek5e2XLoBZqt6w@mail.gmail.com>
2020-05-22 17:56           ` Ian Rogers
2020-05-23 22:19             ` Jiri Olsa
2020-05-25 13:34               ` Arnaldo Carvalho de Melo
2020-05-22  9:25 ` kajoljain
2020-05-22 14:31   ` Arnaldo Carvalho de Melo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).