Add a test that all perf metrics (for your architecture) are parsable
with the simple expression parser. Attempt to parse all events in
metrics but only fail if the metric is for the current CPU. Fix bugs
in the expr parser, x86 and powerpc metrics. Improve debug messages
around add PMU config term failures.
v4 rebases after Kajol Jain's patches and fixes an asprintf warning.
v3 adds parse event testing of ids and improves debug messages for add
PMU. These messages are paticular visible with 'perf test 10
-vvv'. It moves the testing logic from tests/expr.c to
tests/pmu-events.c as suggested by John Garry
<[email protected]>.
v2 adds Fixes tags to commit messages for when broken metrics were
first added. Adds a debug warning for division by zero in expr, and
adds a workaround for id values in the expr test necessary for
powerpc. It also fixes broken power8 and power9 metrics.
Ian Rogers (12):
perf expr: unlimited escaped characters in a symbol
perf metrics: fix parse errors in cascade lake metrics
perf metrics: fix parse errors in skylake metrics
perf expr: allow ',' to be an other token
perf expr: increase max other
perf expr: parse numbers as doubles
perf expr: debug lex if debugging yacc
perf metrics: fix parse errors in power8 metrics
perf metrics: fix parse errors in power9 metrics
perf expr: print a debug message for division by zero
perf parse-events: expand add PMU error/verbose messages
perf test: improve pmu event metric testing
tools/perf/arch/x86/util/intel-pt.c | 32 ++--
.../arch/powerpc/power8/metrics.json | 2 +-
.../arch/powerpc/power9/metrics.json | 2 +-
.../arch/x86/cascadelakex/clx-metrics.json | 10 +-
.../arch/x86/skylakex/skx-metrics.json | 4 +-
tools/perf/tests/builtin-test.c | 5 +
tools/perf/tests/expr.c | 1 +
tools/perf/tests/pmu-events.c | 156 +++++++++++++++++-
tools/perf/tests/pmu.c | 4 +-
tools/perf/tests/tests.h | 2 +
tools/perf/util/expr.c | 1 +
tools/perf/util/expr.h | 2 +-
tools/perf/util/expr.l | 16 +-
tools/perf/util/expr.y | 16 +-
tools/perf/util/parse-events.c | 29 +++-
tools/perf/util/pmu.c | 33 ++--
tools/perf/util/pmu.h | 2 +-
17 files changed, 262 insertions(+), 55 deletions(-)
--
2.26.2.526.g744177e7f7-goog
Mismatched parentheses.
Fixes: dd81eafacc52 (perf vendor events power8: Cpi_breakdown & estimated_dcache_miss_cpi metrics)
Reviewed-by: Paul A. Clarke <[email protected]>
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/pmu-events/arch/powerpc/power8/metrics.json | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/pmu-events/arch/powerpc/power8/metrics.json b/tools/perf/pmu-events/arch/powerpc/power8/metrics.json
index bffb2d4a6420..fc4aa6c2ddc9 100644
--- a/tools/perf/pmu-events/arch/powerpc/power8/metrics.json
+++ b/tools/perf/pmu-events/arch/powerpc/power8/metrics.json
@@ -169,7 +169,7 @@
},
{
"BriefDescription": "Cycles GCT empty where dispatch was held",
- "MetricExpr": "(PM_GCT_NOSLOT_DISP_HELD_MAP + PM_GCT_NOSLOT_DISP_HELD_SRQ + PM_GCT_NOSLOT_DISP_HELD_ISSQ + PM_GCT_NOSLOT_DISP_HELD_OTHER) / PM_RUN_INST_CMPL)",
+ "MetricExpr": "(PM_GCT_NOSLOT_DISP_HELD_MAP + PM_GCT_NOSLOT_DISP_HELD_SRQ + PM_GCT_NOSLOT_DISP_HELD_ISSQ + PM_GCT_NOSLOT_DISP_HELD_OTHER) / PM_RUN_INST_CMPL",
"MetricGroup": "cpi_breakdown",
"MetricName": "gct_empty_disp_held_cpi"
},
--
2.26.2.526.g744177e7f7-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 | 156 ++++++++++++++++++++++++++++++--
tools/perf/tests/tests.h | 2 +
4 files changed, 158 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..5ab1809b741b 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,146 @@ 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) {
+ pr_debug("Parse event failed: id '%s' metric '%s' expr '%s'\n",
+ id, pe->metric_name, pe->metric_expr);
+ pr_debug("Error string '%s' help '%s'\n",
+ error.str, error.help);
+ } else if (ret) {
+ pr_debug("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);
+ return same_cpu ? ret : 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.526.g744177e7f7-goog
If an expression yields 0 and is then divided-by/modulus-by then the
parsing aborts. Add a debug error message to better enable debugging
when this happens.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/expr.y | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 54260094b947..21e82a1e11a2 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -103,8 +103,18 @@ expr: NUMBER
| expr '+' expr { $$ = $1 + $3; }
| expr '-' expr { $$ = $1 - $3; }
| expr '*' expr { $$ = $1 * $3; }
- | expr '/' expr { if ($3 == 0) YYABORT; $$ = $1 / $3; }
- | expr '%' expr { if ((long)$3 == 0) YYABORT; $$ = (long)$1 % (long)$3; }
+ | expr '/' expr { if ($3 == 0) {
+ pr_debug("division by zero\n");
+ YYABORT;
+ }
+ $$ = $1 / $3;
+ }
+ | expr '%' expr { if ((long)$3 == 0) {
+ pr_debug("division by zero\n");
+ YYABORT;
+ }
+ $$ = (long)$1 % (long)$3;
+ }
| '-' expr %prec NEG { $$ = -$2; }
| '(' if_expr ')' { $$ = $2; }
| MIN '(' expr ',' expr ')' { $$ = $3 < $5 ? $3 : $5; }
--
2.26.2.526.g744177e7f7-goog
Mismatched parentheses.
Fixes: 7f3cf5ac7743 (perf vendor events power9: Cpi_breakdown & estimated_dcache_miss_cpi metrics)
Reviewed-by: Paul A. Clarke <[email protected]>
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/pmu-events/arch/powerpc/power9/metrics.json | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/pmu-events/arch/powerpc/power9/metrics.json b/tools/perf/pmu-events/arch/powerpc/power9/metrics.json
index 811c2a8c1c9e..f427436f2c0a 100644
--- a/tools/perf/pmu-events/arch/powerpc/power9/metrics.json
+++ b/tools/perf/pmu-events/arch/powerpc/power9/metrics.json
@@ -362,7 +362,7 @@
},
{
"BriefDescription": "Completion stall for other reasons",
- "MetricExpr": "PM_CMPLU_STALL - PM_CMPLU_STALL_NTC_DISP_FIN - PM_CMPLU_STALL_NTC_FLUSH - PM_CMPLU_STALL_LSU - PM_CMPLU_STALL_EXEC_UNIT - PM_CMPLU_STALL_BRU)/PM_RUN_INST_CMPL",
+ "MetricExpr": "(PM_CMPLU_STALL - PM_CMPLU_STALL_NTC_DISP_FIN - PM_CMPLU_STALL_NTC_FLUSH - PM_CMPLU_STALL_LSU - PM_CMPLU_STALL_EXEC_UNIT - PM_CMPLU_STALL_BRU)/PM_RUN_INST_CMPL",
"MetricGroup": "cpi_breakdown",
"MetricName": "other_stall_cpi"
},
--
2.26.2.526.g744177e7f7-goog
Remove over escaping with \\.
Fixes: fd5500989c8f (perf vendor events intel: Update metrics from TMAM 3.5)
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/pmu-events/arch/x86/skylakex/skx-metrics.json | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/pmu-events/arch/x86/skylakex/skx-metrics.json b/tools/perf/pmu-events/arch/x86/skylakex/skx-metrics.json
index b4f91137f40c..390bdab1be9d 100644
--- a/tools/perf/pmu-events/arch/x86/skylakex/skx-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/skylakex/skx-metrics.json
@@ -328,13 +328,13 @@
},
{
"BriefDescription": "Average latency of data read request to external memory (in nanoseconds). Accounts for demand loads and L1/L2 prefetches",
- "MetricExpr": "1000000000 * ( cha@event\\=0x36\\\\\\,umask\\=0x21@ / cha@event\\=0x35\\\\\\,umask\\=0x21@ ) / ( cha_0@event\\=0x0@ / duration_time )",
+ "MetricExpr": "1000000000 * ( cha@event\\=0x36\\,umask\\=0x21@ / cha@event\\=0x35\\,umask\\=0x21@ ) / ( cha_0@event\\=0x0@ / duration_time )",
"MetricGroup": "Memory_Lat",
"MetricName": "DRAM_Read_Latency"
},
{
"BriefDescription": "Average number of parallel data read requests to external memory. Accounts for demand loads and L1/L2 prefetches",
- "MetricExpr": "cha@event\\=0x36\\\\\\,umask\\=0x21@ / cha@event\\=0x36\\\\\\,umask\\=0x21\\\\\\,thresh\\=1@",
+ "MetricExpr": "cha@event\\=0x36\\,umask\\=0x21@ / cha@event\\=0x36\\,umask\\=0x21\\,thresh\\=1@",
"MetricGroup": "Memory_BW",
"MetricName": "DRAM_Parallel_Reads"
},
--
2.26.2.526.g744177e7f7-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 a929a936d49b..a3bfa087833c 100644
--- a/tools/perf/arch/x86/util/intel-pt.c
+++ b/tools/perf/arch/x86/util/intel-pt.c
@@ -58,7 +58,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)
{
@@ -77,7 +78,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;
@@ -87,11 +89,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)
@@ -228,7 +231,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;
}
@@ -336,13 +340,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 b7a0518d607d..17c42de24e8e 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);
@@ -1424,6 +1425,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;
@@ -1460,6 +1474,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 cb6fbec50313..cd85689977b4 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.526.g744177e7f7-goog
This is expected in expr.y and metrics use floating point values such as
x86 broadwell IFetch_Line_Utilization.
Fixes: 26226a97724d (perf expr: Move expr lexer to flex)
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/expr.l | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
index 73db6a9ef97e..ceab11bea6f9 100644
--- a/tools/perf/util/expr.l
+++ b/tools/perf/util/expr.l
@@ -10,12 +10,12 @@
char *expr_get_text(yyscan_t yyscanner);
YYSTYPE *expr_get_lval(yyscan_t yyscanner);
-static int __value(YYSTYPE *yylval, char *str, int base, int token)
+static double __value(YYSTYPE *yylval, char *str, int token)
{
- u64 num;
+ double num;
errno = 0;
- num = strtoull(str, NULL, base);
+ num = strtod(str, NULL);
if (errno)
return EXPR_ERROR;
@@ -23,12 +23,12 @@ static int __value(YYSTYPE *yylval, char *str, int base, int token)
return token;
}
-static int value(yyscan_t scanner, int base)
+static int value(yyscan_t scanner)
{
YYSTYPE *yylval = expr_get_lval(scanner);
char *text = expr_get_text(scanner);
- return __value(yylval, text, base, NUMBER);
+ return __value(yylval, text, NUMBER);
}
/*
@@ -81,7 +81,7 @@ static int str(yyscan_t scanner, int token, int runtime)
}
%}
-number [0-9]+
+number [0-9]*\.?[0-9]+
sch [-,=]
spec \\{sch}
@@ -105,7 +105,7 @@ min { return MIN; }
if { return IF; }
else { return ELSE; }
#smt_on { return SMT_ON; }
-{number} { return value(yyscanner, 10); }
+{number} { return value(yyscanner); }
{symbol} { return str(yyscanner, ID, sctx->runtime); }
"|" { return '|'; }
"^" { return '^'; }
--
2.26.2.526.g744177e7f7-goog
On Sun, May 03, 2020 at 08:26:22AM -0700, Ian Rogers wrote:
> On Sun, May 3, 2020 at 7:56 AM Jiri Olsa <[email protected]> wrote:
> >
> > On Fri, May 01, 2020 at 10:33:33AM -0700, Ian Rogers wrote:
> > > 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.
> >
> > yep, failing for me (-vvv output below).. could you plz
> > detect that and skip the test ?
>
> Thanks, filtering the verbose output we have just 1 parse event failure:
>
> Parse event failed: id 'arb/event=0x80,umask=0x2,thresh=1/' metric
> 'DRAM_Parallel_Reads' expr 'arb@event\=0x80\,umask\=0x2@ /
> arb@event\=0x80\,umask\=0x2\,thresh\=1@'
> Error string 'unknown term 'thresh' for pmu 'uncore_arb'' help 'valid
> terms: event,edge,inv,umask,cmask,config,config1,config2,name,period,freq,branch_type,time,call-graph,stack-size,no-inherit,inherit,max-stack,nr,no-overwrite,overwrite,driver-config,percore,aux-output,aux-sample-size'
>
> This looks like a bug in skl-metrics.json:
>
> {
> "BriefDescription": "Average number of parallel data read
> requests to external memory. Accounts for demand loads and L1/L2
> prefetches",
> "MetricExpr": "arb@event\\=0x80\\,umask\\=0x2@ /
> arb@event\\=0x80\\,umask\\=0x2\\,thresh\\=1@",
> "MetricGroup": "Memory_BW",
> "MetricName": "DRAM_Parallel_Reads"
> },
>
> which can be fixed by removing "\\,thresh\\=1" but looking at the
> expression this will just make the expression yield a value of 1. As
> this is an Intel json file could they comment? Jiri, could you be
> missing a patch on the kernel side? We could lower this failure to
> just a diagnostic message to land this set of patches, let me know
> what you'd like me to do.
I applied this on current Arnaldo's perf/core.. not sure there's
more pending changes out there
I'd like not to delay this patchset too long.. could we push the
first 10 patches and solve the rest in separate change?
thanks,
jirka
On Sun, May 3, 2020 at 10:06 AM Jiri Olsa <[email protected]> wrote:
>
> On Sun, May 03, 2020 at 08:26:22AM -0700, Ian Rogers wrote:
> > On Sun, May 3, 2020 at 7:56 AM Jiri Olsa <[email protected]> wrote:
> > >
> > > On Fri, May 01, 2020 at 10:33:33AM -0700, Ian Rogers wrote:
> > > > 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.
> > >
> > > yep, failing for me (-vvv output below).. could you plz
> > > detect that and skip the test ?
> >
> > Thanks, filtering the verbose output we have just 1 parse event failure:
> >
> > Parse event failed: id 'arb/event=0x80,umask=0x2,thresh=1/' metric
> > 'DRAM_Parallel_Reads' expr 'arb@event\=0x80\,umask\=0x2@ /
> > arb@event\=0x80\,umask\=0x2\,thresh\=1@'
> > Error string 'unknown term 'thresh' for pmu 'uncore_arb'' help 'valid
> > terms: event,edge,inv,umask,cmask,config,config1,config2,name,period,freq,branch_type,time,call-graph,stack-size,no-inherit,inherit,max-stack,nr,no-overwrite,overwrite,driver-config,percore,aux-output,aux-sample-size'
> >
> > This looks like a bug in skl-metrics.json:
> >
> > {
> > "BriefDescription": "Average number of parallel data read
> > requests to external memory. Accounts for demand loads and L1/L2
> > prefetches",
> > "MetricExpr": "arb@event\\=0x80\\,umask\\=0x2@ /
> > arb@event\\=0x80\\,umask\\=0x2\\,thresh\\=1@",
> > "MetricGroup": "Memory_BW",
> > "MetricName": "DRAM_Parallel_Reads"
> > },
> >
> > which can be fixed by removing "\\,thresh\\=1" but looking at the
> > expression this will just make the expression yield a value of 1. As
> > this is an Intel json file could they comment? Jiri, could you be
> > missing a patch on the kernel side? We could lower this failure to
> > just a diagnostic message to land this set of patches, let me know
> > what you'd like me to do.
>
> I applied this on current Arnaldo's perf/core.. not sure there's
> more pending changes out there
>
> I'd like not to delay this patchset too long.. could we push the
> first 10 patches and solve the rest in separate change?
Thanks, I've attached a patch that can be squashed into 12 to make the
error non-fatal. Patch 11 is trying to make the diagnostics around
adding a PMU event clearer and aside warning messages, and removal of,
has no functional effect. I don't mind the first 10 being merged and
these coming later. I don't mind just patch 11 coming later as it'd be
nice to have the test so metrics can get fixed.
Thanks,
Ian
> thanks,
> jirka
>
On Sun, May 03, 2020 at 10:31:37AM -0700, Ian Rogers wrote:
SNIP
> > >
> > > This looks like a bug in skl-metrics.json:
> > >
> > > {
> > > "BriefDescription": "Average number of parallel data read
> > > requests to external memory. Accounts for demand loads and L1/L2
> > > prefetches",
> > > "MetricExpr": "arb@event\\=0x80\\,umask\\=0x2@ /
> > > arb@event\\=0x80\\,umask\\=0x2\\,thresh\\=1@",
> > > "MetricGroup": "Memory_BW",
> > > "MetricName": "DRAM_Parallel_Reads"
> > > },
> > >
> > > which can be fixed by removing "\\,thresh\\=1" but looking at the
> > > expression this will just make the expression yield a value of 1. As
> > > this is an Intel json file could they comment? Jiri, could you be
> > > missing a patch on the kernel side? We could lower this failure to
> > > just a diagnostic message to land this set of patches, let me know
> > > what you'd like me to do.
> >
> > I applied this on current Arnaldo's perf/core.. not sure there's
> > more pending changes out there
> >
> > I'd like not to delay this patchset too long.. could we push the
> > first 10 patches and solve the rest in separate change?
>
> Thanks, I've attached a patch that can be squashed into 12 to make the
> error non-fatal. Patch 11 is trying to make the diagnostics around
> adding a PMU event clearer and aside warning messages, and removal of,
> has no functional effect. I don't mind the first 10 being merged and
> these coming later. I don't mind just patch 11 coming later as it'd be
> nice to have the test so metrics can get fixed.
sounds good, for patches 1 - 10:
Acked-by: Jiri Olsa <[email protected]>
thanks,
jirka
On 5/1/20 11:03 PM, Ian Rogers wrote:
> This is expected in expr.y and metrics use floating point values such as
> x86 broadwell IFetch_Line_Utilization.
>
> Fixes: 26226a97724d (perf expr: Move expr lexer to flex)
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/expr.l | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index 73db6a9ef97e..ceab11bea6f9 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -10,12 +10,12 @@
> char *expr_get_text(yyscan_t yyscanner);
> YYSTYPE *expr_get_lval(yyscan_t yyscanner);
>
> -static int __value(YYSTYPE *yylval, char *str, int base, int token)
> +static double __value(YYSTYPE *yylval, char *str, int token)
> {
> - u64 num;
> + double num;
>
> errno = 0;
> - num = strtoull(str, NULL, base);
> + num = strtod(str, NULL);
> if (errno)
> return EXPR_ERROR;
>
> @@ -23,12 +23,12 @@ static int __value(YYSTYPE *yylval, char *str, int base, int token)
> return token;
> }
>
> -static int value(yyscan_t scanner, int base)
> +static int value(yyscan_t scanner)
> {
> YYSTYPE *yylval = expr_get_lval(scanner);
> char *text = expr_get_text(scanner);
>
> - return __value(yylval, text, base, NUMBER);
> + return __value(yylval, text, NUMBER);
> }
>
> /*
> @@ -81,7 +81,7 @@ static int str(yyscan_t scanner, int token, int runtime)
> }
> %}
>
> -number [0-9]+
> +number [0-9]*\.?[0-9]+
>
Acked and Tested by: Kajol Jain <[email protected]>
> sch [-,=]
> spec \\{sch}
> @@ -105,7 +105,7 @@ min { return MIN; }
> if { return IF; }
> else { return ELSE; }
> #smt_on { return SMT_ON; }
> -{number} { return value(yyscanner, 10); }
> +{number} { return value(yyscanner); }
> {symbol} { return str(yyscanner, ID, sctx->runtime); }
> "|" { return '|'; }
> "^" { return '^'; }
>
On Fri, May 01, 2020 at 10:33:21AM -0700, Ian Rogers wrote:
> Add a test that all perf metrics (for your architecture) are parsable
> with the simple expression parser. Attempt to parse all events in
> metrics but only fail if the metric is for the current CPU. Fix bugs
> in the expr parser, x86 and powerpc metrics. Improve debug messages
> around add PMU config term failures.
>
> v4 rebases after Kajol Jain's patches and fixes an asprintf warning.
> v3 adds parse event testing of ids and improves debug messages for add
> PMU. These messages are paticular visible with 'perf test 10
> -vvv'. It moves the testing logic from tests/expr.c to
> tests/pmu-events.c as suggested by John Garry
> <[email protected]>.
> v2 adds Fixes tags to commit messages for when broken metrics were
> first added. Adds a debug warning for division by zero in expr, and
> adds a workaround for id values in the expr test necessary for
> powerpc. It also fixes broken power8 and power9 metrics.
>
> Ian Rogers (12):
> perf expr: unlimited escaped characters in a symbol
> perf metrics: fix parse errors in cascade lake metrics
> perf metrics: fix parse errors in skylake metrics
> perf expr: allow ',' to be an other token
> perf expr: increase max other
> perf expr: parse numbers as doubles
> perf expr: debug lex if debugging yacc
> perf metrics: fix parse errors in power8 metrics
> perf metrics: fix parse errors in power9 metrics
> perf expr: print a debug message for division by zero
heya,
could we please get the 1st 10 patches in? they are important,
and let's not block them with new versions for patches 11/12
Acked-by: Jiri Olsa <[email protected]>
thanks,
jirka
> perf parse-events: expand add PMU error/verbose messages
> perf test: improve pmu event metric testing
>
> tools/perf/arch/x86/util/intel-pt.c | 32 ++--
> .../arch/powerpc/power8/metrics.json | 2 +-
> .../arch/powerpc/power9/metrics.json | 2 +-
> .../arch/x86/cascadelakex/clx-metrics.json | 10 +-
> .../arch/x86/skylakex/skx-metrics.json | 4 +-
> tools/perf/tests/builtin-test.c | 5 +
> tools/perf/tests/expr.c | 1 +
> tools/perf/tests/pmu-events.c | 156 +++++++++++++++++-
> tools/perf/tests/pmu.c | 4 +-
> tools/perf/tests/tests.h | 2 +
> tools/perf/util/expr.c | 1 +
> tools/perf/util/expr.h | 2 +-
> tools/perf/util/expr.l | 16 +-
> tools/perf/util/expr.y | 16 +-
> tools/perf/util/parse-events.c | 29 +++-
> tools/perf/util/pmu.c | 33 ++--
> tools/perf/util/pmu.h | 2 +-
> 17 files changed, 262 insertions(+), 55 deletions(-)
>
> --
> 2.26.2.526.g744177e7f7-goog
>
Em Thu, May 07, 2020 at 10:44:45AM +0200, Jiri Olsa escreveu:
> On Fri, May 01, 2020 at 10:33:21AM -0700, Ian Rogers wrote:
> > Add a test that all perf metrics (for your architecture) are parsable
> > with the simple expression parser. Attempt to parse all events in
> > metrics but only fail if the metric is for the current CPU. Fix bugs
> > in the expr parser, x86 and powerpc metrics. Improve debug messages
> > around add PMU config term failures.
> >
> > v4 rebases after Kajol Jain's patches and fixes an asprintf warning.
> > v3 adds parse event testing of ids and improves debug messages for add
> > PMU. These messages are paticular visible with 'perf test 10
> > -vvv'. It moves the testing logic from tests/expr.c to
> > tests/pmu-events.c as suggested by John Garry
> > <[email protected]>.
> > v2 adds Fixes tags to commit messages for when broken metrics were
> > first added. Adds a debug warning for division by zero in expr, and
> > adds a workaround for id values in the expr test necessary for
> > powerpc. It also fixes broken power8 and power9 metrics.
> >
> > Ian Rogers (12):
> > perf expr: unlimited escaped characters in a symbol
> > perf metrics: fix parse errors in cascade lake metrics
> > perf metrics: fix parse errors in skylake metrics
> > perf expr: allow ',' to be an other token
> > perf expr: increase max other
> > perf expr: parse numbers as doubles
> > perf expr: debug lex if debugging yacc
> > perf metrics: fix parse errors in power8 metrics
> > perf metrics: fix parse errors in power9 metrics
> > perf expr: print a debug message for division by zero
>
> heya,
> could we please get the 1st 10 patches in? they are important,
Ok sir, processing these now,
- Arnaldo
> and let's not block them with new versions for patches 11/12
>
> Acked-by: Jiri Olsa <[email protected]>
Em Thu, May 07, 2020 at 10:44:45AM +0200, Jiri Olsa escreveu:
> On Fri, May 01, 2020 at 10:33:21AM -0700, Ian Rogers wrote:
> > Add a test that all perf metrics (for your architecture) are parsable
> > with the simple expression parser. Attempt to parse all events in
> > metrics but only fail if the metric is for the current CPU. Fix bugs
> > in the expr parser, x86 and powerpc metrics. Improve debug messages
> > around add PMU config term failures.
> >
> > v4 rebases after Kajol Jain's patches and fixes an asprintf warning.
> > v3 adds parse event testing of ids and improves debug messages for add
> > PMU. These messages are paticular visible with 'perf test 10
> > -vvv'. It moves the testing logic from tests/expr.c to
> > tests/pmu-events.c as suggested by John Garry
> > <[email protected]>.
> > v2 adds Fixes tags to commit messages for when broken metrics were
> > first added. Adds a debug warning for division by zero in expr, and
> > adds a workaround for id values in the expr test necessary for
> > powerpc. It also fixes broken power8 and power9 metrics.
> >
> > Ian Rogers (12):
> > perf expr: unlimited escaped characters in a symbol
> > perf metrics: fix parse errors in cascade lake metrics
> > perf metrics: fix parse errors in skylake metrics
> > perf expr: allow ',' to be an other token
> > perf expr: increase max other
> > perf expr: parse numbers as doubles
> > perf expr: debug lex if debugging yacc
> > perf metrics: fix parse errors in power8 metrics
> > perf metrics: fix parse errors in power9 metrics
> > perf expr: print a debug message for division by zero
>
> heya,
> could we please get the 1st 10 patches in? they are important,
> and let's not block them with new versions for patches 11/12
>
> Acked-by: Jiri Olsa <[email protected]>
The first ten patches are in, can we go with what Ian suggested for the
last two patches?
- Arnaldo
> thanks,
> jirka
>
> > perf parse-events: expand add PMU error/verbose messages
> > perf test: improve pmu event metric testing
> >
> > tools/perf/arch/x86/util/intel-pt.c | 32 ++--
> > .../arch/powerpc/power8/metrics.json | 2 +-
> > .../arch/powerpc/power9/metrics.json | 2 +-
> > .../arch/x86/cascadelakex/clx-metrics.json | 10 +-
> > .../arch/x86/skylakex/skx-metrics.json | 4 +-
> > tools/perf/tests/builtin-test.c | 5 +
> > tools/perf/tests/expr.c | 1 +
> > tools/perf/tests/pmu-events.c | 156 +++++++++++++++++-
> > tools/perf/tests/pmu.c | 4 +-
> > tools/perf/tests/tests.h | 2 +
> > tools/perf/util/expr.c | 1 +
> > tools/perf/util/expr.h | 2 +-
> > tools/perf/util/expr.l | 16 +-
> > tools/perf/util/expr.y | 16 +-
> > tools/perf/util/parse-events.c | 29 +++-
> > tools/perf/util/pmu.c | 33 ++--
> > tools/perf/util/pmu.h | 2 +-
> > 17 files changed, 262 insertions(+), 55 deletions(-)
> >
> > --
> > 2.26.2.526.g744177e7f7-goog
> >
>
--
- Arnaldo
On Mon, May 11, 2020 at 8:32 AM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
> Em Thu, May 07, 2020 at 10:44:45AM +0200, Jiri Olsa escreveu:
> > On Fri, May 01, 2020 at 10:33:21AM -0700, Ian Rogers wrote:
> > > Add a test that all perf metrics (for your architecture) are parsable
> > > with the simple expression parser. Attempt to parse all events in
> > > metrics but only fail if the metric is for the current CPU. Fix bugs
> > > in the expr parser, x86 and powerpc metrics. Improve debug messages
> > > around add PMU config term failures.
> > >
> > > v4 rebases after Kajol Jain's patches and fixes an asprintf warning.
> > > v3 adds parse event testing of ids and improves debug messages for add
> > > PMU. These messages are paticular visible with 'perf test 10
> > > -vvv'. It moves the testing logic from tests/expr.c to
> > > tests/pmu-events.c as suggested by John Garry
> > > <[email protected]>.
> > > v2 adds Fixes tags to commit messages for when broken metrics were
> > > first added. Adds a debug warning for division by zero in expr, and
> > > adds a workaround for id values in the expr test necessary for
> > > powerpc. It also fixes broken power8 and power9 metrics.
> > >
> > > Ian Rogers (12):
> > > perf expr: unlimited escaped characters in a symbol
> > > perf metrics: fix parse errors in cascade lake metrics
> > > perf metrics: fix parse errors in skylake metrics
> > > perf expr: allow ',' to be an other token
> > > perf expr: increase max other
> > > perf expr: parse numbers as doubles
> > > perf expr: debug lex if debugging yacc
> > > perf metrics: fix parse errors in power8 metrics
> > > perf metrics: fix parse errors in power9 metrics
> > > perf expr: print a debug message for division by zero
> >
> > heya,
> > could we please get the 1st 10 patches in? they are important,
> > and let's not block them with new versions for patches 11/12
> >
> > Acked-by: Jiri Olsa <[email protected]>
>
> The first ten patches are in, can we go with what Ian suggested for the
> last two patches?
It seems sad not to make this test fail for broken event encodings, as
perf test does a good job of swallowing the test's debug output
(fork...). I was thinking that I'll split out the test and various
fixes in:
https://lore.kernel.org/lkml/[email protected]/
The test will only do something if there is a MetricExpr for an
architecture. There are currently only MetricExpr for x86 and powerpc.
I've tested power 9 with the test, which tests the expressions for
power 8 but not the events, and everything passes. The area the test
can fail is x86 and I should be able to get reasonable coverage there
by grabbing cloud machines. When an x86 test fails it is hard to know
what to do. In Jiri's Skylake case naively 'fixing' the expression to
remove 'thresh=1' yields an expression that will only give the value
1. Perhaps a better 'fix' is just to remove the expressions in those
cases and wait for someone like Intel to refresh them? If that
expression will only work on a newer kernel then we need to identify a
constraint, etc. However, we couldn't see thresh as an event parameter
in a recent tree. Removing the expression seems like the most
expedient fix to get the test in, so that new expressions don't break
things :-)
Thanks Arnaldo for following up on this!
Ian
> - Arnaldo
>
> > thanks,
> > jirka
> >
> > > perf parse-events: expand add PMU error/verbose messages
> > > perf test: improve pmu event metric testing
> > >
> > > tools/perf/arch/x86/util/intel-pt.c | 32 ++--
> > > .../arch/powerpc/power8/metrics.json | 2 +-
> > > .../arch/powerpc/power9/metrics.json | 2 +-
> > > .../arch/x86/cascadelakex/clx-metrics.json | 10 +-
> > > .../arch/x86/skylakex/skx-metrics.json | 4 +-
> > > tools/perf/tests/builtin-test.c | 5 +
> > > tools/perf/tests/expr.c | 1 +
> > > tools/perf/tests/pmu-events.c | 156 +++++++++++++++++-
> > > tools/perf/tests/pmu.c | 4 +-
> > > tools/perf/tests/tests.h | 2 +
> > > tools/perf/util/expr.c | 1 +
> > > tools/perf/util/expr.h | 2 +-
> > > tools/perf/util/expr.l | 16 +-
> > > tools/perf/util/expr.y | 16 +-
> > > tools/perf/util/parse-events.c | 29 +++-
> > > tools/perf/util/pmu.c | 33 ++--
> > > tools/perf/util/pmu.h | 2 +-
> > > 17 files changed, 262 insertions(+), 55 deletions(-)
> > >
> > > --
> > > 2.26.2.526.g744177e7f7-goog
> > >
> >
>
> --
>
> - Arnaldo
On Mon, May 11, 2020 at 9:12 AM Ian Rogers <[email protected]> wrote:
>
> On Mon, May 11, 2020 at 8:32 AM Arnaldo Carvalho de Melo
> <[email protected]> wrote:
> >
> > Em Thu, May 07, 2020 at 10:44:45AM +0200, Jiri Olsa escreveu:
> > > On Fri, May 01, 2020 at 10:33:21AM -0700, Ian Rogers wrote:
> > > > Add a test that all perf metrics (for your architecture) are parsable
> > > > with the simple expression parser. Attempt to parse all events in
> > > > metrics but only fail if the metric is for the current CPU. Fix bugs
> > > > in the expr parser, x86 and powerpc metrics. Improve debug messages
> > > > around add PMU config term failures.
> > > >
> > > > v4 rebases after Kajol Jain's patches and fixes an asprintf warning.
> > > > v3 adds parse event testing of ids and improves debug messages for add
> > > > PMU. These messages are paticular visible with 'perf test 10
> > > > -vvv'. It moves the testing logic from tests/expr.c to
> > > > tests/pmu-events.c as suggested by John Garry
> > > > <[email protected]>.
> > > > v2 adds Fixes tags to commit messages for when broken metrics were
> > > > first added. Adds a debug warning for division by zero in expr, and
> > > > adds a workaround for id values in the expr test necessary for
> > > > powerpc. It also fixes broken power8 and power9 metrics.
> > > >
> > > > Ian Rogers (12):
> > > > perf expr: unlimited escaped characters in a symbol
> > > > perf metrics: fix parse errors in cascade lake metrics
> > > > perf metrics: fix parse errors in skylake metrics
> > > > perf expr: allow ',' to be an other token
> > > > perf expr: increase max other
> > > > perf expr: parse numbers as doubles
> > > > perf expr: debug lex if debugging yacc
> > > > perf metrics: fix parse errors in power8 metrics
> > > > perf metrics: fix parse errors in power9 metrics
> > > > perf expr: print a debug message for division by zero
> > >
> > > heya,
> > > could we please get the 1st 10 patches in? they are important,
> > > and let's not block them with new versions for patches 11/12
> > >
> > > Acked-by: Jiri Olsa <[email protected]>
> >
> > The first ten patches are in, can we go with what Ian suggested for the
> > last two patches?
To follow up, I've resent the last two patches here:
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/T/#t
The event parsing errors don't cause the test to fail in the latter.
Thanks,
Ian
> It seems sad not to make this test fail for broken event encodings, as
> perf test does a good job of swallowing the test's debug output
> (fork...). I was thinking that I'll split out the test and various
> fixes in:
> https://lore.kernel.org/lkml/[email protected]/
> The test will only do something if there is a MetricExpr for an
> architecture. There are currently only MetricExpr for x86 and powerpc.
> I've tested power 9 with the test, which tests the expressions for
> power 8 but not the events, and everything passes. The area the test
> can fail is x86 and I should be able to get reasonable coverage there
> by grabbing cloud machines. When an x86 test fails it is hard to know
> what to do. In Jiri's Skylake case naively 'fixing' the expression to
> remove 'thresh=1' yields an expression that will only give the value
> 1. Perhaps a better 'fix' is just to remove the expressions in those
> cases and wait for someone like Intel to refresh them? If that
> expression will only work on a newer kernel then we need to identify a
> constraint, etc. However, we couldn't see thresh as an event parameter
> in a recent tree. Removing the expression seems like the most
> expedient fix to get the test in, so that new expressions don't break
> things :-)
>
> Thanks Arnaldo for following up on this!
>
> Ian
>
> > - Arnaldo
> >
> > > thanks,
> > > jirka
> > >
> > > > perf parse-events: expand add PMU error/verbose messages
> > > > perf test: improve pmu event metric testing
> > > >
> > > > tools/perf/arch/x86/util/intel-pt.c | 32 ++--
> > > > .../arch/powerpc/power8/metrics.json | 2 +-
> > > > .../arch/powerpc/power9/metrics.json | 2 +-
> > > > .../arch/x86/cascadelakex/clx-metrics.json | 10 +-
> > > > .../arch/x86/skylakex/skx-metrics.json | 4 +-
> > > > tools/perf/tests/builtin-test.c | 5 +
> > > > tools/perf/tests/expr.c | 1 +
> > > > tools/perf/tests/pmu-events.c | 156 +++++++++++++++++-
> > > > tools/perf/tests/pmu.c | 4 +-
> > > > tools/perf/tests/tests.h | 2 +
> > > > tools/perf/util/expr.c | 1 +
> > > > tools/perf/util/expr.h | 2 +-
> > > > tools/perf/util/expr.l | 16 +-
> > > > tools/perf/util/expr.y | 16 +-
> > > > tools/perf/util/parse-events.c | 29 +++-
> > > > tools/perf/util/pmu.c | 33 ++--
> > > > tools/perf/util/pmu.h | 2 +-
> > > > 17 files changed, 262 insertions(+), 55 deletions(-)
> > > >
> > > > --
> > > > 2.26.2.526.g744177e7f7-goog
> > > >
> > >
> >
> > --
> >
> > - Arnaldo