Add a test that all perf metrics (for your architecture) are
parsable. Fix bugs in the expr parser and in x86 metrics. Untested on
architectures other than x86.
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 (11):
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 test: add expr test for pmu metrics
.../arch/powerpc/power8/metrics.json | 4 +-
.../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 | 96 ++++++++++++++++++-
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 +++-
11 files changed, 135 insertions(+), 23 deletions(-)
--
2.26.2.303.gf8c07b1a785-goog
Remove over escaping with \\.
Remove extraneous if 1 if 0 == 1 else 0 else 0.
Fixes: fd5500989c8f (perf vendor events intel: Update metrics from TMAM 3.5)
Signed-off-by: Ian Rogers <[email protected]>
---
.../pmu-events/arch/x86/cascadelakex/clx-metrics.json | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json b/tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json
index 7fde0d2943cd..d25eebce34c9 100644
--- a/tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json
@@ -328,31 +328,31 @@
},
{
"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"
},
{
"BriefDescription": "Average latency of data read request to external 3D X-Point memory [in nanoseconds]. Accounts for demand loads and L1/L2 data-read prefetches",
- "MetricExpr": "( 1000000000 * ( imc@event\\=0xe0\\\\\\,umask\\=0x1@ / imc@event\\=0xe3@ ) / imc_0@event\\=0x0@ ) if 1 if 0 == 1 else 0 else 0",
+ "MetricExpr": "( 1000000000 * ( imc@event\\=0xe0\\,umask\\=0x1@ / imc@event\\=0xe3@ ) / imc_0@event\\=0x0@ )",
"MetricGroup": "Memory_Lat",
"MetricName": "MEM_PMM_Read_Latency"
},
{
"BriefDescription": "Average 3DXP Memory Bandwidth Use for reads [GB / sec]",
- "MetricExpr": "( ( 64 * imc@event\\=0xe3@ / 1000000000 ) / duration_time ) if 1 if 0 == 1 else 0 else 0",
+ "MetricExpr": "( ( 64 * imc@event\\=0xe3@ / 1000000000 ) / duration_time )",
"MetricGroup": "Memory_BW",
"MetricName": "PMM_Read_BW"
},
{
"BriefDescription": "Average 3DXP Memory Bandwidth Use for Writes [GB / sec]",
- "MetricExpr": "( ( 64 * imc@event\\=0xe7@ / 1000000000 ) / duration_time ) if 1 if 0 == 1 else 0 else 0",
+ "MetricExpr": "( ( 64 * imc@event\\=0xe7@ / 1000000000 ) / duration_time )",
"MetricGroup": "Memory_BW",
"MetricName": "PMM_Write_BW"
},
--
2.26.2.303.gf8c07b1a785-goog
Corrects parse errors in expr__find_other of expressions with min.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/expr.y | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index cd17486c1c5d..54260094b947 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -80,7 +80,7 @@ other: ID
ctx->ids[ctx->num_ids++].name = $1;
}
|
-MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')'
+MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')' | ','
all_expr: if_expr { *final_val = $1; }
--
2.26.2.303.gf8c07b1a785-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 95bcf3629edf..0efda2ce2766 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);
}
/*
@@ -68,7 +68,7 @@ static int str(yyscan_t scanner, int token)
}
%}
-number [0-9]+
+number [0-9]*\.?[0-9]+
sch [-,=]
spec \\{sch}
@@ -92,7 +92,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); }
"|" { return '|'; }
"^" { return '^'; }
--
2.26.2.303.gf8c07b1a785-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.303.gf8c07b1a785-goog
Add basic floating point number test.
Verify that all pmu metrics, for the current architecture, parse.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/tests/builtin-test.c | 5 ++
tools/perf/tests/expr.c | 96 ++++++++++++++++++++++++++++++++-
tools/perf/tests/tests.h | 2 +
3 files changed, 102 insertions(+), 1 deletion(-)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index b6322eb0f423..28d547951f6b 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -63,6 +63,11 @@ static struct test generic_tests[] = {
{
.desc = "Simple expression parser",
.func = test__expr,
+ .subtest = {
+ .get_nr = test__expr_subtest_get_nr,
+ .get_desc = test__expr_subtest_get_desc,
+ },
+
},
{
.desc = "PERF_RECORD_* events & perf_sample fields",
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index ea10fc4412c4..35af232eb01d 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -1,9 +1,11 @@
// SPDX-License-Identifier: GPL-2.0
+#include "pmu-events/pmu-events.h"
#include "util/debug.h"
#include "util/expr.h"
#include "tests.h"
#include <stdlib.h>
#include <string.h>
+#include <linux/kernel.h>
#include <linux/zalloc.h>
static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
@@ -16,7 +18,7 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
return 0;
}
-int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
+static int parse_tests(void)
{
const char *p;
const char **other;
@@ -39,6 +41,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;
@@ -65,3 +68,94 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
return 0;
}
+
+static int pmu_tests(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;
+
+ 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) {
+ 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);
+
+ if (expr__parse(&result, &ctx, pe->metric_expr)) {
+ 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;
+} expr_testcase_table[] = {
+ {
+ .func = parse_tests,
+ .desc = "Basic expressions",
+ },
+ {
+ .func = pmu_tests,
+ .desc = "Parsing of pmu metrics",
+ },
+};
+
+const char *test__expr_subtest_get_desc(int i)
+{
+ if (i < 0 || i >= (int)ARRAY_SIZE(expr_testcase_table))
+ return NULL;
+ return expr_testcase_table[i].desc;
+}
+
+int test__expr_subtest_get_nr(void)
+{
+ return (int)ARRAY_SIZE(expr_testcase_table);
+}
+
+int test__expr(struct test *test __maybe_unused, int i __maybe_unused)
+{
+ if (i < 0 || i >= (int)ARRAY_SIZE(expr_testcase_table))
+ return TEST_FAIL;
+ return expr_testcase_table[i].func();
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 61a1ab032080..315d64ffd14c 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -72,6 +72,8 @@ int test__keep_tracking(struct test *test, int subtest);
int test__parse_no_sample_id_all(struct test *test, int subtest);
int test__dwarf_unwind(struct test *test, int subtest);
int test__expr(struct test *test, int subtest);
+const char *test__expr_subtest_get_desc(int subtest);
+int test__expr_subtest_get_nr(void);
int test__hists_filter(struct test *test, int subtest);
int test__mmap_thread_lookup(struct test *test, int subtest);
int test__thread_maps_share(struct test *test, int subtest);
--
2.26.2.303.gf8c07b1a785-goog
Only effects parser debugging (disabled by default). Enables displaying
'--accepting rule at line .. ("...").
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/expr.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index c3382d58cf40..8694bec86f1a 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -44,6 +44,7 @@ __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
#ifdef PARSER_DEBUG
expr_debug = 1;
+ expr_set_debug(1, scanner);
#endif
ret = expr_parse(val, ctx, scanner);
--
2.26.2.303.gf8c07b1a785-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.303.gf8c07b1a785-goog
Large metrics such as Branch_Misprediction_Cost_SMT on x86 broadwell
need more space.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/expr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 0938ad166ece..4938bfc608b7 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -2,7 +2,7 @@
#ifndef PARSE_CTX_H
#define PARSE_CTX_H 1
-#define EXPR_MAX_OTHER 20
+#define EXPR_MAX_OTHER 64
#define MAX_PARSE_ID EXPR_MAX_OTHER
struct expr_parse_id {
--
2.26.2.303.gf8c07b1a785-goog
Mismatched parentheses.
Fixes: dd81eafacc52 (perf vendor events power8: Cpi_breakdown & estimated_dcache_miss_cpi metrics)
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/pmu-events/arch/powerpc/power8/metrics.json | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/pmu-events/arch/powerpc/power8/metrics.json b/tools/perf/pmu-events/arch/powerpc/power8/metrics.json
index bffb2d4a6420..ad71486a38e3 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"
},
@@ -886,7 +886,7 @@
},
{
"BriefDescription": "GCT slot utilization (11 to 14) as a % of cycles this thread had atleast 1 slot valid",
- "MetricExpr": "PM_GCT_UTIL_11_14_ENTRIES / ( PM_RUN_CYC - PM_GCT_NOSLOT_CYC) * 100",
+ "MetricExpr": "PM_GCT_UTIL_11_14_ENTRIES / ( PM_RUN_CYC - PM_GCT_NOSLOT_CYC ) * 100",
"MetricGroup": "general",
"MetricName": "gct_util_11to14_slots_percent"
},
--
2.26.2.303.gf8c07b1a785-goog
Mismatched parentheses.
Fixes: 7f3cf5ac7743 (perf vendor events power9: Cpi_breakdown & estimated_dcache_miss_cpi metrics)
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.303.gf8c07b1a785-goog
On 4/22/20 5:04 PM, Ian Rogers wrote:
> Mismatched parentheses.
>
> Fixes: 7f3cf5ac7743 (perf vendor events power9: Cpi_breakdown & estimated_dcache_miss_cpi metrics)
> 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",
OK. (Thank you!!)
> "MetricGroup": "cpi_breakdown",
> "MetricName": "other_stall_cpi"
> },
Reviewed-by: Paul A. Clarke <[email protected]>
(I like how you got the "power8" changes to be 8th in the series and the "power9" changes to be 9th ;-)
PC
On 4/22/20 5:04 PM, Ian Rogers wrote:
> Mismatched parentheses.
>
> Fixes: dd81eafacc52 (perf vendor events power8: Cpi_breakdown & estimated_dcache_miss_cpi metrics)
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/pmu-events/arch/powerpc/power8/metrics.json | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/pmu-events/arch/powerpc/power8/metrics.json b/tools/perf/pmu-events/arch/powerpc/power8/metrics.json
> index bffb2d4a6420..ad71486a38e3 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",
OK. (Thank you!)
> "MetricGroup": "cpi_breakdown",
> "MetricName": "gct_empty_disp_held_cpi"
> },
> @@ -886,7 +886,7 @@
> },
> {
> "BriefDescription": "GCT slot utilization (11 to 14) as a % of cycles this thread had atleast 1 slot valid",
> - "MetricExpr": "PM_GCT_UTIL_11_14_ENTRIES / ( PM_RUN_CYC - PM_GCT_NOSLOT_CYC) * 100",
> + "MetricExpr": "PM_GCT_UTIL_11_14_ENTRIES / ( PM_RUN_CYC - PM_GCT_NOSLOT_CYC ) * 100",
I think this is just a whitespace change? Is it necessary?
Curiosity, more than complaint.
> "MetricGroup": "general",
> "MetricName": "gct_util_11to14_slots_percent"
> },
Reviewed-by: Paul A. Clarke <[email protected]>
PC
On Wed, Apr 22, 2020 at 3:31 PM Paul Clarke <[email protected]> wrote:
>
> On 4/22/20 5:04 PM, Ian Rogers wrote:
> > Mismatched parentheses.
> >
> > Fixes: dd81eafacc52 (perf vendor events power8: Cpi_breakdown & estimated_dcache_miss_cpi metrics)
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/pmu-events/arch/powerpc/power8/metrics.json | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/pmu-events/arch/powerpc/power8/metrics.json b/tools/perf/pmu-events/arch/powerpc/power8/metrics.json
> > index bffb2d4a6420..ad71486a38e3 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",
>
> OK. (Thank you!)
>
> > "MetricGroup": "cpi_breakdown",
> > "MetricName": "gct_empty_disp_held_cpi"
> > },
> > @@ -886,7 +886,7 @@
> > },
> > {
> > "BriefDescription": "GCT slot utilization (11 to 14) as a % of cycles this thread had atleast 1 slot valid",
> > - "MetricExpr": "PM_GCT_UTIL_11_14_ENTRIES / ( PM_RUN_CYC - PM_GCT_NOSLOT_CYC) * 100",
> > + "MetricExpr": "PM_GCT_UTIL_11_14_ENTRIES / ( PM_RUN_CYC - PM_GCT_NOSLOT_CYC ) * 100",
>
> I think this is just a whitespace change? Is it necessary?
> Curiosity, more than complaint.
Sorry about that, the space isn't necessary and this doesn't need to
change. For the curious, originally the parse test would make all
metrics equal to 1.0 and this metric would trigger a divide by zero
because of this. This motivated adding a debug print for this case.
Thanks,
Ian
> > "MetricGroup": "general",
> > "MetricName": "gct_util_11to14_slots_percent"
> > },
>
> Reviewed-by: Paul A. Clarke <[email protected]>
>
> PC
On Wed, Apr 22, 2020 at 03:04:19PM -0700, Ian Rogers wrote:
> Add a test that all perf metrics (for your architecture) are
> parsable. Fix bugs in the expr parser and in x86 metrics. Untested on
> architectures other than x86.
>
> 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.
looks good to me
Jin Yao, is there a metric that's not working for you with this patchset
applied?
thanks,
jirka
>
> Ian Rogers (11):
> 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 test: add expr test for pmu metrics
>
> .../arch/powerpc/power8/metrics.json | 4 +-
> .../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 | 96 ++++++++++++++++++-
> 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 +++-
> 11 files changed, 135 insertions(+), 23 deletions(-)
>
> --
> 2.26.2.303.gf8c07b1a785-goog
>
On Wed, Apr 22, 2020 at 03:04:29PM -0700, Ian Rogers wrote:
> 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) {
is that (long) cast necessary? it's missing for the '/' case
jirka
> + 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.303.gf8c07b1a785-goog
>
On Wed, Apr 22, 2020 at 03:04:30PM -0700, Ian Rogers wrote:
SNIP
> +
> +static int pmu_tests(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;
> +
> + 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;
so we go throught all the metrics for the current cpu
and test the parsing on them.. great!
thanks,
jirka
On Wed, Apr 22, 2020 at 03:04:23PM -0700, Ian Rogers wrote:
> Corrects parse errors in expr__find_other of expressions with min.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/expr.y | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index cd17486c1c5d..54260094b947 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -80,7 +80,7 @@ other: ID
> ctx->ids[ctx->num_ids++].name = $1;
> }
> |
> -MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')'
> +MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')' | ','
ugh, thanks
jirka
>
>
> all_expr: if_expr { *final_val = $1; }
> --
> 2.26.2.303.gf8c07b1a785-goog
>
On Wed, Apr 22, 2020 at 03:04:25PM -0700, Ian Rogers wrote:
> This is expected in expr.y and metrics use floating point values such as
> x86 broadwell IFetch_Line_Utilization.
ugh, I wonder why we did not get any compiler warning when expr.y
expects double.. good catch
jirka
>
> 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 95bcf3629edf..0efda2ce2766 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);
> }
>
> /*
> @@ -68,7 +68,7 @@ static int str(yyscan_t scanner, int token)
> }
> %}
>
> -number [0-9]+
> +number [0-9]*\.?[0-9]+
>
> sch [-,=]
> spec \\{sch}
> @@ -92,7 +92,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); }
> "|" { return '|'; }
> "^" { return '^'; }
> --
> 2.26.2.303.gf8c07b1a785-goog
>
On Wed, Apr 22, 2020 at 03:04:24PM -0700, Ian Rogers wrote:
> Large metrics such as Branch_Misprediction_Cost_SMT on x86 broadwell
> need more space.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/expr.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 0938ad166ece..4938bfc608b7 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -2,7 +2,7 @@
> #ifndef PARSE_CTX_H
> #define PARSE_CTX_H 1
>
> -#define EXPR_MAX_OTHER 20
> +#define EXPR_MAX_OTHER 64
> #define MAX_PARSE_ID EXPR_MAX_OTHER
>
> struct expr_parse_id {
> --
> 2.26.2.303.gf8c07b1a785-goog
>
ok, and we should probably start to think about what Andi suggested
in here: https://lore.kernel.org/lkml/[email protected]/
jirka
Hi Jiri,
On 4/23/2020 7:28 PM, Jiri Olsa wrote:
> On Wed, Apr 22, 2020 at 03:04:19PM -0700, Ian Rogers wrote:
>> Add a test that all perf metrics (for your architecture) are
>> parsable. Fix bugs in the expr parser and in x86 metrics. Untested on
>> architectures other than x86.
>>
>> 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.
>
> looks good to me
>
> Jin Yao, is there a metric that's not working for you with this patchset
> applied?
>
> thanks,
> jirka
>
Let me look for a CLX for testing, but maybe need some time.
BTW, suppose this patchset can work well, does it mean we will change
the json file format in future?
For example,
before:
cha@event\\=0x36\\\\\\
after:
cha@event\\=0x36\\
"\\\\" are removed.
If so, we need to change our event generation script.
Thanks
Jin Yao
>>
>> Ian Rogers (11):
>> 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 test: add expr test for pmu metrics
>>
>> .../arch/powerpc/power8/metrics.json | 4 +-
>> .../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 | 96 ++++++++++++++++++-
>> 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 +++-
>> 11 files changed, 135 insertions(+), 23 deletions(-)
>>
>> --
>> 2.26.2.303.gf8c07b1a785-goog
>>
>
On Thu, Apr 23, 2020 at 4:28 AM Jiri Olsa <[email protected]> wrote:
>
> On Wed, Apr 22, 2020 at 03:04:29PM -0700, Ian Rogers wrote:
> > 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) {
>
> is that (long) cast necessary? it's missing for the '/' case
Andi Kleen could say for sure :-) From my observation, the values are
typically doubles and definitely need to be in the divide case. There
is also code handling them as longs such as |, &, ^:
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/tools/perf/util/expr.y#n100
There's an experiment to see if all of the long handling code could be
removed. This change isn't modifying the existing behavior.
Thanks,
Ian
> jirka
>
> > + 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.303.gf8c07b1a785-goog
> >
>
On Thu, Apr 23, 2020 at 4:29 AM Jiri Olsa <[email protected]> wrote:
>
> On Wed, Apr 22, 2020 at 03:04:30PM -0700, Ian Rogers wrote:
>
> SNIP
>
> > +
> > +static int pmu_tests(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;
> > +
> > + 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;
>
> so we go throught all the metrics for the current cpu
> and test the parsing on them.. great!
It's not just the current CPU (such as skylake) it is every map
(skylake, cascade lake, etc), but this only works for the architecture
that jevents built.
If jevents built all architectures then this could check them as well.
Perhaps there should be a jevents test suite, but I think even then
this test has value.
A worthy addition to this is checking that the events within the
expression parse, but this is good progress and worth landing.
Thanks,
Ian
> thanks,
> jirka
>
On Thu, Apr 23, 2020 at 4:29 AM Jiri Olsa <[email protected]> wrote:
>
> On Wed, Apr 22, 2020 at 03:04:24PM -0700, Ian Rogers wrote:
> > Large metrics such as Branch_Misprediction_Cost_SMT on x86 broadwell
> > need more space.
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/util/expr.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> > index 0938ad166ece..4938bfc608b7 100644
> > --- a/tools/perf/util/expr.h
> > +++ b/tools/perf/util/expr.h
> > @@ -2,7 +2,7 @@
> > #ifndef PARSE_CTX_H
> > #define PARSE_CTX_H 1
> >
> > -#define EXPR_MAX_OTHER 20
> > +#define EXPR_MAX_OTHER 64
> > #define MAX_PARSE_ID EXPR_MAX_OTHER
> >
> > struct expr_parse_id {
> > --
> > 2.26.2.303.gf8c07b1a785-goog
> >
>
> ok, and we should probably start to think about what Andi suggested
> in here: https://lore.kernel.org/lkml/[email protected]/
Agreed, a hash table would make sense. This was the smallest value
that would let the test on x86 pass.
Thanks,
Ian
> jirka
>
On Thu, Apr 23, 2020 at 7:03 AM Jiri Olsa <[email protected]> wrote:
>
> On Thu, Apr 23, 2020 at 09:44:24PM +0800, Jin, Yao wrote:
> > Hi Jiri,
> >
> > On 4/23/2020 7:28 PM, Jiri Olsa wrote:
> > > On Wed, Apr 22, 2020 at 03:04:19PM -0700, Ian Rogers wrote:
> > > > Add a test that all perf metrics (for your architecture) are
> > > > parsable. Fix bugs in the expr parser and in x86 metrics. Untested on
> > > > architectures other than x86.
> > > >
> > > > 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.
> > >
> > > looks good to me
> > >
> > > Jin Yao, is there a metric that's not working for you with this patchset
> > > applied?
> > >
> > > thanks,
> > > jirka
> > >
> >
> > Let me look for a CLX for testing, but maybe need some time.
> >
> > BTW, suppose this patchset can work well, does it mean we will change the
> > json file format in future?
> >
> > For example,
> >
> > before:
> > cha@event\\=0x36\\\\\\
> >
> > after:
> > cha@event\\=0x36\\
> >
> > "\\\\" are removed.
> >
> > If so, we need to change our event generation script.
>
> ok, maybe I got the wrong idea that the extra \\\\ were just
> superfluous, what was the actual error there? and what's the
> reason for that many '\' in there?
I believe they are superfluous and break even before the flex change.
I commented on it here with a reproduction of a parse events error on
skylake:
https://lore.kernel.org/lkml/CAP-5=fUnWAycQehCJ9=btquV2c3DVDX+tTEc85H8py9Kfehq4w@mail.gmail.com/
Fixing the script that generates this would be great! With the test
landed it should be much harder for this to be broken in the future.
Thanks,
Ian
> jirka
>
On Thu, Apr 23, 2020 at 8:11 AM John Garry <[email protected]> wrote:
>
> On 23/04/2020 15:22, Ian Rogers wrote:
> > On Thu, Apr 23, 2020 at 4:29 AM Jiri Olsa <[email protected]> wrote:
> >>
> >> On Wed, Apr 22, 2020 at 03:04:30PM -0700, Ian Rogers wrote:
> >>
> >> SNIP
> >>
> >>> +
> >>> +static int pmu_tests(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;
> >>> +
> >>> + 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;
> >>
> >> so we go throught all the metrics for the current cpu
> >> and test the parsing on them.. great!
> >
> > It's not just the current CPU (such as skylake) it is every map
> > (skylake, cascade lake, etc), but this only works for the architecture
> > that jevents built.
> > If jevents built all architectures then this could check them as well.
> > Perhaps there should be a jevents test suite, but I think even then
> > this test has value.
>
> note: there is test__pmu_events(), which verifies that some test events
> generated in pmu-events.c are as expected, and also verifies that we
> create PMU events aliases as expected (for those test events). Nothing
> is done for metrics, ATM.
Thanks John, that sounds like the right place to start.
Ian
> Thanks,
> John
On Thu, Apr 23, 2020 at 09:44:24PM +0800, Jin, Yao wrote:
> Hi Jiri,
>
> On 4/23/2020 7:28 PM, Jiri Olsa wrote:
> > On Wed, Apr 22, 2020 at 03:04:19PM -0700, Ian Rogers wrote:
> > > Add a test that all perf metrics (for your architecture) are
> > > parsable. Fix bugs in the expr parser and in x86 metrics. Untested on
> > > architectures other than x86.
> > >
> > > 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.
> >
> > looks good to me
> >
> > Jin Yao, is there a metric that's not working for you with this patchset
> > applied?
> >
> > thanks,
> > jirka
> >
>
> Let me look for a CLX for testing, but maybe need some time.
>
> BTW, suppose this patchset can work well, does it mean we will change the
> json file format in future?
>
> For example,
>
> before:
> cha@event\\=0x36\\\\\\
>
> after:
> cha@event\\=0x36\\
>
> "\\\\" are removed.
>
> If so, we need to change our event generation script.
ok, maybe I got the wrong idea that the extra \\\\ were just
superfluous, what was the actual error there? and what's the
reason for that many '\' in there?
jirka
On 23/04/2020 15:22, Ian Rogers wrote:
> On Thu, Apr 23, 2020 at 4:29 AM Jiri Olsa <[email protected]> wrote:
>>
>> On Wed, Apr 22, 2020 at 03:04:30PM -0700, Ian Rogers wrote:
>>
>> SNIP
>>
>>> +
>>> +static int pmu_tests(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;
>>> +
>>> + 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;
>>
>> so we go throught all the metrics for the current cpu
>> and test the parsing on them.. great!
>
> It's not just the current CPU (such as skylake) it is every map
> (skylake, cascade lake, etc), but this only works for the architecture
> that jevents built.
> If jevents built all architectures then this could check them as well.
> Perhaps there should be a jevents test suite, but I think even then
> this test has value.
note: there is test__pmu_events(), which verifies that some test events
generated in pmu-events.c are as expected, and also verifies that we
create PMU events aliases as expected (for those test events). Nothing
is done for metrics, ATM.
Thanks,
John
On 4/23/2020 10:31 PM, Ian Rogers wrote:
> On Thu, Apr 23, 2020 at 7:03 AM Jiri Olsa <[email protected]> wrote:
>>
>> On Thu, Apr 23, 2020 at 09:44:24PM +0800, Jin, Yao wrote:
>>> Hi Jiri,
>>>
>>> On 4/23/2020 7:28 PM, Jiri Olsa wrote:
>>>> On Wed, Apr 22, 2020 at 03:04:19PM -0700, Ian Rogers wrote:
>>>>> Add a test that all perf metrics (for your architecture) are
>>>>> parsable. Fix bugs in the expr parser and in x86 metrics. Untested on
>>>>> architectures other than x86.
>>>>>
>>>>> 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.
>>>>
>>>> looks good to me
>>>>
>>>> Jin Yao, is there a metric that's not working for you with this patchset
>>>> applied?
>>>>
>>>> thanks,
>>>> jirka
>>>>
>>>
>>> Let me look for a CLX for testing, but maybe need some time.
>>>
>>> BTW, suppose this patchset can work well, does it mean we will change the
>>> json file format in future?
>>>
>>> For example,
>>>
>>> before:
>>> cha@event\\=0x36\\\\\\
>>>
>>> after:
>>> cha@event\\=0x36\\
>>>
>>> "\\\\" are removed.
>>>
>>> If so, we need to change our event generation script.
>>
>> ok, maybe I got the wrong idea that the extra \\\\ were just
>> superfluous, what was the actual error there? and what's the
>> reason for that many '\' in there?
>
> I believe they are superfluous and break even before the flex change.
> I commented on it here with a reproduction of a parse events error on
> skylake:
> https://lore.kernel.org/lkml/CAP-5=fUnWAycQehCJ9=btquV2c3DVDX+tTEc85H8py9Kfehq4w@mail.gmail.com/
>
> Fixing the script that generates this would be great! With the test
> landed it should be much harder for this to be broken in the future.
>
> Thanks,
> Ian
>
Tested on CLX and the error disappeared.
BTW, I will post the latest version of event list once the testing is
completed.
Thanks
Jin Yao
>
>> jirka
>>
On 4/23/20 3:34 AM, 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 95bcf3629edf..0efda2ce2766 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);
> }
>
> /*
> @@ -68,7 +68,7 @@ static int str(yyscan_t scanner, int token)
> }
> %}
>
> -number [0-9]+
> +number [0-9]*\.?[0-9]+
>
Hi Ian,
In this patch I saw the parsing of expression with '+number [0-9]*\.?[0-9]+'
Could you please explain why '?' is introduced here, so that I can be sure that this is
not conflicting with my change to add '?'
In this patch : https://lkml.org/lkml/2020/4/1/1427
I have used '?' symbol as part of metric expression in order to replace '?' with runtime
parameter.
Thanks,
Kajol Jain
> sch [-,=]
> spec \\{sch}
> @@ -92,7 +92,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); }
> "|" { return '|'; }
> "^" { return '^'; }
>
On Mon, Apr 27, 2020 at 4:12 AM kajoljain <[email protected]> wrote:
>
> On 4/23/20 3:34 AM, 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 95bcf3629edf..0efda2ce2766 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);
> > }
> >
> > /*
> > @@ -68,7 +68,7 @@ static int str(yyscan_t scanner, int token)
> > }
> > %}
> >
> > -number [0-9]+
> > +number [0-9]*\.?[0-9]+
> >
>
> Hi Ian,
> In this patch I saw the parsing of expression with '+number [0-9]*\.?[0-9]+'
> Could you please explain why '?' is introduced here, so that I can be sure that this is
> not conflicting with my change to add '?'
Hi Kajol,
the '?' here is part of the regular expression. Basically it is saying
that a number is a possible set of integers possibly followed by a '.'
and then 1 or more integers. The expression comes from having seen
Intel's metrics are of the form '.1234' in some of their topdown
spreadsheets, so we need to be able to handle cases like '1.2', '123'
and '.1234'. Having looked at your patch I don't believe it
interferes.
> In this patch : https://lkml.org/lkml/2020/4/1/1427
> I have used '?' symbol as part of metric expression in order to replace '?' with runtime
> parameter.
Interesting, I'll follow up with comments on that e-mail.
Thanks!
Ian
> Thanks,
> Kajol Jain
>
>
> > sch [-,=]
> > spec \\{sch}
> > @@ -92,7 +92,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); }
> > "|" { return '|'; }
> > "^" { return '^'; }
> >
On 4/23/20 3:34 AM, 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 95bcf3629edf..0efda2ce2766 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);
> }
>
> /*
> @@ -68,7 +68,7 @@ static int str(yyscan_t scanner, int token)
> }
> %}
>
> -number [0-9]+
> +number [0-9]*\.?[0-9]+
>
Acked By: Kajol Jain <[email protected]>
Thanks,
Kajol Jain
> sch [-,=]
> spec \\{sch}
> @@ -92,7 +92,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); }
> "|" { return '|'; }
> "^" { return '^'; }
>
On Thu, Apr 23, 2020 at 7:23 AM Ian Rogers <[email protected]> wrote:
>
> On Thu, Apr 23, 2020 at 4:29 AM Jiri Olsa <[email protected]> wrote:
> >
> > On Wed, Apr 22, 2020 at 03:04:24PM -0700, Ian Rogers wrote:
> > > Large metrics such as Branch_Misprediction_Cost_SMT on x86 broadwell
> > > need more space.
> > >
> > > Signed-off-by: Ian Rogers <[email protected]>
> > > ---
> > > tools/perf/util/expr.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> > > index 0938ad166ece..4938bfc608b7 100644
> > > --- a/tools/perf/util/expr.h
> > > +++ b/tools/perf/util/expr.h
> > > @@ -2,7 +2,7 @@
> > > #ifndef PARSE_CTX_H
> > > #define PARSE_CTX_H 1
> > >
> > > -#define EXPR_MAX_OTHER 20
> > > +#define EXPR_MAX_OTHER 64
> > > #define MAX_PARSE_ID EXPR_MAX_OTHER
> > >
> > > struct expr_parse_id {
> > > --
> > > 2.26.2.303.gf8c07b1a785-goog
> > >
> >
> > ok, and we should probably start to think about what Andi suggested
> > in here: https://lore.kernel.org/lkml/[email protected]/
>
> Agreed, a hash table would make sense. This was the smallest value
> that would let the test on x86 pass.
Fwiw, I have done this based on tools/lib/bpf/hashmap.h in CLs
following on from this patch set. I'm holding off sending so I can
rebase on acme's perf/next when the CLs acked by Jirka land:
https://lore.kernel.org/lkml/20200503221650.GA1916255@krava/
The libbpf dependency for a hashmap is counter intuitive, so maybe
there's something better to do there.
Thanks,
Ian
> Thanks,
> Ian
>
> > jirka
> >