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.
RFC because:
- without this change events within a metric may get scheduled
together, after they may appear as part of a larger group and be
multiplexed at different times, lowering accuracy - however, less
multiplexing may compensate for this.
- libbpf's hashmap is used, however, libbpf is an optional
requirement for building perf.
- other things I'm not thinking of.
Thanks!
Example on Sandybridge:
$ perf stat -a --metric-no-merge -M TopDownL1_SMT sleep 1
Performance counter stats for 'system wide':
14931177 cpu_clk_unhalted.one_thread_active # 0.47 Backend_Bound_SMT (12.45%)
32314653 int_misc.recovery_cycles_any (16.23%)
555020905 uops_issued.any (18.85%)
1038651176 idq_uops_not_delivered.core (24.95%)
43003170 cpu_clk_unhalted.ref_xclk (25.20%)
1154926272 cpu_clk_unhalted.thread (31.50%)
656873544 uops_retired.retire_slots (31.11%)
16491988 cpu_clk_unhalted.one_thread_active # 0.06 Bad_Speculation_SMT (31.10%)
32064061 int_misc.recovery_cycles_any (31.04%)
648394934 uops_issued.any (31.14%)
42107506 cpu_clk_unhalted.ref_xclk (24.94%)
1124565282 cpu_clk_unhalted.thread (31.14%)
523430886 uops_retired.retire_slots (31.05%)
12328380 cpu_clk_unhalted.one_thread_active # 0.35 Frontend_Bound_SMT (10.08%)
42651836 cpu_clk_unhalted.ref_xclk (10.08%)
1006287722 idq_uops_not_delivered.core (10.08%)
1130593027 cpu_clk_unhalted.thread (10.08%)
14209258 cpu_clk_unhalted.one_thread_active # 0.18 Retiring_SMT (6.39%)
41904474 cpu_clk_unhalted.ref_xclk (6.39%)
522251584 uops_retired.retire_slots (6.39%)
1111257754 cpu_clk_unhalted.thread (6.39%)
12930094 cpu_clk_unhalted.one_thread_active # 2865823806.05 SLOTS_SMT (11.06%)
40975376 cpu_clk_unhalted.ref_xclk (11.06%)
1089204936 cpu_clk_unhalted.thread (11.06%)
1.002165509 seconds time elapsed
$ perf stat -a -M TopDownL1_SMT sleep 1
Performance counter stats for 'system wide':
11893411 cpu_clk_unhalted.one_thread_active # 2715516883.49 SLOTS_SMT
# 0.19 Retiring_SMT
# 0.33 Frontend_Bound_SMT
# 0.04 Bad_Speculation_SMT
# 0.44 Backend_Bound_SMT (71.46%)
28458253 int_misc.recovery_cycles_any (71.44%)
562710994 uops_issued.any (71.42%)
907105260 idq_uops_not_delivered.core (57.12%)
39797715 cpu_clk_unhalted.ref_xclk (57.12%)
1045357060 cpu_clk_unhalted.thread (71.41%)
504809283 uops_retired.retire_slots (71.44%)
1.001939294 seconds time elapsed
Note that without merging the metrics sum to 1.06, but with merging
the sum is 1.
Example on Cascadelake:
$ perf stat -a --metric-no-merge -M TopDownL1_SMT sleep 1
Performance counter stats for 'system wide':
13678949 cpu_clk_unhalted.one_thread_active # 0.59 Backend_Bound_SMT (13.35%)
121286613 int_misc.recovery_cycles_any (18.58%)
4041490966 uops_issued.any (18.81%)
2665605457 idq_uops_not_delivered.core (24.81%)
111757608 cpu_clk_unhalted.ref_xclk (25.03%)
7579026491 cpu_clk_unhalted.thread (31.27%)
3848429110 uops_retired.retire_slots (31.23%)
15554046 cpu_clk_unhalted.one_thread_active # 0.02 Bad_Speculation_SMT (31.19%)
119582342 int_misc.recovery_cycles_any (31.16%)
3813943706 uops_issued.any (31.14%)
113151605 cpu_clk_unhalted.ref_xclk (24.89%)
7621196102 cpu_clk_unhalted.thread (31.12%)
3735690253 uops_retired.retire_slots (31.12%)
13727352 cpu_clk_unhalted.one_thread_active # 0.16 Frontend_Bound_SMT (12.50%)
115441454 cpu_clk_unhalted.ref_xclk (12.50%)
2824946246 idq_uops_not_delivered.core (12.50%)
7817227775 cpu_clk_unhalted.thread (12.50%)
13267908 cpu_clk_unhalted.one_thread_active # 0.21 Retiring_SMT (6.31%)
114015605 cpu_clk_unhalted.ref_xclk (6.31%)
3722498773 uops_retired.retire_slots (6.31%)
7771438396 cpu_clk_unhalted.thread (6.31%)
14948307 cpu_clk_unhalted.one_thread_active # 18085611559.36 SLOTS_SMT (6.30%)
115632797 cpu_clk_unhalted.ref_xclk (6.30%)
8007628156 cpu_clk_unhalted.thread (6.30%)
1.006256703 seconds time elapsed
$ perf stat -a -M TopDownL1_SMT sleep 1
Performance counter stats for 'system wide':
35999534 cpu_clk_unhalted.one_thread_active # 25969550384.66 SLOTS_SMT
# 0.40 Retiring_SMT
# 0.14 Frontend_Bound_SMT
# 0.02 Bad_Speculation_SMT
# 0.44 Backend_Bound_SMT (71.35%)
133499018 int_misc.recovery_cycles_any (71.36%)
10736468874 uops_issued.any (71.40%)
3518076530 idq_uops_not_delivered.core (57.24%)
78296616 cpu_clk_unhalted.ref_xclk (57.25%)
8894997400 cpu_clk_unhalted.thread (71.50%)
10409738753 uops_retired.retire_slots (71.40%)
1.011611791 seconds time elapsed
Note that without merging the metrics sum to 0.98, but with merging
the sum is 1.
v3. is a rebase with following the merging of patches in v2. It also
adds the metric-no-group and metric-no-merge flags.
v2. is the entire patch set based on acme's perf/core tree and includes a
cherry-picks. Patch 13 was sent for review to the bpf maintainers here:
https://lore.kernel.org/lkml/[email protected]/
v1. was based on the perf metrics fixes and test sent here:
https://lore.kernel.org/lkml/[email protected]/
Andrii Nakryiko (1):
libbpf: Fix memory leak and possible double-free in hashmap__clear
Ian Rogers (13):
perf parse-events: expand add PMU error/verbose messages
perf test: improve pmu event metric testing
lib/bpf hashmap: increase portability
perf expr: fix memory leaks in bison
perf evsel: fix 2 memory leaks
perf expr: migrate expr ids table to libbpf's hashmap
perf metricgroup: change evlist_used to a bitmap
perf metricgroup: free metric_events on error
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
tools/lib/bpf/hashmap.c | 7 +
tools/lib/bpf/hashmap.h | 3 +-
tools/perf/Documentation/perf-stat.txt | 19 ++
tools/perf/arch/x86/util/intel-pt.c | 32 +--
tools/perf/builtin-stat.c | 11 +-
tools/perf/tests/builtin-test.c | 5 +
tools/perf/tests/expr.c | 41 ++--
tools/perf/tests/pmu-events.c | 159 +++++++++++++-
tools/perf/tests/pmu.c | 4 +-
tools/perf/tests/tests.h | 2 +
tools/perf/util/evsel.c | 2 +
tools/perf/util/expr.c | 129 +++++++-----
tools/perf/util/expr.h | 22 +-
tools/perf/util/expr.y | 25 +--
tools/perf/util/metricgroup.c | 277 ++++++++++++++++---------
tools/perf/util/metricgroup.h | 6 +-
tools/perf/util/parse-events.c | 29 ++-
tools/perf/util/pmu.c | 33 +--
tools/perf/util/pmu.h | 2 +-
tools/perf/util/stat-shadow.c | 49 +++--
tools/perf/util/stat.h | 2 +
21 files changed, 592 insertions(+), 267 deletions(-)
--
2.26.2.645.ge9eca65c58-goog
Don't include libbpf_internal.h as it is unused and has conflicting
definitions, for example, with tools/perf/util/debug.h.
Fix a non-glibc include path.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/lib/bpf/hashmap.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h
index bae8879cdf58..d5ef212a55ba 100644
--- a/tools/lib/bpf/hashmap.h
+++ b/tools/lib/bpf/hashmap.h
@@ -13,9 +13,8 @@
#ifdef __GLIBC__
#include <bits/wordsize.h>
#else
-#include <bits/reg.h>
+#include <linux/bitops.h>
#endif
-#include "libbpf_internal.h"
static inline size_t hash_bits(size_t h, int bits)
{
--
2.26.2.645.ge9eca65c58-goog
Add a basic floating point number test to expr.
Break pmu-events test into 2 and add a test to verify that all pmu metric
expressions simply parse. Try to parse all metric ids/events, failing if
metrics for the current architecture fail to parse.
Tested on skylakex with the patch set in place. May fail on other
architectures if metrics are invalid.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/tests/builtin-test.c | 5 +
tools/perf/tests/expr.c | 1 +
tools/perf/tests/pmu-events.c | 158 ++++++++++++++++++++++++++++++--
tools/perf/tests/tests.h | 2 +
4 files changed, 160 insertions(+), 6 deletions(-)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 3471ec52ea11..8147c17c71ab 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -75,6 +75,11 @@ static struct test generic_tests[] = {
{
.desc = "PMU events",
.func = test__pmu_events,
+ .subtest = {
+ .get_nr = test__pmu_events_subtest_get_nr,
+ .get_desc = test__pmu_events_subtest_get_desc,
+ },
+
},
{
.desc = "DSO data read",
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index f9e8e5628836..3f742612776a 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -39,6 +39,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
ret |= test(&ctx, "min(1,2) + 1", 2);
ret |= test(&ctx, "max(1,2) + 1", 3);
ret |= test(&ctx, "1+1 if 3*4 else 0", 2);
+ ret |= test(&ctx, "1.1 + 2.1", 3.2);
if (ret)
return ret;
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index d64261da8bf7..c18b9ce8cace 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -8,6 +8,10 @@
#include <linux/zalloc.h>
#include "debug.h"
#include "../pmu-events/pmu-events.h"
+#include "util/evlist.h"
+#include "util/expr.h"
+#include "util/parse-events.h"
+#include <ctype.h>
struct perf_pmu_test_event {
struct pmu_event event;
@@ -144,7 +148,7 @@ static struct pmu_events_map *__test_pmu_get_events_map(void)
}
/* Verify generated events from pmu-events.c is as expected */
-static int __test_pmu_event_table(void)
+static int test_pmu_event_table(void)
{
struct pmu_events_map *map = __test_pmu_get_events_map();
struct pmu_event *table;
@@ -347,14 +351,11 @@ static int __test__pmu_event_aliases(char *pmu_name, int *count)
return res;
}
-int test__pmu_events(struct test *test __maybe_unused,
- int subtest __maybe_unused)
+
+static int test_aliases(void)
{
struct perf_pmu *pmu = NULL;
- if (__test_pmu_event_table())
- return -1;
-
while ((pmu = perf_pmu__scan(pmu)) != NULL) {
int count = 0;
@@ -377,3 +378,148 @@ int test__pmu_events(struct test *test __maybe_unused,
return 0;
}
+
+static bool is_number(const char *str)
+{
+ size_t i;
+
+ for (i = 0; i < strlen(str); i++) {
+ if (!isdigit(str[i]) && str[i] != '.')
+ return false;
+ }
+ return true;
+}
+
+static int check_parse_id(const char *id, bool same_cpu, struct pmu_event *pe)
+{
+ struct parse_events_error error;
+ struct evlist *evlist;
+ int ret;
+
+ /* Numbers are always valid. */
+ if (is_number(id))
+ return 0;
+
+ evlist = evlist__new();
+ memset(&error, 0, sizeof(error));
+ ret = parse_events(evlist, id, &error);
+ if (ret && same_cpu) {
+ fprintf(stderr,
+ "\nWARNING: Parse event failed metric '%s' id '%s' expr '%s'\n",
+ pe->metric_name, id, pe->metric_expr);
+ fprintf(stderr, "Error string '%s' help '%s'\n",
+ error.str, error.help);
+ } else if (ret) {
+ pr_debug3("Parse event failed, but for an event that may not be supported by this CPU.\nid '%s' metric '%s' expr '%s'\n",
+ id, pe->metric_name, pe->metric_expr);
+ }
+ evlist__delete(evlist);
+ free(error.str);
+ free(error.help);
+ free(error.first_str);
+ free(error.first_help);
+ /* TODO: too many metrics are broken to fail on this test currently. */
+ return 0;
+}
+
+static int test_parsing(void)
+{
+ struct pmu_events_map *cpus_map = perf_pmu__find_map(NULL);
+ struct pmu_events_map *map;
+ struct pmu_event *pe;
+ int i, j, k;
+ const char **ids;
+ int idnum;
+ int ret = 0;
+ struct expr_parse_ctx ctx;
+ double result;
+
+ i = 0;
+ for (;;) {
+ map = &pmu_events_map[i++];
+ if (!map->table) {
+ map = NULL;
+ break;
+ }
+ j = 0;
+ for (;;) {
+ pe = &map->table[j++];
+ if (!pe->name && !pe->metric_group && !pe->metric_name)
+ break;
+ if (!pe->metric_expr)
+ continue;
+ if (expr__find_other(pe->metric_expr, NULL,
+ &ids, &idnum, 0) < 0) {
+ pr_debug("Parse other failed for map %s %s %s\n",
+ map->cpuid, map->version, map->type);
+ pr_debug("On metric %s\n", pe->metric_name);
+ pr_debug("On expression %s\n", pe->metric_expr);
+ ret++;
+ continue;
+ }
+ expr__ctx_init(&ctx);
+
+ /*
+ * Add all ids with a made up value. The value may
+ * trigger divide by zero when subtracted and so try to
+ * make them unique.
+ */
+ for (k = 0; k < idnum; k++)
+ expr__add_id(&ctx, ids[k], k + 1);
+
+ for (k = 0; k < idnum; k++) {
+ if (check_parse_id(ids[k], map == cpus_map, pe))
+ ret++;
+ }
+
+ if (expr__parse(&result, &ctx, pe->metric_expr, 0)) {
+ pr_debug("Parse failed for map %s %s %s\n",
+ map->cpuid, map->version, map->type);
+ pr_debug("On metric %s\n", pe->metric_name);
+ pr_debug("On expression %s\n", pe->metric_expr);
+ ret++;
+ }
+ for (k = 0; k < idnum; k++)
+ zfree(&ids[k]);
+ free(ids);
+ }
+ }
+ return ret;
+}
+
+static const struct {
+ int (*func)(void);
+ const char *desc;
+} pmu_events_testcase_table[] = {
+ {
+ .func = test_pmu_event_table,
+ .desc = "PMU event table sanity",
+ },
+ {
+ .func = test_aliases,
+ .desc = "PMU event map aliases",
+ },
+ {
+ .func = test_parsing,
+ .desc = "Parsing of PMU event table metrics",
+ },
+};
+
+const char *test__pmu_events_subtest_get_desc(int i)
+{
+ if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+ return NULL;
+ return pmu_events_testcase_table[i].desc;
+}
+
+int test__pmu_events_subtest_get_nr(void)
+{
+ return (int)ARRAY_SIZE(pmu_events_testcase_table);
+}
+
+int test__pmu_events(struct test *test __maybe_unused, int i)
+{
+ if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+ return TEST_FAIL;
+ return pmu_events_testcase_table[i].func();
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index d6d4ac34eeb7..8e316c30ed3c 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -50,6 +50,8 @@ int test__perf_evsel__tp_sched_test(struct test *test, int subtest);
int test__syscall_openat_tp_fields(struct test *test, int subtest);
int test__pmu(struct test *test, int subtest);
int test__pmu_events(struct test *test, int subtest);
+const char *test__pmu_events_subtest_get_desc(int subtest);
+int test__pmu_events_subtest_get_nr(void);
int test__attr(struct test *test, int subtest);
int test__dso_data(struct test *test, int subtest);
int test__dso_data_cache(struct test *test, int subtest);
--
2.26.2.645.ge9eca65c58-goog
Use a bitmap rather than an array of bools.
Signed-off-by: Ian Rogers <[email protected]>
---
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 37be5a368d6e..4f7e36bc49d9 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])
@@ -150,7 +150,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];
@@ -166,13 +166,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;
@@ -210,7 +208,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.645.ge9eca65c58-goog
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 <[email protected]>
---
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 2a6456fa178b..69fbff47089f 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -520,7 +520,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.645.ge9eca65c58-goog
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 <[email protected]>
---
tools/perf/Documentation/perf-stat.txt | 19 ++++++++++
tools/perf/builtin-stat.c | 11 +++++-
tools/perf/util/metricgroup.c | 51 ++++++++++++++++++--------
tools/perf/util/metricgroup.h | 6 ++-
tools/perf/util/stat.h | 2 +
5 files changed, 71 insertions(+), 18 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 e0c1ad23c768..5d444b1d82fb 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 c97dc87c2a31..9dcaac4fdebd 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
@@ -175,6 +183,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 +209,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);
@@ -523,7 +533,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;
@@ -536,7 +548,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);
@@ -563,7 +575,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);
@@ -588,7 +601,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;
@@ -601,7 +616,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)
@@ -625,7 +641,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;
@@ -640,7 +657,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);
@@ -667,8 +685,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;
@@ -678,7 +698,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);
@@ -689,8 +710,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.645.ge9eca65c58-goog
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 <[email protected]>
---
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 2c684fd3c4e3..2a6456fa178b 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,
@@ -497,8 +498,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;
@@ -511,6 +512,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);
@@ -518,14 +520,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;
@@ -536,6 +530,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)
@@ -554,7 +549,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;
@@ -565,13 +561,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.645.ge9eca65c58-goog
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 <[email protected]>
---
tools/perf/util/metricgroup.c | 96 ++++++++++++++++++++++-------------
1 file changed, 60 insertions(+), 36 deletions(-)
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 69fbff47089f..c97dc87c2a31 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -93,45 +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 (hashmap__find(&pctx->ids, ev->name,
- (void **)&val_ptr)) {
- if (!metric_events[i])
- metric_events[i] = ev;
- i++;
- if (i == idnum)
- 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;
}
@@ -139,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);
}
@@ -166,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);
@@ -182,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",
@@ -212,6 +230,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.645.ge9eca65c58-goog
Avoid a simple memory leak.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/metricgroup.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 4f7e36bc49d9..7e1725d61c39 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -186,6 +186,7 @@ static int metricgroup__setup_events(struct list_head *groups,
if (!evsel) {
pr_debug("Cannot resolve %s: %s\n",
eg->metric_name, eg->metric_expr);
+ free(metric_events);
continue;
}
for (i = 0; metric_events[i]; i++)
@@ -193,11 +194,13 @@ static int metricgroup__setup_events(struct list_head *groups,
me = metricgroup__lookup(metric_events_list, evsel, true);
if (!me) {
ret = -ENOMEM;
+ free(metric_events);
break;
}
expr = malloc(sizeof(struct metric_expr));
if (!expr) {
ret = -ENOMEM;
+ free(metric_events);
break;
}
expr->metric_expr = eg->metric_expr;
--
2.26.2.645.ge9eca65c58-goog
Use a hashmap between a char* string and a double* value. While bpf's
hashmap entries are size_t in size, we can't guarantee sizeof(size_t) >=
sizeof(double). Avoid a memory allocation when gathering ids by making 0.0
a special value encoded as NULL.
Suggested by Andi Kleen:
https://lore.kernel.org/lkml/[email protected]/
and seconded by Jiri Olsa:
https://lore.kernel.org/lkml/20200423112915.GH1136647@krava/
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/tests/expr.c | 40 ++++++-----
tools/perf/tests/pmu-events.c | 25 +++----
tools/perf/util/expr.c | 129 +++++++++++++++++++---------------
tools/perf/util/expr.h | 22 +++---
tools/perf/util/expr.y | 22 +-----
tools/perf/util/metricgroup.c | 87 +++++++++++------------
tools/perf/util/stat-shadow.c | 49 ++++++++-----
7 files changed, 193 insertions(+), 181 deletions(-)
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 3f742612776a..5e606fd5a2c6 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -19,11 +19,9 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
{
const char *p;
- const char **other;
- double val;
- int i, ret;
+ double val, *val_ptr;
+ int ret;
struct expr_parse_ctx ctx;
- int num_other;
expr__ctx_init(&ctx);
expr__add_id(&ctx, "FOO", 1);
@@ -52,25 +50,29 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
ret = expr__parse(&val, &ctx, p, 1);
TEST_ASSERT_VAL("missing operand", ret == -1);
+ hashmap__clear(&ctx.ids);
TEST_ASSERT_VAL("find other",
- expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", &other, &num_other, 1) == 0);
- TEST_ASSERT_VAL("find other", num_other == 3);
- TEST_ASSERT_VAL("find other", !strcmp(other[0], "BAR"));
- TEST_ASSERT_VAL("find other", !strcmp(other[1], "BAZ"));
- TEST_ASSERT_VAL("find other", !strcmp(other[2], "BOZO"));
- TEST_ASSERT_VAL("find other", other[3] == NULL);
+ expr__find_other("FOO + BAR + BAZ + BOZO", "FOO",
+ &ctx, 1) == 0);
+ TEST_ASSERT_VAL("find other", hashmap__size(&ctx.ids) == 3);
+ TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "BAR",
+ (void **)&val_ptr));
+ TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "BAZ",
+ (void **)&val_ptr));
+ TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "BOZO",
+ (void **)&val_ptr));
+ hashmap__clear(&ctx.ids);
TEST_ASSERT_VAL("find other",
- expr__find_other("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@", NULL,
- &other, &num_other, 3) == 0);
- TEST_ASSERT_VAL("find other", num_other == 2);
- TEST_ASSERT_VAL("find other", !strcmp(other[0], "EVENT1,param=3/"));
- TEST_ASSERT_VAL("find other", !strcmp(other[1], "EVENT2,param=3/"));
- TEST_ASSERT_VAL("find other", other[2] == NULL);
+ expr__find_other("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@",
+ NULL, &ctx, 3) == 0);
+ TEST_ASSERT_VAL("find other", hashmap__size(&ctx.ids) == 2);
+ TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "EVENT1,param=3/",
+ (void **)&val_ptr));
+ TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "EVENT2,param=3/",
+ (void **)&val_ptr));
- for (i = 0; i < num_other; i++)
- zfree(&other[i]);
- free((void *)other);
+ expr__ctx_clear(&ctx);
return 0;
}
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index c18b9ce8cace..054550ee811c 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -428,8 +428,6 @@ static int test_parsing(void)
struct pmu_events_map *map;
struct pmu_event *pe;
int i, j, k;
- const char **ids;
- int idnum;
int ret = 0;
struct expr_parse_ctx ctx;
double result;
@@ -443,13 +441,17 @@ static int test_parsing(void)
}
j = 0;
for (;;) {
+ struct hashmap_entry *cur;
+ size_t bkt;
+
pe = &map->table[j++];
if (!pe->name && !pe->metric_group && !pe->metric_name)
break;
if (!pe->metric_expr)
continue;
- if (expr__find_other(pe->metric_expr, NULL,
- &ids, &idnum, 0) < 0) {
+ expr__ctx_init(&ctx);
+ if (expr__find_other(pe->metric_expr, NULL, &ctx, 0)
+ < 0) {
pr_debug("Parse other failed for map %s %s %s\n",
map->cpuid, map->version, map->type);
pr_debug("On metric %s\n", pe->metric_name);
@@ -457,18 +459,19 @@ static int test_parsing(void)
ret++;
continue;
}
- expr__ctx_init(&ctx);
/*
* Add all ids with a made up value. The value may
* trigger divide by zero when subtracted and so try to
* make them unique.
*/
- for (k = 0; k < idnum; k++)
- expr__add_id(&ctx, ids[k], k + 1);
+ k = 1;
+ hashmap__for_each_entry((&ctx.ids), cur, bkt)
+ expr__add_id(&ctx, strdup(cur->key), k++);
- for (k = 0; k < idnum; k++) {
- if (check_parse_id(ids[k], map == cpus_map, pe))
+ hashmap__for_each_entry((&ctx.ids), cur, bkt) {
+ if (check_parse_id(cur->key, map == cpus_map,
+ pe))
ret++;
}
@@ -479,9 +482,7 @@ static int test_parsing(void)
pr_debug("On expression %s\n", pe->metric_expr);
ret++;
}
- for (k = 0; k < idnum; k++)
- zfree(&ids[k]);
- free(ids);
+ expr__ctx_clear(&ctx);
}
}
return ret;
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 8b4ce704a68d..f64ab91c432b 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -4,25 +4,76 @@
#include "expr.h"
#include "expr-bison.h"
#include "expr-flex.h"
+#include <linux/kernel.h>
#ifdef PARSER_DEBUG
extern int expr_debug;
#endif
+static size_t key_hash(const void *key, void *ctx __maybe_unused)
+{
+ const char *str = (const char *)key;
+ size_t hash = 0;
+
+ while (*str != '\0') {
+ hash *= 31;
+ hash += *str;
+ str++;
+ }
+ return hash;
+}
+
+static bool key_equal(const void *key1, const void *key2,
+ void *ctx __maybe_unused)
+{
+ return !strcmp((const char *)key1, (const char *)key2);
+}
+
/* Caller must make sure id is allocated */
-void expr__add_id(struct expr_parse_ctx *ctx, const char *name, double val)
+int expr__add_id(struct expr_parse_ctx *ctx, const char *name, double val)
{
- int idx;
+ double *val_ptr = NULL, *old_val = NULL;
+ char *old_key = NULL;
+ int ret;
+
+ if (val != 0.0) {
+ val_ptr = malloc(sizeof(double));
+ if (!val_ptr)
+ return -ENOMEM;
+ *val_ptr = val;
+ }
+ ret = hashmap__set(&ctx->ids, name, val_ptr,
+ (const void **)&old_key, (void **)&old_val);
+ free(old_key);
+ free(old_val);
+ return ret;
+}
+
+int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr)
+{
+ double *data;
- assert(ctx->num_ids < MAX_PARSE_ID);
- idx = ctx->num_ids++;
- ctx->ids[idx].name = name;
- ctx->ids[idx].val = val;
+ if (!hashmap__find(&ctx->ids, id, (void **)&data))
+ return -1;
+ *val_ptr = (data == NULL) ? 0.0 : *data;
+ return 0;
}
void expr__ctx_init(struct expr_parse_ctx *ctx)
{
- ctx->num_ids = 0;
+ hashmap__init(&ctx->ids, key_hash, key_equal, NULL);
+}
+
+void expr__ctx_clear(struct expr_parse_ctx *ctx)
+{
+ struct hashmap_entry *cur;
+ size_t bkt;
+
+ hashmap__for_each_entry((&ctx->ids), cur, bkt) {
+ free((char *)cur->key);
+ free(cur->value);
+ }
+ hashmap__clear(&ctx->ids);
}
static int
@@ -56,61 +107,25 @@ __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
return ret;
}
-int expr__parse(double *final_val, struct expr_parse_ctx *ctx, const char *expr, int runtime)
+int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
+ const char *expr, int runtime)
{
return __expr__parse(final_val, ctx, expr, EXPR_PARSE, runtime) ? -1 : 0;
}
-static bool
-already_seen(const char *val, const char *one, const char **other,
- int num_other)
-{
- int i;
-
- if (one && !strcasecmp(one, val))
- return true;
- for (i = 0; i < num_other; i++)
- if (!strcasecmp(other[i], val))
- return true;
- return false;
-}
-
-int expr__find_other(const char *expr, const char *one, const char ***other,
- int *num_other, int runtime)
+int expr__find_other(const char *expr, const char *one,
+ struct expr_parse_ctx *ctx, int runtime)
{
- int err, i = 0, j = 0;
- struct expr_parse_ctx ctx;
-
- expr__ctx_init(&ctx);
- err = __expr__parse(NULL, &ctx, expr, EXPR_OTHER, runtime);
- if (err)
- return -1;
-
- *other = malloc((ctx.num_ids + 1) * sizeof(char *));
- if (!*other)
- return -ENOMEM;
-
- for (i = 0, j = 0; i < ctx.num_ids; i++) {
- const char *str = ctx.ids[i].name;
-
- if (already_seen(str, one, *other, j))
- continue;
-
- str = strdup(str);
- if (!str)
- goto out;
- (*other)[j++] = str;
- }
- (*other)[j] = NULL;
-
-out:
- if (i != ctx.num_ids) {
- while (--j)
- free((char *) (*other)[i]);
- free(*other);
- err = -1;
+ double *old_val = NULL;
+ char *old_key = NULL;
+ int ret = __expr__parse(NULL, ctx, expr, EXPR_OTHER, runtime);
+
+ if (one) {
+ hashmap__delete(&ctx->ids, one,
+ (const void **)&old_key, (void **)&old_val);
+ free(old_key);
+ free(old_val);
}
- *num_other = j;
- return err;
+ return ret;
}
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 40fc452b0f2b..f01bd5ecb09d 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -2,17 +2,10 @@
#ifndef PARSE_CTX_H
#define PARSE_CTX_H 1
-#define EXPR_MAX_OTHER 64
-#define MAX_PARSE_ID EXPR_MAX_OTHER
-
-struct expr_parse_id {
- const char *name;
- double val;
-};
+#include <bpf/hashmap.h>
struct expr_parse_ctx {
- int num_ids;
- struct expr_parse_id ids[MAX_PARSE_ID];
+ struct hashmap ids;
};
struct expr_scanner_ctx {
@@ -21,9 +14,12 @@ struct expr_scanner_ctx {
};
void expr__ctx_init(struct expr_parse_ctx *ctx);
-void expr__add_id(struct expr_parse_ctx *ctx, const char *id, double val);
-int expr__parse(double *final_val, struct expr_parse_ctx *ctx, const char *expr, int runtime);
-int expr__find_other(const char *expr, const char *one, const char ***other,
- int *num_other, int runtime);
+void expr__ctx_clear(struct expr_parse_ctx *ctx);
+int expr__add_id(struct expr_parse_ctx *ctx, const char *id, double val);
+int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr);
+int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
+ const char *expr, int runtime);
+int expr__find_other(const char *expr, const char *one,
+ struct expr_parse_ctx *ids, int runtime);
#endif
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 3b49b230b111..bf3e898e3055 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -47,19 +47,6 @@ static void expr_error(double *final_val __maybe_unused,
pr_debug("%s\n", s);
}
-static int lookup_id(struct expr_parse_ctx *ctx, char *id, double *val)
-{
- int i;
-
- for (i = 0; i < ctx->num_ids; i++) {
- if (!strcasecmp(ctx->ids[i].name, id)) {
- *val = ctx->ids[i].val;
- return 0;
- }
- }
- return -1;
-}
-
%}
%%
@@ -73,12 +60,7 @@ all_other: all_other other
other: ID
{
- if (ctx->num_ids + 1 >= EXPR_MAX_OTHER) {
- pr_err("failed: way too many variables");
- YYABORT;
- }
-
- ctx->ids[ctx->num_ids++].name = $1;
+ expr__add_id(ctx, $1, 0.0);
}
|
MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')' | ','
@@ -93,7 +75,7 @@ if_expr:
;
expr: NUMBER
- | ID { if (lookup_id(ctx, $1, &$$) < 0) {
+ | ID { if (expr__get_id(ctx, $1, &$$)) {
pr_debug("%s not found\n", $1);
free($1);
YYABORT;
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index b071df373f8b..37be5a368d6e 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -85,8 +85,7 @@ static void metricgroup__rblist_init(struct rblist *metric_events)
struct egroup {
struct list_head nd;
- int idnum;
- const char **ids;
+ struct expr_parse_ctx pctx;
const char *metric_name;
const char *metric_expr;
const char *metric_unit;
@@ -94,19 +93,21 @@ struct egroup {
};
static struct evsel *find_evsel_group(struct evlist *perf_evlist,
- const char **ids,
- int idnum,
+ struct expr_parse_ctx *pctx,
struct evsel **metric_events,
bool *evlist_used)
{
struct evsel *ev;
- int i = 0, j = 0;
bool leader_found;
+ const size_t idnum = hashmap__size(&pctx->ids);
+ size_t i = 0;
+ int j = 0;
+ double *val_ptr;
evlist__for_each_entry (perf_evlist, ev) {
if (evlist_used[j++])
continue;
- if (!strcmp(ev->name, ids[i])) {
+ if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
if (!metric_events[i])
metric_events[i] = ev;
i++;
@@ -118,7 +119,8 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
memset(metric_events, 0,
sizeof(struct evsel *) * idnum);
- if (!strcmp(ev->name, ids[i])) {
+ if (hashmap__find(&pctx->ids, ev->name,
+ (void **)&val_ptr)) {
if (!metric_events[i])
metric_events[i] = ev;
i++;
@@ -175,19 +177,20 @@ static int metricgroup__setup_events(struct list_head *groups,
list_for_each_entry (eg, groups, nd) {
struct evsel **metric_events;
- metric_events = calloc(sizeof(void *), eg->idnum + 1);
+ metric_events = calloc(sizeof(void *),
+ hashmap__size(&eg->pctx.ids) + 1);
if (!metric_events) {
ret = -ENOMEM;
break;
}
- evsel = find_evsel_group(perf_evlist, eg->ids, eg->idnum,
- metric_events, evlist_used);
+ evsel = find_evsel_group(perf_evlist, &eg->pctx, metric_events,
+ evlist_used);
if (!evsel) {
pr_debug("Cannot resolve %s: %s\n",
eg->metric_name, eg->metric_expr);
continue;
}
- for (i = 0; i < eg->idnum; i++)
+ for (i = 0; metric_events[i]; i++)
metric_events[i]->collect_stat = true;
me = metricgroup__lookup(metric_events_list, evsel, true);
if (!me) {
@@ -415,20 +418,20 @@ void metricgroup__print(bool metrics, bool metricgroups, char *filter,
}
static void metricgroup__add_metric_weak_group(struct strbuf *events,
- const char **ids,
- int idnum)
+ struct expr_parse_ctx *ctx)
{
+ struct hashmap_entry *cur;
+ size_t bkt, i = 0;
bool no_group = false;
- int i;
- for (i = 0; i < idnum; i++) {
- pr_debug("found event %s\n", ids[i]);
+ hashmap__for_each_entry((&ctx->ids), cur, bkt) {
+ pr_debug("found event %s\n", (const char *)cur->key);
/*
* Duration time maps to a software event and can make
* groups not count. Always use it outside a
* group.
*/
- if (!strcmp(ids[i], "duration_time")) {
+ if (!strcmp(cur->key, "duration_time")) {
if (i > 0)
strbuf_addf(events, "}:W,");
strbuf_addf(events, "duration_time");
@@ -437,21 +440,22 @@ static void metricgroup__add_metric_weak_group(struct strbuf *events,
}
strbuf_addf(events, "%s%s",
i == 0 || no_group ? "{" : ",",
- ids[i]);
+ (const char *)cur->key);
no_group = false;
+ i++;
}
if (!no_group)
strbuf_addf(events, "}:W");
}
static void metricgroup__add_metric_non_group(struct strbuf *events,
- const char **ids,
- int idnum)
+ struct expr_parse_ctx *ctx)
{
- int i;
+ struct hashmap_entry *cur;
+ size_t bkt;
- for (i = 0; i < idnum; i++)
- strbuf_addf(events, ",%s", ids[i]);
+ hashmap__for_each_entry((&ctx->ids), cur, bkt)
+ strbuf_addf(events, ",%s", (const char *)cur->key);
}
static void metricgroup___watchdog_constraint_hint(const char *name, bool foot)
@@ -495,32 +499,32 @@ int __weak arch_get_runtimeparam(void)
static int __metricgroup__add_metric(struct strbuf *events,
struct list_head *group_list, struct pmu_event *pe, int runtime)
{
-
- const char **ids;
- int idnum;
struct egroup *eg;
- if (expr__find_other(pe->metric_expr, NULL, &ids, &idnum, runtime) < 0)
- return -EINVAL;
-
- if (events->len > 0)
- strbuf_addf(events, ",");
-
- if (metricgroup__has_constraint(pe))
- metricgroup__add_metric_non_group(events, ids, idnum);
- else
- metricgroup__add_metric_weak_group(events, ids, idnum);
-
eg = malloc(sizeof(*eg));
if (!eg)
return -ENOMEM;
- eg->ids = ids;
- eg->idnum = idnum;
+ expr__ctx_init(&eg->pctx);
eg->metric_name = pe->metric_name;
eg->metric_expr = pe->metric_expr;
eg->metric_unit = pe->unit;
eg->runtime = runtime;
+
+ if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
+ expr__ctx_clear(&eg->pctx);
+ free(eg);
+ 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;
@@ -603,12 +607,9 @@ static int metricgroup__add_metric_list(const char *list, struct strbuf *events,
static void metricgroup__free_egroups(struct list_head *group_list)
{
struct egroup *eg, *egtmp;
- int i;
list_for_each_entry_safe (eg, egtmp, group_list, nd) {
- for (i = 0; i < eg->idnum; i++)
- zfree(&eg->ids[i]);
- zfree(&eg->ids);
+ expr__ctx_clear(&eg->pctx);
list_del_init(&eg->nd);
free(eg);
}
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 9bd7a8d2a858..c44dc814b377 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -323,35 +323,46 @@ void perf_stat__collect_metric_expr(struct evlist *evsel_list)
{
struct evsel *counter, *leader, **metric_events, *oc;
bool found;
- const char **metric_names;
+ struct expr_parse_ctx ctx;
+ struct hashmap_entry *cur;
+ size_t bkt;
int i;
- int num_metric_names;
+ expr__ctx_init(&ctx);
evlist__for_each_entry(evsel_list, counter) {
bool invalid = false;
leader = counter->leader;
if (!counter->metric_expr)
continue;
+
+ expr__ctx_clear(&ctx);
metric_events = counter->metric_events;
if (!metric_events) {
- if (expr__find_other(counter->metric_expr, counter->name,
- &metric_names, &num_metric_names, 1) < 0)
+ if (expr__find_other(counter->metric_expr,
+ counter->name,
+ &ctx, 1) < 0)
continue;
metric_events = calloc(sizeof(struct evsel *),
- num_metric_names + 1);
- if (!metric_events)
+ hashmap__size(&ctx.ids) + 1);
+ if (!metric_events) {
+ expr__ctx_clear(&ctx);
return;
+ }
counter->metric_events = metric_events;
}
- for (i = 0; i < num_metric_names; i++) {
+ i = 0;
+ hashmap__for_each_entry((&ctx.ids), cur, bkt) {
+ const char *metric_name = (const char *)cur->key;
+
found = false;
if (leader) {
/* Search in group */
for_each_group_member (oc, leader) {
- if (!strcasecmp(oc->name, metric_names[i]) &&
+ if (!strcasecmp(oc->name,
+ metric_name) &&
!oc->collect_stat) {
found = true;
break;
@@ -360,7 +371,8 @@ void perf_stat__collect_metric_expr(struct evlist *evsel_list)
}
if (!found) {
/* Search ignoring groups */
- oc = perf_stat__find_event(evsel_list, metric_names[i]);
+ oc = perf_stat__find_event(evsel_list,
+ metric_name);
}
if (!oc) {
/* Deduping one is good enough to handle duplicated PMUs. */
@@ -373,27 +385,28 @@ void perf_stat__collect_metric_expr(struct evlist *evsel_list)
* of events. So we ask the user instead to add the missing
* events.
*/
- if (!printed || strcasecmp(printed, metric_names[i])) {
+ if (!printed ||
+ strcasecmp(printed, metric_name)) {
fprintf(stderr,
"Add %s event to groups to get metric expression for %s\n",
- metric_names[i],
+ metric_name,
counter->name);
- printed = strdup(metric_names[i]);
+ printed = strdup(metric_name);
}
invalid = true;
continue;
}
- metric_events[i] = oc;
+ metric_events[i++] = oc;
oc->collect_stat = true;
}
metric_events[i] = NULL;
- free(metric_names);
if (invalid) {
free(metric_events);
counter->metric_events = NULL;
counter->metric_expr = NULL;
}
}
+ expr__ctx_clear(&ctx);
}
static double runtime_stat_avg(struct runtime_stat *st,
@@ -738,7 +751,10 @@ static void generic_metric(struct perf_stat_config *config,
expr__ctx_init(&pctx);
/* Must be first id entry */
- expr__add_id(&pctx, name, avg);
+ n = strdup(name);
+ if (!n)
+ return;
+ expr__add_id(&pctx, n, avg);
for (i = 0; metric_events[i]; i++) {
struct saved_value *v;
struct stats *stats;
@@ -814,8 +830,7 @@ static void generic_metric(struct perf_stat_config *config,
(metric_name ? metric_name : name) : "", 0);
}
- for (i = 1; i < pctx.num_ids; i++)
- zfree(&pctx.ids[i].name);
+ expr__ctx_clear(&pctx);
}
void perf_stat__print_shadow_stats(struct perf_stat_config *config,
--
2.26.2.645.ge9eca65c58-goog
If allocated, perf_pkg_mask and metric_events need freeing.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/evsel.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 28683b0eb738..05bb46baad6a 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1263,6 +1263,8 @@ void evsel__exit(struct evsel *evsel)
zfree(&evsel->group_name);
zfree(&evsel->name);
zfree(&evsel->pmu_name);
+ zfree(&evsel->per_pkg_mask);
+ zfree(&evsel->metric_events);
perf_evsel__object.fini(evsel);
}
--
2.26.2.645.ge9eca65c58-goog
From: Andrii Nakryiko <[email protected]>
Fix memory leak in hashmap_clear() not freeing hashmap_entry structs for each
of the remaining entries. Also NULL-out bucket list to prevent possible
double-free between hashmap__clear() and hashmap__free().
Running test_progs-asan flavor clearly showed this problem.
Reported-by: Alston Tang <[email protected]>
Signed-off-by: Andrii Nakryiko <[email protected]>
Signed-off-by: Alexei Starovoitov <[email protected]>
Link: https://lore.kernel.org/bpf/[email protected]
Signed-off-by: Ian Rogers <[email protected]>
---
tools/lib/bpf/hashmap.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tools/lib/bpf/hashmap.c b/tools/lib/bpf/hashmap.c
index 54c30c802070..cffb96202e0d 100644
--- a/tools/lib/bpf/hashmap.c
+++ b/tools/lib/bpf/hashmap.c
@@ -59,7 +59,14 @@ struct hashmap *hashmap__new(hashmap_hash_fn hash_fn,
void hashmap__clear(struct hashmap *map)
{
+ struct hashmap_entry *cur, *tmp;
+ int bkt;
+
+ hashmap__for_each_entry_safe(map, cur, tmp, bkt) {
+ free(cur);
+ }
free(map->buckets);
+ map->buckets = NULL;
map->cap = map->cap_bits = map->sz = 0;
}
--
2.26.2.645.ge9eca65c58-goog
On a CPU like skylakex an uncore_iio_0 PMU may alias with
uncore_iio_free_running_0. The latter PMU doesn't support fc_mask
as a parameter and so pmu_config_term fails. Typically
parse_events_add_pmu is called in a loop where if one alias succeeds
errors are ignored, however, if multiple errors occur
parse_events__handle_error will currently give a WARN_ONCE.
This change removes the WARN_ONCE in parse_events__handle_error and
makes it a pr_debug. It adds verbose messages to parse_events_add_pmu
warning that non-fatal errors may occur, while giving details on the pmu
and config terms for useful context. pmu_config_term is altered so the
failing term and pmu are present in the case of the 'unknown term'
error which makes spotting the free_running case more straightforward.
Before:
$ perf --debug verbose=3 stat -M llc_misses.pcie_read sleep 1
Using CPUID GenuineIntel-6-55-4
metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
found event unc_iio_data_req_of_cpu.mem_read.part0
found event unc_iio_data_req_of_cpu.mem_read.part1
found event unc_iio_data_req_of_cpu.mem_read.part2
found event unc_iio_data_req_of_cpu.mem_read.part3
metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
found event unc_iio_data_req_of_cpu.mem_read.part0
found event unc_iio_data_req_of_cpu.mem_read.part1
found event unc_iio_data_req_of_cpu.mem_read.part2
found event unc_iio_data_req_of_cpu.mem_read.part3
adding {unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W,{unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W
intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
WARNING: multiple event parsing errors
...
Invalid event/parameter 'fc_mask'
...
After:
$ perf --debug verbose=3 stat -M llc_misses.pcie_read sleep 1
Using CPUID GenuineIntel-6-55-4
metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
found event unc_iio_data_req_of_cpu.mem_read.part0
found event unc_iio_data_req_of_cpu.mem_read.part1
found event unc_iio_data_req_of_cpu.mem_read.part2
found event unc_iio_data_req_of_cpu.mem_read.part3
metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
found event unc_iio_data_req_of_cpu.mem_read.part0
found event unc_iio_data_req_of_cpu.mem_read.part1
found event unc_iio_data_req_of_cpu.mem_read.part2
found event unc_iio_data_req_of_cpu.mem_read.part3
adding {unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W,{unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W
intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
Attempting to add event pmu 'uncore_iio_free_running_5' with 'unc_iio_data_req_of_cpu.mem_read.part0,' that may result in non-fatal errors
After aliases, add event pmu 'uncore_iio_free_running_5' with 'fc_mask,ch_mask,umask,event,' that may result in non-fatal errors
Attempting to add event pmu 'uncore_iio_free_running_3' with 'unc_iio_data_req_of_cpu.mem_read.part0,' that may result in non-fatal errors
After aliases, add event pmu 'uncore_iio_free_running_3' with 'fc_mask,ch_mask,umask,event,' that may result in non-fatal errors
Attempting to add event pmu 'uncore_iio_free_running_1' with 'unc_iio_data_req_of_cpu.mem_read.part0,' that may result in non-fatal errors
After aliases, add event pmu 'uncore_iio_free_running_1' with 'fc_mask,ch_mask,umask,event,' that may result in non-fatal errors
Multiple errors dropping message: unknown term 'fc_mask' for pmu 'uncore_iio_free_running_3' (valid terms: event,umask,config,config1,config2,name,period,percore)
...
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/arch/x86/util/intel-pt.c | 32 +++++++++++++++++-----------
tools/perf/tests/pmu.c | 4 ++--
tools/perf/util/parse-events.c | 29 ++++++++++++++++++++++++-
tools/perf/util/pmu.c | 33 ++++++++++++++++++-----------
tools/perf/util/pmu.h | 2 +-
5 files changed, 72 insertions(+), 28 deletions(-)
diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c
index fd9e22d1e366..0fe401ad3347 100644
--- a/tools/perf/arch/x86/util/intel-pt.c
+++ b/tools/perf/arch/x86/util/intel-pt.c
@@ -59,7 +59,8 @@ struct intel_pt_recording {
size_t priv_size;
};
-static int intel_pt_parse_terms_with_default(struct list_head *formats,
+static int intel_pt_parse_terms_with_default(const char *pmu_name,
+ struct list_head *formats,
const char *str,
u64 *config)
{
@@ -78,7 +79,8 @@ static int intel_pt_parse_terms_with_default(struct list_head *formats,
goto out_free;
attr.config = *config;
- err = perf_pmu__config_terms(formats, &attr, terms, true, NULL);
+ err = perf_pmu__config_terms(pmu_name, formats, &attr, terms, true,
+ NULL);
if (err)
goto out_free;
@@ -88,11 +90,12 @@ static int intel_pt_parse_terms_with_default(struct list_head *formats,
return err;
}
-static int intel_pt_parse_terms(struct list_head *formats, const char *str,
- u64 *config)
+static int intel_pt_parse_terms(const char *pmu_name, struct list_head *formats,
+ const char *str, u64 *config)
{
*config = 0;
- return intel_pt_parse_terms_with_default(formats, str, config);
+ return intel_pt_parse_terms_with_default(pmu_name, formats, str,
+ config);
}
static u64 intel_pt_masked_bits(u64 mask, u64 bits)
@@ -229,7 +232,8 @@ static u64 intel_pt_default_config(struct perf_pmu *intel_pt_pmu)
pr_debug2("%s default config: %s\n", intel_pt_pmu->name, buf);
- intel_pt_parse_terms(&intel_pt_pmu->format, buf, &config);
+ intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format, buf,
+ &config);
return config;
}
@@ -337,13 +341,16 @@ static int intel_pt_info_fill(struct auxtrace_record *itr,
if (priv_size != ptr->priv_size)
return -EINVAL;
- intel_pt_parse_terms(&intel_pt_pmu->format, "tsc", &tsc_bit);
- intel_pt_parse_terms(&intel_pt_pmu->format, "noretcomp",
- &noretcomp_bit);
- intel_pt_parse_terms(&intel_pt_pmu->format, "mtc", &mtc_bit);
+ intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
+ "tsc", &tsc_bit);
+ intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
+ "noretcomp", &noretcomp_bit);
+ intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
+ "mtc", &mtc_bit);
mtc_freq_bits = perf_pmu__format_bits(&intel_pt_pmu->format,
"mtc_period");
- intel_pt_parse_terms(&intel_pt_pmu->format, "cyc", &cyc_bit);
+ intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
+ "cyc", &cyc_bit);
intel_pt_tsc_ctc_ratio(&tsc_ctc_ratio_n, &tsc_ctc_ratio_d);
@@ -768,7 +775,8 @@ static int intel_pt_recording_options(struct auxtrace_record *itr,
}
}
- intel_pt_parse_terms(&intel_pt_pmu->format, "tsc", &tsc_bit);
+ intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
+ "tsc", &tsc_bit);
if (opts->full_auxtrace && (intel_pt_evsel->core.attr.config & tsc_bit))
have_timing_info = true;
diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index 74379ff1f7fa..5c11fe2b3040 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -156,8 +156,8 @@ int test__pmu(struct test *test __maybe_unused, int subtest __maybe_unused)
if (ret)
break;
- ret = perf_pmu__config_terms(&formats, &attr, terms,
- false, NULL);
+ ret = perf_pmu__config_terms("perf-pmu-test", &formats, &attr,
+ terms, false, NULL);
if (ret)
break;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index e9464b04f149..0ebc0fd9385a 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -204,7 +204,8 @@ void parse_events__handle_error(struct parse_events_error *err, int idx,
err->help = help;
break;
default:
- WARN_ONCE(1, "WARNING: multiple event parsing errors\n");
+ pr_debug("Multiple errors dropping message: %s (%s)\n",
+ err->str, err->help);
free(err->str);
err->str = str;
free(err->help);
@@ -1422,6 +1423,19 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
bool use_uncore_alias;
LIST_HEAD(config_terms);
+ if (verbose > 1) {
+ fprintf(stderr, "Attempting to add event pmu '%s' with '",
+ name);
+ if (head_config) {
+ struct parse_events_term *term;
+
+ list_for_each_entry(term, head_config, list) {
+ fprintf(stderr, "%s,", term->config);
+ }
+ }
+ fprintf(stderr, "' that may result in non-fatal errors\n");
+ }
+
pmu = perf_pmu__find(name);
if (!pmu) {
char *err_str;
@@ -1458,6 +1472,19 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
if (perf_pmu__check_alias(pmu, head_config, &info))
return -EINVAL;
+ if (verbose > 1) {
+ fprintf(stderr, "After aliases, add event pmu '%s' with '",
+ name);
+ if (head_config) {
+ struct parse_events_term *term;
+
+ list_for_each_entry(term, head_config, list) {
+ fprintf(stderr, "%s,", term->config);
+ }
+ }
+ fprintf(stderr, "' that may result in non-fatal errors\n");
+ }
+
/*
* Configure hardcoded terms first, no need to check
* return value when called with fail == 0 ;)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 92bd7fafcce6..71d0290b616a 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1056,7 +1056,8 @@ static char *pmu_formats_string(struct list_head *formats)
* Setup one of config[12] attr members based on the
* user input data - term parameter.
*/
-static int pmu_config_term(struct list_head *formats,
+static int pmu_config_term(const char *pmu_name,
+ struct list_head *formats,
struct perf_event_attr *attr,
struct parse_events_term *term,
struct list_head *head_terms,
@@ -1082,16 +1083,24 @@ static int pmu_config_term(struct list_head *formats,
format = pmu_find_format(formats, term->config);
if (!format) {
- if (verbose > 0)
- printf("Invalid event/parameter '%s'\n", term->config);
+ char *pmu_term = pmu_formats_string(formats);
+ char *unknown_term;
+ char *help_msg;
+
+ if (asprintf(&unknown_term,
+ "unknown term '%s' for pmu '%s'",
+ term->config, pmu_name) < 0)
+ unknown_term = strdup("unknown term");
+ help_msg = parse_events_formats_error_string(pmu_term);
if (err) {
- char *pmu_term = pmu_formats_string(formats);
-
parse_events__handle_error(err, term->err_term,
- strdup("unknown term"),
- parse_events_formats_error_string(pmu_term));
- free(pmu_term);
+ unknown_term,
+ help_msg);
+ } else {
+ pr_debug("%s (%s)\n", unknown_term, help_msg);
+ free(unknown_term);
}
+ free(pmu_term);
return -EINVAL;
}
@@ -1168,7 +1177,7 @@ static int pmu_config_term(struct list_head *formats,
return 0;
}
-int perf_pmu__config_terms(struct list_head *formats,
+int perf_pmu__config_terms(const char *pmu_name, struct list_head *formats,
struct perf_event_attr *attr,
struct list_head *head_terms,
bool zero, struct parse_events_error *err)
@@ -1176,7 +1185,7 @@ int perf_pmu__config_terms(struct list_head *formats,
struct parse_events_term *term;
list_for_each_entry(term, head_terms, list) {
- if (pmu_config_term(formats, attr, term, head_terms,
+ if (pmu_config_term(pmu_name, formats, attr, term, head_terms,
zero, err))
return -EINVAL;
}
@@ -1196,8 +1205,8 @@ int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
bool zero = !!pmu->default_config;
attr->type = pmu->type;
- return perf_pmu__config_terms(&pmu->format, attr, head_terms,
- zero, err);
+ return perf_pmu__config_terms(pmu->name, &pmu->format, attr,
+ head_terms, zero, err);
}
static struct perf_pmu_alias *pmu_find_alias(struct perf_pmu *pmu,
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index e119333e93ba..85e0c7f2515c 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -76,7 +76,7 @@ struct perf_pmu *perf_pmu__find_by_type(unsigned int type);
int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
struct list_head *head_terms,
struct parse_events_error *error);
-int perf_pmu__config_terms(struct list_head *formats,
+int perf_pmu__config_terms(const char *pmu_name, struct list_head *formats,
struct perf_event_attr *attr,
struct list_head *head_terms,
bool zero, struct parse_events_error *error);
--
2.26.2.645.ge9eca65c58-goog
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 <[email protected]>
---
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 7e1725d61c39..2c684fd3c4e3 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -422,8 +422,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);
@@ -433,20 +433,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.645.ge9eca65c58-goog
Add a destructor for strings to reclaim memory in the event of errors.
Free the ID given for a lookup.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/expr.y | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 21e82a1e11a2..3b49b230b111 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -27,6 +27,7 @@
%token EXPR_PARSE EXPR_OTHER EXPR_ERROR
%token <num> NUMBER
%token <str> ID
+%destructor { free ($$); } <str>
%token MIN MAX IF ELSE SMT_ON
%left MIN MAX IF
%left '|'
@@ -94,8 +95,10 @@ if_expr:
expr: NUMBER
| ID { if (lookup_id(ctx, $1, &$$) < 0) {
pr_debug("%s not found\n", $1);
+ free($1);
YYABORT;
}
+ free($1);
}
| expr '|' expr { $$ = (long)$1 | (long)$3; }
| expr '&' expr { $$ = (long)$1 & (long)$3; }
--
2.26.2.645.ge9eca65c58-goog
On Thu, May 07, 2020 at 10:36:15PM -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.
Yes some improvements here are great.
>
> 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.
Could we also make this a per metric option, like
-M foo:nomerge,...
or somesuch? Okay i suppose this could be a followon.
Ultimatively like you said we probably want to configure
defaults in the event file.
-Andi
This seems like a independent bug fix and should probably
be pushed independently of the rest.
Perhaps add a Fixes: tag.
Reviewed-by: Andi Kleen <[email protected]>
On Thu, May 07, 2020 at 10:36:16PM -0700, Ian Rogers wrote:
> On a CPU like skylakex an uncore_iio_0 PMU may alias with
> uncore_iio_free_running_0. The latter PMU doesn't support fc_mask
> as a parameter and so pmu_config_term fails. Typically
> parse_events_add_pmu is called in a loop where if one alias succeeds
> errors are ignored, however, if multiple errors occur
> parse_events__handle_error will currently give a WARN_ONCE.
>
> This change removes the WARN_ONCE in parse_events__handle_error and
> makes it a pr_debug. It adds verbose messages to parse_events_add_pmu
> warning that non-fatal errors may occur, while giving details on the pmu
> and config terms for useful context. pmu_config_term is altered so the
> failing term and pmu are present in the case of the 'unknown term'
> error which makes spotting the free_running case more straightforward.
>
> Before:
> $ perf --debug verbose=3 stat -M llc_misses.pcie_read sleep 1
> Using CPUID GenuineIntel-6-55-4
> metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
> found event unc_iio_data_req_of_cpu.mem_read.part0
> found event unc_iio_data_req_of_cpu.mem_read.part1
> found event unc_iio_data_req_of_cpu.mem_read.part2
> found event unc_iio_data_req_of_cpu.mem_read.part3
> metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
> found event unc_iio_data_req_of_cpu.mem_read.part0
> found event unc_iio_data_req_of_cpu.mem_read.part1
> found event unc_iio_data_req_of_cpu.mem_read.part2
> found event unc_iio_data_req_of_cpu.mem_read.part3
> adding {unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W,{unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W
> intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
> WARNING: multiple event parsing errors
> ...
> Invalid event/parameter 'fc_mask'
> ...
>
> After:
> $ perf --debug verbose=3 stat -M llc_misses.pcie_read sleep 1
> Using CPUID GenuineIntel-6-55-4
> metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
> found event unc_iio_data_req_of_cpu.mem_read.part0
> found event unc_iio_data_req_of_cpu.mem_read.part1
> found event unc_iio_data_req_of_cpu.mem_read.part2
> found event unc_iio_data_req_of_cpu.mem_read.part3
> metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
> found event unc_iio_data_req_of_cpu.mem_read.part0
> found event unc_iio_data_req_of_cpu.mem_read.part1
> found event unc_iio_data_req_of_cpu.mem_read.part2
> found event unc_iio_data_req_of_cpu.mem_read.part3
> adding {unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W,{unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W
> intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
> Attempting to add event pmu 'uncore_iio_free_running_5' with 'unc_iio_data_req_of_cpu.mem_read.part0,' that may result in non-fatal errors
> After aliases, add event pmu 'uncore_iio_free_running_5' with 'fc_mask,ch_mask,umask,event,' that may result in non-fatal errors
> Attempting to add event pmu 'uncore_iio_free_running_3' with 'unc_iio_data_req_of_cpu.mem_read.part0,' that may result in non-fatal errors
> After aliases, add event pmu 'uncore_iio_free_running_3' with 'fc_mask,ch_mask,umask,event,' that may result in non-fatal errors
> Attempting to add event pmu 'uncore_iio_free_running_1' with 'unc_iio_data_req_of_cpu.mem_read.part0,' that may result in non-fatal errors
> After aliases, add event pmu 'uncore_iio_free_running_1' with 'fc_mask,ch_mask,umask,event,' that may result in non-fatal errors
> Multiple errors dropping message: unknown term 'fc_mask' for pmu 'uncore_iio_free_running_3' (valid terms: event,umask,config,config1,config2,name,period,percore)
> ...
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/arch/x86/util/intel-pt.c | 32 +++++++++++++++++-----------
> tools/perf/tests/pmu.c | 4 ++--
> tools/perf/util/parse-events.c | 29 ++++++++++++++++++++++++-
> tools/perf/util/pmu.c | 33 ++++++++++++++++++-----------
> tools/perf/util/pmu.h | 2 +-
> 5 files changed, 72 insertions(+), 28 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c
> index fd9e22d1e366..0fe401ad3347 100644
> --- a/tools/perf/arch/x86/util/intel-pt.c
> +++ b/tools/perf/arch/x86/util/intel-pt.c
> @@ -59,7 +59,8 @@ struct intel_pt_recording {
> size_t priv_size;
> };
>
> -static int intel_pt_parse_terms_with_default(struct list_head *formats,
> +static int intel_pt_parse_terms_with_default(const char *pmu_name,
> + struct list_head *formats,
> const char *str,
> u64 *config)
> {
> @@ -78,7 +79,8 @@ static int intel_pt_parse_terms_with_default(struct list_head *formats,
> goto out_free;
>
> attr.config = *config;
> - err = perf_pmu__config_terms(formats, &attr, terms, true, NULL);
> + err = perf_pmu__config_terms(pmu_name, formats, &attr, terms, true,
> + NULL);
> if (err)
> goto out_free;
>
> @@ -88,11 +90,12 @@ static int intel_pt_parse_terms_with_default(struct list_head *formats,
> return err;
> }
>
> -static int intel_pt_parse_terms(struct list_head *formats, const char *str,
> - u64 *config)
> +static int intel_pt_parse_terms(const char *pmu_name, struct list_head *formats,
> + const char *str, u64 *config)
> {
> *config = 0;
> - return intel_pt_parse_terms_with_default(formats, str, config);
> + return intel_pt_parse_terms_with_default(pmu_name, formats, str,
> + config);
> }
>
> static u64 intel_pt_masked_bits(u64 mask, u64 bits)
> @@ -229,7 +232,8 @@ static u64 intel_pt_default_config(struct perf_pmu *intel_pt_pmu)
>
> pr_debug2("%s default config: %s\n", intel_pt_pmu->name, buf);
>
> - intel_pt_parse_terms(&intel_pt_pmu->format, buf, &config);
> + intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format, buf,
> + &config);
>
> return config;
> }
> @@ -337,13 +341,16 @@ static int intel_pt_info_fill(struct auxtrace_record *itr,
> if (priv_size != ptr->priv_size)
> return -EINVAL;
>
> - intel_pt_parse_terms(&intel_pt_pmu->format, "tsc", &tsc_bit);
> - intel_pt_parse_terms(&intel_pt_pmu->format, "noretcomp",
> - &noretcomp_bit);
> - intel_pt_parse_terms(&intel_pt_pmu->format, "mtc", &mtc_bit);
> + intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
> + "tsc", &tsc_bit);
> + intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
> + "noretcomp", &noretcomp_bit);
> + intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
> + "mtc", &mtc_bit);
> mtc_freq_bits = perf_pmu__format_bits(&intel_pt_pmu->format,
> "mtc_period");
> - intel_pt_parse_terms(&intel_pt_pmu->format, "cyc", &cyc_bit);
> + intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
> + "cyc", &cyc_bit);
>
> intel_pt_tsc_ctc_ratio(&tsc_ctc_ratio_n, &tsc_ctc_ratio_d);
>
> @@ -768,7 +775,8 @@ static int intel_pt_recording_options(struct auxtrace_record *itr,
> }
> }
>
> - intel_pt_parse_terms(&intel_pt_pmu->format, "tsc", &tsc_bit);
> + intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
> + "tsc", &tsc_bit);
>
> if (opts->full_auxtrace && (intel_pt_evsel->core.attr.config & tsc_bit))
> have_timing_info = true;
> diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
> index 74379ff1f7fa..5c11fe2b3040 100644
> --- a/tools/perf/tests/pmu.c
> +++ b/tools/perf/tests/pmu.c
> @@ -156,8 +156,8 @@ int test__pmu(struct test *test __maybe_unused, int subtest __maybe_unused)
> if (ret)
> break;
>
> - ret = perf_pmu__config_terms(&formats, &attr, terms,
> - false, NULL);
> + ret = perf_pmu__config_terms("perf-pmu-test", &formats, &attr,
> + terms, false, NULL);
> if (ret)
> break;
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index e9464b04f149..0ebc0fd9385a 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -204,7 +204,8 @@ void parse_events__handle_error(struct parse_events_error *err, int idx,
> err->help = help;
> break;
> default:
> - WARN_ONCE(1, "WARNING: multiple event parsing errors\n");
> + pr_debug("Multiple errors dropping message: %s (%s)\n",
> + err->str, err->help);
> free(err->str);
> err->str = str;
> free(err->help);
> @@ -1422,6 +1423,19 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> bool use_uncore_alias;
> LIST_HEAD(config_terms);
>
> + if (verbose > 1) {
> + fprintf(stderr, "Attempting to add event pmu '%s' with '",
> + name);
> + if (head_config) {
> + struct parse_events_term *term;
> +
> + list_for_each_entry(term, head_config, list) {
> + fprintf(stderr, "%s,", term->config);
> + }
> + }
> + fprintf(stderr, "' that may result in non-fatal errors\n");
> + }
> +
> pmu = perf_pmu__find(name);
> if (!pmu) {
> char *err_str;
> @@ -1458,6 +1472,19 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> if (perf_pmu__check_alias(pmu, head_config, &info))
> return -EINVAL;
>
> + if (verbose > 1) {
> + fprintf(stderr, "After aliases, add event pmu '%s' with '",
> + name);
> + if (head_config) {
> + struct parse_events_term *term;
> +
> + list_for_each_entry(term, head_config, list) {
> + fprintf(stderr, "%s,", term->config);
> + }
> + }
> + fprintf(stderr, "' that may result in non-fatal errors\n");
> + }
> +
> /*
> * Configure hardcoded terms first, no need to check
> * return value when called with fail == 0 ;)
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 92bd7fafcce6..71d0290b616a 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1056,7 +1056,8 @@ static char *pmu_formats_string(struct list_head *formats)
> * Setup one of config[12] attr members based on the
> * user input data - term parameter.
> */
> -static int pmu_config_term(struct list_head *formats,
> +static int pmu_config_term(const char *pmu_name,
> + struct list_head *formats,
> struct perf_event_attr *attr,
> struct parse_events_term *term,
> struct list_head *head_terms,
> @@ -1082,16 +1083,24 @@ static int pmu_config_term(struct list_head *formats,
>
> format = pmu_find_format(formats, term->config);
> if (!format) {
> - if (verbose > 0)
> - printf("Invalid event/parameter '%s'\n", term->config);
> + char *pmu_term = pmu_formats_string(formats);
> + char *unknown_term;
> + char *help_msg;
> +
> + if (asprintf(&unknown_term,
> + "unknown term '%s' for pmu '%s'",
> + term->config, pmu_name) < 0)
> + unknown_term = strdup("unknown term");
> + help_msg = parse_events_formats_error_string(pmu_term);
> if (err) {
> - char *pmu_term = pmu_formats_string(formats);
> -
> parse_events__handle_error(err, term->err_term,
> - strdup("unknown term"),
> - parse_events_formats_error_string(pmu_term));
> - free(pmu_term);
> + unknown_term,
> + help_msg);
> + } else {
> + pr_debug("%s (%s)\n", unknown_term, help_msg);
> + free(unknown_term);
> }
> + free(pmu_term);
> return -EINVAL;
> }
>
> @@ -1168,7 +1177,7 @@ static int pmu_config_term(struct list_head *formats,
> return 0;
> }
>
> -int perf_pmu__config_terms(struct list_head *formats,
> +int perf_pmu__config_terms(const char *pmu_name, struct list_head *formats,
> struct perf_event_attr *attr,
> struct list_head *head_terms,
> bool zero, struct parse_events_error *err)
> @@ -1176,7 +1185,7 @@ int perf_pmu__config_terms(struct list_head *formats,
> struct parse_events_term *term;
>
> list_for_each_entry(term, head_terms, list) {
> - if (pmu_config_term(formats, attr, term, head_terms,
> + if (pmu_config_term(pmu_name, formats, attr, term, head_terms,
> zero, err))
> return -EINVAL;
> }
> @@ -1196,8 +1205,8 @@ int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
> bool zero = !!pmu->default_config;
>
> attr->type = pmu->type;
> - return perf_pmu__config_terms(&pmu->format, attr, head_terms,
> - zero, err);
> + return perf_pmu__config_terms(pmu->name, &pmu->format, attr,
> + head_terms, zero, err);
> }
>
> static struct perf_pmu_alias *pmu_find_alias(struct perf_pmu *pmu,
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index e119333e93ba..85e0c7f2515c 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -76,7 +76,7 @@ struct perf_pmu *perf_pmu__find_by_type(unsigned int type);
> int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
> struct list_head *head_terms,
> struct parse_events_error *error);
> -int perf_pmu__config_terms(struct list_head *formats,
> +int perf_pmu__config_terms(const char *pmu_name, struct list_head *formats,
> struct perf_event_attr *attr,
> struct list_head *head_terms,
> bool zero, struct parse_events_error *error);
> --
> 2.26.2.645.ge9eca65c58-goog
>
On Thu, May 07, 2020 at 10:36:27PM -0700, Ian Rogers wrote:
> 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.
I'm not sure if size is that great an heuristic. The dedup algorithm should
work in any case even if you don't order by size, right?
I suppose in theory some kind of topological sort would be better.
-Andi
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> 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 2a6456fa178b..69fbff47089f 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -520,7 +520,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.645.ge9eca65c58-goog
>
> 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);
BTW standard perf data structure would be a rblist or strlist
I think it would be really better to do the deduping in a separate
pass than trying to add it to find_evsel_group. This leads
to very complicated logic.
This will likely make it easier to implement more sophisticated
algorithms too.
-Andi
On Fri, May 8, 2020 at 5:25 PM Andi Kleen <[email protected]> wrote:
>
> On Thu, May 07, 2020 at 10:36:27PM -0700, Ian Rogers wrote:
> > 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.
>
> I'm not sure if size is that great an heuristic. The dedup algorithm should
> work in any case even if you don't order by size, right?
Consider two metrics:
- metric 1 with events {A,B}
- metric 2 with events {A,B,C,D}
If the list isn't sorted then as the matching takes the first group
with all the events, metric 1 will match {A,B} and metric 2 {A,B,C,D}.
If the order is sorted to {A,B,C,D},{A,B} then metric 1 matches within
the {A,B,C,D} group as does metric 2. The events in metric 1 aren't
used and are removed.
The dedup algorithm is very naive :-)
> I suppose in theory some kind of topological sort would be better.
We could build something more complex, such as a directed acyclic
graph where metrics with a subset of events are children of parent
metrics. Children could have >1 parent for example
{A,B,C,D},{A,B,E},{A,B} where {A,B} is a subset of both {A,B,C,D} and
{A,B,E} and so a child of both. Presumably in that case it'd be better
to match {A,B} with {A,B,E} to reduce multiplexing. As we're merging
smaller groups into bigger, the sorting of the list is a quick and
dirty approximation of this.
Thanks,
Ian
> -Andi
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > 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 2a6456fa178b..69fbff47089f 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -520,7 +520,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.645.ge9eca65c58-goog
> >
On Thu, May 07, 2020 at 10:36:21PM -0700, Ian Rogers wrote:
> If allocated, perf_pkg_mask and metric_events need freeing.
All these patches at the beginning look like straight forward
bug fixes and are really independent of the new features.
For them
Reviewed-by: Andi Kleen <[email protected]>
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/evsel.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 28683b0eb738..05bb46baad6a 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1263,6 +1263,8 @@ void evsel__exit(struct evsel *evsel)
> zfree(&evsel->group_name);
> zfree(&evsel->name);
> zfree(&evsel->pmu_name);
> + zfree(&evsel->per_pkg_mask);
> + zfree(&evsel->metric_events);
> perf_evsel__object.fini(evsel);
> }
>
> --
> 2.26.2.645.ge9eca65c58-goog
>
On Fri, May 8, 2020 at 5:39 PM Andi Kleen <[email protected]> wrote:
>
> On Thu, May 07, 2020 at 10:36:21PM -0700, Ian Rogers wrote:
> > If allocated, perf_pkg_mask and metric_events need freeing.
>
> All these patches at the beginning look like straight forward
> bug fixes and are really independent of the new features.
Thanks, for context I added them after v1 to make it easier to apply
the patches.
Ian
> For them
>
> Reviewed-by: Andi Kleen <[email protected]>
>
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/util/evsel.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index 28683b0eb738..05bb46baad6a 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -1263,6 +1263,8 @@ void evsel__exit(struct evsel *evsel)
> > zfree(&evsel->group_name);
> > zfree(&evsel->name);
> > zfree(&evsel->pmu_name);
> > + zfree(&evsel->per_pkg_mask);
> > + zfree(&evsel->metric_events);
> > perf_evsel__object.fini(evsel);
> > }
> >
> > --
> > 2.26.2.645.ge9eca65c58-goog
> >
> > I'm not sure if size is that great an heuristic. The dedup algorithm should
> > work in any case even if you don't order by size, right?
>
> Consider two metrics:
> - metric 1 with events {A,B}
> - metric 2 with events {A,B,C,D}
> If the list isn't sorted then as the matching takes the first group
> with all the events, metric 1 will match {A,B} and metric 2 {A,B,C,D}.
> If the order is sorted to {A,B,C,D},{A,B} then metric 1 matches within
> the {A,B,C,D} group as does metric 2. The events in metric 1 aren't
> used and are removed.
Ok. It's better for the longer metric if they stay together.
>
> The dedup algorithm is very naive :-)
I guess what matters is that it gives reasonable results on the current
metrics. I assume it does?
How much deduping is happening if you run all metrics?
For toplev on my long term todo list was to compare it against
a hopefully better schedule generated by or-tools, but I never
got around to coding that up.
-Andi
On Fri, May 8, 2020 at 7:40 PM Andi Kleen <[email protected]> wrote:
>
> > > I'm not sure if size is that great an heuristic. The dedup algorithm should
> > > work in any case even if you don't order by size, right?
> >
> > Consider two metrics:
> > - metric 1 with events {A,B}
> > - metric 2 with events {A,B,C,D}
> > If the list isn't sorted then as the matching takes the first group
> > with all the events, metric 1 will match {A,B} and metric 2 {A,B,C,D}.
> > If the order is sorted to {A,B,C,D},{A,B} then metric 1 matches within
> > the {A,B,C,D} group as does metric 2. The events in metric 1 aren't
> > used and are removed.
>
> Ok. It's better for the longer metric if they stay together.
>
> >
> > The dedup algorithm is very naive :-)
>
> I guess what matters is that it gives reasonable results on the current
> metrics. I assume it does?
>
> How much deduping is happening if you run all metrics?
Hi Andi,
I included this data in the latest cover-letter:
https://lore.kernel.org/lkml/[email protected]/
> For toplev on my long term todo list was to compare it against
> a hopefully better schedule generated by or-tools, but I never
> got around to coding that up.
>
> -Andi
Agreed, and this relates to your comments about doing the algorithm as
a separate pass outside of find_evsel_group. For that, I don't
disagree but would like to land what we have and then follow up with
improvements.
Thanks,
Ian
Em Thu, May 07, 2020 at 10:36:24PM -0700, Ian Rogers escreveu:
> Avoid a simple memory leak.
Thanks, applied.
- Arnaldo
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/metricgroup.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 4f7e36bc49d9..7e1725d61c39 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -186,6 +186,7 @@ static int metricgroup__setup_events(struct list_head *groups,
> if (!evsel) {
> pr_debug("Cannot resolve %s: %s\n",
> eg->metric_name, eg->metric_expr);
> + free(metric_events);
> continue;
> }
> for (i = 0; metric_events[i]; i++)
> @@ -193,11 +194,13 @@ static int metricgroup__setup_events(struct list_head *groups,
> me = metricgroup__lookup(metric_events_list, evsel, true);
> if (!me) {
> ret = -ENOMEM;
> + free(metric_events);
> break;
> }
> expr = malloc(sizeof(struct metric_expr));
> if (!expr) {
> ret = -ENOMEM;
> + free(metric_events);
> break;
> }
> expr->metric_expr = eg->metric_expr;
> --
> 2.26.2.645.ge9eca65c58-goog
>
--
- Arnaldo