[PATCH v2 00/11] perf metric fixes and test

[PATCH v2 00/11] perf metric fixes and test

[Ian Rogers]

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

[PATCH v2 01/11] perf expr: unlimited escaped characters in a symbol

[Ian Rogers]

Current expression allows 2 escaped '-,=' characters. However, some
metrics require more, for example Haswell DRAM_BW_Use.

Fixes: 26226a97724d (perf expr: Move expr lexer to flex)
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/expr.l | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
index 2582c2464938..95bcf3629edf 100644
--- a/tools/perf/util/expr.l
+++ b/tools/perf/util/expr.l
@@ -73,7 +73,7 @@ number		[0-9]+
 sch		[-,=]
 spec		\\{sch}
 sym		[0-9a-zA-Z_\.:@]+
-symbol		{spec}*{sym}*{spec}*{sym}*
+symbol		({spec}|{sym})+
 
 %%
 	struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH v2 01/11] perf expr: unlimited escaped characters in a symbol

[Jiri Olsa]

On Wed, Apr 22, 2020 at 03:04:20PM -0700, Ian Rogers wrote:
> Current expression allows 2 escaped '-,=' characters. However, some
> metrics require more, for example Haswell DRAM_BW_Use.
> 
> Fixes: 26226a97724d (perf expr: Move expr lexer to flex)
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/expr.l | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index 2582c2464938..95bcf3629edf 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -73,7 +73,7 @@ number		[0-9]+
>  sch		[-,=]
>  spec		\\{sch}
>  sym		[0-9a-zA-Z_\.:@]+
> -symbol		{spec}*{sym}*{spec}*{sym}*
> +symbol		({spec}|{sym})+

yep, much better ;-)

thanks,
jirka

>  
>  %%
>  	struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
> -- 
> 2.26.2.303.gf8c07b1a785-goog
> 

Re: [PATCH v2 01/11] perf expr: unlimited escaped characters in a symbol

[kajoljain]

On 4/23/20 4:59 PM, Jiri Olsa wrote:
> On Wed, Apr 22, 2020 at 03:04:20PM -0700, Ian Rogers wrote:
>> Current expression allows 2 escaped '-,=' characters. However, some
>> metrics require more, for example Haswell DRAM_BW_Use.
>>
>> Fixes: 26226a97724d (perf expr: Move expr lexer to flex)
>> Signed-off-by: Ian Rogers <irogers@google.com>
>> ---
>>  tools/perf/util/expr.l | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
>> index 2582c2464938..95bcf3629edf 100644
>> --- a/tools/perf/util/expr.l
>> +++ b/tools/perf/util/expr.l
>> @@ -73,7 +73,7 @@ number		[0-9]+
>>  sch		[-,=]
>>  spec		\\{sch}
>>  sym		[0-9a-zA-Z_\.:@]+
>> -symbol		{spec}*{sym}*{spec}*{sym}*
>> +symbol		({spec}|{sym})+

Tested by: kjain@linux.ibm.com

Thanks,
Kajol Jain
> 
> yep, much better ;-)
> 
> thanks,
> jirka
> 
>>  
>>  %%
>>  	struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
>> -- 
>> 2.26.2.303.gf8c07b1a785-goog
>>
> 

[PATCH v2 02/11] perf metrics: fix parse errors in cascade lake metrics

[Ian Rogers]

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 <irogers@google.com>
---
 .../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

[PATCH v2 03/11] perf metrics: fix parse errors in skylake metrics

[Ian Rogers]

Remove over escaping with \\.

Fixes: fd5500989c8f (perf vendor events intel: Update metrics from TMAM 3.5)
Signed-off-by: Ian Rogers <irogers@google.com>
---
 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

[PATCH v2 04/11] perf expr: allow ',' to be an other token

[Ian Rogers]

Corrects parse errors in expr__find_other of expressions with min.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 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

Re: [PATCH v2 04/11] perf expr: allow ',' to be an other token

[Jiri Olsa]

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 <irogers@google.com>
> ---
>  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
> 

[PATCH v2 05/11] perf expr: increase max other

[Ian Rogers]

Large metrics such as Branch_Misprediction_Cost_SMT on x86 broadwell
need more space.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 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

Re: [PATCH v2 05/11] perf expr: increase max other

[Jiri Olsa]

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 <irogers@google.com>
> ---
>  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/20200224210308.GQ160988@tassilo.jf.intel.com/

jirka

Re: [PATCH v2 05/11] perf expr: increase max other

[Ian Rogers]

On Thu, Apr 23, 2020 at 4:29 AM Jiri Olsa <jolsa@redhat.com> 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 <irogers@google.com>
> > ---
> >  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/20200224210308.GQ160988@tassilo.jf.intel.com/

Agreed, a hash table would make sense. This was the smallest value
that would let the test on x86 pass.

Thanks,
Ian

> jirka
>

Re: [PATCH v2 05/11] perf expr: increase max other

[Ian Rogers]

On Thu, Apr 23, 2020 at 7:23 AM Ian Rogers <irogers@google.com> wrote:
>
> On Thu, Apr 23, 2020 at 4:29 AM Jiri Olsa <jolsa@redhat.com> 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 <irogers@google.com>
> > > ---
> > >  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/20200224210308.GQ160988@tassilo.jf.intel.com/
>
> 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
> >

[PATCH v2 06/11] perf expr: parse numbers as doubles

[Ian Rogers]

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 <irogers@google.com>
---
 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

Re: [PATCH v2 06/11] perf expr: parse numbers as doubles

[Jiri Olsa]

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 <irogers@google.com>
> ---
>  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
> 

Re: [PATCH v2 06/11] perf expr: parse numbers as doubles

[kajoljain]

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 <irogers@google.com>
> ---
>  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 '^'; }
> 

Re: [PATCH v2 06/11] perf expr: parse numbers as doubles

[Ian Rogers]

On Mon, Apr 27, 2020 at 4:12 AM kajoljain <kjain@linux.ibm.com> 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 <irogers@google.com>
> > ---
> >  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 '^'; }
> >

Re: [PATCH v2 06/11] perf expr: parse numbers as doubles

[kajoljain]

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 <irogers@google.com>
> ---
>  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 <kjain@linux.ibm.com>

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 '^'; }
> 

[PATCH v2 07/11] perf expr: debug lex if debugging yacc

[Ian Rogers]

Only effects parser debugging (disabled by default). Enables displaying
'--accepting rule at line .. ("...").

Signed-off-by: Ian Rogers <irogers@google.com>
---
 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

[PATCH v2 08/11] perf metrics: fix parse errors in power8 metrics

[Ian Rogers]

Mismatched parentheses.

Fixes: dd81eafacc52 (perf vendor events power8: Cpi_breakdown & estimated_dcache_miss_cpi metrics)
Signed-off-by: Ian Rogers <irogers@google.com>
---
 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

Re: [PATCH v2 08/11] perf metrics: fix parse errors in power8 metrics

[Paul Clarke]

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 <irogers@google.com>
> ---
>  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 <pc@us.ibm.com>

PC

Re: [PATCH v2 08/11] perf metrics: fix parse errors in power8 metrics

[Ian Rogers]

On Wed, Apr 22, 2020 at 3:31 PM Paul Clarke <pc@us.ibm.com> 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 <irogers@google.com>
> > ---
> >  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 <pc@us.ibm.com>
>
> PC

[PATCH v2 09/11] perf metrics: fix parse errors in power9 metrics

[Ian Rogers]

Mismatched parentheses.

Fixes: 7f3cf5ac7743 (perf vendor events power9: Cpi_breakdown & estimated_dcache_miss_cpi metrics)
Signed-off-by: Ian Rogers <irogers@google.com>
---
 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

Re: [PATCH v2 09/11] perf metrics: fix parse errors in power9 metrics

[Paul Clarke]

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 <irogers@google.com>
> ---
>  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 <pc@us.ibm.com>

(I like how you got the "power8" changes to be 8th in the series and the "power9" changes to be 9th ;-)

PC

[PATCH v2 10/11] perf expr: print a debug message for division by zero

[Ian Rogers]

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 <irogers@google.com>
---
 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

Re: [PATCH v2 10/11] perf expr: print a debug message for division by zero

[Jiri Olsa]

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 <irogers@google.com>
> ---
>  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
> 

Re: [PATCH v2 10/11] perf expr: print a debug message for division by zero

[Ian Rogers]

On Thu, Apr 23, 2020 at 4:28 AM Jiri Olsa <jolsa@redhat.com> 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 <irogers@google.com>
> > ---
> >  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
> >
>

[PATCH v2 11/11] perf test: add expr test for pmu metrics

[Ian Rogers]

Add basic floating point number test.
Verify that all pmu metrics, for the current architecture, parse.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 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

Re: [PATCH v2 11/11] perf test: add expr test for pmu metrics

[Jiri Olsa]

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

Re: [PATCH v2 11/11] perf test: add expr test for pmu metrics

[Ian Rogers]

On Thu, Apr 23, 2020 at 4:29 AM Jiri Olsa <jolsa@redhat.com> 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
>

Re: [PATCH v2 11/11] perf test: add expr test for pmu metrics

[John Garry]

On 23/04/2020 15:22, Ian Rogers wrote:
> On Thu, Apr 23, 2020 at 4:29 AM Jiri Olsa <jolsa@redhat.com> 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

Re: [PATCH v2 11/11] perf test: add expr test for pmu metrics

[Ian Rogers]

On Thu, Apr 23, 2020 at 8:11 AM John Garry <john.garry@huawei.com> wrote:
>
> On 23/04/2020 15:22, Ian Rogers wrote:
> > On Thu, Apr 23, 2020 at 4:29 AM Jiri Olsa <jolsa@redhat.com> 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

Re: [PATCH v2 00/11] perf metric fixes and test

[Jiri Olsa]

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
> 

Re: [PATCH v2 00/11] perf metric fixes and test

[Jin, Yao]

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
>>
> 

Re: [PATCH v2 00/11] perf metric fixes and test

[Jiri Olsa]

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

Re: [PATCH v2 00/11] perf metric fixes and test

[Ian Rogers]

On Thu, Apr 23, 2020 at 7:03 AM Jiri Olsa <jolsa@redhat.com> 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
>

Re: [PATCH v2 00/11] perf metric fixes and test

[Jin, Yao]

On 4/23/2020 10:31 PM, Ian Rogers wrote:
> On Thu, Apr 23, 2020 at 7:03 AM Jiri Olsa <jolsa@redhat.com> 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
>>