linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v2 00/23] Share events between metrics
@ 2020-05-07 14:07 Ian Rogers
  2020-05-07 14:07 ` [RFC PATCH v2 01/23] perf expr: unlimited escaped characters in a symbol Ian Rogers
                   ` (22 more replies)
  0 siblings, 23 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:07 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

Metric groups contain metrics. Metrics create groups of events to
ideally be scheduled together. Often metrics refer to the same events,
for example, a cache hit and cache miss rate. Using separate event
groups means these metrics are multiplexed at different times and the
counts don't sum to 100%. More multiplexing also decreases the
accuracy of the measurement.

This change orders metrics from groups or the command line, so that
the ones with the most events are set up first. Later metrics see if
groups already provide their events, and reuse them if
possible. Unnecessary events and groups are eliminated.

RFC because:
 - without this change events within a metric may get scheduled
   together, after they may appear as part of a larger group and be
   multiplexed at different times, lowering accuracy - however, less
   multiplexing may compensate for this.
 - libbpf's hashmap is used, however, libbpf is an optional
   requirement for building perf.
 - other things I'm not thinking of.

Thanks!

v2. is the entire patch set based on acme's perf/core tree and includes a
cherry-picks. Patch 13 was sent for review to the bpf maintainers here:
https://lore.kernel.org/lkml/20200506205257.8964-2-irogers@google.com/
v1. was based on the perf metrics fixes and test sent here:
https://lore.kernel.org/lkml/20200501173333.227162-1-irogers@google.com/

Andrii Nakryiko (1):
  libbpf: Fix memory leak and possible double-free in hashmap__clear

Ian Rogers (22):
  perf expr: unlimited escaped characters in a symbol
  perf metrics: fix parse errors in cascade lake metrics
  perf metrics: fix parse errors in skylake metrics
  perf expr: allow ',' to be an other token
  perf expr: increase max other
  perf expr: parse numbers as doubles
  perf expr: debug lex if debugging yacc
  perf metrics: fix parse errors in power8 metrics
  perf metrics: fix parse errors in power9 metrics
  perf expr: print a debug message for division by zero
  perf parse-events: expand add PMU error/verbose messages
  perf test: improve pmu event metric testing
  lib/bpf hashmap: increase portability
  perf expr: fix memory leaks in bison
  perf evsel: fix 2 memory leaks
  perf expr: migrate expr ids table to libbpf's hashmap
  perf metricgroup: change evlist_used to a bitmap
  perf metricgroup: free metric_events on error
  perf metricgroup: always place duration_time last
  perf metricgroup: delay events string creation
  perf metricgroup: order event groups by size
  perf metricgroup: remove duped metric group events

 tools/lib/bpf/hashmap.c                       |   7 +
 tools/lib/bpf/hashmap.h                       |   3 +-
 tools/perf/arch/x86/util/intel-pt.c           |  32 ++-
 .../arch/powerpc/power8/metrics.json          |   2 +-
 .../arch/powerpc/power9/metrics.json          |   2 +-
 .../arch/x86/cascadelakex/clx-metrics.json    |  10 +-
 .../arch/x86/skylakex/skx-metrics.json        |   4 +-
 tools/perf/tests/builtin-test.c               |   5 +
 tools/perf/tests/expr.c                       |  33 ++-
 tools/perf/tests/pmu-events.c                 | 158 +++++++++++-
 tools/perf/tests/pmu.c                        |   4 +-
 tools/perf/tests/tests.h                      |   2 +
 tools/perf/util/evsel.c                       |   2 +
 tools/perf/util/expr.c                        | 126 ++++-----
 tools/perf/util/expr.h                        |  22 +-
 tools/perf/util/expr.l                        |  16 +-
 tools/perf/util/expr.y                        |  41 ++-
 tools/perf/util/metricgroup.c                 | 242 +++++++++++-------
 tools/perf/util/parse-events.c                |  29 ++-
 tools/perf/util/pmu.c                         |  33 ++-
 tools/perf/util/pmu.h                         |   2 +-
 tools/perf/util/stat-shadow.c                 |  46 ++--
 22 files changed, 545 insertions(+), 276 deletions(-)

-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 01/23] perf expr: unlimited escaped characters in a symbol
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
@ 2020-05-07 14:07 ` Ian Rogers
  2020-05-07 14:07 ` [RFC PATCH v2 02/23] perf metrics: fix parse errors in cascade lake metrics Ian Rogers
                   ` (21 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:07 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, 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 74b9b59b1aa5..73db6a9ef97e 100644
--- a/tools/perf/util/expr.l
+++ b/tools/perf/util/expr.l
@@ -86,7 +86,7 @@ number		[0-9]+
 sch		[-,=]
 spec		\\{sch}
 sym		[0-9a-zA-Z_\.:@?]+
-symbol		{spec}*{sym}*{spec}*{sym}*{spec}*{sym}
+symbol		({spec}|{sym})+
 
 %%
 	struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 02/23] perf metrics: fix parse errors in cascade lake metrics
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
  2020-05-07 14:07 ` [RFC PATCH v2 01/23] perf expr: unlimited escaped characters in a symbol Ian Rogers
@ 2020-05-07 14:07 ` Ian Rogers
  2020-05-07 14:07 ` [RFC PATCH v2 03/23] perf metrics: fix parse errors in skylake metrics Ian Rogers
                   ` (20 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:07 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, 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.526.g744177e7f7-goog

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

* [RFC PATCH v2 03/23] perf metrics: fix parse errors in skylake metrics
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
  2020-05-07 14:07 ` [RFC PATCH v2 01/23] perf expr: unlimited escaped characters in a symbol Ian Rogers
  2020-05-07 14:07 ` [RFC PATCH v2 02/23] perf metrics: fix parse errors in cascade lake metrics Ian Rogers
@ 2020-05-07 14:07 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 04/23] perf expr: allow ',' to be an other token Ian Rogers
                   ` (19 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:07 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, 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.526.g744177e7f7-goog

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

* [RFC PATCH v2 04/23] perf expr: allow ',' to be an other token
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (2 preceding siblings ...)
  2020-05-07 14:07 ` [RFC PATCH v2 03/23] perf metrics: fix parse errors in skylake metrics Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 05/23] perf expr: increase max other Ian Rogers
                   ` (18 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, 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.526.g744177e7f7-goog

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

* [RFC PATCH v2 05/23] perf expr: increase max other
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (3 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 04/23] perf expr: allow ',' to be an other token Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 06/23] perf expr: parse numbers as doubles Ian Rogers
                   ` (17 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, 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 87d627bb699b..40fc452b0f2b 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.526.g744177e7f7-goog

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

* [RFC PATCH v2 06/23] perf expr: parse numbers as doubles
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (4 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 05/23] perf expr: increase max other Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 07/23] perf expr: debug lex if debugging yacc Ian Rogers
                   ` (16 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, 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 73db6a9ef97e..ceab11bea6f9 100644
--- a/tools/perf/util/expr.l
+++ b/tools/perf/util/expr.l
@@ -10,12 +10,12 @@
 char *expr_get_text(yyscan_t yyscanner);
 YYSTYPE *expr_get_lval(yyscan_t yyscanner);
 
-static int __value(YYSTYPE *yylval, char *str, int base, int token)
+static double __value(YYSTYPE *yylval, char *str, int token)
 {
-	u64 num;
+	double num;
 
 	errno = 0;
-	num = strtoull(str, NULL, base);
+	num = strtod(str, NULL);
 	if (errno)
 		return EXPR_ERROR;
 
@@ -23,12 +23,12 @@ static int __value(YYSTYPE *yylval, char *str, int base, int token)
 	return token;
 }
 
-static int value(yyscan_t scanner, int base)
+static int value(yyscan_t scanner)
 {
 	YYSTYPE *yylval = expr_get_lval(scanner);
 	char *text = expr_get_text(scanner);
 
-	return __value(yylval, text, base, NUMBER);
+	return __value(yylval, text, NUMBER);
 }
 
 /*
@@ -81,7 +81,7 @@ static int str(yyscan_t scanner, int token, int runtime)
 }
 %}
 
-number		[0-9]+
+number		[0-9]*\.?[0-9]+
 
 sch		[-,=]
 spec		\\{sch}
@@ -105,7 +105,7 @@ min		{ return MIN; }
 if		{ return IF; }
 else		{ return ELSE; }
 #smt_on		{ return SMT_ON; }
-{number}	{ return value(yyscanner, 10); }
+{number}	{ return value(yyscanner); }
 {symbol}	{ return str(yyscanner, ID, sctx->runtime); }
 "|"		{ return '|'; }
 "^"		{ return '^'; }
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 07/23] perf expr: debug lex if debugging yacc
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (5 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 06/23] perf expr: parse numbers as doubles Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 08/23] perf metrics: fix parse errors in power8 metrics Ian Rogers
                   ` (15 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, 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 aa631e37ad1e..8b4ce704a68d 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -45,6 +45,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.526.g744177e7f7-goog

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

* [RFC PATCH v2 08/23] perf metrics: fix parse errors in power8 metrics
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (6 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 07/23] perf expr: debug lex if debugging yacc Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 09/23] perf metrics: fix parse errors in power9 metrics Ian Rogers
                   ` (14 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers,
	Paul A . Clarke

Mismatched parentheses.

Fixes: dd81eafacc52 (perf vendor events power8: Cpi_breakdown & estimated_dcache_miss_cpi metrics)
Reviewed-by: Paul A. Clarke <pc@us.ibm.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/pmu-events/arch/powerpc/power8/metrics.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/pmu-events/arch/powerpc/power8/metrics.json b/tools/perf/pmu-events/arch/powerpc/power8/metrics.json
index bffb2d4a6420..fc4aa6c2ddc9 100644
--- a/tools/perf/pmu-events/arch/powerpc/power8/metrics.json
+++ b/tools/perf/pmu-events/arch/powerpc/power8/metrics.json
@@ -169,7 +169,7 @@
     },
     {
         "BriefDescription": "Cycles GCT empty where dispatch was held",
-        "MetricExpr": "(PM_GCT_NOSLOT_DISP_HELD_MAP + PM_GCT_NOSLOT_DISP_HELD_SRQ + PM_GCT_NOSLOT_DISP_HELD_ISSQ + PM_GCT_NOSLOT_DISP_HELD_OTHER) / PM_RUN_INST_CMPL)",
+        "MetricExpr": "(PM_GCT_NOSLOT_DISP_HELD_MAP + PM_GCT_NOSLOT_DISP_HELD_SRQ + PM_GCT_NOSLOT_DISP_HELD_ISSQ + PM_GCT_NOSLOT_DISP_HELD_OTHER) / PM_RUN_INST_CMPL",
         "MetricGroup": "cpi_breakdown",
         "MetricName": "gct_empty_disp_held_cpi"
     },
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 09/23] perf metrics: fix parse errors in power9 metrics
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (7 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 08/23] perf metrics: fix parse errors in power8 metrics Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 10/23] perf expr: print a debug message for division by zero Ian Rogers
                   ` (13 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers,
	Paul A . Clarke

Mismatched parentheses.

Fixes: 7f3cf5ac7743 (perf vendor events power9: Cpi_breakdown & estimated_dcache_miss_cpi metrics)
Reviewed-by: Paul A. Clarke <pc@us.ibm.com>
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.526.g744177e7f7-goog

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

* [RFC PATCH v2 10/23] perf expr: print a debug message for division by zero
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (8 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 09/23] perf metrics: fix parse errors in power9 metrics Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 11/23] perf parse-events: expand add PMU error/verbose messages Ian Rogers
                   ` (12 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, 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.526.g744177e7f7-goog

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

* [RFC PATCH v2 11/23] perf parse-events: expand add PMU error/verbose messages
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (9 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 10/23] perf expr: print a debug message for division by zero Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 12/23] perf test: improve pmu event metric testing Ian Rogers
                   ` (11 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

On a CPU like skylakex an uncore_iio_0 PMU may alias with
uncore_iio_free_running_0. The latter PMU doesn't support fc_mask
as a parameter and so pmu_config_term fails. Typically
parse_events_add_pmu is called in a loop where if one alias succeeds
errors are ignored, however, if multiple errors occur
parse_events__handle_error will currently give a WARN_ONCE.

This change removes the WARN_ONCE in parse_events__handle_error and
makes it a pr_debug. It adds verbose messages to parse_events_add_pmu
warning that non-fatal errors may occur, while giving details on the pmu
and config terms for useful context. pmu_config_term is altered so the
failing term and pmu are present in the case of the 'unknown term'
error which makes spotting the free_running case more straightforward.

Before:
$ perf --debug verbose=3 stat -M llc_misses.pcie_read sleep 1
Using CPUID GenuineIntel-6-55-4
metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
found event unc_iio_data_req_of_cpu.mem_read.part0
found event unc_iio_data_req_of_cpu.mem_read.part1
found event unc_iio_data_req_of_cpu.mem_read.part2
found event unc_iio_data_req_of_cpu.mem_read.part3
metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
found event unc_iio_data_req_of_cpu.mem_read.part0
found event unc_iio_data_req_of_cpu.mem_read.part1
found event unc_iio_data_req_of_cpu.mem_read.part2
found event unc_iio_data_req_of_cpu.mem_read.part3
adding {unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W,{unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W
intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
WARNING: multiple event parsing errors
...
Invalid event/parameter 'fc_mask'
...

After:
$ perf --debug verbose=3 stat -M llc_misses.pcie_read sleep 1
Using CPUID GenuineIntel-6-55-4
metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
found event unc_iio_data_req_of_cpu.mem_read.part0
found event unc_iio_data_req_of_cpu.mem_read.part1
found event unc_iio_data_req_of_cpu.mem_read.part2
found event unc_iio_data_req_of_cpu.mem_read.part3
metric expr unc_iio_data_req_of_cpu.mem_read.part0 + unc_iio_data_req_of_cpu.mem_read.part1 + unc_iio_data_req_of_cpu.mem_read.part2 + unc_iio_data_req_of_cpu.mem_read.part3 for LLC_MISSES.PCIE_READ
found event unc_iio_data_req_of_cpu.mem_read.part0
found event unc_iio_data_req_of_cpu.mem_read.part1
found event unc_iio_data_req_of_cpu.mem_read.part2
found event unc_iio_data_req_of_cpu.mem_read.part3
adding {unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W,{unc_iio_data_req_of_cpu.mem_read.part0,unc_iio_data_req_of_cpu.mem_read.part1,unc_iio_data_req_of_cpu.mem_read.part2,unc_iio_data_req_of_cpu.mem_read.part3}:W
intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
Attempting to add event pmu 'uncore_iio_free_running_5' with 'unc_iio_data_req_of_cpu.mem_read.part0,' that may result in non-fatal errors
After aliases, add event pmu 'uncore_iio_free_running_5' with 'fc_mask,ch_mask,umask,event,' that may result in non-fatal errors
Attempting to add event pmu 'uncore_iio_free_running_3' with 'unc_iio_data_req_of_cpu.mem_read.part0,' that may result in non-fatal errors
After aliases, add event pmu 'uncore_iio_free_running_3' with 'fc_mask,ch_mask,umask,event,' that may result in non-fatal errors
Attempting to add event pmu 'uncore_iio_free_running_1' with 'unc_iio_data_req_of_cpu.mem_read.part0,' that may result in non-fatal errors
After aliases, add event pmu 'uncore_iio_free_running_1' with 'fc_mask,ch_mask,umask,event,' that may result in non-fatal errors
Multiple errors dropping message: unknown term 'fc_mask' for pmu 'uncore_iio_free_running_3' (valid terms: event,umask,config,config1,config2,name,period,percore)
...

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/x86/util/intel-pt.c | 32 +++++++++++++++++-----------
 tools/perf/tests/pmu.c              |  4 ++--
 tools/perf/util/parse-events.c      | 29 ++++++++++++++++++++++++-
 tools/perf/util/pmu.c               | 33 ++++++++++++++++++-----------
 tools/perf/util/pmu.h               |  2 +-
 5 files changed, 72 insertions(+), 28 deletions(-)

diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c
index 3f7c20cc7b79..0f63fd2fa3ad 100644
--- a/tools/perf/arch/x86/util/intel-pt.c
+++ b/tools/perf/arch/x86/util/intel-pt.c
@@ -59,7 +59,8 @@ struct intel_pt_recording {
 	size_t				priv_size;
 };
 
-static int intel_pt_parse_terms_with_default(struct list_head *formats,
+static int intel_pt_parse_terms_with_default(const char *pmu_name,
+					     struct list_head *formats,
 					     const char *str,
 					     u64 *config)
 {
@@ -78,7 +79,8 @@ static int intel_pt_parse_terms_with_default(struct list_head *formats,
 		goto out_free;
 
 	attr.config = *config;
-	err = perf_pmu__config_terms(formats, &attr, terms, true, NULL);
+	err = perf_pmu__config_terms(pmu_name, formats, &attr, terms, true,
+				     NULL);
 	if (err)
 		goto out_free;
 
@@ -88,11 +90,12 @@ static int intel_pt_parse_terms_with_default(struct list_head *formats,
 	return err;
 }
 
-static int intel_pt_parse_terms(struct list_head *formats, const char *str,
-				u64 *config)
+static int intel_pt_parse_terms(const char *pmu_name, struct list_head *formats,
+				const char *str, u64 *config)
 {
 	*config = 0;
-	return intel_pt_parse_terms_with_default(formats, str, config);
+	return intel_pt_parse_terms_with_default(pmu_name, formats, str,
+						 config);
 }
 
 static u64 intel_pt_masked_bits(u64 mask, u64 bits)
@@ -229,7 +232,8 @@ static u64 intel_pt_default_config(struct perf_pmu *intel_pt_pmu)
 
 	pr_debug2("%s default config: %s\n", intel_pt_pmu->name, buf);
 
-	intel_pt_parse_terms(&intel_pt_pmu->format, buf, &config);
+	intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format, buf,
+			     &config);
 
 	return config;
 }
@@ -337,13 +341,16 @@ static int intel_pt_info_fill(struct auxtrace_record *itr,
 	if (priv_size != ptr->priv_size)
 		return -EINVAL;
 
-	intel_pt_parse_terms(&intel_pt_pmu->format, "tsc", &tsc_bit);
-	intel_pt_parse_terms(&intel_pt_pmu->format, "noretcomp",
-			     &noretcomp_bit);
-	intel_pt_parse_terms(&intel_pt_pmu->format, "mtc", &mtc_bit);
+	intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
+			     "tsc", &tsc_bit);
+	intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
+			     "noretcomp", &noretcomp_bit);
+	intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
+			     "mtc", &mtc_bit);
 	mtc_freq_bits = perf_pmu__format_bits(&intel_pt_pmu->format,
 					      "mtc_period");
-	intel_pt_parse_terms(&intel_pt_pmu->format, "cyc", &cyc_bit);
+	intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
+			     "cyc", &cyc_bit);
 
 	intel_pt_tsc_ctc_ratio(&tsc_ctc_ratio_n, &tsc_ctc_ratio_d);
 
@@ -769,7 +776,8 @@ static int intel_pt_recording_options(struct auxtrace_record *itr,
 		}
 	}
 
-	intel_pt_parse_terms(&intel_pt_pmu->format, "tsc", &tsc_bit);
+	intel_pt_parse_terms(intel_pt_pmu->name, &intel_pt_pmu->format,
+			     "tsc", &tsc_bit);
 
 	if (opts->full_auxtrace && (intel_pt_evsel->core.attr.config & tsc_bit))
 		have_timing_info = true;
diff --git a/tools/perf/tests/pmu.c b/tools/perf/tests/pmu.c
index 74379ff1f7fa..5c11fe2b3040 100644
--- a/tools/perf/tests/pmu.c
+++ b/tools/perf/tests/pmu.c
@@ -156,8 +156,8 @@ int test__pmu(struct test *test __maybe_unused, int subtest __maybe_unused)
 		if (ret)
 			break;
 
-		ret = perf_pmu__config_terms(&formats, &attr, terms,
-					     false, NULL);
+		ret = perf_pmu__config_terms("perf-pmu-test", &formats, &attr,
+					     terms, false, NULL);
 		if (ret)
 			break;
 
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index b7a0518d607d..17c42de24e8e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -204,7 +204,8 @@ void parse_events__handle_error(struct parse_events_error *err, int idx,
 		err->help = help;
 		break;
 	default:
-		WARN_ONCE(1, "WARNING: multiple event parsing errors\n");
+		pr_debug("Multiple errors dropping message: %s (%s)\n",
+			err->str, err->help);
 		free(err->str);
 		err->str = str;
 		free(err->help);
@@ -1424,6 +1425,19 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 	bool use_uncore_alias;
 	LIST_HEAD(config_terms);
 
+	if (verbose > 1) {
+		fprintf(stderr, "Attempting to add event pmu '%s' with '",
+			name);
+		if (head_config) {
+			struct parse_events_term *term;
+
+			list_for_each_entry(term, head_config, list) {
+				fprintf(stderr, "%s,", term->config);
+			}
+		}
+		fprintf(stderr, "' that may result in non-fatal errors\n");
+	}
+
 	pmu = perf_pmu__find(name);
 	if (!pmu) {
 		char *err_str;
@@ -1460,6 +1474,19 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 	if (perf_pmu__check_alias(pmu, head_config, &info))
 		return -EINVAL;
 
+	if (verbose > 1) {
+		fprintf(stderr, "After aliases, add event pmu '%s' with '",
+			name);
+		if (head_config) {
+			struct parse_events_term *term;
+
+			list_for_each_entry(term, head_config, list) {
+				fprintf(stderr, "%s,", term->config);
+			}
+		}
+		fprintf(stderr, "' that may result in non-fatal errors\n");
+	}
+
 	/*
 	 * Configure hardcoded terms first, no need to check
 	 * return value when called with fail == 0 ;)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 92bd7fafcce6..71d0290b616a 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1056,7 +1056,8 @@ static char *pmu_formats_string(struct list_head *formats)
  * Setup one of config[12] attr members based on the
  * user input data - term parameter.
  */
-static int pmu_config_term(struct list_head *formats,
+static int pmu_config_term(const char *pmu_name,
+			   struct list_head *formats,
 			   struct perf_event_attr *attr,
 			   struct parse_events_term *term,
 			   struct list_head *head_terms,
@@ -1082,16 +1083,24 @@ static int pmu_config_term(struct list_head *formats,
 
 	format = pmu_find_format(formats, term->config);
 	if (!format) {
-		if (verbose > 0)
-			printf("Invalid event/parameter '%s'\n", term->config);
+		char *pmu_term = pmu_formats_string(formats);
+		char *unknown_term;
+		char *help_msg;
+
+		if (asprintf(&unknown_term,
+				"unknown term '%s' for pmu '%s'",
+				term->config, pmu_name) < 0)
+			unknown_term = strdup("unknown term");
+		help_msg = parse_events_formats_error_string(pmu_term);
 		if (err) {
-			char *pmu_term = pmu_formats_string(formats);
-
 			parse_events__handle_error(err, term->err_term,
-				strdup("unknown term"),
-				parse_events_formats_error_string(pmu_term));
-			free(pmu_term);
+						   unknown_term,
+						   help_msg);
+		} else {
+			pr_debug("%s (%s)\n", unknown_term, help_msg);
+			free(unknown_term);
 		}
+		free(pmu_term);
 		return -EINVAL;
 	}
 
@@ -1168,7 +1177,7 @@ static int pmu_config_term(struct list_head *formats,
 	return 0;
 }
 
-int perf_pmu__config_terms(struct list_head *formats,
+int perf_pmu__config_terms(const char *pmu_name, struct list_head *formats,
 			   struct perf_event_attr *attr,
 			   struct list_head *head_terms,
 			   bool zero, struct parse_events_error *err)
@@ -1176,7 +1185,7 @@ int perf_pmu__config_terms(struct list_head *formats,
 	struct parse_events_term *term;
 
 	list_for_each_entry(term, head_terms, list) {
-		if (pmu_config_term(formats, attr, term, head_terms,
+		if (pmu_config_term(pmu_name, formats, attr, term, head_terms,
 				    zero, err))
 			return -EINVAL;
 	}
@@ -1196,8 +1205,8 @@ int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
 	bool zero = !!pmu->default_config;
 
 	attr->type = pmu->type;
-	return perf_pmu__config_terms(&pmu->format, attr, head_terms,
-				      zero, err);
+	return perf_pmu__config_terms(pmu->name, &pmu->format, attr,
+				      head_terms, zero, err);
 }
 
 static struct perf_pmu_alias *pmu_find_alias(struct perf_pmu *pmu,
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index cb6fbec50313..cd85689977b4 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -76,7 +76,7 @@ struct perf_pmu *perf_pmu__find_by_type(unsigned int type);
 int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
 		     struct list_head *head_terms,
 		     struct parse_events_error *error);
-int perf_pmu__config_terms(struct list_head *formats,
+int perf_pmu__config_terms(const char *pmu_name, struct list_head *formats,
 			   struct perf_event_attr *attr,
 			   struct list_head *head_terms,
 			   bool zero, struct parse_events_error *error);
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 12/23] perf test: improve pmu event metric testing
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (10 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 11/23] perf parse-events: expand add PMU error/verbose messages Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 13/23] lib/bpf hashmap: increase portability Ian Rogers
                   ` (10 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

Add a basic floating point number test to expr.
Break pmu-events test into 2 and add a test to verify that all pmu metric
expressions simply parse. Try to parse all metric ids/events, failing if
metrics for the current architecture fail to parse.

Tested on skylakex with the patch set in place. May fail on other
architectures if metrics are invalid.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/builtin-test.c |   5 +
 tools/perf/tests/expr.c         |   1 +
 tools/perf/tests/pmu-events.c   | 158 ++++++++++++++++++++++++++++++--
 tools/perf/tests/tests.h        |   2 +
 4 files changed, 160 insertions(+), 6 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 3471ec52ea11..8147c17c71ab 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -75,6 +75,11 @@ static struct test generic_tests[] = {
 	{
 		.desc = "PMU events",
 		.func = test__pmu_events,
+		.subtest = {
+			.get_nr		= test__pmu_events_subtest_get_nr,
+			.get_desc	= test__pmu_events_subtest_get_desc,
+		},
+
 	},
 	{
 		.desc = "DSO data read",
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index f9e8e5628836..3f742612776a 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -39,6 +39,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 	ret |= test(&ctx, "min(1,2) + 1", 2);
 	ret |= test(&ctx, "max(1,2) + 1", 3);
 	ret |= test(&ctx, "1+1 if 3*4 else 0", 2);
+	ret |= test(&ctx, "1.1 + 2.1", 3.2);
 
 	if (ret)
 		return ret;
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index d64261da8bf7..c18b9ce8cace 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -8,6 +8,10 @@
 #include <linux/zalloc.h>
 #include "debug.h"
 #include "../pmu-events/pmu-events.h"
+#include "util/evlist.h"
+#include "util/expr.h"
+#include "util/parse-events.h"
+#include <ctype.h>
 
 struct perf_pmu_test_event {
 	struct pmu_event event;
@@ -144,7 +148,7 @@ static struct pmu_events_map *__test_pmu_get_events_map(void)
 }
 
 /* Verify generated events from pmu-events.c is as expected */
-static int __test_pmu_event_table(void)
+static int test_pmu_event_table(void)
 {
 	struct pmu_events_map *map = __test_pmu_get_events_map();
 	struct pmu_event *table;
@@ -347,14 +351,11 @@ static int __test__pmu_event_aliases(char *pmu_name, int *count)
 	return res;
 }
 
-int test__pmu_events(struct test *test __maybe_unused,
-		     int subtest __maybe_unused)
+
+static int test_aliases(void)
 {
 	struct perf_pmu *pmu = NULL;
 
-	if (__test_pmu_event_table())
-		return -1;
-
 	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
 		int count = 0;
 
@@ -377,3 +378,148 @@ int test__pmu_events(struct test *test __maybe_unused,
 
 	return 0;
 }
+
+static bool is_number(const char *str)
+{
+	size_t i;
+
+	for (i = 0; i < strlen(str); i++) {
+		if (!isdigit(str[i]) && str[i] != '.')
+			return false;
+	}
+	return true;
+}
+
+static int check_parse_id(const char *id, bool same_cpu, struct pmu_event *pe)
+{
+	struct parse_events_error error;
+	struct evlist *evlist;
+	int ret;
+
+	/* Numbers are always valid. */
+	if (is_number(id))
+		return 0;
+
+	evlist = evlist__new();
+	memset(&error, 0, sizeof(error));
+	ret = parse_events(evlist, id, &error);
+	if (ret && same_cpu) {
+		fprintf(stderr,
+			"\nWARNING: Parse event failed metric '%s' id '%s' expr '%s'\n",
+			pe->metric_name, id, pe->metric_expr);
+		fprintf(stderr, "Error string '%s' help '%s'\n",
+			error.str, error.help);
+	} else if (ret) {
+		pr_debug3("Parse event failed, but for an event that may not be supported by this CPU.\nid '%s' metric '%s' expr '%s'\n",
+			id, pe->metric_name, pe->metric_expr);
+	}
+	evlist__delete(evlist);
+	free(error.str);
+	free(error.help);
+	free(error.first_str);
+	free(error.first_help);
+	/* TODO: too many metrics are broken to fail on this test currently. */
+	return 0;
+}
+
+static int test_parsing(void)
+{
+	struct pmu_events_map *cpus_map = perf_pmu__find_map(NULL);
+	struct pmu_events_map *map;
+	struct pmu_event *pe;
+	int i, j, k;
+	const char **ids;
+	int idnum;
+	int ret = 0;
+	struct expr_parse_ctx ctx;
+	double result;
+
+	i = 0;
+	for (;;) {
+		map = &pmu_events_map[i++];
+		if (!map->table) {
+			map = NULL;
+			break;
+		}
+		j = 0;
+		for (;;) {
+			pe = &map->table[j++];
+			if (!pe->name && !pe->metric_group && !pe->metric_name)
+				break;
+			if (!pe->metric_expr)
+				continue;
+			if (expr__find_other(pe->metric_expr, NULL,
+						&ids, &idnum, 0) < 0) {
+				pr_debug("Parse other failed for map %s %s %s\n",
+					map->cpuid, map->version, map->type);
+				pr_debug("On metric %s\n", pe->metric_name);
+				pr_debug("On expression %s\n", pe->metric_expr);
+				ret++;
+				continue;
+			}
+			expr__ctx_init(&ctx);
+
+			/*
+			 * Add all ids with a made up value. The value may
+			 * trigger divide by zero when subtracted and so try to
+			 * make them unique.
+			 */
+			for (k = 0; k < idnum; k++)
+				expr__add_id(&ctx, ids[k], k + 1);
+
+			for (k = 0; k < idnum; k++) {
+				if (check_parse_id(ids[k], map == cpus_map, pe))
+					ret++;
+			}
+
+			if (expr__parse(&result, &ctx, pe->metric_expr, 0)) {
+				pr_debug("Parse failed for map %s %s %s\n",
+					map->cpuid, map->version, map->type);
+				pr_debug("On metric %s\n", pe->metric_name);
+				pr_debug("On expression %s\n", pe->metric_expr);
+				ret++;
+			}
+			for (k = 0; k < idnum; k++)
+				zfree(&ids[k]);
+			free(ids);
+		}
+	}
+	return ret;
+}
+
+static const struct {
+	int (*func)(void);
+	const char *desc;
+} pmu_events_testcase_table[] = {
+	{
+		.func = test_pmu_event_table,
+		.desc = "PMU event table sanity",
+	},
+	{
+		.func = test_aliases,
+		.desc = "PMU event map aliases",
+	},
+	{
+		.func = test_parsing,
+		.desc = "Parsing of PMU event table metrics",
+	},
+};
+
+const char *test__pmu_events_subtest_get_desc(int i)
+{
+	if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return NULL;
+	return pmu_events_testcase_table[i].desc;
+}
+
+int test__pmu_events_subtest_get_nr(void)
+{
+	return (int)ARRAY_SIZE(pmu_events_testcase_table);
+}
+
+int test__pmu_events(struct test *test __maybe_unused, int i)
+{
+	if (i < 0 || i >= (int)ARRAY_SIZE(pmu_events_testcase_table))
+		return TEST_FAIL;
+	return pmu_events_testcase_table[i].func();
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index d6d4ac34eeb7..8e316c30ed3c 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -50,6 +50,8 @@ int test__perf_evsel__tp_sched_test(struct test *test, int subtest);
 int test__syscall_openat_tp_fields(struct test *test, int subtest);
 int test__pmu(struct test *test, int subtest);
 int test__pmu_events(struct test *test, int subtest);
+const char *test__pmu_events_subtest_get_desc(int subtest);
+int test__pmu_events_subtest_get_nr(void);
 int test__attr(struct test *test, int subtest);
 int test__dso_data(struct test *test, int subtest);
 int test__dso_data_cache(struct test *test, int subtest);
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 13/23] lib/bpf hashmap: increase portability
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (11 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 12/23] perf test: improve pmu event metric testing Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 14/23] libbpf: Fix memory leak and possible double-free in hashmap__clear Ian Rogers
                   ` (9 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

Don't include libbpf_internal.h as it is unused and has conflicting
definitions, for example, with tools/perf/util/debug.h.
Fix a non-glibc include path.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/bpf/hashmap.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h
index bae8879cdf58..d5ef212a55ba 100644
--- a/tools/lib/bpf/hashmap.h
+++ b/tools/lib/bpf/hashmap.h
@@ -13,9 +13,8 @@
 #ifdef __GLIBC__
 #include <bits/wordsize.h>
 #else
-#include <bits/reg.h>
+#include <linux/bitops.h>
 #endif
-#include "libbpf_internal.h"
 
 static inline size_t hash_bits(size_t h, int bits)
 {
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 14/23] libbpf: Fix memory leak and possible double-free in hashmap__clear
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (12 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 13/23] lib/bpf hashmap: increase portability Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 15/23] perf expr: fix memory leaks in bison Ian Rogers
                   ` (8 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Alston Tang, Ian Rogers

From: Andrii Nakryiko <andriin@fb.com>

Fix memory leak in hashmap_clear() not freeing hashmap_entry structs for each
of the remaining entries. Also NULL-out bucket list to prevent possible
double-free between hashmap__clear() and hashmap__free().

Running test_progs-asan flavor clearly showed this problem.

Reported-by: Alston Tang <alston64@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Link: https://lore.kernel.org/bpf/20200429012111.277390-5-andriin@fb.com
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/bpf/hashmap.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/lib/bpf/hashmap.c b/tools/lib/bpf/hashmap.c
index 54c30c802070..cffb96202e0d 100644
--- a/tools/lib/bpf/hashmap.c
+++ b/tools/lib/bpf/hashmap.c
@@ -59,7 +59,14 @@ struct hashmap *hashmap__new(hashmap_hash_fn hash_fn,
 
 void hashmap__clear(struct hashmap *map)
 {
+	struct hashmap_entry *cur, *tmp;
+	int bkt;
+
+	hashmap__for_each_entry_safe(map, cur, tmp, bkt) {
+		free(cur);
+	}
 	free(map->buckets);
+	map->buckets = NULL;
 	map->cap = map->cap_bits = map->sz = 0;
 }
 
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 15/23] perf expr: fix memory leaks in bison
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (13 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 14/23] libbpf: Fix memory leak and possible double-free in hashmap__clear Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 16/23] perf evsel: fix 2 memory leaks Ian Rogers
                   ` (7 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

Add a destructor for strings to reclaim memory in the event of errors.
Free the ID given for a lookup.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/expr.y | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 21e82a1e11a2..3b49b230b111 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -27,6 +27,7 @@
 %token EXPR_PARSE EXPR_OTHER EXPR_ERROR
 %token <num> NUMBER
 %token <str> ID
+%destructor { free ($$); } <str>
 %token MIN MAX IF ELSE SMT_ON
 %left MIN MAX IF
 %left '|'
@@ -94,8 +95,10 @@ if_expr:
 expr:	  NUMBER
 	| ID			{ if (lookup_id(ctx, $1, &$$) < 0) {
 					pr_debug("%s not found\n", $1);
+					free($1);
 					YYABORT;
 				  }
+				  free($1);
 				}
 	| expr '|' expr		{ $$ = (long)$1 | (long)$3; }
 	| expr '&' expr		{ $$ = (long)$1 & (long)$3; }
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 16/23] perf evsel: fix 2 memory leaks
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (14 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 15/23] perf expr: fix memory leaks in bison Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 17/23] perf expr: migrate expr ids table to libbpf's hashmap Ian Rogers
                   ` (6 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

If allocated, perf_pkg_mask and metric_events need freeing.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index f3e60c45d59a..d5c28e583986 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1268,6 +1268,8 @@ void evsel__exit(struct evsel *evsel)
 	zfree(&evsel->group_name);
 	zfree(&evsel->name);
 	zfree(&evsel->pmu_name);
+	zfree(&evsel->per_pkg_mask);
+	zfree(&evsel->metric_events);
 	perf_evsel__object.fini(evsel);
 }
 
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 17/23] perf expr: migrate expr ids table to libbpf's hashmap
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (15 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 16/23] perf evsel: fix 2 memory leaks Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 18/23] perf metricgroup: change evlist_used to a bitmap Ian Rogers
                   ` (5 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

Use a hashmap between a char* string and a double* value. While bpf's
hashmap entries are size_t in size, we can't guarantee sizeof(size_t) >=
sizeof(double). Avoid a memory allocation when gathering ids by making 0.0
a special value encoded as NULL.

Suggested by Andi Kleen:
https://lore.kernel.org/lkml/20200224210308.GQ160988@tassilo.jf.intel.com/
and seconded by Jiri Olsa:
https://lore.kernel.org/lkml/20200423112915.GH1136647@krava/

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/expr.c       |  32 ++++-----
 tools/perf/tests/pmu-events.c |  22 +++---
 tools/perf/util/expr.c        | 125 ++++++++++++++++++----------------
 tools/perf/util/expr.h        |  22 +++---
 tools/perf/util/expr.y        |  22 +-----
 tools/perf/util/metricgroup.c |  86 +++++++++++------------
 tools/perf/util/stat-shadow.c |  46 ++++++++-----
 7 files changed, 176 insertions(+), 179 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 3f742612776a..bb948226be1d 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -19,11 +19,9 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
 int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 {
 	const char *p;
-	const char **other;
-	double val;
-	int i, ret;
+	double val, *val_ptr;
+	int ret;
 	struct expr_parse_ctx ctx;
-	int num_other;
 
 	expr__ctx_init(&ctx);
 	expr__add_id(&ctx, "FOO", 1);
@@ -52,25 +50,23 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 	ret = expr__parse(&val, &ctx, p, 1);
 	TEST_ASSERT_VAL("missing operand", ret == -1);
 
+	hashmap__clear(&ctx.ids);
 	TEST_ASSERT_VAL("find other",
-			expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", &other, &num_other, 1) == 0);
-	TEST_ASSERT_VAL("find other", num_other == 3);
-	TEST_ASSERT_VAL("find other", !strcmp(other[0], "BAR"));
-	TEST_ASSERT_VAL("find other", !strcmp(other[1], "BAZ"));
-	TEST_ASSERT_VAL("find other", !strcmp(other[2], "BOZO"));
-	TEST_ASSERT_VAL("find other", other[3] == NULL);
+			expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", &ctx, 1) == 0);
+	TEST_ASSERT_VAL("find other", hashmap__size(&ctx.ids) == 3);
+	TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "BAR", (void**)&val_ptr));
+	TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "BAZ", (void**)&val_ptr));
+	TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "BOZO", (void**)&val_ptr));
 
+	hashmap__clear(&ctx.ids);
 	TEST_ASSERT_VAL("find other",
 			expr__find_other("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@", NULL,
-				   &other, &num_other, 3) == 0);
-	TEST_ASSERT_VAL("find other", num_other == 2);
-	TEST_ASSERT_VAL("find other", !strcmp(other[0], "EVENT1,param=3/"));
-	TEST_ASSERT_VAL("find other", !strcmp(other[1], "EVENT2,param=3/"));
-	TEST_ASSERT_VAL("find other", other[2] == NULL);
+				   &ctx, 3) == 0);
+	TEST_ASSERT_VAL("find other", hashmap__size(&ctx.ids) == 2);
+	TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "EVENT1,param=3/", (void**)&val_ptr));
+	TEST_ASSERT_VAL("find other", hashmap__find(&ctx.ids, "EVENT2,param=3/", (void**)&val_ptr));
 
-	for (i = 0; i < num_other; i++)
-		zfree(&other[i]);
-	free((void *)other);
+	expr__ctx_clear(&ctx);
 
 	return 0;
 }
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index c18b9ce8cace..6ce6b8a31e1f 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -428,8 +428,6 @@ static int test_parsing(void)
 	struct pmu_events_map *map;
 	struct pmu_event *pe;
 	int i, j, k;
-	const char **ids;
-	int idnum;
 	int ret = 0;
 	struct expr_parse_ctx ctx;
 	double result;
@@ -443,13 +441,16 @@ static int test_parsing(void)
 		}
 		j = 0;
 		for (;;) {
+			struct hashmap_entry *cur;
+			size_t bkt;
+
 			pe = &map->table[j++];
 			if (!pe->name && !pe->metric_group && !pe->metric_name)
 				break;
 			if (!pe->metric_expr)
 				continue;
-			if (expr__find_other(pe->metric_expr, NULL,
-						&ids, &idnum, 0) < 0) {
+			if (expr__find_other(pe->metric_expr, NULL, &ctx, 0)
+				  < 0) {
 				pr_debug("Parse other failed for map %s %s %s\n",
 					map->cpuid, map->version, map->type);
 				pr_debug("On metric %s\n", pe->metric_name);
@@ -464,11 +465,12 @@ static int test_parsing(void)
 			 * trigger divide by zero when subtracted and so try to
 			 * make them unique.
 			 */
-			for (k = 0; k < idnum; k++)
-				expr__add_id(&ctx, ids[k], k + 1);
+			k = 1;
+			hashmap__for_each_entry((&ctx.ids), cur, bkt)
+				expr__add_id(&ctx, cur->key, k++);
 
-			for (k = 0; k < idnum; k++) {
-				if (check_parse_id(ids[k], map == cpus_map, pe))
+			hashmap__for_each_entry((&ctx.ids), cur, bkt) {
+				if (check_parse_id(cur->key, map == cpus_map, pe))
 					ret++;
 			}
 
@@ -479,9 +481,7 @@ static int test_parsing(void)
 				pr_debug("On expression %s\n", pe->metric_expr);
 				ret++;
 			}
-			for (k = 0; k < idnum; k++)
-				zfree(&ids[k]);
-			free(ids);
+			expr__ctx_clear(&ctx);
 		}
 	}
 	return ret;
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 8b4ce704a68d..70e4e468d7c7 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -4,25 +4,73 @@
 #include "expr.h"
 #include "expr-bison.h"
 #include "expr-flex.h"
+#include <linux/kernel.h>
 
 #ifdef PARSER_DEBUG
 extern int expr_debug;
 #endif
 
+static size_t double_hash(const void *key, void *ctx __maybe_unused)
+{
+	const char *str = (const char*)key;
+	size_t hash = 0;
+	while (*str != '\0') {
+		hash <<= 31;
+		hash += *str;
+		str++;
+	}
+	return hash;
+}
+
+static bool double_equal(const void *key1, const void *key2,
+		    void *ctx __maybe_unused)
+{
+	return !strcmp((const char*)key1, (const char*)key2);
+}
+
 /* Caller must make sure id is allocated */
-void expr__add_id(struct expr_parse_ctx *ctx, const char *name, double val)
+int expr__add_id(struct expr_parse_ctx *ctx, const char *name, double val)
 {
-	int idx;
+	double *val_ptr = NULL, *old_val = NULL;
+	char *old_key = NULL;
+	int ret;
 
-	assert(ctx->num_ids < MAX_PARSE_ID);
-	idx = ctx->num_ids++;
-	ctx->ids[idx].name = name;
-	ctx->ids[idx].val = val;
+	if (val != 0.0) {
+		val_ptr = malloc(sizeof(double));
+		if (!val_ptr)
+			return -ENOMEM;
+		*val_ptr = val;
+	}
+	ret = hashmap__set(&ctx->ids, name, val_ptr, (const void**)&old_key, (void**)&old_val);
+	free(old_key);
+	free(old_val);
+	return ret;
+}
+
+int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr)
+{
+	double *data;
+	if (!hashmap__find(&ctx->ids, id, (void**)&data))
+		return -1;
+	*val_ptr = (data == NULL) ?  0.0 : *data;
+	return 0;
 }
 
 void expr__ctx_init(struct expr_parse_ctx *ctx)
 {
-	ctx->num_ids = 0;
+	hashmap__init(&ctx->ids, double_hash, double_equal, NULL);
+}
+
+void expr__ctx_clear(struct expr_parse_ctx *ctx)
+{
+	struct hashmap_entry *cur;
+	size_t bkt;
+
+	hashmap__for_each_entry((&ctx->ids), cur, bkt) {
+		free((char*)cur->key);
+		free(cur->value);
+	}
+	hashmap__clear(&ctx->ids);
 }
 
 static int
@@ -56,61 +104,24 @@ __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
 	return ret;
 }
 
-int expr__parse(double *final_val, struct expr_parse_ctx *ctx, const char *expr, int runtime)
+int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
+		const char *expr, int runtime)
 {
 	return __expr__parse(final_val, ctx, expr, EXPR_PARSE, runtime) ? -1 : 0;
 }
 
-static bool
-already_seen(const char *val, const char *one, const char **other,
-	     int num_other)
+int expr__find_other(const char *expr, const char *one,
+		     struct expr_parse_ctx *ctx, int runtime)
 {
-	int i;
-
-	if (one && !strcasecmp(one, val))
-		return true;
-	for (i = 0; i < num_other; i++)
-		if (!strcasecmp(other[i], val))
-			return true;
-	return false;
-}
-
-int expr__find_other(const char *expr, const char *one, const char ***other,
-		     int *num_other, int runtime)
-{
-	int err, i = 0, j = 0;
-	struct expr_parse_ctx ctx;
-
-	expr__ctx_init(&ctx);
-	err = __expr__parse(NULL, &ctx, expr, EXPR_OTHER, runtime);
-	if (err)
-		return -1;
-
-	*other = malloc((ctx.num_ids + 1) * sizeof(char *));
-	if (!*other)
-		return -ENOMEM;
-
-	for (i = 0, j = 0; i < ctx.num_ids; i++) {
-		const char *str = ctx.ids[i].name;
-
-		if (already_seen(str, one, *other, j))
-			continue;
-
-		str = strdup(str);
-		if (!str)
-			goto out;
-		(*other)[j++] = str;
-	}
-	(*other)[j] = NULL;
-
-out:
-	if (i != ctx.num_ids) {
-		while (--j)
-			free((char *) (*other)[i]);
-		free(*other);
-		err = -1;
+	double *old_val = NULL;
+	char *old_key = NULL;
+	int ret = __expr__parse(NULL, ctx, expr, EXPR_OTHER, runtime);
+
+	if (one) {
+		hashmap__delete(&ctx->ids, one, (const void**)&old_key, (void**)&old_val);
+		free(old_key);
+		free(old_val);
 	}
 
-	*num_other = j;
-	return err;
+	return ret;
 }
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 40fc452b0f2b..f01bd5ecb09d 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -2,17 +2,10 @@
 #ifndef PARSE_CTX_H
 #define PARSE_CTX_H 1
 
-#define EXPR_MAX_OTHER 64
-#define MAX_PARSE_ID EXPR_MAX_OTHER
-
-struct expr_parse_id {
-	const char *name;
-	double val;
-};
+#include <bpf/hashmap.h>
 
 struct expr_parse_ctx {
-	int num_ids;
-	struct expr_parse_id ids[MAX_PARSE_ID];
+	struct hashmap ids;
 };
 
 struct expr_scanner_ctx {
@@ -21,9 +14,12 @@ struct expr_scanner_ctx {
 };
 
 void expr__ctx_init(struct expr_parse_ctx *ctx);
-void expr__add_id(struct expr_parse_ctx *ctx, const char *id, double val);
-int expr__parse(double *final_val, struct expr_parse_ctx *ctx, const char *expr, int runtime);
-int expr__find_other(const char *expr, const char *one, const char ***other,
-		int *num_other, int runtime);
+void expr__ctx_clear(struct expr_parse_ctx *ctx);
+int expr__add_id(struct expr_parse_ctx *ctx, const char *id, double val);
+int expr__get_id(struct expr_parse_ctx *ctx, const char *id, double *val_ptr);
+int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
+		const char *expr, int runtime);
+int expr__find_other(const char *expr, const char *one,
+		struct expr_parse_ctx *ids, int runtime);
 
 #endif
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index 3b49b230b111..bf3e898e3055 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -47,19 +47,6 @@ static void expr_error(double *final_val __maybe_unused,
 	pr_debug("%s\n", s);
 }
 
-static int lookup_id(struct expr_parse_ctx *ctx, char *id, double *val)
-{
-	int i;
-
-	for (i = 0; i < ctx->num_ids; i++) {
-		if (!strcasecmp(ctx->ids[i].name, id)) {
-			*val = ctx->ids[i].val;
-			return 0;
-		}
-	}
-	return -1;
-}
-
 %}
 %%
 
@@ -73,12 +60,7 @@ all_other: all_other other
 
 other: ID
 {
-	if (ctx->num_ids + 1 >= EXPR_MAX_OTHER) {
-		pr_err("failed: way too many variables");
-		YYABORT;
-	}
-
-	ctx->ids[ctx->num_ids++].name = $1;
+	expr__add_id(ctx, $1, 0.0);
 }
 |
 MIN | MAX | IF | ELSE | SMT_ON | NUMBER | '|' | '^' | '&' | '-' | '+' | '*' | '/' | '%' | '(' | ')' | ','
@@ -93,7 +75,7 @@ if_expr:
 	;
 
 expr:	  NUMBER
-	| ID			{ if (lookup_id(ctx, $1, &$$) < 0) {
+	| ID			{ if (expr__get_id(ctx, $1, &$$)) {
 					pr_debug("%s not found\n", $1);
 					free($1);
 					YYABORT;
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index b071df373f8b..2f92dbc05226 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -85,8 +85,7 @@ static void metricgroup__rblist_init(struct rblist *metric_events)
 
 struct egroup {
 	struct list_head nd;
-	int idnum;
-	const char **ids;
+	struct expr_parse_ctx pctx;
 	const char *metric_name;
 	const char *metric_expr;
 	const char *metric_unit;
@@ -94,19 +93,21 @@ struct egroup {
 };
 
 static struct evsel *find_evsel_group(struct evlist *perf_evlist,
-				      const char **ids,
-				      int idnum,
+				      struct expr_parse_ctx *pctx,
 				      struct evsel **metric_events,
 				      bool *evlist_used)
 {
 	struct evsel *ev;
-	int i = 0, j = 0;
 	bool leader_found;
+	const size_t idnum = hashmap__size(&pctx->ids);
+	size_t i = 0;
+	int j = 0;
+	double *val_ptr;
 
 	evlist__for_each_entry (perf_evlist, ev) {
 		if (evlist_used[j++])
 			continue;
-		if (!strcmp(ev->name, ids[i])) {
+		if (hashmap__find(&pctx->ids, ev->name, (void**)&val_ptr)) {
 			if (!metric_events[i])
 				metric_events[i] = ev;
 			i++;
@@ -118,7 +119,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 			memset(metric_events, 0,
 				sizeof(struct evsel *) * idnum);
 
-			if (!strcmp(ev->name, ids[i])) {
+			if (hashmap__find(&pctx->ids, ev->name, (void**)&val_ptr)) {
 				if (!metric_events[i])
 					metric_events[i] = ev;
 				i++;
@@ -175,19 +176,20 @@ static int metricgroup__setup_events(struct list_head *groups,
 	list_for_each_entry (eg, groups, nd) {
 		struct evsel **metric_events;
 
-		metric_events = calloc(sizeof(void *), eg->idnum + 1);
+		metric_events = calloc(sizeof(void *),
+				hashmap__size(&eg->pctx.ids) + 1);
 		if (!metric_events) {
 			ret = -ENOMEM;
 			break;
 		}
-		evsel = find_evsel_group(perf_evlist, eg->ids, eg->idnum,
-					 metric_events, evlist_used);
+		evsel = find_evsel_group(perf_evlist, &eg->pctx, metric_events,
+					evlist_used);
 		if (!evsel) {
 			pr_debug("Cannot resolve %s: %s\n",
 					eg->metric_name, eg->metric_expr);
 			continue;
 		}
-		for (i = 0; i < eg->idnum; i++)
+		for (i = 0; metric_events[i]; i++)
 			metric_events[i]->collect_stat = true;
 		me = metricgroup__lookup(metric_events_list, evsel, true);
 		if (!me) {
@@ -415,20 +417,20 @@ void metricgroup__print(bool metrics, bool metricgroups, char *filter,
 }
 
 static void metricgroup__add_metric_weak_group(struct strbuf *events,
-					       const char **ids,
-					       int idnum)
+					       struct expr_parse_ctx *ctx)
 {
+	struct hashmap_entry *cur;
+	size_t bkt, i = 0;
 	bool no_group = false;
-	int i;
 
-	for (i = 0; i < idnum; i++) {
-		pr_debug("found event %s\n", ids[i]);
+	hashmap__for_each_entry((&ctx->ids), cur, bkt) {
+		pr_debug("found event %s\n", (const char*)cur->key);
 		/*
 		 * Duration time maps to a software event and can make
 		 * groups not count. Always use it outside a
 		 * group.
 		 */
-		if (!strcmp(ids[i], "duration_time")) {
+		if (!strcmp(cur->key, "duration_time")) {
 			if (i > 0)
 				strbuf_addf(events, "}:W,");
 			strbuf_addf(events, "duration_time");
@@ -437,21 +439,22 @@ static void metricgroup__add_metric_weak_group(struct strbuf *events,
 		}
 		strbuf_addf(events, "%s%s",
 			i == 0 || no_group ? "{" : ",",
-			ids[i]);
+			(const char*)cur->key);
 		no_group = false;
+		i++;
 	}
 	if (!no_group)
 		strbuf_addf(events, "}:W");
 }
 
 static void metricgroup__add_metric_non_group(struct strbuf *events,
-					      const char **ids,
-					      int idnum)
+					      struct expr_parse_ctx *ctx)
 {
-	int i;
+	struct hashmap_entry *cur;
+	size_t bkt;
 
-	for (i = 0; i < idnum; i++)
-		strbuf_addf(events, ",%s", ids[i]);
+	hashmap__for_each_entry((&ctx->ids), cur, bkt)
+		strbuf_addf(events, ",%s", (const char*)cur->key);
 }
 
 static void metricgroup___watchdog_constraint_hint(const char *name, bool foot)
@@ -495,32 +498,32 @@ int __weak arch_get_runtimeparam(void)
 static int __metricgroup__add_metric(struct strbuf *events,
 		struct list_head *group_list, struct pmu_event *pe, int runtime)
 {
-
-	const char **ids;
-	int idnum;
 	struct egroup *eg;
 
-	if (expr__find_other(pe->metric_expr, NULL, &ids, &idnum, runtime) < 0)
-		return -EINVAL;
-
-	if (events->len > 0)
-		strbuf_addf(events, ",");
-
-	if (metricgroup__has_constraint(pe))
-		metricgroup__add_metric_non_group(events, ids, idnum);
-	else
-		metricgroup__add_metric_weak_group(events, ids, idnum);
-
 	eg = malloc(sizeof(*eg));
 	if (!eg)
 		return -ENOMEM;
 
-	eg->ids = ids;
-	eg->idnum = idnum;
+	expr__ctx_init(&eg->pctx);
 	eg->metric_name = pe->metric_name;
 	eg->metric_expr = pe->metric_expr;
 	eg->metric_unit = pe->unit;
 	eg->runtime = runtime;
+
+	if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
+		expr__ctx_clear(&eg->pctx);
+		free(eg);
+		return -EINVAL;
+	}
+
+	if (events->len > 0)
+		strbuf_addf(events, ",");
+
+	if (metricgroup__has_constraint(pe))
+		metricgroup__add_metric_non_group(events, &eg->pctx);
+	else
+		metricgroup__add_metric_weak_group(events, &eg->pctx);
+
 	list_add_tail(&eg->nd, group_list);
 
 	return 0;
@@ -603,12 +606,9 @@ static int metricgroup__add_metric_list(const char *list, struct strbuf *events,
 static void metricgroup__free_egroups(struct list_head *group_list)
 {
 	struct egroup *eg, *egtmp;
-	int i;
 
 	list_for_each_entry_safe (eg, egtmp, group_list, nd) {
-		for (i = 0; i < eg->idnum; i++)
-			zfree(&eg->ids[i]);
-		zfree(&eg->ids);
+		expr__ctx_clear(&eg->pctx);
 		list_del_init(&eg->nd);
 		free(eg);
 	}
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 129b8c5f2538..f8ba66c305f5 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -323,35 +323,45 @@ void perf_stat__collect_metric_expr(struct evlist *evsel_list)
 {
 	struct evsel *counter, *leader, **metric_events, *oc;
 	bool found;
-	const char **metric_names;
+	struct expr_parse_ctx ctx;
+	struct hashmap_entry *cur;
+	size_t bkt;
 	int i;
-	int num_metric_names;
 
+	expr__ctx_init(&ctx);
 	evlist__for_each_entry(evsel_list, counter) {
 		bool invalid = false;
 
 		leader = counter->leader;
 		if (!counter->metric_expr)
 			continue;
+
+		expr__ctx_clear(&ctx);
 		metric_events = counter->metric_events;
 		if (!metric_events) {
-			if (expr__find_other(counter->metric_expr, counter->name,
-						&metric_names, &num_metric_names, 1) < 0)
+			if (expr__find_other(counter->metric_expr,
+					     counter->name,
+					     &ctx, 1) < 0)
 				continue;
 
 			metric_events = calloc(sizeof(struct evsel *),
-					       num_metric_names + 1);
-			if (!metric_events)
+					       hashmap__size(&ctx.ids) + 1);
+			if (!metric_events) {
+				expr__ctx_clear(&ctx);
 				return;
+			}
 			counter->metric_events = metric_events;
 		}
 
-		for (i = 0; i < num_metric_names; i++) {
+		i = 0;
+		hashmap__for_each_entry((&ctx.ids), cur, bkt) {
+			const char* metric_name = (const char*)cur->key;
+
 			found = false;
 			if (leader) {
 				/* Search in group */
 				for_each_group_member (oc, leader) {
-					if (!strcasecmp(oc->name, metric_names[i]) &&
+					if (!strcasecmp(oc->name, metric_name) &&
 						!oc->collect_stat) {
 						found = true;
 						break;
@@ -360,7 +370,7 @@ void perf_stat__collect_metric_expr(struct evlist *evsel_list)
 			}
 			if (!found) {
 				/* Search ignoring groups */
-				oc = perf_stat__find_event(evsel_list, metric_names[i]);
+				oc = perf_stat__find_event(evsel_list, metric_name);
 			}
 			if (!oc) {
 				/* Deduping one is good enough to handle duplicated PMUs. */
@@ -373,27 +383,27 @@ void perf_stat__collect_metric_expr(struct evlist *evsel_list)
 				 * of events. So we ask the user instead to add the missing
 				 * events.
 				 */
-				if (!printed || strcasecmp(printed, metric_names[i])) {
+				if (!printed || strcasecmp(printed, metric_name)) {
 					fprintf(stderr,
 						"Add %s event to groups to get metric expression for %s\n",
-						metric_names[i],
+						metric_name,
 						counter->name);
-					printed = strdup(metric_names[i]);
+					printed = strdup(metric_name);
 				}
 				invalid = true;
 				continue;
 			}
-			metric_events[i] = oc;
+			metric_events[i++] = oc;
 			oc->collect_stat = true;
 		}
 		metric_events[i] = NULL;
-		free(metric_names);
 		if (invalid) {
 			free(metric_events);
 			counter->metric_events = NULL;
 			counter->metric_expr = NULL;
 		}
 	}
+	expr__ctx_clear(&ctx);
 }
 
 static double runtime_stat_avg(struct runtime_stat *st,
@@ -738,7 +748,10 @@ static void generic_metric(struct perf_stat_config *config,
 
 	expr__ctx_init(&pctx);
 	/* Must be first id entry */
-	expr__add_id(&pctx, name, avg);
+	n = strdup(name);
+	if (!n)
+		return;
+	expr__add_id(&pctx, n, avg);
 	for (i = 0; metric_events[i]; i++) {
 		struct saved_value *v;
 		struct stats *stats;
@@ -814,8 +827,7 @@ static void generic_metric(struct perf_stat_config *config,
 			     (metric_name ? metric_name : name) : "", 0);
 	}
 
-	for (i = 1; i < pctx.num_ids; i++)
-		zfree(&pctx.ids[i].name);
+	expr__ctx_clear(&pctx);
 }
 
 void perf_stat__print_shadow_stats(struct perf_stat_config *config,
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 18/23] perf metricgroup: change evlist_used to a bitmap
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (16 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 17/23] perf expr: migrate expr ids table to libbpf's hashmap Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 19/23] perf metricgroup: free metric_events on error Ian Rogers
                   ` (4 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

Use a bitmap rather than an array of bools.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 2f92dbc05226..dcd175c05872 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -95,7 +95,7 @@ struct egroup {
 static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 				      struct expr_parse_ctx *pctx,
 				      struct evsel **metric_events,
-				      bool *evlist_used)
+				      unsigned long *evlist_used)
 {
 	struct evsel *ev;
 	bool leader_found;
@@ -105,7 +105,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 	double *val_ptr;
 
 	evlist__for_each_entry (perf_evlist, ev) {
-		if (evlist_used[j++])
+		if (test_bit(j++, evlist_used))
 			continue;
 		if (hashmap__find(&pctx->ids, ev->name, (void**)&val_ptr)) {
 			if (!metric_events[i])
@@ -149,7 +149,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 			j++;
 		}
 		ev = metric_events[i];
-		evlist_used[ev->idx] = true;
+		set_bit(ev->idx, evlist_used);
 	}
 
 	return metric_events[0];
@@ -165,13 +165,11 @@ static int metricgroup__setup_events(struct list_head *groups,
 	int ret = 0;
 	struct egroup *eg;
 	struct evsel *evsel;
-	bool *evlist_used;
+	unsigned long *evlist_used;
 
-	evlist_used = calloc(perf_evlist->core.nr_entries, sizeof(bool));
-	if (!evlist_used) {
-		ret = -ENOMEM;
-		return ret;
-	}
+	evlist_used = bitmap_alloc(perf_evlist->core.nr_entries);
+	if (!evlist_used)
+		return -ENOMEM;
 
 	list_for_each_entry (eg, groups, nd) {
 		struct evsel **metric_events;
@@ -209,7 +207,7 @@ static int metricgroup__setup_events(struct list_head *groups,
 		list_add(&expr->nd, &me->head);
 	}
 
-	free(evlist_used);
+	bitmap_free(evlist_used);
 
 	return ret;
 }
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 19/23] perf metricgroup: free metric_events on error
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (17 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 18/23] perf metricgroup: change evlist_used to a bitmap Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 20/23] perf metricgroup: always place duration_time last Ian Rogers
                   ` (3 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

Avoid a simple memory leak.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index dcd175c05872..2356dda92a07 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -185,6 +185,7 @@ static int metricgroup__setup_events(struct list_head *groups,
 		if (!evsel) {
 			pr_debug("Cannot resolve %s: %s\n",
 					eg->metric_name, eg->metric_expr);
+			free(metric_events);
 			continue;
 		}
 		for (i = 0; metric_events[i]; i++)
@@ -192,11 +193,13 @@ static int metricgroup__setup_events(struct list_head *groups,
 		me = metricgroup__lookup(metric_events_list, evsel, true);
 		if (!me) {
 			ret = -ENOMEM;
+			free(metric_events);
 			break;
 		}
 		expr = malloc(sizeof(struct metric_expr));
 		if (!expr) {
 			ret = -ENOMEM;
+			free(metric_events);
 			break;
 		}
 		expr->metric_expr = eg->metric_expr;
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 20/23] perf metricgroup: always place duration_time last
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (18 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 19/23] perf metricgroup: free metric_events on error Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 21/23] perf metricgroup: delay events string creation Ian Rogers
                   ` (2 subsequent siblings)
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

If a metric contains the duration_time event then the event is placed
outside of the metric's group of events. Rather than split the group,
make it so the duration_time is immediately after the group.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 2356dda92a07..48d0143b4b0c 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -421,8 +421,8 @@ static void metricgroup__add_metric_weak_group(struct strbuf *events,
 					       struct expr_parse_ctx *ctx)
 {
 	struct hashmap_entry *cur;
-	size_t bkt, i = 0;
-	bool no_group = false;
+	size_t bkt;
+	bool no_group = true, has_duration = false;
 
 	hashmap__for_each_entry((&ctx->ids), cur, bkt) {
 		pr_debug("found event %s\n", (const char*)cur->key);
@@ -432,20 +432,20 @@ static void metricgroup__add_metric_weak_group(struct strbuf *events,
 		 * group.
 		 */
 		if (!strcmp(cur->key, "duration_time")) {
-			if (i > 0)
-				strbuf_addf(events, "}:W,");
-			strbuf_addf(events, "duration_time");
-			no_group = true;
+			has_duration = true;
 			continue;
 		}
 		strbuf_addf(events, "%s%s",
-			i == 0 || no_group ? "{" : ",",
+			no_group ? "{" : ",",
 			(const char*)cur->key);
 		no_group = false;
-		i++;
 	}
-	if (!no_group)
-		strbuf_addf(events, "}:W");
+	if (!no_group) {
+                strbuf_addf(events, "}:W");
+		if (has_duration)
+			strbuf_addf(events, ",duration_time");
+	} else if (has_duration)
+		strbuf_addf(events, "duration_time");
 }
 
 static void metricgroup__add_metric_non_group(struct strbuf *events,
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 21/23] perf metricgroup: delay events string creation
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (19 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 20/23] perf metricgroup: always place duration_time last Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 22/23] perf metricgroup: order event groups by size Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events Ian Rogers
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

Currently event groups are placed into groups_list at the same time as
the events string containing the events is built. Separate these two
operations and build the groups_list first, then the event string from
the groups_list. This adds an ability to reorder the groups_list that
will be used in a later patch.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 38 +++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 48d0143b4b0c..0a00c0f87872 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -90,6 +90,7 @@ struct egroup {
 	const char *metric_expr;
 	const char *metric_unit;
 	int runtime;
+	bool has_constraint;
 };
 
 static struct evsel *find_evsel_group(struct evlist *perf_evlist,
@@ -496,8 +497,8 @@ int __weak arch_get_runtimeparam(void)
 	return 1;
 }
 
-static int __metricgroup__add_metric(struct strbuf *events,
-		struct list_head *group_list, struct pmu_event *pe, int runtime)
+static int __metricgroup__add_metric(struct list_head *group_list,
+				     struct pmu_event *pe, int runtime)
 {
 	struct egroup *eg;
 
@@ -510,6 +511,7 @@ static int __metricgroup__add_metric(struct strbuf *events,
 	eg->metric_expr = pe->metric_expr;
 	eg->metric_unit = pe->unit;
 	eg->runtime = runtime;
+	eg->has_constraint = metricgroup__has_constraint(pe);
 
 	if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) {
 		expr__ctx_clear(&eg->pctx);
@@ -517,14 +519,6 @@ static int __metricgroup__add_metric(struct strbuf *events,
 		return -EINVAL;
 	}
 
-	if (events->len > 0)
-		strbuf_addf(events, ",");
-
-	if (metricgroup__has_constraint(pe))
-		metricgroup__add_metric_non_group(events, &eg->pctx);
-	else
-		metricgroup__add_metric_weak_group(events, &eg->pctx);
-
 	list_add_tail(&eg->nd, group_list);
 
 	return 0;
@@ -535,6 +529,7 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 {
 	struct pmu_events_map *map = perf_pmu__find_map(NULL);
 	struct pmu_event *pe;
+	struct egroup *eg;
 	int i, ret = -EINVAL;
 
 	if (!map)
@@ -553,7 +548,8 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 			pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
 
 			if (!strstr(pe->metric_expr, "?")) {
-				ret = __metricgroup__add_metric(events, group_list, pe, 1);
+				ret = __metricgroup__add_metric(group_list,
+								pe, 1);
 			} else {
 				int j, count;
 
@@ -564,13 +560,29 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
 				 * those events to group_list.
 				 */
 
-				for (j = 0; j < count; j++)
-					ret = __metricgroup__add_metric(events, group_list, pe, j);
+				for (j = 0; j < count; j++) {
+					ret = __metricgroup__add_metric(
+						group_list, pe, j);
+				}
 			}
 			if (ret == -ENOMEM)
 				break;
 		}
 	}
+	if (!ret) {
+		list_for_each_entry (eg, group_list, nd) {
+			if (events->len > 0)
+				strbuf_addf(events, ",");
+
+			if (eg->has_constraint) {
+				metricgroup__add_metric_non_group(events,
+								  &eg->pctx);
+			} else {
+				metricgroup__add_metric_weak_group(events,
+								   &eg->pctx);
+			}
+		}
+	}
 	return ret;
 }
 
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 22/23] perf metricgroup: order event groups by size
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (20 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 21/23] perf metricgroup: delay events string creation Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-05-07 14:08 ` [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events Ian Rogers
  22 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

When adding event groups to the group list, insert them in size order.
This performs an insertion sort on the group list. By placing the
largest groups at the front of the group list it is possible to see if a
larger group contains the same events as a later group. This can make
the later group redundant - it can reuse the events from the large group.
A later patch will add this sharing.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 0a00c0f87872..25e3e8df6b45 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -519,7 +519,21 @@ static int __metricgroup__add_metric(struct list_head *group_list,
 		return -EINVAL;
 	}
 
-	list_add_tail(&eg->nd, group_list);
+	if (list_empty(group_list))
+		list_add(&eg->nd, group_list);
+	else {
+		struct list_head *pos;
+
+		/* Place the largest groups at the front. */
+		list_for_each_prev(pos, group_list) {
+			struct egroup *old = list_entry(pos, struct egroup, nd);
+
+			if (hashmap__size(&eg->pctx.ids) <=
+			    hashmap__size(&old->pctx.ids))
+				break;
+		}
+		list_add(&eg->nd, pos);
+	}
 
 	return 0;
 }
-- 
2.26.2.526.g744177e7f7-goog

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

* [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events
  2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
                   ` (21 preceding siblings ...)
  2020-05-07 14:08 ` [RFC PATCH v2 22/23] perf metricgroup: order event groups by size Ian Rogers
@ 2020-05-07 14:08 ` Ian Rogers
  2020-10-02 11:57   ` Issue of metrics for multiple uncore PMUs (was Re: [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events) John Garry
  22 siblings, 1 reply; 31+ messages in thread
From: Ian Rogers @ 2020-05-07 14:08 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, John Garry, Jin Yao, Kan Liang, Cong
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian, Ian Rogers

A metric group contains multiple metrics. These metrics may use the same
events. If metrics use separate events then it leads to more
multiplexing and overall metric counts fail to sum to 100%.
Modify how metrics are associated with events so that if the events in
an earlier group satisfy the current metric, the same events are used.
A record of used events is kept and at the end of processing unnecessary
events are eliminated.

Before:
$ perf stat -a -M TopDownL1 sleep 1

 Performance counter stats for 'system wide':

       920,211,343      uops_issued.any           #      0.5 Backend_Bound            (16.56%)
     1,977,733,128      idq_uops_not_delivered.core                                     (16.56%)
        51,668,510      int_misc.recovery_cycles                                      (16.56%)
       732,305,692      uops_retired.retire_slots                                     (16.56%)
     1,497,621,849      cycles                                                        (16.56%)
       721,098,274      uops_issued.any           #      0.1 Bad_Speculation          (16.79%)
     1,332,681,791      cycles                                                        (16.79%)
       552,475,482      uops_retired.retire_slots                                     (16.79%)
        47,708,340      int_misc.recovery_cycles                                      (16.79%)
     1,383,713,292      cycles
                                                  #      0.4 Frontend_Bound           (16.76%)
     2,013,757,701      idq_uops_not_delivered.core                                     (16.76%)
     1,373,363,790      cycles
                                                  #      0.1 Retiring                 (33.54%)
       577,302,589      uops_retired.retire_slots                                     (33.54%)
       392,766,987      inst_retired.any          #      0.3 IPC                      (50.24%)
     1,351,873,350      cpu_clk_unhalted.thread                                       (50.24%)
     1,332,510,318      cycles
                                                  # 5330041272.0 SLOTS                (49.90%)

       1.006336145 seconds time elapsed

After:
$ perf stat -a -M TopDownL1 sleep 1

 Performance counter stats for 'system wide':

       765,949,145      uops_issued.any           #      0.1 Bad_Speculation
                                                  #      0.5 Backend_Bound            (50.09%)
     1,883,830,591      idq_uops_not_delivered.core #      0.3 Frontend_Bound           (50.09%)
        48,237,080      int_misc.recovery_cycles                                      (50.09%)
       581,798,385      uops_retired.retire_slots #      0.1 Retiring                 (50.09%)
     1,361,628,527      cycles
                                                  # 5446514108.0 SLOTS                (50.09%)
       391,415,714      inst_retired.any          #      0.3 IPC                      (49.91%)
     1,336,486,781      cpu_clk_unhalted.thread                                       (49.91%)

       1.005469298 seconds time elapsed

Note: Bad_Speculation + Backend_Bound + Frontend_Bound + Retiring = 100%
after, where as before it is 110%. After there are 2 groups, whereas
before there are 6. After the cycles event appears once, before it
appeared 5 times.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 97 ++++++++++++++++++++++-------------
 1 file changed, 61 insertions(+), 36 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 25e3e8df6b45..8bb2aeeb70ad 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -93,44 +93,72 @@ struct egroup {
 	bool has_constraint;
 };
 
+/**
+ * Find a group of events in perf_evlist that correpond to those from a parsed
+ * metric expression.
+ * @perf_evlist: a list of events something like: {metric1 leader, metric1
+ * sibling, metric1 sibling}:W,duration_time,{metric2 leader, metric2 sibling,
+ * metric2 sibling}:W,duration_time
+ * @pctx: the parse context for the metric expression.
+ * @has_constraint: is there a contraint on the group of events? In which case
+ * the events won't be grouped.
+ * @metric_events: out argument, null terminated array of evsel's associated
+ * with the metric.
+ * @evlist_used: in/out argument, bitmap tracking which evlist events are used.
+ * @return the first metric event or NULL on failure.
+ */
 static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 				      struct expr_parse_ctx *pctx,
+				      bool has_constraint,
 				      struct evsel **metric_events,
 				      unsigned long *evlist_used)
 {
-	struct evsel *ev;
-	bool leader_found;
-	const size_t idnum = hashmap__size(&pctx->ids);
-	size_t i = 0;
-	int j = 0;
-	double *val_ptr;
+	struct evsel *ev, *current_leader = NULL;
+        double *val_ptr;
+	int i = 0, matched_events = 0, events_to_match;
+	const int idnum = (int)hashmap__size(&pctx->ids);
+
+	/* duration_time is grouped separately. */
+	if (!has_constraint &&
+	    hashmap__find(&pctx->ids, "duration_time", (void**)&val_ptr))
+		events_to_match = idnum - 1;
+	else
+		events_to_match = idnum;
 
 	evlist__for_each_entry (perf_evlist, ev) {
-		if (test_bit(j++, evlist_used))
+		/*
+		 * Events with a constraint aren't grouped and match the first
+		 * events available.
+		 */
+		if (has_constraint && ev->weak_group)
 			continue;
-		if (hashmap__find(&pctx->ids, ev->name, (void**)&val_ptr)) {
-			if (!metric_events[i])
-				metric_events[i] = ev;
-			i++;
-			if (i == idnum)
-				break;
-		} else {
-			/* Discard the whole match and start again */
-			i = 0;
+		if (!has_constraint && ev->leader != current_leader) {
+			/*
+			 * Start of a new group, discard the whole match and
+			 * start again.
+			 */
+			matched_events = 0;
 			memset(metric_events, 0,
 				sizeof(struct evsel *) * idnum);
+			current_leader = ev->leader;
+		}
+		if (hashmap__find(&pctx->ids, ev->name, (void**)&val_ptr))
+			metric_events[matched_events++] = ev;
+		if (matched_events == events_to_match)
+			break;
+	}
 
-			if (hashmap__find(&pctx->ids, ev->name, (void**)&val_ptr)) {
-				if (!metric_events[i])
-					metric_events[i] = ev;
-				i++;
-				if (i == idnum)
-					break;
+	if (events_to_match != idnum) {
+		/* Add the first duration_time. */
+		evlist__for_each_entry (perf_evlist, ev) {
+			if (!strcmp(ev->name, "duration_time")) {
+				metric_events[matched_events++] = ev;
+				break;
 			}
 		}
 	}
 
-	if (i != idnum) {
+	if (matched_events != idnum) {
 		/* Not whole match */
 		return NULL;
 	}
@@ -138,18 +166,8 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 	metric_events[idnum] = NULL;
 
 	for (i = 0; i < idnum; i++) {
-		leader_found = false;
-		evlist__for_each_entry(perf_evlist, ev) {
-			if (!leader_found && (ev == metric_events[i]))
-				leader_found = true;
-
-			if (leader_found &&
-			    !strcmp(ev->name, metric_events[i]->name)) {
-				ev->metric_leader = metric_events[i];
-			}
-			j++;
-		}
 		ev = metric_events[i];
+		ev->metric_leader = ev;
 		set_bit(ev->idx, evlist_used);
 	}
 
@@ -165,7 +183,7 @@ static int metricgroup__setup_events(struct list_head *groups,
 	int i = 0;
 	int ret = 0;
 	struct egroup *eg;
-	struct evsel *evsel;
+	struct evsel *evsel, *tmp;
 	unsigned long *evlist_used;
 
 	evlist_used = bitmap_alloc(perf_evlist->core.nr_entries);
@@ -181,7 +199,8 @@ static int metricgroup__setup_events(struct list_head *groups,
 			ret = -ENOMEM;
 			break;
 		}
-		evsel = find_evsel_group(perf_evlist, &eg->pctx, metric_events,
+		evsel = find_evsel_group(perf_evlist, &eg->pctx,
+					eg->has_constraint, metric_events,
 					evlist_used);
 		if (!evsel) {
 			pr_debug("Cannot resolve %s: %s\n",
@@ -211,6 +230,12 @@ static int metricgroup__setup_events(struct list_head *groups,
 		list_add(&expr->nd, &me->head);
 	}
 
+	evlist__for_each_entry_safe (perf_evlist, tmp, evsel) {
+		if (!test_bit(evsel->idx, evlist_used)) {
+			evlist__remove(perf_evlist, evsel);
+			evsel__delete(evsel);
+		}
+	}
 	bitmap_free(evlist_used);
 
 	return ret;
-- 
2.26.2.526.g744177e7f7-goog

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

* Issue of metrics for multiple uncore PMUs (was Re: [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events)
  2020-05-07 14:08 ` [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events Ian Rogers
@ 2020-10-02 11:57   ` John Garry
  2020-10-02 20:46     ` Ian Rogers
  0 siblings, 1 reply; 31+ messages in thread
From: John Garry @ 2020-10-02 11:57 UTC (permalink / raw)
  To: Ian Rogers, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	John Fastabend, KP Singh, Kajol Jain, Andi Kleen, Jin Yao,
	Kan Liang, Cong Wang
  Cc: netdev, bpf, linux-perf-users, Stephane Eranian

On 07/05/2020 15:08, Ian Rogers wrote:

Hi Ian,

I was wondering if you ever tested commit 2440689d62e9 ("perf 
metricgroup: Remove duped metric group events") for when we have a 
metric which aliases multiple instances of the same uncore PMU in the 
system?

I have been rebasing some of my arm64 perf work to v5.9-rc7, and find an 
issue where find_evsel_group() fails for the uncore metrics under the 
condition mentioned above.

Unfortunately I don't have an x86 machine to which this test applies. 
However, as an experiment, I added a test metric to my broadwell JSON:

diff --git a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json 
b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
index 8cdc7c13dc2a..fc6d9adf996a 100644
--- a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
@@ -348,5 +348,11 @@
         "MetricExpr": "(cstate_pkg@c7\\-residency@ / msr@tsc@) * 100",
         "MetricGroup": "Power",
         "MetricName": "C7_Pkg_Residency"
+    },
+    {
+        "BriefDescription": "test metric",
+        "MetricExpr": "UNC_CBO_XSNP_RESPONSE.MISS_XCORE * 
UNC_CBO_XSNP_RESPONSE.MISS_EVICTION",
+        "MetricGroup": "Test",
+        "MetricName": "test_metric_inc"
     }
]


And get this:

john@localhost:~/linux/tools/perf> sudo ./perf stat -v -M 
test_metric_inc sleep 1
Using CPUID GenuineIntel-6-3D-4
metric expr unc_cbo_xsnp_response.miss_xcore * 
unc_cbo_xsnp_response.miss_eviction for test_metric_inc
found event unc_cbo_xsnp_response.miss_eviction
found event unc_cbo_xsnp_response.miss_xcore
adding 
{unc_cbo_xsnp_response.miss_eviction,unc_cbo_xsnp_response.miss_xcore}:W
unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_1/umask=0x81,event=0x22/
unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_0/umask=0x81,event=0x22/
unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_1/umask=0x41,event=0x22/
unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_0/umask=0x41,event=0x22/
Cannot resolve test_metric_inc: unc_cbo_xsnp_response.miss_xcore * 
unc_cbo_xsnp_response.miss_eviction
task-clock: 688876 688876 688876
context-switches: 2 688876 688876
cpu-migrations: 0 688876 688876
page-faults: 69 688876 688876
cycles: 2101719 695690 695690
instructions: 1180534 695690 695690
branches: 249450 695690 695690
branch-misses: 10815 695690 695690

Performance counter stats for 'sleep 1':

              0.69 msec task-clock                #    0.001 CPUs 
utilized
                 2      context-switches          #    0.003 M/sec 

                 0      cpu-migrations            #    0.000 K/sec 

                69      page-faults               #    0.100 M/sec 

         2,101,719      cycles                    #    3.051 GHz 

         1,180,534      instructions              #    0.56  insn per 
cycle
           249,450      branches                  #  362.112 M/sec 

            10,815      branch-misses             #    4.34% of all 
branches

       1.001177693 seconds time elapsed

       0.001149000 seconds user
       0.000000000 seconds sys


john@localhost:~/linux/tools/perf>


Any idea what is going wrong here, before I have to dive in? The issue 
seems to be this named commit.

Thanks,
John

> A metric group contains multiple metrics. These metrics may use the same
> events. If metrics use separate events then it leads to more
> multiplexing and overall metric counts fail to sum to 100%.
> Modify how metrics are associated with events so that if the events in
> an earlier group satisfy the current metric, the same events are used.
> A record of used events is kept and at the end of processing unnecessary
> events are eliminated.
> 
> Before:

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

* Re: Issue of metrics for multiple uncore PMUs (was Re: [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events)
  2020-10-02 11:57   ` Issue of metrics for multiple uncore PMUs (was Re: [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events) John Garry
@ 2020-10-02 20:46     ` Ian Rogers
  2020-10-05 10:03       ` John Garry
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Rogers @ 2020-10-02 20:46 UTC (permalink / raw)
  To: John Garry
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, Jin Yao, Kan Liang, Cong Wang

On Fri, Oct 2, 2020 at 5:00 AM John Garry <john.garry@huawei.com> wrote:
>
> On 07/05/2020 15:08, Ian Rogers wrote:
>
> Hi Ian,
>
> I was wondering if you ever tested commit 2440689d62e9 ("perf
> metricgroup: Remove duped metric group events") for when we have a
> metric which aliases multiple instances of the same uncore PMU in the
> system?

Sorry for this, I hadn't tested such a metric and wasn't aware of how
the aliasing worked. I sent a fix for this issue here:
https://lore.kernel.org/lkml/20200917201807.4090224-1-irogers@google.com/
Could you see if this addresses the issue for you? I don't see the
change in Arnaldo's trees yet.

Thanks,
Ian

> I have been rebasing some of my arm64 perf work to v5.9-rc7, and find an
> issue where find_evsel_group() fails for the uncore metrics under the
> condition mentioned above.
>
> Unfortunately I don't have an x86 machine to which this test applies.
> However, as an experiment, I added a test metric to my broadwell JSON:
>
> diff --git a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> index 8cdc7c13dc2a..fc6d9adf996a 100644
> --- a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> +++ b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> @@ -348,5 +348,11 @@
>          "MetricExpr": "(cstate_pkg@c7\\-residency@ / msr@tsc@) * 100",
>          "MetricGroup": "Power",
>          "MetricName": "C7_Pkg_Residency"
> +    },
> +    {
> +        "BriefDescription": "test metric",
> +        "MetricExpr": "UNC_CBO_XSNP_RESPONSE.MISS_XCORE *
> UNC_CBO_XSNP_RESPONSE.MISS_EVICTION",
> +        "MetricGroup": "Test",
> +        "MetricName": "test_metric_inc"
>      }
> ]
>
>
> And get this:
>
> john@localhost:~/linux/tools/perf> sudo ./perf stat -v -M
> test_metric_inc sleep 1
> Using CPUID GenuineIntel-6-3D-4
> metric expr unc_cbo_xsnp_response.miss_xcore *
> unc_cbo_xsnp_response.miss_eviction for test_metric_inc
> found event unc_cbo_xsnp_response.miss_eviction
> found event unc_cbo_xsnp_response.miss_xcore
> adding
> {unc_cbo_xsnp_response.miss_eviction,unc_cbo_xsnp_response.miss_xcore}:W
> unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_1/umask=0x81,event=0x22/
> unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_0/umask=0x81,event=0x22/
> unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_1/umask=0x41,event=0x22/
> unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_0/umask=0x41,event=0x22/
> Cannot resolve test_metric_inc: unc_cbo_xsnp_response.miss_xcore *
> unc_cbo_xsnp_response.miss_eviction
> task-clock: 688876 688876 688876
> context-switches: 2 688876 688876
> cpu-migrations: 0 688876 688876
> page-faults: 69 688876 688876
> cycles: 2101719 695690 695690
> instructions: 1180534 695690 695690
> branches: 249450 695690 695690
> branch-misses: 10815 695690 695690
>
> Performance counter stats for 'sleep 1':
>
>               0.69 msec task-clock                #    0.001 CPUs
> utilized
>                  2      context-switches          #    0.003 M/sec
>
>                  0      cpu-migrations            #    0.000 K/sec
>
>                 69      page-faults               #    0.100 M/sec
>
>          2,101,719      cycles                    #    3.051 GHz
>
>          1,180,534      instructions              #    0.56  insn per
> cycle
>            249,450      branches                  #  362.112 M/sec
>
>             10,815      branch-misses             #    4.34% of all
> branches
>
>        1.001177693 seconds time elapsed
>
>        0.001149000 seconds user
>        0.000000000 seconds sys
>
>
> john@localhost:~/linux/tools/perf>
>
>
> Any idea what is going wrong here, before I have to dive in? The issue
> seems to be this named commit.
>
> Thanks,
> John
>
> > A metric group contains multiple metrics. These metrics may use the same
> > events. If metrics use separate events then it leads to more
> > multiplexing and overall metric counts fail to sum to 100%.
> > Modify how metrics are associated with events so that if the events in
> > an earlier group satisfy the current metric, the same events are used.
> > A record of used events is kept and at the end of processing unnecessary
> > events are eliminated.
> >
> > Before:

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

* Re: Issue of metrics for multiple uncore PMUs (was Re: [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events)
  2020-10-02 20:46     ` Ian Rogers
@ 2020-10-05 10:03       ` John Garry
  2020-10-05 16:28         ` Ian Rogers
  0 siblings, 1 reply; 31+ messages in thread
From: John Garry @ 2020-10-05 10:03 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, Jin Yao, Kan Liang, Cong Wang

On 02/10/2020 21:46, Ian Rogers wrote:
> On Fri, Oct 2, 2020 at 5:00 AM John Garry <john.garry@huawei.com> wrote:
>>
>> On 07/05/2020 15:08, Ian Rogers wrote:
>>
>> Hi Ian,
>>
>> I was wondering if you ever tested commit 2440689d62e9 ("perf
>> metricgroup: Remove duped metric group events") for when we have a
>> metric which aliases multiple instances of the same uncore PMU in the
>> system?
> 
> Sorry for this, I hadn't tested such a metric and wasn't aware of how
> the aliasing worked. I sent a fix for this issue here:
> https://lore.kernel.org/lkml/20200917201807.4090224-1-irogers@google.com/
> Could you see if this addresses the issue for you? I don't see the
> change in Arnaldo's trees yet.

Unfortunately this does not seem to fix my issue.

So for that patch, you say you fix metric expression for DRAM_BW_Use, 
which is:

{
  "BriefDescription": "Average external Memory Bandwidth Use for reads 
and writes [GB / sec]",
  "MetricExpr": "( 64 * ( uncore_imc@cas_count_read@ + 
uncore_imc@cas_count_write@ ) / 1000000000 ) / duration_time",
  "MetricGroup": "Memory_BW",
"MetricName": "DRAM_BW_Use"
},

But this metric expression does not include any alias events; rather I 
think it is just cas_count_write + cas_count_read event count for PMU 
uncore_imc / duration_time.

When I say alias, I mean - as an example, we have event:

     {
         "BriefDescription": "write requests to memory controller. 
Derived from unc_m_cas_count.wr",
         "Counter": "0,1,2,3",
         "EventCode": "0x4",
         "EventName": "LLC_MISSES.MEM_WRITE",
         "PerPkg": "1",
         "ScaleUnit": "64Bytes",
         "UMask": "0xC",
         "Unit": "iMC"
     },

And then reference LLC_MISSES.MEM_WRITE in a metric expression:

"MetricExpr": "LLC_MISSES.MEM_WRITE / duration_time",

This is what seems to be broken for when the alias matches > 1 PMU.

Please check this.

Thanks,
John

> 
> Thanks,
> Ian
> 
>> I have been rebasing some of my arm64 perf work to v5.9-rc7, and find an
>> issue where find_evsel_group() fails for the uncore metrics under the
>> condition mentioned above.
>>
>> Unfortunately I don't have an x86 machine to which this test applies.
>> However, as an experiment, I added a test metric to my broadwell JSON:
>>
>> diff --git a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
>> b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
>> index 8cdc7c13dc2a..fc6d9adf996a 100644
>> --- a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
>> +++ b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
>> @@ -348,5 +348,11 @@
>>           "MetricExpr": "(cstate_pkg@c7\\-residency@ / msr@tsc@) * 100",
>>           "MetricGroup": "Power",
>>           "MetricName": "C7_Pkg_Residency"
>> +    },
>> +    {
>> +        "BriefDescription": "test metric",
>> +        "MetricExpr": "UNC_CBO_XSNP_RESPONSE.MISS_XCORE *
>> UNC_CBO_XSNP_RESPONSE.MISS_EVICTION",
>> +        "MetricGroup": "Test",
>> +        "MetricName": "test_metric_inc"
>>       }
>> ]
>>
>>
>> And get this:
>>
>> john@localhost:~/linux/tools/perf> sudo ./perf stat -v -M
>> test_metric_inc sleep 1
>> Using CPUID GenuineIntel-6-3D-4
>> metric expr unc_cbo_xsnp_response.miss_xcore *
>> unc_cbo_xsnp_response.miss_eviction for test_metric_inc
>> found event unc_cbo_xsnp_response.miss_eviction
>> found event unc_cbo_xsnp_response.miss_xcore
>> adding
>> {unc_cbo_xsnp_response.miss_eviction,unc_cbo_xsnp_response.miss_xcore}:W
>> unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_1/umask=0x81,event=0x22/
>> unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_0/umask=0x81,event=0x22/
>> unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_1/umask=0x41,event=0x22/
>> unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_0/umask=0x41,event=0x22/
>> Cannot resolve test_metric_inc: unc_cbo_xsnp_response.miss_xcore *
>> unc_cbo_xsnp_response.miss_eviction
>> task-clock: 688876 688876 688876
>> context-switches: 2 688876 688876
>> cpu-migrations: 0 688876 688876
>> page-faults: 69 688876 688876
>> cycles: 2101719 695690 695690
>> instructions: 1180534 695690 695690
>> branches: 249450 695690 695690
>> branch-misses: 10815 695690 695690
>>
>> Performance counter stats for 'sleep 1':
>>
>>                0.69 msec task-clock                #    0.001 CPUs
>> utilized
>>                   2      context-switches          #    0.003 M/sec
>>
>>                   0      cpu-migrations            #    0.000 K/sec
>>
>>                  69      page-faults               #    0.100 M/sec
>>
>>           2,101,719      cycles                    #    3.051 GHz
>>
>>           1,180,534      instructions              #    0.56  insn per
>> cycle
>>             249,450      branches                  #  362.112 M/sec
>>
>>              10,815      branch-misses             #    4.34% of all
>> branches
>>
>>         1.001177693 seconds time elapsed
>>
>>         0.001149000 seconds user
>>         0.000000000 seconds sys
>>
>>
>> john@localhost:~/linux/tools/perf>
>>
>>
>> Any idea what is going wrong here, before I have to dive in? The issue
>> seems to be this named commit.
>>
>> Thanks,
>> John
>>
>>> A metric group contains multiple metrics. These metrics may use the same
>>> events. If metrics use separate events then it leads to more
>>> multiplexing and overall metric counts fail to sum to 100%.
>>> Modify how metrics are associated with events so that if the events in
>>> an earlier group satisfy the current metric, the same events are used.
>>> A record of used events is kept and at the end of processing unnecessary
>>> events are eliminated.
>>>
>>> Before:
> .
> 

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

* Re: Issue of metrics for multiple uncore PMUs (was Re: [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events)
  2020-10-05 10:03       ` John Garry
@ 2020-10-05 16:28         ` Ian Rogers
  2020-10-05 18:05           ` John Garry
  0 siblings, 1 reply; 31+ messages in thread
From: Ian Rogers @ 2020-10-05 16:28 UTC (permalink / raw)
  To: John Garry
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, Jin Yao, Kan Liang, Cong Wang

On Mon, Oct 5, 2020 at 3:06 AM John Garry <john.garry@huawei.com> wrote:
>
> On 02/10/2020 21:46, Ian Rogers wrote:
> > On Fri, Oct 2, 2020 at 5:00 AM John Garry <john.garry@huawei.com> wrote:
> >>
> >> On 07/05/2020 15:08, Ian Rogers wrote:
> >>
> >> Hi Ian,
> >>
> >> I was wondering if you ever tested commit 2440689d62e9 ("perf
> >> metricgroup: Remove duped metric group events") for when we have a
> >> metric which aliases multiple instances of the same uncore PMU in the
> >> system?
> >
> > Sorry for this, I hadn't tested such a metric and wasn't aware of how
> > the aliasing worked. I sent a fix for this issue here:
> > https://lore.kernel.org/lkml/20200917201807.4090224-1-irogers@google.com/
> > Could you see if this addresses the issue for you? I don't see the
> > change in Arnaldo's trees yet.
>
> Unfortunately this does not seem to fix my issue.
>
> So for that patch, you say you fix metric expression for DRAM_BW_Use,
> which is:
>
> {
>   "BriefDescription": "Average external Memory Bandwidth Use for reads
> and writes [GB / sec]",
>   "MetricExpr": "( 64 * ( uncore_imc@cas_count_read@ +
> uncore_imc@cas_count_write@ ) / 1000000000 ) / duration_time",
>   "MetricGroup": "Memory_BW",
> "MetricName": "DRAM_BW_Use"
> },
>
> But this metric expression does not include any alias events; rather I
> think it is just cas_count_write + cas_count_read event count for PMU
> uncore_imc / duration_time.
>
> When I say alias, I mean - as an example, we have event:
>
>      {
>          "BriefDescription": "write requests to memory controller.
> Derived from unc_m_cas_count.wr",
>          "Counter": "0,1,2,3",
>          "EventCode": "0x4",
>          "EventName": "LLC_MISSES.MEM_WRITE",
>          "PerPkg": "1",
>          "ScaleUnit": "64Bytes",
>          "UMask": "0xC",
>          "Unit": "iMC"
>      },
>
> And then reference LLC_MISSES.MEM_WRITE in a metric expression:
>
> "MetricExpr": "LLC_MISSES.MEM_WRITE / duration_time",
>
> This is what seems to be broken for when the alias matches > 1 PMU.
>
> Please check this.

Happy to check. Can you provide a reproduction? Looking on broadwell
this metric doesn't exist.

Thanks,
Ian

> Thanks,
> John
>
> >
> > Thanks,
> > Ian
> >
> >> I have been rebasing some of my arm64 perf work to v5.9-rc7, and find an
> >> issue where find_evsel_group() fails for the uncore metrics under the
> >> condition mentioned above.
> >>
> >> Unfortunately I don't have an x86 machine to which this test applies.
> >> However, as an experiment, I added a test metric to my broadwell JSON:
> >>
> >> diff --git a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> >> b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> >> index 8cdc7c13dc2a..fc6d9adf996a 100644
> >> --- a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> >> +++ b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> >> @@ -348,5 +348,11 @@
> >>           "MetricExpr": "(cstate_pkg@c7\\-residency@ / msr@tsc@) * 100",
> >>           "MetricGroup": "Power",
> >>           "MetricName": "C7_Pkg_Residency"
> >> +    },
> >> +    {
> >> +        "BriefDescription": "test metric",
> >> +        "MetricExpr": "UNC_CBO_XSNP_RESPONSE.MISS_XCORE *
> >> UNC_CBO_XSNP_RESPONSE.MISS_EVICTION",
> >> +        "MetricGroup": "Test",
> >> +        "MetricName": "test_metric_inc"
> >>       }
> >> ]
> >>
> >>
> >> And get this:
> >>
> >> john@localhost:~/linux/tools/perf> sudo ./perf stat -v -M
> >> test_metric_inc sleep 1
> >> Using CPUID GenuineIntel-6-3D-4
> >> metric expr unc_cbo_xsnp_response.miss_xcore *
> >> unc_cbo_xsnp_response.miss_eviction for test_metric_inc
> >> found event unc_cbo_xsnp_response.miss_eviction
> >> found event unc_cbo_xsnp_response.miss_xcore
> >> adding
> >> {unc_cbo_xsnp_response.miss_eviction,unc_cbo_xsnp_response.miss_xcore}:W
> >> unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_1/umask=0x81,event=0x22/
> >> unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_0/umask=0x81,event=0x22/
> >> unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_1/umask=0x41,event=0x22/
> >> unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_0/umask=0x41,event=0x22/
> >> Cannot resolve test_metric_inc: unc_cbo_xsnp_response.miss_xcore *
> >> unc_cbo_xsnp_response.miss_eviction
> >> task-clock: 688876 688876 688876
> >> context-switches: 2 688876 688876
> >> cpu-migrations: 0 688876 688876
> >> page-faults: 69 688876 688876
> >> cycles: 2101719 695690 695690
> >> instructions: 1180534 695690 695690
> >> branches: 249450 695690 695690
> >> branch-misses: 10815 695690 695690
> >>
> >> Performance counter stats for 'sleep 1':
> >>
> >>                0.69 msec task-clock                #    0.001 CPUs
> >> utilized
> >>                   2      context-switches          #    0.003 M/sec
> >>
> >>                   0      cpu-migrations            #    0.000 K/sec
> >>
> >>                  69      page-faults               #    0.100 M/sec
> >>
> >>           2,101,719      cycles                    #    3.051 GHz
> >>
> >>           1,180,534      instructions              #    0.56  insn per
> >> cycle
> >>             249,450      branches                  #  362.112 M/sec
> >>
> >>              10,815      branch-misses             #    4.34% of all
> >> branches
> >>
> >>         1.001177693 seconds time elapsed
> >>
> >>         0.001149000 seconds user
> >>         0.000000000 seconds sys
> >>
> >>
> >> john@localhost:~/linux/tools/perf>
> >>
> >>
> >> Any idea what is going wrong here, before I have to dive in? The issue
> >> seems to be this named commit.
> >>
> >> Thanks,
> >> John
> >>
> >>> A metric group contains multiple metrics. These metrics may use the same
> >>> events. If metrics use separate events then it leads to more
> >>> multiplexing and overall metric counts fail to sum to 100%.
> >>> Modify how metrics are associated with events so that if the events in
> >>> an earlier group satisfy the current metric, the same events are used.
> >>> A record of used events is kept and at the end of processing unnecessary
> >>> events are eliminated.
> >>>
> >>> Before:
> > .
> >
>

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

* Re: Issue of metrics for multiple uncore PMUs (was Re: [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events)
  2020-10-05 16:28         ` Ian Rogers
@ 2020-10-05 18:05           ` John Garry
  2020-10-06 14:19             ` John Garry
  0 siblings, 1 reply; 31+ messages in thread
From: John Garry @ 2020-10-05 18:05 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, Jin Yao, Kan Liang, Cong Wang

On 05/10/2020 17:28, Ian Rogers wrote:
> On Mon, Oct 5, 2020 at 3:06 AM John Garry <john.garry@huawei.com> wrote:
>>
>> On 02/10/2020 21:46, Ian Rogers wrote:
>>> On Fri, Oct 2, 2020 at 5:00 AM John Garry <john.garry@huawei.com> wrote:
>>>>
>>>> On 07/05/2020 15:08, Ian Rogers wrote:
>>>>
>>>> Hi Ian,
>>>>
>>>> I was wondering if you ever tested commit 2440689d62e9 ("perf
>>>> metricgroup: Remove duped metric group events") for when we have a
>>>> metric which aliases multiple instances of the same uncore PMU in the
>>>> system?
>>>
>>> Sorry for this, I hadn't tested such a metric and wasn't aware of how
>>> the aliasing worked. I sent a fix for this issue here:
>>> https://lore.kernel.org/lkml/20200917201807.4090224-1-irogers@google.com/
>>> Could you see if this addresses the issue for you? I don't see the
>>> change in Arnaldo's trees yet.
>>
>> Unfortunately this does not seem to fix my issue.
>>
>> So for that patch, you say you fix metric expression for DRAM_BW_Use,
>> which is:
>>
>> {
>>    "BriefDescription": "Average external Memory Bandwidth Use for reads
>> and writes [GB / sec]",
>>    "MetricExpr": "( 64 * ( uncore_imc@cas_count_read@ +
>> uncore_imc@cas_count_write@ ) / 1000000000 ) / duration_time",
>>    "MetricGroup": "Memory_BW",
>> "MetricName": "DRAM_BW_Use"
>> },
>>
>> But this metric expression does not include any alias events; rather I
>> think it is just cas_count_write + cas_count_read event count for PMU
>> uncore_imc / duration_time.
>>
>> When I say alias, I mean - as an example, we have event:
>>
>>       {
>>           "BriefDescription": "write requests to memory controller.
>> Derived from unc_m_cas_count.wr",
>>           "Counter": "0,1,2,3",
>>           "EventCode": "0x4",
>>           "EventName": "LLC_MISSES.MEM_WRITE",
>>           "PerPkg": "1",
>>           "ScaleUnit": "64Bytes",
>>           "UMask": "0xC",
>>           "Unit": "iMC"
>>       },
>>
>> And then reference LLC_MISSES.MEM_WRITE in a metric expression:
>>
>> "MetricExpr": "LLC_MISSES.MEM_WRITE / duration_time",
>>
>> This is what seems to be broken for when the alias matches > 1 PMU.
>>
>> Please check this.
> 
Hi Ian,

> Happy to check. 

So I am, but the code is a little complicated :)

> Can you provide a reproduction? Looking on broadwell
> this metric doesn't exist.

Right, I just added this test metric as my 2x x86 platform has no 
examples which I can find:

diff --git a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json 
b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
index 8cdc7c13dc2a..fc6d9adf996a 100644
--- a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
+++ b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
@@ -348,5 +348,11 @@
         "MetricExpr": "(cstate_pkg@c7\\-residency@ / msr@tsc@) * 100",
         "MetricGroup": "Power",
         "MetricName": "C7_Pkg_Residency"
+    },
+    {
+        "BriefDescription": "test metric",
+        "MetricExpr": "UNC_CBO_XSNP_RESPONSE.MISS_XCORE * 
UNC_CBO_XSNP_RESPONSE.MISS_EVICTION",
+        "MetricGroup": "Test",
+        "MetricName": "test_metric_inc"
     }
]

I'll try to find a better mainline example, though, but I'm not hopeful ...

Thanks,
John

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

* Re: Issue of metrics for multiple uncore PMUs (was Re: [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events)
  2020-10-05 18:05           ` John Garry
@ 2020-10-06 14:19             ` John Garry
  2020-10-06 14:42               ` Ian Rogers
  0 siblings, 1 reply; 31+ messages in thread
From: John Garry @ 2020-10-06 14:19 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, Jin Yao, Kan Liang, Cong Wang

On 05/10/2020 19:05, John Garry wrote:
>> Can you provide a reproduction? Looking on broadwell
>> this metric doesn't exist.
> 
> Right, I just added this test metric as my 2x x86 platform has no 
> examples which I can find:
> 
> diff --git a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json 
> b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> index 8cdc7c13dc2a..fc6d9adf996a 100644
> --- a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> +++ b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> @@ -348,5 +348,11 @@
>          "MetricExpr": "(cstate_pkg@c7\\-residency@ / msr@tsc@) * 100",
>          "MetricGroup": "Power",
>          "MetricName": "C7_Pkg_Residency"
> +    },
> +    {
> +        "BriefDescription": "test metric",
> +        "MetricExpr": "UNC_CBO_XSNP_RESPONSE.MISS_XCORE * 
> UNC_CBO_XSNP_RESPONSE.MISS_EVICTION",
> +        "MetricGroup": "Test",
> +        "MetricName": "test_metric_inc"
>      }
> ]
> 

It seems that the code in find_evsel_group() does not properly handle 
the scenario of event alias matching different PMUs (as I already said).

So I got it working on top of "perf metricgroup: Fix uncore metric 
expressions" with the following change:

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index d948a7f910cf..6293378c019c 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -213,7 +213,8 @@ static struct evsel *find_evsel_group(struct evlist 
*perf_evlist,
  		/* Ignore event if already used and merging is disabled. */
  		if (metric_no_merge && test_bit(ev->idx, evlist_used))
  			continue;
-		if (!has_constraint && ev->leader != current_leader) {
+		if (!has_constraint && (!current_leader || 
strcmp(current_leader->name, ev->leader->name))) {
  			/*
  			 * Start of a new group, discard the whole match and
  			 * start again.
@@ -279,7 +280,8 @@ static struct evsel *find_evsel_group(struct evlist 
*perf_evlist,
  			 * when then group is left.
  			 */
  			if (!has_constraint &&
-			    ev->leader != metric_events[i]->leader)
+			    strcmp(ev->leader->name, metric_events[i]->leader->name))
  				break;
  			if (!strcmp(metric_events[i]->name, ev->name)) {
  				set_bit(ev->idx, evlist_used);

which gives for my test metric:

./perf stat -v -M test_metric_inc sleep 1
Using CPUID GenuineIntel-6-3D-4
metric expr unc_cbo_xsnp_response.miss_xcore / 
unc_cbo_xsnp_response.miss_eviction for test_metric_inc
found event unc_cbo_xsnp_response.miss_eviction
found event unc_cbo_xsnp_response.miss_xcore
adding 
{unc_cbo_xsnp_response.miss_eviction,unc_cbo_xsnp_response.miss_xcore}:W
unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_1/umask=0x81,event=0x22/
unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_0/umask=0x81,event=0x22/
unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_1/umask=0x41,event=0x22/
unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_0/umask=0x41,event=0x22/
Control descriptor is not initialized
unc_cbo_xsnp_response.miss_eviction: 595175 1001021311 1001021311
unc_cbo_xsnp_response.miss_eviction: 592516 1001020037 1001020037
unc_cbo_xsnp_response.miss_xcore: 39139 1001021311 1001021311
unc_cbo_xsnp_response.miss_xcore: 38718 1001020037 1001020037

Performance counter stats for 'system wide':

         1,187,691      unc_cbo_xsnp_response.miss_eviction #     0.07 
test_metric_inc
            77,857      unc_cbo_xsnp_response.miss_xcore 


       1.001068918 seconds time elapsed

John

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

* Re: Issue of metrics for multiple uncore PMUs (was Re: [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events)
  2020-10-06 14:19             ` John Garry
@ 2020-10-06 14:42               ` Ian Rogers
  0 siblings, 0 replies; 31+ messages in thread
From: Ian Rogers @ 2020-10-06 14:42 UTC (permalink / raw)
  To: John Garry
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
	Kajol Jain, Andi Kleen, Jin Yao, Kan Liang, Cong Wang

On Tue, Oct 6, 2020 at 7:22 AM John Garry <john.garry@huawei.com> wrote:
>
> On 05/10/2020 19:05, John Garry wrote:
> >> Can you provide a reproduction? Looking on broadwell
> >> this metric doesn't exist.
> >
> > Right, I just added this test metric as my 2x x86 platform has no
> > examples which I can find:
> >
> > diff --git a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> > b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> > index 8cdc7c13dc2a..fc6d9adf996a 100644
> > --- a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> > +++ b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> > @@ -348,5 +348,11 @@
> >          "MetricExpr": "(cstate_pkg@c7\\-residency@ / msr@tsc@) * 100",
> >          "MetricGroup": "Power",
> >          "MetricName": "C7_Pkg_Residency"
> > +    },
> > +    {
> > +        "BriefDescription": "test metric",
> > +        "MetricExpr": "UNC_CBO_XSNP_RESPONSE.MISS_XCORE *
> > UNC_CBO_XSNP_RESPONSE.MISS_EVICTION",
> > +        "MetricGroup": "Test",
> > +        "MetricName": "test_metric_inc"
> >      }
> > ]
> >
>
> It seems that the code in find_evsel_group() does not properly handle
> the scenario of event alias matching different PMUs (as I already said).
>
> So I got it working on top of "perf metricgroup: Fix uncore metric
> expressions" with the following change:
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index d948a7f910cf..6293378c019c 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -213,7 +213,8 @@ static struct evsel *find_evsel_group(struct evlist
> *perf_evlist,
>                 /* Ignore event if already used and merging is disabled. */
>                 if (metric_no_merge && test_bit(ev->idx, evlist_used))
>                         continue;
> -               if (!has_constraint && ev->leader != current_leader) {
> +               if (!has_constraint && (!current_leader ||
> strcmp(current_leader->name, ev->leader->name))) {
>                         /*
>                          * Start of a new group, discard the whole match and
>                          * start again.
> @@ -279,7 +280,8 @@ static struct evsel *find_evsel_group(struct evlist
> *perf_evlist,
>                          * when then group is left.
>                          */
>                         if (!has_constraint &&
> -                           ev->leader != metric_events[i]->leader)
> +                           strcmp(ev->leader->name, metric_events[i]->leader->name))
>                                 break;
>                         if (!strcmp(metric_events[i]->name, ev->name)) {
>                                 set_bit(ev->idx, evlist_used);
>
> which gives for my test metric:
>
> ./perf stat -v -M test_metric_inc sleep 1
> Using CPUID GenuineIntel-6-3D-4
> metric expr unc_cbo_xsnp_response.miss_xcore /
> unc_cbo_xsnp_response.miss_eviction for test_metric_inc
> found event unc_cbo_xsnp_response.miss_eviction
> found event unc_cbo_xsnp_response.miss_xcore
> adding
> {unc_cbo_xsnp_response.miss_eviction,unc_cbo_xsnp_response.miss_xcore}:W
> unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_1/umask=0x81,event=0x22/
> unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_0/umask=0x81,event=0x22/
> unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_1/umask=0x41,event=0x22/
> unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_0/umask=0x41,event=0x22/
> Control descriptor is not initialized
> unc_cbo_xsnp_response.miss_eviction: 595175 1001021311 1001021311
> unc_cbo_xsnp_response.miss_eviction: 592516 1001020037 1001020037
> unc_cbo_xsnp_response.miss_xcore: 39139 1001021311 1001021311
> unc_cbo_xsnp_response.miss_xcore: 38718 1001020037 1001020037
>
> Performance counter stats for 'system wide':
>
>          1,187,691      unc_cbo_xsnp_response.miss_eviction #     0.07
> test_metric_inc
>             77,857      unc_cbo_xsnp_response.miss_xcore
>
>
>        1.001068918 seconds time elapsed
>
> John

Thanks John! I was able to repro the problem, let me investigate what
is happening here as it seems there may be something wrong with the
grouping logic.

Ian

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

end of thread, other threads:[~2020-10-06 14:42 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-07 14:07 [RFC PATCH v2 00/23] Share events between metrics Ian Rogers
2020-05-07 14:07 ` [RFC PATCH v2 01/23] perf expr: unlimited escaped characters in a symbol Ian Rogers
2020-05-07 14:07 ` [RFC PATCH v2 02/23] perf metrics: fix parse errors in cascade lake metrics Ian Rogers
2020-05-07 14:07 ` [RFC PATCH v2 03/23] perf metrics: fix parse errors in skylake metrics Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 04/23] perf expr: allow ',' to be an other token Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 05/23] perf expr: increase max other Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 06/23] perf expr: parse numbers as doubles Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 07/23] perf expr: debug lex if debugging yacc Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 08/23] perf metrics: fix parse errors in power8 metrics Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 09/23] perf metrics: fix parse errors in power9 metrics Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 10/23] perf expr: print a debug message for division by zero Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 11/23] perf parse-events: expand add PMU error/verbose messages Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 12/23] perf test: improve pmu event metric testing Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 13/23] lib/bpf hashmap: increase portability Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 14/23] libbpf: Fix memory leak and possible double-free in hashmap__clear Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 15/23] perf expr: fix memory leaks in bison Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 16/23] perf evsel: fix 2 memory leaks Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 17/23] perf expr: migrate expr ids table to libbpf's hashmap Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 18/23] perf metricgroup: change evlist_used to a bitmap Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 19/23] perf metricgroup: free metric_events on error Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 20/23] perf metricgroup: always place duration_time last Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 21/23] perf metricgroup: delay events string creation Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 22/23] perf metricgroup: order event groups by size Ian Rogers
2020-05-07 14:08 ` [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events Ian Rogers
2020-10-02 11:57   ` Issue of metrics for multiple uncore PMUs (was Re: [RFC PATCH v2 23/23] perf metricgroup: remove duped metric group events) John Garry
2020-10-02 20:46     ` Ian Rogers
2020-10-05 10:03       ` John Garry
2020-10-05 16:28         ` Ian Rogers
2020-10-05 18:05           ` John Garry
2020-10-06 14:19             ` John Garry
2020-10-06 14:42               ` Ian Rogers

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