[PATCH 00/21] perf metric: Fixes and allow modifiers

[PATCH 00/21] perf metric: Fixes and allow modifiers

[Ian Rogers]

There are 4 main changes in this patch set:
 - perf metric: Modify resolution and recursion check.
 - perf parse-events: Add new "metric-id" term.
 - perf metrics: Modify setup and deduplication
 - perf metric: Allow modifiers on metrics.

In overview the changes start by trying to simplify the metric code,
then it fixes various bugs and finally it builds a new feature of
allowing metrics like:

$ perf stat -M IPC:u,IPC:k -a sleep 1

 Performance counter stats for 'system wide':

        93,269,988      inst_retired.any:k        #     0.26 IPC:k                  
       352,037,460      cpu_clk_unhalted.thread:k                                   
        70,317,865      inst_retired.any:u        #     0.76 IPC:u                  
        92,762,220      cpu_clk_unhalted.thread:u                                   

       1.003754577 seconds time elapsed

Previous complexity came from using the evsel->name as the identifier
for events in metrics, however, this name isn't stable and has issues
around wildcard expansion. These changes fix this by adding a
dedicated metric_id to evsels, performing deduplication on IDs before
event parsing and not handling all evsels on a single evlist.

The recursion and metric_ref logic is simplified, the first by moving
data from the heap to the stack, the latter by building in an array
rather than a linked list. This logic is integral to metric set up and
simplification makes the effects of the changes easier to follow, in
particular as there are fewer structs being maintained.

Event parsing is modified to allow qualifiers on kernel PMU events,
this is necessary to allow the metric-id to be added, but allows
qualifiers in other cases like specifying callgraph or a name.

There is a certain amount of comment adding and const-ification, this
is with a view to making the code more intention revealing and to aid
following its logic. For example, the pmu event tables should never
change and it'd be a bug if they ever did, it's therefore strange to
access it using non-const pointers.

The kernel list_sort.c/h are added for use sorting metrics in order to
deduplicate/reuse events from a larger group in a smaller one. This
was previously done by inserting in size order, but that only worked
within a metric group.

Some of the commit messages show the TopDownL1 metrics being used on a
SkylakeX machine. These metrics were removed by
c4ad8fabd03f76ed3a2a4c8aef6baf6cd4f24542 ("perf vendor events: Update
metrics for SkyLake Server") and the data was gathered with this patch
reverted.

Ian Rogers (21):
  tools lib: Add list_sort.
  perf pmu: Add const to pmu_events_map.
  perf pmu: Make pmu_sys_event_tables const.
  perf pmu: Make pmu_event tables const.
  perf metric: Move runtime value to the expr context
  perf metric: Add documentation and rename a variable.
  perf metric: Add metric new and free
  perf metric: Only add a referenced metric once
  perf metric: Modify resolution and recursion check.
  perf metric: Comment data structures.
  perf metric: Document the internal 'struct metric'
  perf metric: Simplify metric_refs calculation.
  perf parse-events: Add const to evsel name
  perf parse-events: Add new "metric-id" term.
  perf parse-events: Allow config on kernel PMU events
  perf metric: Encode and use metric-id as qualifier
  perf expr: Add subset utility.
  perf metrics: Modify setup and deduplication
  perf metric: Switch fprintf to pr_err.
  perf parse-events: Identify broken modifiers.
  perf metric: Allow modifiers on metrics.

 tools/include/linux/list_sort.h       |   14 +
 tools/lib/list_sort.c                 |  252 +++++
 tools/perf/arch/powerpc/util/header.c |    2 +-
 tools/perf/check-headers.sh           |    2 +
 tools/perf/pmu-events/jevents.c       |    6 +-
 tools/perf/pmu-events/pmu-events.h    |    8 +-
 tools/perf/tests/expand-cgroup.c      |    2 +-
 tools/perf/tests/expr.c               |   29 +-
 tools/perf/tests/parse-metric.c       |    2 +-
 tools/perf/tests/pmu-events.c         |   59 +-
 tools/perf/util/Build                 |    5 +
 tools/perf/util/evsel.c               |   17 +
 tools/perf/util/evsel.h               |    2 +
 tools/perf/util/expr.c                |   56 +-
 tools/perf/util/expr.h                |   16 +-
 tools/perf/util/expr.l                |    6 +-
 tools/perf/util/expr.y                |    2 +-
 tools/perf/util/metricgroup.c         | 1441 ++++++++++++++-----------
 tools/perf/util/metricgroup.h         |   35 +-
 tools/perf/util/parse-events-hybrid.c |   34 +-
 tools/perf/util/parse-events-hybrid.h |    6 +-
 tools/perf/util/parse-events.c        |  166 +--
 tools/perf/util/parse-events.h        |   11 +-
 tools/perf/util/parse-events.l        |   18 +-
 tools/perf/util/parse-events.y        |   27 +-
 tools/perf/util/pfm.c                 |    3 +-
 tools/perf/util/pmu.c                 |   22 +-
 tools/perf/util/pmu.h                 |   10 +-
 tools/perf/util/s390-sample-raw.c     |    6 +-
 tools/perf/util/stat-shadow.c         |   27 +-
 30 files changed, 1448 insertions(+), 838 deletions(-)
 create mode 100644 tools/include/linux/list_sort.h
 create mode 100644 tools/lib/list_sort.c

-- 
2.33.0.882.g93a45727a2-goog

[PATCH 01/21] tools lib: Add list_sort.

[Ian Rogers]

Add list_sort.[ch] from the main kernel tree. The linux/bug.h #include
is removed due to conflicting definitions. Add check-headers and modify
perf build accordingly.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/include/linux/list_sort.h |  14 ++
 tools/lib/list_sort.c           | 252 ++++++++++++++++++++++++++++++++
 tools/perf/check-headers.sh     |   2 +
 tools/perf/util/Build           |   5 +
 4 files changed, 273 insertions(+)
 create mode 100644 tools/include/linux/list_sort.h
 create mode 100644 tools/lib/list_sort.c

diff --git a/tools/include/linux/list_sort.h b/tools/include/linux/list_sort.h
new file mode 100644
index 000000000000..453105f74e05
--- /dev/null
+++ b/tools/include/linux/list_sort.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_LIST_SORT_H
+#define _LINUX_LIST_SORT_H
+
+#include <linux/types.h>
+
+struct list_head;
+
+typedef int __attribute__((nonnull(2,3))) (*list_cmp_func_t)(void *,
+		const struct list_head *, const struct list_head *);
+
+__attribute__((nonnull(2,3)))
+void list_sort(void *priv, struct list_head *head, list_cmp_func_t cmp);
+#endif
diff --git a/tools/lib/list_sort.c b/tools/lib/list_sort.c
new file mode 100644
index 000000000000..10c067e3a8d2
--- /dev/null
+++ b/tools/lib/list_sort.c
@@ -0,0 +1,252 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/kernel.h>
+#include <linux/compiler.h>
+#include <linux/export.h>
+#include <linux/string.h>
+#include <linux/list_sort.h>
+#include <linux/list.h>
+
+/*
+ * Returns a list organized in an intermediate format suited
+ * to chaining of merge() calls: null-terminated, no reserved or
+ * sentinel head node, "prev" links not maintained.
+ */
+__attribute__((nonnull(2,3,4)))
+static struct list_head *merge(void *priv, list_cmp_func_t cmp,
+				struct list_head *a, struct list_head *b)
+{
+	struct list_head *head, **tail = &head;
+
+	for (;;) {
+		/* if equal, take 'a' -- important for sort stability */
+		if (cmp(priv, a, b) <= 0) {
+			*tail = a;
+			tail = &a->next;
+			a = a->next;
+			if (!a) {
+				*tail = b;
+				break;
+			}
+		} else {
+			*tail = b;
+			tail = &b->next;
+			b = b->next;
+			if (!b) {
+				*tail = a;
+				break;
+			}
+		}
+	}
+	return head;
+}
+
+/*
+ * Combine final list merge with restoration of standard doubly-linked
+ * list structure.  This approach duplicates code from merge(), but
+ * runs faster than the tidier alternatives of either a separate final
+ * prev-link restoration pass, or maintaining the prev links
+ * throughout.
+ */
+__attribute__((nonnull(2,3,4,5)))
+static void merge_final(void *priv, list_cmp_func_t cmp, struct list_head *head,
+			struct list_head *a, struct list_head *b)
+{
+	struct list_head *tail = head;
+	u8 count = 0;
+
+	for (;;) {
+		/* if equal, take 'a' -- important for sort stability */
+		if (cmp(priv, a, b) <= 0) {
+			tail->next = a;
+			a->prev = tail;
+			tail = a;
+			a = a->next;
+			if (!a)
+				break;
+		} else {
+			tail->next = b;
+			b->prev = tail;
+			tail = b;
+			b = b->next;
+			if (!b) {
+				b = a;
+				break;
+			}
+		}
+	}
+
+	/* Finish linking remainder of list b on to tail */
+	tail->next = b;
+	do {
+		/*
+		 * If the merge is highly unbalanced (e.g. the input is
+		 * already sorted), this loop may run many iterations.
+		 * Continue callbacks to the client even though no
+		 * element comparison is needed, so the client's cmp()
+		 * routine can invoke cond_resched() periodically.
+		 */
+		if (unlikely(!++count))
+			cmp(priv, b, b);
+		b->prev = tail;
+		tail = b;
+		b = b->next;
+	} while (b);
+
+	/* And the final links to make a circular doubly-linked list */
+	tail->next = head;
+	head->prev = tail;
+}
+
+/**
+ * list_sort - sort a list
+ * @priv: private data, opaque to list_sort(), passed to @cmp
+ * @head: the list to sort
+ * @cmp: the elements comparison function
+ *
+ * The comparison function @cmp must return > 0 if @a should sort after
+ * @b ("@a > @b" if you want an ascending sort), and <= 0 if @a should
+ * sort before @b *or* their original order should be preserved.  It is
+ * always called with the element that came first in the input in @a,
+ * and list_sort is a stable sort, so it is not necessary to distinguish
+ * the @a < @b and @a == @b cases.
+ *
+ * This is compatible with two styles of @cmp function:
+ * - The traditional style which returns <0 / =0 / >0, or
+ * - Returning a boolean 0/1.
+ * The latter offers a chance to save a few cycles in the comparison
+ * (which is used by e.g. plug_ctx_cmp() in block/blk-mq.c).
+ *
+ * A good way to write a multi-word comparison is::
+ *
+ *	if (a->high != b->high)
+ *		return a->high > b->high;
+ *	if (a->middle != b->middle)
+ *		return a->middle > b->middle;
+ *	return a->low > b->low;
+ *
+ *
+ * This mergesort is as eager as possible while always performing at least
+ * 2:1 balanced merges.  Given two pending sublists of size 2^k, they are
+ * merged to a size-2^(k+1) list as soon as we have 2^k following elements.
+ *
+ * Thus, it will avoid cache thrashing as long as 3*2^k elements can
+ * fit into the cache.  Not quite as good as a fully-eager bottom-up
+ * mergesort, but it does use 0.2*n fewer comparisons, so is faster in
+ * the common case that everything fits into L1.
+ *
+ *
+ * The merging is controlled by "count", the number of elements in the
+ * pending lists.  This is beautifully simple code, but rather subtle.
+ *
+ * Each time we increment "count", we set one bit (bit k) and clear
+ * bits k-1 .. 0.  Each time this happens (except the very first time
+ * for each bit, when count increments to 2^k), we merge two lists of
+ * size 2^k into one list of size 2^(k+1).
+ *
+ * This merge happens exactly when the count reaches an odd multiple of
+ * 2^k, which is when we have 2^k elements pending in smaller lists,
+ * so it's safe to merge away two lists of size 2^k.
+ *
+ * After this happens twice, we have created two lists of size 2^(k+1),
+ * which will be merged into a list of size 2^(k+2) before we create
+ * a third list of size 2^(k+1), so there are never more than two pending.
+ *
+ * The number of pending lists of size 2^k is determined by the
+ * state of bit k of "count" plus two extra pieces of information:
+ *
+ * - The state of bit k-1 (when k == 0, consider bit -1 always set), and
+ * - Whether the higher-order bits are zero or non-zero (i.e.
+ *   is count >= 2^(k+1)).
+ *
+ * There are six states we distinguish.  "x" represents some arbitrary
+ * bits, and "y" represents some arbitrary non-zero bits:
+ * 0:  00x: 0 pending of size 2^k;           x pending of sizes < 2^k
+ * 1:  01x: 0 pending of size 2^k; 2^(k-1) + x pending of sizes < 2^k
+ * 2: x10x: 0 pending of size 2^k; 2^k     + x pending of sizes < 2^k
+ * 3: x11x: 1 pending of size 2^k; 2^(k-1) + x pending of sizes < 2^k
+ * 4: y00x: 1 pending of size 2^k; 2^k     + x pending of sizes < 2^k
+ * 5: y01x: 2 pending of size 2^k; 2^(k-1) + x pending of sizes < 2^k
+ * (merge and loop back to state 2)
+ *
+ * We gain lists of size 2^k in the 2->3 and 4->5 transitions (because
+ * bit k-1 is set while the more significant bits are non-zero) and
+ * merge them away in the 5->2 transition.  Note in particular that just
+ * before the 5->2 transition, all lower-order bits are 11 (state 3),
+ * so there is one list of each smaller size.
+ *
+ * When we reach the end of the input, we merge all the pending
+ * lists, from smallest to largest.  If you work through cases 2 to
+ * 5 above, you can see that the number of elements we merge with a list
+ * of size 2^k varies from 2^(k-1) (cases 3 and 5 when x == 0) to
+ * 2^(k+1) - 1 (second merge of case 5 when x == 2^(k-1) - 1).
+ */
+__attribute__((nonnull(2,3)))
+void list_sort(void *priv, struct list_head *head, list_cmp_func_t cmp)
+{
+	struct list_head *list = head->next, *pending = NULL;
+	size_t count = 0;	/* Count of pending */
+
+	if (list == head->prev)	/* Zero or one elements */
+		return;
+
+	/* Convert to a null-terminated singly-linked list. */
+	head->prev->next = NULL;
+
+	/*
+	 * Data structure invariants:
+	 * - All lists are singly linked and null-terminated; prev
+	 *   pointers are not maintained.
+	 * - pending is a prev-linked "list of lists" of sorted
+	 *   sublists awaiting further merging.
+	 * - Each of the sorted sublists is power-of-two in size.
+	 * - Sublists are sorted by size and age, smallest & newest at front.
+	 * - There are zero to two sublists of each size.
+	 * - A pair of pending sublists are merged as soon as the number
+	 *   of following pending elements equals their size (i.e.
+	 *   each time count reaches an odd multiple of that size).
+	 *   That ensures each later final merge will be at worst 2:1.
+	 * - Each round consists of:
+	 *   - Merging the two sublists selected by the highest bit
+	 *     which flips when count is incremented, and
+	 *   - Adding an element from the input as a size-1 sublist.
+	 */
+	do {
+		size_t bits;
+		struct list_head **tail = &pending;
+
+		/* Find the least-significant clear bit in count */
+		for (bits = count; bits & 1; bits >>= 1)
+			tail = &(*tail)->prev;
+		/* Do the indicated merge */
+		if (likely(bits)) {
+			struct list_head *a = *tail, *b = a->prev;
+
+			a = merge(priv, cmp, b, a);
+			/* Install the merged result in place of the inputs */
+			a->prev = b->prev;
+			*tail = a;
+		}
+
+		/* Move one element from input list to pending */
+		list->prev = pending;
+		pending = list;
+		list = list->next;
+		pending->next = NULL;
+		count++;
+	} while (list);
+
+	/* End of input; merge together all the pending lists. */
+	list = pending;
+	pending = pending->prev;
+	for (;;) {
+		struct list_head *next = pending->prev;
+
+		if (!next)
+			break;
+		list = merge(priv, cmp, pending, list);
+		pending = next;
+	}
+	/* The final merge, rebuilding prev links */
+	merge_final(priv, cmp, head, pending, list);
+}
+EXPORT_SYMBOL(list_sort);
diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index f1e46277e822..30ecf3a0f68b 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -26,6 +26,7 @@ include/vdso/bits.h
 include/linux/const.h
 include/vdso/const.h
 include/linux/hash.h
+include/linux/list-sort.h
 include/uapi/linux/hw_breakpoint.h
 arch/x86/include/asm/disabled-features.h
 arch/x86/include/asm/required-features.h
@@ -150,6 +151,7 @@ check include/uapi/linux/mman.h       '-I "^#include <\(uapi/\)*asm/mman.h>"'
 check include/linux/build_bug.h       '-I "^#\(ifndef\|endif\)\( \/\/\)* static_assert$"'
 check include/linux/ctype.h	      '-I "isdigit("'
 check lib/ctype.c		      '-I "^EXPORT_SYMBOL" -I "^#include <linux/export.h>" -B'
+check lib/list_sort.c		      '-I "^#include <linux/bug.h>"'
 
 # diff non-symmetric files
 check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index f2914d5bed6e..15b2366ad384 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -138,6 +138,7 @@ perf-y += expr.o
 perf-y += branch.o
 perf-y += mem2node.o
 perf-y += clockid.o
+perf-y += list_sort.o
 
 perf-$(CONFIG_LIBBPF) += bpf-loader.o
 perf-$(CONFIG_LIBBPF) += bpf_map.o
@@ -315,3 +316,7 @@ $(OUTPUT)util/hweight.o: ../lib/hweight.c FORCE
 $(OUTPUT)util/vsprintf.o: ../lib/vsprintf.c FORCE
 	$(call rule_mkdir)
 	$(call if_changed_dep,cc_o_c)
+
+$(OUTPUT)util/list_sort.o: ../lib/list_sort.c FORCE
+	$(call rule_mkdir)
+	$(call if_changed_dep,cc_o_c)
-- 
2.33.0.882.g93a45727a2-goog

Re: [PATCH 01/21] tools lib: Add list_sort.

[Andi Kleen]

On 10/7/2021 9:56 AM, Ian Rogers wrote:
> Add list_sort.[ch] from the main kernel tree. The linux/bug.h #include
> is removed due to conflicting definitions. Add check-headers and modify
> perf build accordingly.


It would be nicer if we could share the kernel files somehow, but I 
guess it might be a long term maintenance issue if the kernel version 
does changes that make it incompatible with the user build.


-Andi

Re: [PATCH 01/21] tools lib: Add list_sort.

[Arnaldo Carvalho de Melo]

Em Thu, Oct 07, 2021 at 09:56:27AM -0700, Ian Rogers escreveu:
> Add list_sort.[ch] from the main kernel tree. The linux/bug.h #include
> is removed due to conflicting definitions. Add check-headers and modify
> perf build accordingly.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

⬢[acme@toolbox perf]$ make -C tools/perf build-test
make: Entering directory '/var/home/acme/git/perf/tools/perf'
- tarpkg: ./tests/perf-targz-src-pkg .
make[1]: *** [tests/make:347: tarpkg] Error 2
make: *** [Makefile:103: build-test] Error 2
make: Leaving directory '/var/home/acme/git/perf/tools/perf'

⬢[acme@toolbox perf]$ ./tools/perf/tests/perf-targz-src-pkg tools/perf
  PERF_VERSION = 5.15.rc4.g0df4a50ab5aa
  PERF_VERSION = 5.15.rc4.g0df4a50ab5aa
make[4]: *** No rule to make target '../lib/list_sort.c', needed by 'util/list_sort.o'.  Stop.
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [/tmp/tmp.FxRkhaWUYd/perf-5.15.0-rc4/tools/build/Makefile.build:139: util] Error 2
make[2]: *** [Makefile.perf:660: perf-in.o] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [Makefile.perf:240: sub-make] Error 2
make: *** [Makefile:70: all] Error 2
⬢[acme@toolbox perf]$

Adding this new file to the file below cures things, I'm doing it:

diff --git a/tools/perf/MANIFEST b/tools/perf/MANIFEST
index f05c4d48fd7e01c0..e728615a3830848f 100644
--- a/tools/perf/MANIFEST
+++ b/tools/perf/MANIFEST
@@ -17,6 +17,7 @@ tools/lib/symbol/kallsyms.c
 tools/lib/symbol/kallsyms.h
 tools/lib/find_bit.c
 tools/lib/bitmap.c
+tools/lib/list_sort.c
 tools/lib/str_error_r.c
 tools/lib/vsprintf.c
 tools/lib/zalloc.c
diff --git a/tools/perf/util/python-ext-sources b/tools/perf/util/python-ext-sources
index d7c976671e3a131a..a685d20165f785a1 100644
--- a/tools/perf/util/python-ext-sources
+++ b/tools/perf/util/python-ext-sources
@@ -18,6 +18,7 @@ util/mmap.c
 util/namespaces.c
 ../lib/bitmap.c
 ../lib/find_bit.c
+../lib/list_sort.c
 ../lib/hweight.c
 ../lib/string.c
 ../lib/vsprintf.c

[PATCH 02/21] perf pmu: Add const to pmu_events_map.

[Ian Rogers]

The pmu_events_map is generated at compile time and used for lookup. For
testing purposes we need to swap the map being used. Having the
pmu_events_map be non-const is misleading as it may be an out argument.
Make it const and update uses so they work on const too.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/pmu-events/jevents.c    |  2 +-
 tools/perf/pmu-events/pmu-events.h |  2 +-
 tools/perf/tests/expand-cgroup.c   |  2 +-
 tools/perf/tests/parse-metric.c    |  2 +-
 tools/perf/tests/pmu-events.c      | 18 +++++++++---------
 tools/perf/util/metricgroup.c      | 20 ++++++++++----------
 tools/perf/util/metricgroup.h      |  4 ++--
 tools/perf/util/pmu.c              | 10 +++++-----
 tools/perf/util/pmu.h              |  6 +++---
 tools/perf/util/s390-sample-raw.c  |  4 ++--
 10 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 19497e4f8a86..5624a37d6f93 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -798,7 +798,7 @@ static bool is_sys_dir(char *fname)
 
 static void print_mapping_table_prefix(FILE *outfp)
 {
-	fprintf(outfp, "struct pmu_events_map pmu_events_map[] = {\n");
+	fprintf(outfp, "const struct pmu_events_map pmu_events_map[] = {\n");
 }
 
 static void print_mapping_table_suffix(FILE *outfp)
diff --git a/tools/perf/pmu-events/pmu-events.h b/tools/perf/pmu-events/pmu-events.h
index 5c2bf7275c1c..42c6db6bedec 100644
--- a/tools/perf/pmu-events/pmu-events.h
+++ b/tools/perf/pmu-events/pmu-events.h
@@ -53,7 +53,7 @@ struct pmu_sys_events {
  * Global table mapping each known CPU for the architecture to its
  * table of PMU events.
  */
-extern struct pmu_events_map pmu_events_map[];
+extern const struct pmu_events_map pmu_events_map[];
 extern struct pmu_sys_events pmu_sys_event_tables[];
 
 #endif
diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
index 0e46aeb843ce..aaad51aba12f 100644
--- a/tools/perf/tests/expand-cgroup.c
+++ b/tools/perf/tests/expand-cgroup.c
@@ -193,7 +193,7 @@ static int expand_metric_events(void)
 			.metric_name	= NULL,
 		},
 	};
-	struct pmu_events_map ev_map = {
+	const struct pmu_events_map ev_map = {
 		.cpuid		= "test",
 		.version	= "1",
 		.type		= "core",
diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
index 4f6f4904e852..dfc797ecc750 100644
--- a/tools/perf/tests/parse-metric.c
+++ b/tools/perf/tests/parse-metric.c
@@ -79,7 +79,7 @@ static struct pmu_event pme_test[] = {
 }
 };
 
-static struct pmu_events_map map = {
+static const struct pmu_events_map map = {
 	.cpuid		= "test",
 	.version	= "1",
 	.type		= "core",
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index d3534960ed25..8a1fdcd072f5 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -242,9 +242,9 @@ static bool is_same(const char *reference, const char *test)
 	return !strcmp(reference, test);
 }
 
-static struct pmu_events_map *__test_pmu_get_events_map(void)
+static const struct pmu_events_map *__test_pmu_get_events_map(void)
 {
-	struct pmu_events_map *map;
+	const struct pmu_events_map *map;
 
 	for (map = &pmu_events_map[0]; map->cpuid; map++) {
 		if (!strcmp(map->cpuid, "testcpu"))
@@ -421,7 +421,7 @@ static int compare_alias_to_test_event(struct perf_pmu_alias *alias,
 static int test_pmu_event_table(void)
 {
 	struct pmu_event *sys_event_tables = __test_pmu_get_sys_events_table();
-	struct pmu_events_map *map = __test_pmu_get_events_map();
+	const struct pmu_events_map *map = __test_pmu_get_events_map();
 	struct pmu_event *table;
 	int map_events = 0, expected_events;
 
@@ -518,7 +518,7 @@ static int __test_core_pmu_event_aliases(char *pmu_name, int *count)
 	struct perf_pmu *pmu;
 	LIST_HEAD(aliases);
 	int res = 0;
-	struct pmu_events_map *map = __test_pmu_get_events_map();
+	const struct pmu_events_map *map = __test_pmu_get_events_map();
 	struct perf_pmu_alias *a, *tmp;
 
 	if (!map)
@@ -571,7 +571,7 @@ static int __test_uncore_pmu_event_aliases(struct perf_pmu_test_pmu *test_pmu)
 	struct perf_pmu *pmu = &test_pmu->pmu;
 	const char *pmu_name = pmu->name;
 	struct perf_pmu_alias *a, *tmp, *alias;
-	struct pmu_events_map *map;
+	const struct pmu_events_map *map;
 	LIST_HEAD(aliases);
 	int res = 0;
 
@@ -825,7 +825,7 @@ struct metric {
 
 static int resolve_metric_simple(struct expr_parse_ctx *pctx,
 				 struct list_head *compound_list,
-				 struct pmu_events_map *map,
+				 const struct pmu_events_map *map,
 				 const char *metric_name)
 {
 	struct hashmap_entry *cur, *cur_tmp;
@@ -885,8 +885,8 @@ static int resolve_metric_simple(struct expr_parse_ctx *pctx,
 
 static int test_parsing(void)
 {
-	struct pmu_events_map *cpus_map = pmu_events_map__find();
-	struct pmu_events_map *map;
+	const struct pmu_events_map *cpus_map = pmu_events_map__find();
+	const struct pmu_events_map *map;
 	struct pmu_event *pe;
 	int i, j, k;
 	int ret = 0;
@@ -1027,7 +1027,7 @@ static int metric_parse_fake(const char *str)
  */
 static int test_parsing_fake(void)
 {
-	struct pmu_events_map *map;
+	const struct pmu_events_map *map;
 	struct pmu_event *pe;
 	unsigned int i, j;
 	int err = 0;
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 8ba5370f5f64..74ea0a3540ce 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -623,7 +623,7 @@ static int metricgroup__print_sys_event_iter(struct pmu_event *pe, void *data)
 void metricgroup__print(bool metrics, bool metricgroups, char *filter,
 			bool raw, bool details)
 {
-	struct pmu_events_map *map = pmu_events_map__find();
+	const struct pmu_events_map *map = pmu_events_map__find();
 	struct pmu_event *pe;
 	int i;
 	struct rblist groups;
@@ -910,7 +910,7 @@ static int __add_metric(struct list_head *metric_list,
 		     match_metric(__pe->metric_name, __metric)))
 
 struct pmu_event *metricgroup__find_metric(const char *metric,
-					   struct pmu_events_map *map)
+					   const struct pmu_events_map *map)
 {
 	struct pmu_event *pe;
 	int i;
@@ -977,7 +977,7 @@ static int add_metric(struct list_head *metric_list,
 static int __resolve_metric(struct metric *m,
 			    bool metric_no_group,
 			    struct list_head *metric_list,
-			    struct pmu_events_map *map,
+			    const struct pmu_events_map *map,
 			    struct expr_ids *ids)
 {
 	struct hashmap_entry *cur;
@@ -1025,7 +1025,7 @@ static int __resolve_metric(struct metric *m,
 
 static int resolve_metric(bool metric_no_group,
 			  struct list_head *metric_list,
-			  struct pmu_events_map *map,
+			  const struct pmu_events_map *map,
 			  struct expr_ids *ids)
 {
 	struct metric *m;
@@ -1099,7 +1099,7 @@ static int metricgroup__add_metric_sys_event_iter(struct pmu_event *pe,
 static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 				   struct strbuf *events,
 				   struct list_head *metric_list,
-				   struct pmu_events_map *map)
+				   const struct pmu_events_map *map)
 {
 	struct expr_ids ids = { .cnt = 0, };
 	struct pmu_event *pe;
@@ -1173,7 +1173,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 					struct strbuf *events,
 					struct list_head *metric_list,
-					struct pmu_events_map *map)
+					const struct pmu_events_map *map)
 {
 	char *llist, *nlist, *p;
 	int ret = -EINVAL;
@@ -1230,7 +1230,7 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 			bool metric_no_merge,
 			struct perf_pmu *fake_pmu,
 			struct rblist *metric_events,
-			struct pmu_events_map *map)
+			const struct pmu_events_map *map)
 {
 	struct parse_events_error parse_error;
 	struct strbuf extra_events;
@@ -1266,14 +1266,14 @@ int metricgroup__parse_groups(const struct option *opt,
 			      struct rblist *metric_events)
 {
 	struct evlist *perf_evlist = *(struct evlist **)opt->value;
-	struct pmu_events_map *map = pmu_events_map__find();
+	const struct pmu_events_map *map = pmu_events_map__find();
 
 	return parse_groups(perf_evlist, str, metric_no_group,
 			    metric_no_merge, NULL, metric_events, map);
 }
 
 int metricgroup__parse_groups_test(struct evlist *evlist,
-				   struct pmu_events_map *map,
+				   const struct pmu_events_map *map,
 				   const char *str,
 				   bool metric_no_group,
 				   bool metric_no_merge,
@@ -1285,7 +1285,7 @@ int metricgroup__parse_groups_test(struct evlist *evlist,
 
 bool metricgroup__has_metric(const char *metric)
 {
-	struct pmu_events_map *map = pmu_events_map__find();
+	const struct pmu_events_map *map = pmu_events_map__find();
 	struct pmu_event *pe;
 	int i;
 
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index cc4a92492a61..c931596557bf 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -44,9 +44,9 @@ int metricgroup__parse_groups(const struct option *opt,
 			      bool metric_no_merge,
 			      struct rblist *metric_events);
 struct pmu_event *metricgroup__find_metric(const char *metric,
-					   struct pmu_events_map *map);
+					   const struct pmu_events_map *map);
 int metricgroup__parse_groups_test(struct evlist *evlist,
-				   struct pmu_events_map *map,
+				   const struct pmu_events_map *map,
 				   const char *str,
 				   bool metric_no_group,
 				   bool metric_no_merge,
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index bdabd62170d2..4bcdc595ce5e 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -710,9 +710,9 @@ static char *perf_pmu__getcpuid(struct perf_pmu *pmu)
 	return cpuid;
 }
 
-struct pmu_events_map *perf_pmu__find_map(struct perf_pmu *pmu)
+const struct pmu_events_map *perf_pmu__find_map(struct perf_pmu *pmu)
 {
-	struct pmu_events_map *map;
+	const struct pmu_events_map *map;
 	char *cpuid = perf_pmu__getcpuid(pmu);
 	int i;
 
@@ -737,7 +737,7 @@ struct pmu_events_map *perf_pmu__find_map(struct perf_pmu *pmu)
 	return map;
 }
 
-struct pmu_events_map *__weak pmu_events_map__find(void)
+const struct pmu_events_map *__weak pmu_events_map__find(void)
 {
 	return perf_pmu__find_map(NULL);
 }
@@ -824,7 +824,7 @@ bool pmu_uncore_alias_match(const char *pmu_name, const char *name)
  * as aliases.
  */
 void pmu_add_cpu_aliases_map(struct list_head *head, struct perf_pmu *pmu,
-			     struct pmu_events_map *map)
+			     const struct pmu_events_map *map)
 {
 	int i;
 	const char *name = pmu->name;
@@ -859,7 +859,7 @@ void pmu_add_cpu_aliases_map(struct list_head *head, struct perf_pmu *pmu,
 
 static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)
 {
-	struct pmu_events_map *map;
+	const struct pmu_events_map *map;
 
 	map = perf_pmu__find_map(pmu);
 	if (!map)
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 394898b07fd9..dd5cdde6a3d0 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -120,10 +120,10 @@ int perf_pmu__test(void);
 
 struct perf_event_attr *perf_pmu__get_default_config(struct perf_pmu *pmu);
 void pmu_add_cpu_aliases_map(struct list_head *head, struct perf_pmu *pmu,
-			     struct pmu_events_map *map);
+			     const struct pmu_events_map *map);
 
-struct pmu_events_map *perf_pmu__find_map(struct perf_pmu *pmu);
-struct pmu_events_map *pmu_events_map__find(void);
+const struct pmu_events_map *perf_pmu__find_map(struct perf_pmu *pmu);
+const struct pmu_events_map *pmu_events_map__find(void);
 bool pmu_uncore_alias_match(const char *pmu_name, const char *name);
 void perf_pmu_free_alias(struct perf_pmu_alias *alias);
 
diff --git a/tools/perf/util/s390-sample-raw.c b/tools/perf/util/s390-sample-raw.c
index 08ec3c3ae0ee..13f33d1ddb78 100644
--- a/tools/perf/util/s390-sample-raw.c
+++ b/tools/perf/util/s390-sample-raw.c
@@ -135,7 +135,7 @@ static int get_counterset_start(int setnr)
  * the name of this counter.
  * If no match is found a NULL pointer is returned.
  */
-static const char *get_counter_name(int set, int nr, struct pmu_events_map *map)
+static const char *get_counter_name(int set, int nr, const struct pmu_events_map *map)
 {
 	int rc, event_nr, wanted = get_counterset_start(set) + nr;
 
@@ -159,7 +159,7 @@ static void s390_cpumcfdg_dump(struct perf_sample *sample)
 	unsigned char *buf = sample->raw_data;
 	const char *color = PERF_COLOR_BLUE;
 	struct cf_ctrset_entry *cep, ce;
-	struct pmu_events_map *map;
+	const struct pmu_events_map *map;
 	u64 *p;
 
 	map = pmu_events_map__find();
-- 
2.33.0.882.g93a45727a2-goog

Re: [PATCH 02/21] perf pmu: Add const to pmu_events_map.

[John Garry]

On 07/10/2021 17:56, Ian Rogers wrote:
> The pmu_events_map is generated at compile time and used for lookup. For
> testing purposes we need to swap the map being used. Having the
> pmu_events_map be non-const is misleading as it may be an out argument.
> Make it const and update uses so they work on const too.
> 
> Signed-off-by: Ian Rogers<irogers@google.com>

Reviewed-by: John Garry <john.garry@huawei.com>

Re: [PATCH 02/21] perf pmu: Add const to pmu_events_map.

[Andrew Kilroy]

On 08/10/2021 12:01, John Garry wrote:
> On 07/10/2021 17:56, Ian Rogers wrote:
>> The pmu_events_map is generated at compile time and used for lookup. For
>> testing purposes we need to swap the map being used. Having the
>> pmu_events_map be non-const is misleading as it may be an out argument.
>> Make it const and update uses so they work on const too.
>>
>> Signed-off-by: Ian Rogers<irogers@google.com>
> 
> Reviewed-by: John Garry <john.garry@huawei.com>

Got a compile error for this on arm64 when basing these patches on 
acme/perf/core (be8ecc57f180415e8a7c1cc5620c5236be2a7e56):

$ make DEBUG=1 O=output
...<snipped>...
arch/arm64/util/pmu.c:6:24: error: conflicting types for 
‘pmu_events_map__find’
  struct pmu_events_map *pmu_events_map__find(void)
                         ^~~~~~~~~~~~~~~~~~~~
In file included from arch/arm64/util/pmu.c:4:0:
arch/arm64/util/../../../util/pmu.h:126:30: note: previous declaration 
of ‘pmu_events_map__find’ was here
  const struct pmu_events_map *pmu_events_map__find(void);
                               ^~~~~~~~~~~~~~~~~~~~
arch/arm64/util/pmu.c: In function ‘pmu_events_map__find’:
arch/arm64/util/pmu.c:21:10: error: return discards ‘const’ qualifier 
from pointer target type [-Werror=discarded-qualifiers]
    return perf_pmu__find_map(pmu);
           ^~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
/home/andkil01/linux/tools/build/Makefile.build:96: recipe for target 
'/home/andkil01/linux/tools/perf/output/arch/arm64/util/pmu.o' failed
make[6]: *** 
[/home/andkil01/linux/tools/perf/output/arch/arm64/util/pmu.o] Error 1
/home/andkil01/linux/tools/build/Makefile.build:139: recipe for target 
'util' failed
make[5]: *** [util] Error 2
/home/andkil01/linux/tools/build/Makefile.build:139: recipe for target 
'arm64' failed
make[4]: *** [arm64] Error 2
/home/andkil01/linux/tools/build/Makefile.build:139: recipe for target 
'arch' failed
make[3]: *** [arch] Error 2
make[3]: *** Waiting for unfinished jobs....


Andrew

[PATCH 03/21] perf pmu: Make pmu_sys_event_tables const.

[Ian Rogers]

Make lookup nature of data structures clearer through their type.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/pmu-events/jevents.c    | 2 +-
 tools/perf/pmu-events/pmu-events.h | 2 +-
 tools/perf/tests/pmu-events.c      | 2 +-
 tools/perf/util/pmu.c              | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 5624a37d6f93..a31de0f77097 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -832,7 +832,7 @@ static void print_mapping_test_table(FILE *outfp)
 
 static void print_system_event_mapping_table_prefix(FILE *outfp)
 {
-	fprintf(outfp, "\nstruct pmu_sys_events pmu_sys_event_tables[] = {");
+	fprintf(outfp, "\nconst struct pmu_sys_events pmu_sys_event_tables[] = {");
 }
 
 static void print_system_event_mapping_table_suffix(FILE *outfp)
diff --git a/tools/perf/pmu-events/pmu-events.h b/tools/perf/pmu-events/pmu-events.h
index 42c6db6bedec..f6c9c9fc4ab2 100644
--- a/tools/perf/pmu-events/pmu-events.h
+++ b/tools/perf/pmu-events/pmu-events.h
@@ -54,6 +54,6 @@ struct pmu_sys_events {
  * table of PMU events.
  */
 extern const struct pmu_events_map pmu_events_map[];
-extern struct pmu_sys_events pmu_sys_event_tables[];
+extern const struct pmu_sys_events pmu_sys_event_tables[];
 
 #endif
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index 8a1fdcd072f5..c0f8b61871c8 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -258,7 +258,7 @@ static const struct pmu_events_map *__test_pmu_get_events_map(void)
 
 static struct pmu_event *__test_pmu_get_sys_events_table(void)
 {
-	struct pmu_sys_events *tables = &pmu_sys_event_tables[0];
+	const struct pmu_sys_events *tables = &pmu_sys_event_tables[0];
 
 	for ( ; tables->name; tables++) {
 		if (!strcmp("pme_test_soc_sys", tables->name))
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 4bcdc595ce5e..c04a89cc7cef 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -873,7 +873,7 @@ void pmu_for_each_sys_event(pmu_sys_event_iter_fn fn, void *data)
 	int i = 0;
 
 	while (1) {
-		struct pmu_sys_events *event_table;
+		const struct pmu_sys_events *event_table;
 		int j = 0;
 
 		event_table = &pmu_sys_event_tables[i++];
-- 
2.33.0.882.g93a45727a2-goog

Re: [PATCH 03/21] perf pmu: Make pmu_sys_event_tables const.

[John Garry]

On 07/10/2021 17:56, Ian Rogers wrote:
> Make lookup nature of data structures clearer through their type.
> 
> Signed-off-by: Ian Rogers<irogers@google.com>


Reviewed-by: John Garry <john.garry@huawei.com>

[PATCH 04/21] perf pmu: Make pmu_event tables const.

[Ian Rogers]

Make lookup nature of data structures clearer through their type.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/powerpc/util/header.c |  2 +-
 tools/perf/pmu-events/jevents.c       |  2 +-
 tools/perf/pmu-events/pmu-events.h    |  4 +--
 tools/perf/tests/pmu-events.c         | 16 ++++++------
 tools/perf/util/metricgroup.c         | 36 +++++++++++++--------------
 tools/perf/util/metricgroup.h         |  6 ++---
 tools/perf/util/pmu.c                 |  8 +++---
 tools/perf/util/pmu.h                 |  2 +-
 tools/perf/util/s390-sample-raw.c     |  2 +-
 9 files changed, 39 insertions(+), 39 deletions(-)

diff --git a/tools/perf/arch/powerpc/util/header.c b/tools/perf/arch/powerpc/util/header.c
index 58b2d610aadb..e8fe36b10d20 100644
--- a/tools/perf/arch/powerpc/util/header.c
+++ b/tools/perf/arch/powerpc/util/header.c
@@ -40,7 +40,7 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
 	return bufp;
 }
 
-int arch_get_runtimeparam(struct pmu_event *pe)
+int arch_get_runtimeparam(const struct pmu_event *pe)
 {
 	int count;
 	char path[PATH_MAX] = "/devices/hv_24x7/interface/";
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index a31de0f77097..b3431c11c9cb 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -362,7 +362,7 @@ static int close_table;
 
 static void print_events_table_prefix(FILE *fp, const char *tblname)
 {
-	fprintf(fp, "struct pmu_event %s[] = {\n", tblname);
+	fprintf(fp, "const struct pmu_event %s[] = {\n", tblname);
 	close_table = 1;
 }
 
diff --git a/tools/perf/pmu-events/pmu-events.h b/tools/perf/pmu-events/pmu-events.h
index f6c9c9fc4ab2..6efe73976440 100644
--- a/tools/perf/pmu-events/pmu-events.h
+++ b/tools/perf/pmu-events/pmu-events.h
@@ -41,12 +41,12 @@ struct pmu_events_map {
 	const char *cpuid;
 	const char *version;
 	const char *type;		/* core, uncore etc */
-	struct pmu_event *table;
+	const struct pmu_event *table;
 };
 
 struct pmu_sys_events {
 	const char *name;
-	struct pmu_event *table;
+	const struct pmu_event *table;
 };
 
 /*
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index c0f8b61871c8..cc5cea141beb 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -256,7 +256,7 @@ static const struct pmu_events_map *__test_pmu_get_events_map(void)
 	return NULL;
 }
 
-static struct pmu_event *__test_pmu_get_sys_events_table(void)
+static const struct pmu_event *__test_pmu_get_sys_events_table(void)
 {
 	const struct pmu_sys_events *tables = &pmu_sys_event_tables[0];
 
@@ -268,7 +268,7 @@ static struct pmu_event *__test_pmu_get_sys_events_table(void)
 	return NULL;
 }
 
-static int compare_pmu_events(struct pmu_event *e1, const struct pmu_event *e2)
+static int compare_pmu_events(const struct pmu_event *e1, const struct pmu_event *e2)
 {
 	if (!is_same(e1->name, e2->name)) {
 		pr_debug2("testing event e1 %s: mismatched name string, %s vs %s\n",
@@ -420,9 +420,9 @@ static int compare_alias_to_test_event(struct perf_pmu_alias *alias,
 /* Verify generated events from pmu-events.c are as expected */
 static int test_pmu_event_table(void)
 {
-	struct pmu_event *sys_event_tables = __test_pmu_get_sys_events_table();
+	const struct pmu_event *sys_event_tables = __test_pmu_get_sys_events_table();
 	const struct pmu_events_map *map = __test_pmu_get_events_map();
-	struct pmu_event *table;
+	const struct pmu_event *table;
 	int map_events = 0, expected_events;
 
 	/* ignore 3x sentinels */
@@ -774,7 +774,7 @@ static int check_parse_id(const char *id, struct parse_events_error *error,
 	return ret;
 }
 
-static int check_parse_cpu(const char *id, bool same_cpu, struct pmu_event *pe)
+static int check_parse_cpu(const char *id, bool same_cpu, const struct pmu_event *pe)
 {
 	struct parse_events_error error = { .idx = 0, };
 
@@ -838,7 +838,7 @@ static int resolve_metric_simple(struct expr_parse_ctx *pctx,
 		all = true;
 		hashmap__for_each_entry_safe(pctx->ids, cur, cur_tmp, bkt) {
 			struct metric_ref *ref;
-			struct pmu_event *pe;
+			const struct pmu_event *pe;
 
 			pe = metricgroup__find_metric(cur->key, map);
 			if (!pe)
@@ -887,7 +887,7 @@ static int test_parsing(void)
 {
 	const struct pmu_events_map *cpus_map = pmu_events_map__find();
 	const struct pmu_events_map *map;
-	struct pmu_event *pe;
+	const struct pmu_event *pe;
 	int i, j, k;
 	int ret = 0;
 	struct expr_parse_ctx *ctx;
@@ -1028,7 +1028,7 @@ static int metric_parse_fake(const char *str)
 static int test_parsing_fake(void)
 {
 	const struct pmu_events_map *map;
-	struct pmu_event *pe;
+	const struct pmu_event *pe;
 	unsigned int i, j;
 	int err = 0;
 
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 74ea0a3540ce..b60ccbbf0829 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -427,7 +427,7 @@ static bool match_metric(const char *n, const char *list)
 	return false;
 }
 
-static bool match_pe_metric(struct pmu_event *pe, const char *metric)
+static bool match_pe_metric(const struct pmu_event *pe, const char *metric)
 {
 	return match_metric(pe->metric_group, metric) ||
 	       match_metric(pe->metric_name, metric);
@@ -511,7 +511,7 @@ static void metricgroup__print_strlist(struct strlist *metrics, bool raw)
 		putchar('\n');
 }
 
-static int metricgroup__print_pmu_event(struct pmu_event *pe,
+static int metricgroup__print_pmu_event(const struct pmu_event *pe,
 					bool metricgroups, char *filter,
 					bool raw, bool details,
 					struct rblist *groups,
@@ -586,14 +586,14 @@ struct metricgroup_print_sys_idata {
 	bool details;
 };
 
-typedef int (*metricgroup_sys_event_iter_fn)(struct pmu_event *pe, void *);
+typedef int (*metricgroup_sys_event_iter_fn)(const struct pmu_event *pe, void *);
 
 struct metricgroup_iter_data {
 	metricgroup_sys_event_iter_fn fn;
 	void *data;
 };
 
-static int metricgroup__sys_event_iter(struct pmu_event *pe, void *data)
+static int metricgroup__sys_event_iter(const struct pmu_event *pe, void *data)
 {
 	struct metricgroup_iter_data *d = data;
 	struct perf_pmu *pmu = NULL;
@@ -612,7 +612,7 @@ static int metricgroup__sys_event_iter(struct pmu_event *pe, void *data)
 	return 0;
 }
 
-static int metricgroup__print_sys_event_iter(struct pmu_event *pe, void *data)
+static int metricgroup__print_sys_event_iter(const struct pmu_event *pe, void *data)
 {
 	struct metricgroup_print_sys_idata *d = data;
 
@@ -624,7 +624,7 @@ void metricgroup__print(bool metrics, bool metricgroups, char *filter,
 			bool raw, bool details)
 {
 	const struct pmu_events_map *map = pmu_events_map__find();
-	struct pmu_event *pe;
+	const struct pmu_event *pe;
 	int i;
 	struct rblist groups;
 	struct rb_node *node, *next;
@@ -756,7 +756,7 @@ static void metricgroup___watchdog_constraint_hint(const char *name, bool foot)
 		   "    echo 1 > /proc/sys/kernel/nmi_watchdog\n");
 }
 
-static bool metricgroup__has_constraint(struct pmu_event *pe)
+static bool metricgroup__has_constraint(const struct pmu_event *pe)
 {
 	if (!pe->metric_constraint)
 		return false;
@@ -770,7 +770,7 @@ static bool metricgroup__has_constraint(struct pmu_event *pe)
 	return false;
 }
 
-int __weak arch_get_runtimeparam(struct pmu_event *pe __maybe_unused)
+int __weak arch_get_runtimeparam(const struct pmu_event *pe __maybe_unused)
 {
 	return 1;
 }
@@ -785,7 +785,7 @@ struct metricgroup_add_iter_data {
 };
 
 static int __add_metric(struct list_head *metric_list,
-			struct pmu_event *pe,
+			const struct pmu_event *pe,
 			bool metric_no_group,
 			int runtime,
 			struct metric **mp,
@@ -909,10 +909,10 @@ static int __add_metric(struct list_head *metric_list,
 		    (match_metric(__pe->metric_group, __metric) ||	\
 		     match_metric(__pe->metric_name, __metric)))
 
-struct pmu_event *metricgroup__find_metric(const char *metric,
-					   const struct pmu_events_map *map)
+const struct pmu_event *metricgroup__find_metric(const char *metric,
+						 const struct pmu_events_map *map)
 {
-	struct pmu_event *pe;
+	const struct pmu_event *pe;
 	int i;
 
 	map_for_each_event(pe, i, map) {
@@ -968,7 +968,7 @@ static int recursion_check(struct metric *m, const char *id, struct expr_id **pa
 }
 
 static int add_metric(struct list_head *metric_list,
-		      struct pmu_event *pe,
+		      const struct pmu_event *pe,
 		      bool metric_no_group,
 		      struct metric **mp,
 		      struct expr_id *parent,
@@ -993,7 +993,7 @@ static int __resolve_metric(struct metric *m,
 		all = true;
 		hashmap__for_each_entry(m->pctx->ids, cur, bkt) {
 			struct expr_id *parent;
-			struct pmu_event *pe;
+			const struct pmu_event *pe;
 
 			pe = metricgroup__find_metric(cur->key, map);
 			if (!pe)
@@ -1040,7 +1040,7 @@ static int resolve_metric(bool metric_no_group,
 }
 
 static int add_metric(struct list_head *metric_list,
-		      struct pmu_event *pe,
+		      const struct pmu_event *pe,
 		      bool metric_no_group,
 		      struct metric **m,
 		      struct expr_id *parent,
@@ -1070,7 +1070,7 @@ static int add_metric(struct list_head *metric_list,
 	return ret;
 }
 
-static int metricgroup__add_metric_sys_event_iter(struct pmu_event *pe,
+static int metricgroup__add_metric_sys_event_iter(const struct pmu_event *pe,
 						  void *data)
 {
 	struct metricgroup_add_iter_data *d = data;
@@ -1102,7 +1102,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 				   const struct pmu_events_map *map)
 {
 	struct expr_ids ids = { .cnt = 0, };
-	struct pmu_event *pe;
+	const struct pmu_event *pe;
 	struct metric *m;
 	LIST_HEAD(list);
 	int i, ret;
@@ -1286,7 +1286,7 @@ int metricgroup__parse_groups_test(struct evlist *evlist,
 bool metricgroup__has_metric(const char *metric)
 {
 	const struct pmu_events_map *map = pmu_events_map__find();
-	struct pmu_event *pe;
+	const struct pmu_event *pe;
 	int i;
 
 	if (!map)
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index c931596557bf..88ba939a3082 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -43,8 +43,8 @@ int metricgroup__parse_groups(const struct option *opt,
 			      bool metric_no_group,
 			      bool metric_no_merge,
 			      struct rblist *metric_events);
-struct pmu_event *metricgroup__find_metric(const char *metric,
-					   const struct pmu_events_map *map);
+const struct pmu_event *metricgroup__find_metric(const char *metric,
+						 const struct pmu_events_map *map);
 int metricgroup__parse_groups_test(struct evlist *evlist,
 				   const struct pmu_events_map *map,
 				   const char *str,
@@ -55,7 +55,7 @@ int metricgroup__parse_groups_test(struct evlist *evlist,
 void metricgroup__print(bool metrics, bool groups, char *filter,
 			bool raw, bool details);
 bool metricgroup__has_metric(const char *metric);
-int arch_get_runtimeparam(struct pmu_event *pe __maybe_unused);
+int arch_get_runtimeparam(const struct pmu_event *pe __maybe_unused);
 void metricgroup__rblist_exit(struct rblist *metric_events);
 
 int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index c04a89cc7cef..cdd6c3f6caf1 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -315,7 +315,7 @@ static bool perf_pmu_merge_alias(struct perf_pmu_alias *newalias,
 }
 
 static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
-				 char *desc, char *val, struct pmu_event *pe)
+				 char *desc, char *val, const struct pmu_event *pe)
 {
 	struct parse_events_term *term;
 	struct perf_pmu_alias *alias;
@@ -834,7 +834,7 @@ void pmu_add_cpu_aliases_map(struct list_head *head, struct perf_pmu *pmu,
 	i = 0;
 	while (1) {
 		const char *cpu_name = is_arm_pmu_core(name) ? name : "cpu";
-		struct pmu_event *pe = &map->table[i++];
+		const struct pmu_event *pe = &map->table[i++];
 		const char *pname = pe->pmu ? pe->pmu : cpu_name;
 
 		if (!pe->name) {
@@ -882,7 +882,7 @@ void pmu_for_each_sys_event(pmu_sys_event_iter_fn fn, void *data)
 			break;
 
 		while (1) {
-			struct pmu_event *pe = &event_table->table[j++];
+			const struct pmu_event *pe = &event_table->table[j++];
 			int ret;
 
 			if (!pe->name && !pe->metric_group && !pe->metric_name)
@@ -900,7 +900,7 @@ struct pmu_sys_event_iter_data {
 	struct perf_pmu *pmu;
 };
 
-static int pmu_add_sys_aliases_iter_fn(struct pmu_event *pe, void *data)
+static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe, void *data)
 {
 	struct pmu_sys_event_iter_data *idata = data;
 	struct perf_pmu *pmu = idata->pmu;
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index dd5cdde6a3d0..cc9f9e001347 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -127,7 +127,7 @@ const struct pmu_events_map *pmu_events_map__find(void);
 bool pmu_uncore_alias_match(const char *pmu_name, const char *name);
 void perf_pmu_free_alias(struct perf_pmu_alias *alias);
 
-typedef int (*pmu_sys_event_iter_fn)(struct pmu_event *pe, void *data);
+typedef int (*pmu_sys_event_iter_fn)(const struct pmu_event *pe, void *data);
 void pmu_for_each_sys_event(pmu_sys_event_iter_fn fn, void *data);
 int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
 
diff --git a/tools/perf/util/s390-sample-raw.c b/tools/perf/util/s390-sample-raw.c
index 13f33d1ddb78..cd3a34840389 100644
--- a/tools/perf/util/s390-sample-raw.c
+++ b/tools/perf/util/s390-sample-raw.c
@@ -140,7 +140,7 @@ static const char *get_counter_name(int set, int nr, const struct pmu_events_map
 	int rc, event_nr, wanted = get_counterset_start(set) + nr;
 
 	if (map) {
-		struct pmu_event *evp = map->table;
+		const struct pmu_event *evp = map->table;
 
 		for (; evp->name || evp->event || evp->desc; ++evp) {
 			if (evp->name == NULL || evp->event == NULL)
-- 
2.33.0.882.g93a45727a2-goog

Re: [PATCH 04/21] perf pmu: Make pmu_event tables const.

[John Garry]

On 07/10/2021 17:56, Ian Rogers wrote:
>   

- bouncing changbin du address

> -int arch_get_runtimeparam(struct pmu_event *pe)
> +int arch_get_runtimeparam(const struct pmu_event *pe)
>   {
>   	int count;
>   	char path[PATH_MAX] = "/devices/hv_24x7/interface/";
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index a31de0f77097..b3431c11c9cb 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -362,7 +362,7 @@ static int close_table;
>   
>   static void print_events_table_prefix(FILE *fp, const char *tblname)
>   {
> -	fprintf(fp, "struct pmu_event %s[] = {\n", tblname);
> +	fprintf(fp, "const struct pmu_event %s[] = {\n", tblname);

can these be static as well?

>   	close_table = 1;
>   }

[PATCH 05/21] perf metric: Move runtime value to the expr context

[Ian Rogers]

The runtime value is needed when recursively parsing metrics, currently
a value of 1 is passed which is incorrect. Rather than add more
arguments to the bison parser, add runtime to the context. Fix call
sites not to pass a value. The runtime value is defaulted to 0, which is
arbitrary. In some places this replaces a value of 1, which was also
arbitrary. This shouldn't affect anything other than PPC. The use of
0 or 1 shouldn't matter as a proper runtime value would be needed in a
case that it did matter.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/expr.c       | 15 ++++++++-------
 tools/perf/tests/pmu-events.c | 10 +++++-----
 tools/perf/util/expr.c        | 15 ++++++++-------
 tools/perf/util/expr.h        |  5 +++--
 tools/perf/util/metricgroup.c |  7 +++----
 tools/perf/util/stat-shadow.c |  7 ++++---
 6 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index f1d8411fce12..3c16f3df1980 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -56,7 +56,7 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
 {
 	double val;
 
-	if (expr__parse(&val, ctx, e, 1))
+	if (expr__parse(&val, ctx, e))
 		TEST_ASSERT_VAL("parse test failed", 0);
 	TEST_ASSERT_VAL("unexpected value", val == val2);
 	return 0;
@@ -104,17 +104,17 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 	}
 
 	p = "FOO/0";
-	ret = expr__parse(&val, ctx, p, 1);
+	ret = expr__parse(&val, ctx, p);
 	TEST_ASSERT_VAL("division by zero", ret == -1);
 
 	p = "BAR/";
-	ret = expr__parse(&val, ctx, p, 1);
+	ret = expr__parse(&val, ctx, p);
 	TEST_ASSERT_VAL("missing operand", ret == -1);
 
 	expr__ctx_clear(ctx);
 	TEST_ASSERT_VAL("find ids",
 			expr__find_ids("FOO + BAR + BAZ + BOZO", "FOO",
-					ctx, 1) == 0);
+					ctx) == 0);
 	TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 3);
 	TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "BAR",
 						    (void **)&val_ptr));
@@ -124,9 +124,10 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 						    (void **)&val_ptr));
 
 	expr__ctx_clear(ctx);
+	ctx->runtime = 3;
 	TEST_ASSERT_VAL("find ids",
 			expr__find_ids("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@",
-					NULL, ctx, 3) == 0);
+					NULL, ctx) == 0);
 	TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 2);
 	TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "EVENT1,param=3/",
 						    (void **)&val_ptr));
@@ -137,7 +138,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 	expr__ctx_clear(ctx);
 	TEST_ASSERT_VAL("find ids",
 			expr__find_ids("EVENT1 if #smt_on else EVENT2",
-				NULL, ctx, 0) == 0);
+				NULL, ctx) == 0);
 	TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 1);
 	TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids,
 						  smt_on() ? "EVENT1" : "EVENT2",
@@ -147,7 +148,7 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 	expr__ctx_clear(ctx);
 	TEST_ASSERT_VAL("find ids",
 			expr__find_ids("1.0 if EVENT1 > 100.0 else 1.0",
-			NULL, ctx, 0) == 0);
+			NULL, ctx) == 0);
 	TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 0);
 
 	expr__ctx_free(ctx);
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index cc5cea141beb..71b08c296410 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -866,7 +866,7 @@ static int resolve_metric_simple(struct expr_parse_ctx *pctx,
 			ref->metric_expr = pe->metric_expr;
 			list_add_tail(&metric->list, compound_list);
 
-			rc = expr__find_ids(pe->metric_expr, NULL, pctx, 0);
+			rc = expr__find_ids(pe->metric_expr, NULL, pctx);
 			if (rc)
 				goto out_err;
 			break; /* The hashmap has been modified, so restart */
@@ -916,7 +916,7 @@ static int test_parsing(void)
 			if (!pe->metric_expr)
 				continue;
 			expr__ctx_clear(ctx);
-			if (expr__find_ids(pe->metric_expr, NULL, ctx, 0) < 0) {
+			if (expr__find_ids(pe->metric_expr, NULL, ctx) < 0) {
 				expr_failure("Parse find ids failed", map, pe);
 				ret++;
 				continue;
@@ -949,7 +949,7 @@ static int test_parsing(void)
 				free(metric);
 			}
 
-			if (expr__parse(&result, ctx, pe->metric_expr, 0)) {
+			if (expr__parse(&result, ctx, pe->metric_expr)) {
 				expr_failure("Parse failed", map, pe);
 				ret++;
 			}
@@ -989,7 +989,7 @@ static int metric_parse_fake(const char *str)
 		pr_debug("expr__ctx_new failed");
 		return TEST_FAIL;
 	}
-	if (expr__find_ids(str, NULL, ctx, 0) < 0) {
+	if (expr__find_ids(str, NULL, ctx) < 0) {
 		pr_err("expr__find_ids failed\n");
 		return -1;
 	}
@@ -1010,7 +1010,7 @@ static int metric_parse_fake(const char *str)
 		}
 	}
 
-	if (expr__parse(&result, ctx, str, 0))
+	if (expr__parse(&result, ctx, str))
 		pr_err("expr__parse failed\n");
 	else
 		ret = 0;
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index db2445677c8c..62fb39fd4d9d 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -246,7 +246,7 @@ int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id,
 			data->ref.metric_name);
 		pr_debug("processing metric: %s ENTRY\n", id);
 		data->kind = EXPR_ID_DATA__REF_VALUE;
-		if (expr__parse(&data->ref.val, ctx, data->ref.metric_expr, 1)) {
+		if (expr__parse(&data->ref.val, ctx, data->ref.metric_expr)) {
 			pr_debug("%s failed to count\n", id);
 			return -1;
 		}
@@ -284,6 +284,7 @@ struct expr_parse_ctx *expr__ctx_new(void)
 
 	ctx->ids = hashmap__new(key_hash, key_equal, NULL);
 	ctx->parent = NULL;
+	ctx->runtime = 0;
 	return ctx;
 }
 
@@ -314,10 +315,10 @@ void expr__ctx_free(struct expr_parse_ctx *ctx)
 
 static int
 __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
-	      bool compute_ids, int runtime)
+	      bool compute_ids)
 {
 	struct expr_scanner_ctx scanner_ctx = {
-		.runtime = runtime,
+		.runtime = ctx->runtime,
 	};
 	YY_BUFFER_STATE buffer;
 	void *scanner;
@@ -345,15 +346,15 @@ __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
 }
 
 int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
-		const char *expr, int runtime)
+		const char *expr)
 {
-	return __expr__parse(final_val, ctx, expr, /*compute_ids=*/false, runtime) ? -1 : 0;
+	return __expr__parse(final_val, ctx, expr, /*compute_ids=*/false) ? -1 : 0;
 }
 
 int expr__find_ids(const char *expr, const char *one,
-		   struct expr_parse_ctx *ctx, int runtime)
+		   struct expr_parse_ctx *ctx)
 {
-	int ret = __expr__parse(NULL, ctx, expr, /*compute_ids=*/true, runtime);
+	int ret = __expr__parse(NULL, ctx, expr, /*compute_ids=*/true);
 
 	if (one)
 		expr__del_id(ctx, one);
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index b20513f0ae59..124475a4f245 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -21,6 +21,7 @@ struct expr_id {
 struct expr_parse_ctx {
 	struct hashmap	*ids;
 	struct expr_id	*parent;
+	int runtime;
 };
 
 struct expr_id_data;
@@ -52,10 +53,10 @@ int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id,
 		     struct expr_id_data **datap);
 
 int expr__parse(double *final_val, struct expr_parse_ctx *ctx,
-		const char *expr, int runtime);
+		const char *expr);
 
 int expr__find_ids(const char *expr, const char *one,
-		struct expr_parse_ctx *ids, int runtime);
+		   struct expr_parse_ctx *ids);
 
 double expr_id_data__value(const struct expr_id_data *data);
 struct expr_id *expr_id_data__parent(struct expr_id_data *data);
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index b60ccbbf0829..139f4a793f92 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -124,7 +124,6 @@ struct metric {
 	const char *metric_unit;
 	struct list_head metric_refs;
 	int metric_refs_cnt;
-	int runtime;
 	bool has_constraint;
 };
 
@@ -391,7 +390,7 @@ static int metricgroup__setup_events(struct list_head *groups,
 		expr->metric_name = m->metric_name;
 		expr->metric_unit = m->metric_unit;
 		expr->metric_events = metric_events;
-		expr->runtime = m->runtime;
+		expr->runtime = m->pctx->runtime;
 		list_add(&expr->nd, &me->head);
 	}
 
@@ -812,7 +811,7 @@ static int __add_metric(struct list_head *metric_list,
 		m->metric_name = pe->metric_name;
 		m->metric_expr = pe->metric_expr;
 		m->metric_unit = pe->unit;
-		m->runtime = runtime;
+		m->pctx->runtime = runtime;
 		m->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
 		INIT_LIST_HEAD(&m->metric_refs);
 		m->metric_refs_cnt = 0;
@@ -862,7 +861,7 @@ static int __add_metric(struct list_head *metric_list,
 	 * For both the parent and referenced metrics, we parse
 	 * all the metric's IDs and add it to the parent context.
 	 */
-	if (expr__find_ids(pe->metric_expr, NULL, m->pctx, runtime) < 0) {
+	if (expr__find_ids(pe->metric_expr, NULL, m->pctx) < 0) {
 		if (m->metric_refs_cnt == 0) {
 			expr__ctx_free(m->pctx);
 			free(m);
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 9bc841e09a0c..20f1b9d0f272 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -394,7 +394,7 @@ void perf_stat__collect_metric_expr(struct evlist *evsel_list)
 		if (!metric_events) {
 			if (expr__find_ids(counter->metric_expr,
 					   counter->name,
-					   ctx, 1) < 0)
+					   ctx) < 0)
 				continue;
 
 			metric_events = calloc(sizeof(struct evsel *),
@@ -894,13 +894,14 @@ static void generic_metric(struct perf_stat_config *config,
 	if (!pctx)
 		return;
 
+	pctx->runtime = runtime;
 	i = prepare_metric(metric_events, metric_refs, pctx, cpu, st);
 	if (i < 0) {
 		expr__ctx_free(pctx);
 		return;
 	}
 	if (!metric_events[i]) {
-		if (expr__parse(&ratio, pctx, metric_expr, runtime) == 0) {
+		if (expr__parse(&ratio, pctx, metric_expr) == 0) {
 			char *unit;
 			char metric_bf[64];
 
@@ -951,7 +952,7 @@ double test_generic_metric(struct metric_expr *mexp, int cpu, struct runtime_sta
 	if (prepare_metric(mexp->metric_events, mexp->metric_refs, pctx, cpu, st) < 0)
 		goto out;
 
-	if (expr__parse(&ratio, pctx, mexp->metric_expr, 1))
+	if (expr__parse(&ratio, pctx, mexp->metric_expr))
 		ratio = 0.0;
 
 out:
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 06/21] perf metric: Add documentation and rename a variable.

[Ian Rogers]

Documentation to make current functionality clearer. Rename a variable
called 'metric' to 'metric_name' as it can be ambiguous as to whether a
string is the name of a metric or the expression.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 139f4a793f92..3e5f02938452 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -776,13 +776,27 @@ int __weak arch_get_runtimeparam(const struct pmu_event *pe __maybe_unused)
 
 struct metricgroup_add_iter_data {
 	struct list_head *metric_list;
-	const char *metric;
+	const char *metric_name;
 	struct expr_ids *ids;
 	int *ret;
 	bool *has_match;
 	bool metric_no_group;
 };
 
+/**
+ * __add_metric - Add a metric to metric_list.
+ * @metric_list: The list the metric is added to.
+ * @pe: The pmu_event containing the metric to be added.
+ * @metric_no_group: Should events written to events be grouped "{}" or
+ *                   global. Grouping is the default but due to multiplexing the
+ *                   user may override.
+ * @runtime: A special argument for the parser only known at runtime.
+ * @mp: The pointer to a location holding the first metric added to metric
+ *      list. It is initialized here if this is the first metric.
+ * @parent: The last entry in a linked list of metrics being
+ *          added/resolved. This is maintained to detect recursion.
+ * @ids: Storage for parent list.
+ */
 static int __add_metric(struct list_head *metric_list,
 			const struct pmu_event *pe,
 			bool metric_no_group,
@@ -1076,7 +1090,7 @@ static int metricgroup__add_metric_sys_event_iter(const struct pmu_event *pe,
 	struct metric *m = NULL;
 	int ret;
 
-	if (!match_pe_metric(pe, d->metric))
+	if (!match_pe_metric(pe, d->metric_name))
 		return 0;
 
 	ret = add_metric(d->metric_list, pe, d->metric_no_group, &m, NULL, d->ids);
@@ -1095,7 +1109,22 @@ static int metricgroup__add_metric_sys_event_iter(const struct pmu_event *pe,
 	return ret;
 }
 
-static int metricgroup__add_metric(const char *metric, bool metric_no_group,
+/**
+ * metricgroup__add_metric - Find and add a metric, or a metric group.
+ * @metric_name: The name of the metric or metric group. For example, "IPC"
+ *               could be the name of a metric and "TopDownL1" the name of a
+ *               metric group.
+ * @metric_no_group: Should events written to events be grouped "{}" or
+ *                   global. Grouping is the default but due to multiplexing the
+ *                   user may override.
+ * @events: an out argument string of events that need to be parsed and
+ *          associated with the metric. For example, the metric "IPC" would
+ *          create an events string like "{instructions,cycles}:W".
+ * @metric_list: The list that the metric or metric group are added to.
+ * @map: The map that is searched for metrics, most commonly the table for the
+ *       architecture perf is running upon.
+ */
+static int metricgroup__add_metric(const char *metric_name, bool metric_no_group,
 				   struct strbuf *events,
 				   struct list_head *metric_list,
 				   const struct pmu_events_map *map)
@@ -1107,7 +1136,11 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 	int i, ret;
 	bool has_match = false;
 
-	map_for_each_metric(pe, i, map, metric) {
+	/*
+	 * Iterate over all metrics seeing if metric matches either the name or
+	 * group. When it does add the metric to the list.
+	 */
+	map_for_each_metric(pe, i, map, metric_name) {
 		has_match = true;
 		m = NULL;
 
@@ -1130,7 +1163,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 			.fn = metricgroup__add_metric_sys_event_iter,
 			.data = (void *) &(struct metricgroup_add_iter_data) {
 				.metric_list = &list,
-				.metric = metric,
+				.metric_name = metric_name,
 				.metric_no_group = metric_no_group,
 				.ids = &ids,
 				.has_match = &has_match,
@@ -1169,6 +1202,22 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
 	return ret;
 }
 
+/**
+ * metricgroup__add_metric_list - Find and add metrics, or metric groups,
+ *                                specified in a list.
+ * @list: the list of metrics or metric groups. For example, "IPC,CPI,TopDownL1"
+ *        would match the IPC and CPI metrics, and TopDownL1 would match all
+ *        the metrics in the TopDownL1 group.
+ * @metric_no_group: Should events written to events be grouped "{}" or
+ *                   global. Grouping is the default but due to multiplexing the
+ *                   user may override.
+ * @events: an out argument string of events that need to be parsed and
+ *          associated with the metric. For example, the metric "IPC" would
+ *          create an events string like "{instructions,cycles}:W".
+ * @metric_list: The list that metrics are added to.
+ * @map: The map that is searched for metrics, most commonly the table for the
+ *       architecture perf is running upon.
+ */
 static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 					struct strbuf *events,
 					struct list_head *metric_list,
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 07/21] perf metric: Add metric new and free

[Ian Rogers]

Metrics are complex enough that a new/free reduces the risk of memory
leaks. Move static functions used in new.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 3e5f02938452..e4ce19389258 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -127,6 +127,78 @@ struct metric {
 	bool has_constraint;
 };
 
+static void metricgroup___watchdog_constraint_hint(const char *name, bool foot)
+{
+	static bool violate_nmi_constraint;
+
+	if (!foot) {
+		pr_warning("Splitting metric group %s into standalone metrics.\n", name);
+		violate_nmi_constraint = true;
+		return;
+	}
+
+	if (!violate_nmi_constraint)
+		return;
+
+	pr_warning("Try disabling the NMI watchdog to comply NO_NMI_WATCHDOG metric constraint:\n"
+		   "    echo 0 > /proc/sys/kernel/nmi_watchdog\n"
+		   "    perf stat ...\n"
+		   "    echo 1 > /proc/sys/kernel/nmi_watchdog\n");
+}
+
+static bool metricgroup__has_constraint(const struct pmu_event *pe)
+{
+	if (!pe->metric_constraint)
+		return false;
+
+	if (!strcmp(pe->metric_constraint, "NO_NMI_WATCHDOG") &&
+	    sysctl__nmi_watchdog_enabled()) {
+		metricgroup___watchdog_constraint_hint(pe->metric_name, false);
+		return true;
+	}
+
+	return false;
+}
+
+static struct metric *metric__new(const struct pmu_event *pe,
+				  bool metric_no_group,
+				  int runtime)
+{
+	struct metric *m;
+
+	m = zalloc(sizeof(*m));
+	if (!m)
+		return NULL;
+
+	m->pctx = expr__ctx_new();
+	if (!m->pctx) {
+		free(m);
+		return NULL;
+	}
+
+	m->metric_name = pe->metric_name;
+	m->metric_expr = pe->metric_expr;
+	m->metric_unit = pe->unit;
+	m->pctx->runtime = runtime;
+	m->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
+	INIT_LIST_HEAD(&m->metric_refs);
+	m->metric_refs_cnt = 0;
+
+	return m;
+}
+
+static void metric__free(struct metric *m)
+{
+	struct metric_ref_node *ref, *tmp;
+
+	list_for_each_entry_safe(ref, tmp, &m->metric_refs, list) {
+		list_del(&ref->list);
+		free(ref);
+	}
+	expr__ctx_free(m->pctx);
+	free(m);
+}
+
 #define RECURSION_ID_MAX 1000
 
 struct expr_ids {
@@ -736,39 +808,6 @@ static void metricgroup__add_metric_non_group(struct strbuf *events,
 	}
 }
 
-static void metricgroup___watchdog_constraint_hint(const char *name, bool foot)
-{
-	static bool violate_nmi_constraint;
-
-	if (!foot) {
-		pr_warning("Splitting metric group %s into standalone metrics.\n", name);
-		violate_nmi_constraint = true;
-		return;
-	}
-
-	if (!violate_nmi_constraint)
-		return;
-
-	pr_warning("Try disabling the NMI watchdog to comply NO_NMI_WATCHDOG metric constraint:\n"
-		   "    echo 0 > /proc/sys/kernel/nmi_watchdog\n"
-		   "    perf stat ...\n"
-		   "    echo 1 > /proc/sys/kernel/nmi_watchdog\n");
-}
-
-static bool metricgroup__has_constraint(const struct pmu_event *pe)
-{
-	if (!pe->metric_constraint)
-		return false;
-
-	if (!strcmp(pe->metric_constraint, "NO_NMI_WATCHDOG") &&
-	    sysctl__nmi_watchdog_enabled()) {
-		metricgroup___watchdog_constraint_hint(pe->metric_name, false);
-		return true;
-	}
-
-	return false;
-}
-
 int __weak arch_get_runtimeparam(const struct pmu_event *pe __maybe_unused)
 {
 	return 1;
@@ -813,23 +852,10 @@ static int __add_metric(struct list_head *metric_list,
 		 * We got in here for the parent group,
 		 * allocate it and put it on the list.
 		 */
-		m = zalloc(sizeof(*m));
+		m = metric__new(pe, metric_no_group, runtime);
 		if (!m)
 			return -ENOMEM;
 
-		m->pctx = expr__ctx_new();
-		if (!m->pctx) {
-			free(m);
-			return -ENOMEM;
-		}
-		m->metric_name = pe->metric_name;
-		m->metric_expr = pe->metric_expr;
-		m->metric_unit = pe->unit;
-		m->pctx->runtime = runtime;
-		m->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
-		INIT_LIST_HEAD(&m->metric_refs);
-		m->metric_refs_cnt = 0;
-
 		parent = expr_ids__alloc(ids);
 		if (!parent) {
 			free(m);
@@ -877,8 +903,7 @@ static int __add_metric(struct list_head *metric_list,
 	 */
 	if (expr__find_ids(pe->metric_expr, NULL, m->pctx) < 0) {
 		if (m->metric_refs_cnt == 0) {
-			expr__ctx_free(m->pctx);
-			free(m);
+			metric__free(m);
 			*mp = NULL;
 		}
 		return -EINVAL;
@@ -1251,25 +1276,13 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 	return ret;
 }
 
-static void metric__free_refs(struct metric *metric)
-{
-	struct metric_ref_node *ref, *tmp;
-
-	list_for_each_entry_safe(ref, tmp, &metric->metric_refs, list) {
-		list_del(&ref->list);
-		free(ref);
-	}
-}
-
 static void metricgroup__free_metrics(struct list_head *metric_list)
 {
 	struct metric *m, *tmp;
 
 	list_for_each_entry_safe (m, tmp, metric_list, nd) {
-		metric__free_refs(m);
-		expr__ctx_free(m->pctx);
 		list_del_init(&m->nd);
-		free(m);
+		metric__free(m);
 	}
 }
 
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 08/21] perf metric: Only add a referenced metric once

[Ian Rogers]

If a metric references other metrics then the same other metrics may be
referenced more than once, but the events and metric ref are only needed
once. An example of this is in tests/parse-metric.c where DCache_L2_Hits
references the metric DCache_L2_All_Hits twice, once directly and once
through DCache_L2_All.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index e4ce19389258..6c4c51e35aa7 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -870,12 +870,18 @@ static int __add_metric(struct list_head *metric_list,
 		*mp = m;
 	} else {
 		/*
-		 * We got here for the referenced metric, via the
-		 * recursive metricgroup__add_metric call, add
-		 * it to the parent group.
+		 * This metric was referenced in a metric higher in the
+		 * tree. Check if the same metric is already resolved in the
+		 * metric_refs list.
 		 */
 		m = *mp;
 
+		list_for_each_entry(ref, &m->metric_refs, list) {
+			if (!strcmp(pe->metric_name, ref->metric_name))
+				return 0;
+		}
+
+		/*Add the new referenced metric to the pare the parent group. */
 		ref = malloc(sizeof(*ref));
 		if (!ref)
 			return -ENOMEM;
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 09/21] perf metric: Modify resolution and recursion check.

[Ian Rogers]

Modify resolution. Rather than resolving a list of metrics, resolve a
metric immediately after it is added. This simplifies knowing the root
of the metric's tree so that IDs may be associated with it. A bug in the
current implementation is that all the IDs were placed on the first
metric in a metric group.

Rather than maintain data on IDs' parents to detect cycles, maintain
a list of visited metrics and detect cycles if the same metric is
visited twice.

Only place the root metric onto the list of metrics.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/expr.c       |  10 +-
 tools/perf/util/expr.c        |  26 +--
 tools/perf/util/expr.h        |   9 +-
 tools/perf/util/expr.y        |   2 +-
 tools/perf/util/metricgroup.c | 402 ++++++++++++++--------------------
 5 files changed, 179 insertions(+), 270 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 3c16f3df1980..718c13e5a0f4 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -24,8 +24,8 @@ static int test_ids_union(void)
 	ids2 = ids__new();
 	TEST_ASSERT_VAL("ids__new", ids2);
 
-	TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids1, strdup("foo"), NULL), 0);
-	TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids1, strdup("bar"), NULL), 0);
+	TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids1, strdup("foo")), 0);
+	TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids1, strdup("bar")), 0);
 
 	ids1 = ids__union(ids1, ids2);
 	TEST_ASSERT_EQUAL("union", (int)hashmap__size(ids1), 2);
@@ -33,7 +33,7 @@ static int test_ids_union(void)
 	/* Union {foo, bar} against {foo}. */
 	ids2 = ids__new();
 	TEST_ASSERT_VAL("ids__new", ids2);
-	TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids2, strdup("foo"), NULL), 0);
+	TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids2, strdup("foo")), 0);
 
 	ids1 = ids__union(ids1, ids2);
 	TEST_ASSERT_EQUAL("union", (int)hashmap__size(ids1), 2);
@@ -41,8 +41,8 @@ static int test_ids_union(void)
 	/* Union {foo, bar} against {bar,baz}. */
 	ids2 = ids__new();
 	TEST_ASSERT_VAL("ids__new", ids2);
-	TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids2, strdup("bar"), NULL), 0);
-	TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids2, strdup("baz"), NULL), 0);
+	TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids2, strdup("bar")), 0);
+	TEST_ASSERT_EQUAL("ids__insert", ids__insert(ids2, strdup("baz")), 0);
 
 	ids1 = ids__union(ids1, ids2);
 	TEST_ASSERT_EQUAL("union", (int)hashmap__size(ids1), 3);
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 62fb39fd4d9d..5657222aaa25 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -25,7 +25,6 @@ struct expr_id_data {
 			const char *metric_name;
 			const char *metric_expr;
 		} ref;
-		struct expr_id	*parent;
 	};
 
 	enum {
@@ -35,8 +34,6 @@ struct expr_id_data {
 		EXPR_ID_DATA__REF,
 		/* A reference but the value has been computed. */
 		EXPR_ID_DATA__REF_VALUE,
-		/* A parent is remembered for the recursion check. */
-		EXPR_ID_DATA__PARENT,
 	} kind;
 };
 
@@ -80,20 +77,12 @@ void ids__free(struct hashmap *ids)
 	hashmap__free(ids);
 }
 
-int ids__insert(struct hashmap *ids, const char *id,
-		struct expr_id *parent)
+int ids__insert(struct hashmap *ids, const char *id)
 {
 	struct expr_id_data *data_ptr = NULL, *old_data = NULL;
 	char *old_key = NULL;
 	int ret;
 
-	data_ptr = malloc(sizeof(*data_ptr));
-	if (!data_ptr)
-		return -ENOMEM;
-
-	data_ptr->parent = parent;
-	data_ptr->kind = EXPR_ID_DATA__PARENT;
-
 	ret = hashmap__set(ids, id, data_ptr,
 			   (const void **)&old_key, (void **)&old_data);
 	if (ret)
@@ -142,7 +131,7 @@ struct hashmap *ids__union(struct hashmap *ids1, struct hashmap *ids2)
 /* Caller must make sure id is allocated */
 int expr__add_id(struct expr_parse_ctx *ctx, const char *id)
 {
-	return ids__insert(ctx->ids, id, ctx->parent);
+	return ids__insert(ctx->ids, id);
 }
 
 /* Caller must make sure id is allocated */
@@ -238,9 +227,6 @@ int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id,
 	case EXPR_ID_DATA__VALUE:
 		pr_debug2("lookup(%s): val %f\n", id, data->val);
 		break;
-	case EXPR_ID_DATA__PARENT:
-		pr_debug2("lookup(%s): parent %s\n", id, data->parent->id);
-		break;
 	case EXPR_ID_DATA__REF:
 		pr_debug2("lookup(%s): ref metric name %s\n", id,
 			data->ref.metric_name);
@@ -283,8 +269,8 @@ struct expr_parse_ctx *expr__ctx_new(void)
 		return NULL;
 
 	ctx->ids = hashmap__new(key_hash, key_equal, NULL);
-	ctx->parent = NULL;
 	ctx->runtime = 0;
+
 	return ctx;
 }
 
@@ -369,9 +355,3 @@ double expr_id_data__value(const struct expr_id_data *data)
 	assert(data->kind == EXPR_ID_DATA__REF_VALUE);
 	return data->ref.val;
 }
-
-struct expr_id *expr_id_data__parent(struct expr_id_data *data)
-{
-	assert(data->kind == EXPR_ID_DATA__PARENT);
-	return data->parent;
-}
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index 124475a4f245..c6e534f633c3 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -13,14 +13,8 @@
 
 struct metric_ref;
 
-struct expr_id {
-	char		*id;
-	struct expr_id	*parent;
-};
-
 struct expr_parse_ctx {
 	struct hashmap	*ids;
-	struct expr_id	*parent;
 	int runtime;
 };
 
@@ -32,7 +26,7 @@ struct expr_scanner_ctx {
 
 struct hashmap *ids__new(void);
 void ids__free(struct hashmap *ids);
-int ids__insert(struct hashmap *ids, const char *id, struct expr_id *parent);
+int ids__insert(struct hashmap *ids, const char *id);
 /*
  * Union two sets of ids (hashmaps) and construct a third, freeing ids1 and
  * ids2.
@@ -59,6 +53,5 @@ int expr__find_ids(const char *expr, const char *one,
 		   struct expr_parse_ctx *ids);
 
 double expr_id_data__value(const struct expr_id_data *data);
-struct expr_id *expr_id_data__parent(struct expr_id_data *data);
 
 #endif
diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
index ba7d3b667fcb..f969dfa525bd 100644
--- a/tools/perf/util/expr.y
+++ b/tools/perf/util/expr.y
@@ -190,7 +190,7 @@ expr: NUMBER
 		 */
 		$$.val = BOTTOM;
 		$$.ids = ids__new();
-		if (!$$.ids || ids__insert($$.ids, $1, ctx->parent))
+		if (!$$.ids || ids__insert($$.ids, $1))
 			YYABORT;
 	}
 }
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 6c4c51e35aa7..c96f9fe163f9 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -18,6 +18,7 @@
 #include "strlist.h"
 #include <assert.h>
 #include <linux/ctype.h>
+#include <linux/list_sort.h>
 #include <linux/string.h>
 #include <linux/zalloc.h>
 #include <subcmd/parse-options.h>
@@ -199,28 +200,6 @@ static void metric__free(struct metric *m)
 	free(m);
 }
 
-#define RECURSION_ID_MAX 1000
-
-struct expr_ids {
-	struct expr_id	id[RECURSION_ID_MAX];
-	int		cnt;
-};
-
-static struct expr_id *expr_ids__alloc(struct expr_ids *ids)
-{
-	if (ids->cnt >= RECURSION_ID_MAX)
-		return NULL;
-	return &ids->id[ids->cnt++];
-}
-
-static void expr_ids__exit(struct expr_ids *ids)
-{
-	int i;
-
-	for (i = 0; i < ids->cnt; i++)
-		free(ids->id[i].id);
-}
-
 static bool contains_event(struct evsel **metric_events, int num_events,
 			const char *event_name)
 {
@@ -813,15 +792,106 @@ int __weak arch_get_runtimeparam(const struct pmu_event *pe __maybe_unused)
 	return 1;
 }
 
+/*
+ * A singly linked list on the stack of the names of metrics being
+ * processed. Used to identify recursion.
+ */
+struct visited_metric {
+	const char *name;
+	const struct visited_metric *parent;
+};
+
 struct metricgroup_add_iter_data {
 	struct list_head *metric_list;
 	const char *metric_name;
-	struct expr_ids *ids;
 	int *ret;
 	bool *has_match;
 	bool metric_no_group;
+	struct metric *root_metric;
+	const struct visited_metric *visited;
+	const struct pmu_events_map *map;
 };
 
+static int add_metric(struct list_head *metric_list,
+		      const struct pmu_event *pe,
+		      bool metric_no_group,
+		      struct metric *root_metric,
+		      const struct visited_metric *visited,
+		      const struct pmu_events_map *map);
+
+/**
+ * resolve_metric - Locate metrics within the root metric and recursively add
+ *                    references to them.
+ * @metric_list: The list the metric is added to.
+ * @metric_no_group: Should events written to events be grouped "{}" or
+ *                   global. Grouping is the default but due to multiplexing the
+ *                   user may override.
+ * @root_metric: Metrics may reference other metrics to form a tree. In this
+ *               case the root_metric holds all the IDs and a list of referenced
+ *               metrics. When adding a root this argument is NULL.
+ * @visited: A singly linked list of metric names being added that is used to
+ *           detect recursion.
+ * @map: The map that is searched for metrics, most commonly the table for the
+ *       architecture perf is running upon.
+ */
+static int resolve_metric(struct list_head *metric_list,
+			  bool metric_no_group,
+			  struct metric *root_metric,
+			  const struct visited_metric *visited,
+			  const struct pmu_events_map *map)
+{
+	struct hashmap_entry *cur;
+	size_t bkt;
+	struct to_resolve {
+		/* The metric to resolve. */
+		const struct pmu_event *pe;
+		/*
+		 * The key in the IDs map, this may differ from in case,
+		 * etc. from pe->metric_name.
+		 */
+		const char *key;
+	} *pending = NULL;
+	int i, ret = 0, pending_cnt = 0;
+
+	/*
+	 * Iterate all the parsed IDs and if there's a matching metric and it to
+	 * the pending array.
+	 */
+	hashmap__for_each_entry(root_metric->pctx->ids, cur, bkt) {
+		const struct pmu_event *pe;
+
+		pe = metricgroup__find_metric(cur->key, map);
+		if (pe) {
+			pending = realloc(pending,
+					(pending_cnt + 1) * sizeof(struct to_resolve));
+			if (!pending)
+				return -ENOMEM;
+
+			pending[pending_cnt].pe = pe;
+			pending[pending_cnt].key = cur->key;
+			pending_cnt++;
+		}
+	}
+
+	/* Remove the metric IDs from the context. */
+	for (i = 0; i < pending_cnt; i++)
+		expr__del_id(root_metric->pctx, pending[i].key);
+
+	/*
+	 * Recursively add all the metrics, IDs are added to the root metric's
+	 * context.
+	 */
+	for (i = 0; i < pending_cnt; i++) {
+		ret = add_metric(metric_list, pending[i].pe, metric_no_group,
+				root_metric, visited, map);
+		if (ret)
+			break;
+	}
+
+	free(pending);
+	return ret;
+}
+
 /**
  * __add_metric - Add a metric to metric_list.
  * @metric_list: The list the metric is added to.
@@ -830,58 +900,59 @@ struct metricgroup_add_iter_data {
  *                   global. Grouping is the default but due to multiplexing the
  *                   user may override.
  * @runtime: A special argument for the parser only known at runtime.
- * @mp: The pointer to a location holding the first metric added to metric
- *      list. It is initialized here if this is the first metric.
- * @parent: The last entry in a linked list of metrics being
- *          added/resolved. This is maintained to detect recursion.
- * @ids: Storage for parent list.
+ * @root_metric: Metrics may reference other metrics to form a tree. In this
+ *               case the root_metric holds all the IDs and a list of referenced
+ *               metrics. When adding a root this argument is NULL.
+ * @visited: A singly linked list of metric names being added that is used to
+ *           detect recursion.
+ * @map: The map that is searched for metrics, most commonly the table for the
+ *       architecture perf is running upon.
  */
 static int __add_metric(struct list_head *metric_list,
 			const struct pmu_event *pe,
 			bool metric_no_group,
 			int runtime,
-			struct metric **mp,
-			struct expr_id *parent,
-			struct expr_ids *ids)
+			struct metric *root_metric,
+			const struct visited_metric *visited,
+			const struct pmu_events_map *map)
 {
 	struct metric_ref_node *ref;
-	struct metric *m;
+	const struct visited_metric *vm;
+	int ret;
+	bool is_root = !root_metric;
+	struct visited_metric visited_node = {
+		.name = pe->metric_name,
+		.parent = visited,
+	};
 
-	if (*mp == NULL) {
+	for (vm = visited; vm; vm = vm->parent) {
+		if (!strcmp(pe->metric_name, vm->name)) {
+			pr_err("failed: recursion detected for %s\n", pe->metric_name);
+			return -1;
+		}
+	}
+
+	if (is_root) {
 		/*
-		 * We got in here for the parent group,
-		 * allocate it and put it on the list.
+		 * This metric is the root of a tree and may reference other
+		 * metrics that are added recursively.
 		 */
-		m = metric__new(pe, metric_no_group, runtime);
-		if (!m)
+		root_metric = metric__new(pe, metric_no_group, runtime);
+		if (!root_metric)
 			return -ENOMEM;
 
-		parent = expr_ids__alloc(ids);
-		if (!parent) {
-			free(m);
-			return -EINVAL;
-		}
-
-		parent->id = strdup(pe->metric_name);
-		if (!parent->id) {
-			free(m);
-			return -ENOMEM;
-		}
-		*mp = m;
 	} else {
 		/*
 		 * This metric was referenced in a metric higher in the
 		 * tree. Check if the same metric is already resolved in the
 		 * metric_refs list.
 		 */
-		m = *mp;
-
-		list_for_each_entry(ref, &m->metric_refs, list) {
+		list_for_each_entry(ref, &root_metric->metric_refs, list) {
 			if (!strcmp(pe->metric_name, ref->metric_name))
 				return 0;
 		}
 
-		/*Add the new referenced metric to the pare the parent group. */
+		/* Create reference */
 		ref = malloc(sizeof(*ref));
 		if (!ref)
 			return -ENOMEM;
@@ -895,50 +966,31 @@ static int __add_metric(struct list_head *metric_list,
 		ref->metric_name = pe->metric_name;
 		ref->metric_expr = pe->metric_expr;
 
-		list_add(&ref->list, &m->metric_refs);
-		m->metric_refs_cnt++;
+		list_add(&ref->list, &root_metric->metric_refs);
+		root_metric->metric_refs_cnt++;
 	}
 
-	/* Force all found IDs in metric to have us as parent ID. */
-	WARN_ON_ONCE(!parent);
-	m->pctx->parent = parent;
-
 	/*
 	 * For both the parent and referenced metrics, we parse
-	 * all the metric's IDs and add it to the parent context.
+	 * all the metric's IDs and add it to the root context.
 	 */
-	if (expr__find_ids(pe->metric_expr, NULL, m->pctx) < 0) {
-		if (m->metric_refs_cnt == 0) {
-			metric__free(m);
-			*mp = NULL;
-		}
-		return -EINVAL;
+	if (expr__find_ids(pe->metric_expr, NULL, root_metric->pctx) < 0) {
+		/* Broken metric. */
+		ret = -EINVAL;
+	} else {
+		/* Resolve referenced metrics. */
+		ret = resolve_metric(metric_list, metric_no_group, root_metric,
+				     &visited_node, map);
 	}
 
-	/*
-	 * We add new group only in the 'parent' call,
-	 * so bail out for referenced metric case.
-	 */
-	if (m->metric_refs_cnt)
-		return 0;
-
-	if (list_empty(metric_list))
-		list_add(&m->nd, metric_list);
-	else {
-		struct list_head *pos;
-
-		/* Place the largest groups at the front. */
-		list_for_each_prev(pos, metric_list) {
-			struct metric *old = list_entry(pos, struct metric, nd);
+	if (ret) {
+		if (is_root)
+			metric__free(root_metric);
 
-			if (hashmap__size(m->pctx->ids) <=
-			    hashmap__size(old->pctx->ids))
-				break;
-		}
-		list_add(&m->nd, pos);
-	}
+	} else if (is_root)
+		list_add(&root_metric->nd, metric_list);
 
-	return 0;
+	return ret;
 }
 
 #define map_for_each_event(__pe, __idx, __map)					\
@@ -967,136 +1019,20 @@ const struct pmu_event *metricgroup__find_metric(const char *metric,
 	return NULL;
 }
 
-static int recursion_check(struct metric *m, const char *id, struct expr_id **parent,
-			   struct expr_ids *ids)
-{
-	struct expr_id_data *data;
-	struct expr_id *p;
-	int ret;
-
-	/*
-	 * We get the parent referenced by 'id' argument and
-	 * traverse through all the parent object IDs to check
-	 * if we already processed 'id', if we did, it's recursion
-	 * and we fail.
-	 */
-	ret = expr__get_id(m->pctx, id, &data);
-	if (ret)
-		return ret;
-
-	p = expr_id_data__parent(data);
-
-	while (p->parent) {
-		if (!strcmp(p->id, id)) {
-			pr_err("failed: recursion detected for %s\n", id);
-			return -1;
-		}
-		p = p->parent;
-	}
-
-	/*
-	 * If we are over the limit of static entris, the metric
-	 * is too difficult/nested to process, fail as well.
-	 */
-	p = expr_ids__alloc(ids);
-	if (!p) {
-		pr_err("failed: too many nested metrics\n");
-		return -EINVAL;
-	}
-
-	p->id     = strdup(id);
-	p->parent = expr_id_data__parent(data);
-	*parent   = p;
-
-	return p->id ? 0 : -ENOMEM;
-}
-
 static int add_metric(struct list_head *metric_list,
 		      const struct pmu_event *pe,
 		      bool metric_no_group,
-		      struct metric **mp,
-		      struct expr_id *parent,
-		      struct expr_ids *ids);
-
-static int __resolve_metric(struct metric *m,
-			    bool metric_no_group,
-			    struct list_head *metric_list,
-			    const struct pmu_events_map *map,
-			    struct expr_ids *ids)
+		      struct metric *root_metric,
+		      const struct visited_metric *visited,
+		      const struct pmu_events_map *map)
 {
-	struct hashmap_entry *cur;
-	size_t bkt;
-	bool all;
-	int ret;
-
-	/*
-	 * Iterate all the parsed IDs and if there's metric,
-	 * add it to the context.
-	 */
-	do {
-		all = true;
-		hashmap__for_each_entry(m->pctx->ids, cur, bkt) {
-			struct expr_id *parent;
-			const struct pmu_event *pe;
-
-			pe = metricgroup__find_metric(cur->key, map);
-			if (!pe)
-				continue;
-
-			ret = recursion_check(m, cur->key, &parent, ids);
-			if (ret)
-				return ret;
-
-			all = false;
-			/* The metric key itself needs to go out.. */
-			expr__del_id(m->pctx, cur->key);
-
-			/* ... and it gets resolved to the parent context. */
-			ret = add_metric(metric_list, pe, metric_no_group, &m, parent, ids);
-			if (ret)
-				return ret;
-
-			/*
-			 * We added new metric to hashmap, so we need
-			 * to break the iteration and start over.
-			 */
-			break;
-		}
-	} while (!all);
-
-	return 0;
-}
-
-static int resolve_metric(bool metric_no_group,
-			  struct list_head *metric_list,
-			  const struct pmu_events_map *map,
-			  struct expr_ids *ids)
-{
-	struct metric *m;
-	int err;
-
-	list_for_each_entry(m, metric_list, nd) {
-		err = __resolve_metric(m, metric_no_group, metric_list, map, ids);
-		if (err)
-			return err;
-	}
-	return 0;
-}
-
-static int add_metric(struct list_head *metric_list,
-		      const struct pmu_event *pe,
-		      bool metric_no_group,
-		      struct metric **m,
-		      struct expr_id *parent,
-		      struct expr_ids *ids)
-{
-	struct metric *orig = *m;
 	int ret = 0;
 
 	pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
 
 	if (!strstr(pe->metric_expr, "?")) {
-		ret = __add_metric(metric_list, pe, metric_no_group, 1, m, parent, ids);
+		ret = __add_metric(metric_list, pe, metric_no_group, 0,
+				   root_metric, visited, map);
 	} else {
 		int j, count;
 
@@ -1107,8 +1043,9 @@ static int add_metric(struct list_head *metric_list,
 		 * those events to metric_list.
 		 */
 
-		for (j = 0; j < count && !ret; j++, *m = orig)
-			ret = __add_metric(metric_list, pe, metric_no_group, j, m, parent, ids);
+		for (j = 0; j < count && !ret; j++)
+			ret = __add_metric(metric_list, pe, metric_no_group, j,
+					root_metric, visited, map);
 	}
 
 	return ret;
@@ -1118,18 +1055,13 @@ static int metricgroup__add_metric_sys_event_iter(const struct pmu_event *pe,
 						  void *data)
 {
 	struct metricgroup_add_iter_data *d = data;
-	struct metric *m = NULL;
 	int ret;
 
 	if (!match_pe_metric(pe, d->metric_name))
 		return 0;
 
-	ret = add_metric(d->metric_list, pe, d->metric_no_group, &m, NULL, d->ids);
-	if (ret)
-		goto out;
-
-	ret = resolve_metric(d->metric_no_group,
-				     d->metric_list, NULL, d->ids);
+	ret = add_metric(d->metric_list, pe, d->metric_no_group,
+			 d->root_metric, d->visited, d->map);
 	if (ret)
 		goto out;
 
@@ -1140,6 +1072,15 @@ static int metricgroup__add_metric_sys_event_iter(const struct pmu_event *pe,
 	return ret;
 }
 
+static int metric_list_cmp(void *priv __maybe_unused, const struct list_head *l,
+			   const struct list_head *r)
+{
+	const struct metric *left = container_of(l, struct metric, nd);
+	const struct metric *right = container_of(r, struct metric, nd);
+
+	return hashmap__size(right->pctx->ids) - hashmap__size(left->pctx->ids);
+}
+
 /**
  * metricgroup__add_metric - Find and add a metric, or a metric group.
  * @metric_name: The name of the metric or metric group. For example, "IPC"
@@ -1160,7 +1101,6 @@ static int metricgroup__add_metric(const char *metric_name, bool metric_no_group
 				   struct list_head *metric_list,
 				   const struct pmu_events_map *map)
 {
-	struct expr_ids ids = { .cnt = 0, };
 	const struct pmu_event *pe;
 	struct metric *m;
 	LIST_HEAD(list);
@@ -1173,18 +1113,9 @@ static int metricgroup__add_metric(const char *metric_name, bool metric_no_group
 	 */
 	map_for_each_metric(pe, i, map, metric_name) {
 		has_match = true;
-		m = NULL;
-
-		ret = add_metric(&list, pe, metric_no_group, &m, NULL, &ids);
-		if (ret)
-			goto out;
-
-		/*
-		 * Process any possible referenced metrics
-		 * included in the expression.
-		 */
-		ret = resolve_metric(metric_no_group,
-				     &list, map, &ids);
+		ret = add_metric(&list, pe, metric_no_group,
+				 /*root_metric=*/NULL,
+				 /*visited_metrics=*/NULL, map);
 		if (ret)
 			goto out;
 	}
@@ -1196,9 +1127,9 @@ static int metricgroup__add_metric(const char *metric_name, bool metric_no_group
 				.metric_list = &list,
 				.metric_name = metric_name,
 				.metric_no_group = metric_no_group,
-				.ids = &ids,
 				.has_match = &has_match,
 				.ret = &ret,
+				.map = map,
 			},
 		};
 
@@ -1210,6 +1141,9 @@ static int metricgroup__add_metric(const char *metric_name, bool metric_no_group
 		goto out;
 	}
 
+	/* Sort metrics from largest to smallest. */
+	list_sort(NULL,  &list, metric_list_cmp);
+
 	list_for_each_entry(m, &list, nd) {
 		if (events->len > 0)
 			strbuf_addf(events, ",");
@@ -1229,7 +1163,9 @@ static int metricgroup__add_metric(const char *metric_name, bool metric_no_group
 	 * even if it's failed
 	 */
 	list_splice(&list, metric_list);
-	expr_ids__exit(&ids);
+
+	/* Sort metrics from largest to smallest. */
+	list_sort(NULL, metric_list, metric_list_cmp);
 	return ret;
 }
 
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 10/21] perf metric: Comment data structures.

[Ian Rogers]

Document the data structures maintained by metricgroup.c and used by
stat-shadow.c for metric output.

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

diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index 88ba939a3082..3a51a84f440b 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -14,24 +14,51 @@ struct rblist;
 struct pmu_events_map;
 struct cgroup;
 
+/**
+ * A node in a rblist keyed by the evsel. The global rblist of metric events
+ * generally exists in perf_stat_config. The evsel is looked up in the rblist
+ * yielding a list of metric_expr.
+ */
 struct metric_event {
 	struct rb_node nd;
 	struct evsel *evsel;
 	struct list_head head; /* list of metric_expr */
 };
 
+/**
+ * A metric referenced by a metric_expr. When parsing a metric expression IDs
+ * will be looked up, matching either a value (from metric_events) or a
+ * metric_ref. A metric_ref will then be parsed recursively. The metric_refs and
+ * metric_events need to be known before parsing so that their values may be
+ * placed in the parse context for lookup.
+ */
 struct metric_ref {
 	const char *metric_name;
 	const char *metric_expr;
 };
 
+/**
+ * One in a list of metric_expr associated with an evsel. The data is used to
+ * generate a metric value during stat output.
+ */
 struct metric_expr {
 	struct list_head nd;
+	/** The expression to parse, for example, "instructions/cycles". */
 	const char *metric_expr;
+	/** The name of the meric such as "IPC". */
 	const char *metric_name;
+	/**
+	 * The "ScaleUnit" that scales and adds a unit to the metric during
+	 * output. For example, "6.4e-05MiB" means to scale the resulting metric
+	 * by 6.4e-05 (typically converting a unit like cache lines to something
+	 * more human intelligible) and then add "MiB" afterward when displayed.
+	 */
 	const char *metric_unit;
+	/** Null terminated array of events used by the metric. */
 	struct evsel **metric_events;
+	/** Null terminated array of referenced metrics. */
 	struct metric_ref *metric_refs;
+	/** A value substituted for '?' during parsing. */
 	int runtime;
 };
 
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 11/21] perf metric: Document the internal 'struct metric'

[Ian Rogers]

Add documentation as part of code tidying.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index c96f9fe163f9..632867cedbae 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -117,14 +117,34 @@ struct metric_ref_node {
 	struct list_head list;
 };
 
+/**
+ * The metric under construction. The data held here will be placed in a
+ * metric_expr.
+ */
 struct metric {
 	struct list_head nd;
+	/**
+	 * The expression parse context importantly holding the IDs contained
+	 * within the expression.
+	 */
 	struct expr_parse_ctx *pctx;
+	/** The name of the metric such as "IPC". */
 	const char *metric_name;
+	/** The expression to parse, for example, "instructions/cycles". */
 	const char *metric_expr;
+	/**
+	 * The "ScaleUnit" that scales and adds a unit to the metric during
+	 * output.
+	 */
 	const char *metric_unit;
+	/** The list of metrics referenced by this one. */
 	struct list_head metric_refs;
+	/** The size of the metric_refs list. */
 	int metric_refs_cnt;
+	/**
+	 * Is there a constraint on the group of events? In which case the
+	 * events won't be grouped.
+	 */
 	bool has_constraint;
 };
 
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 12/21] perf metric: Simplify metric_refs calculation.

[Ian Rogers]

Don't build a list and then turn to an array, just directly build the
array. The size of the array is known due to the search for a duplicate.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 632867cedbae..b48836d7c080 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -137,10 +137,8 @@ struct metric {
 	 * output.
 	 */
 	const char *metric_unit;
-	/** The list of metrics referenced by this one. */
-	struct list_head metric_refs;
-	/** The size of the metric_refs list. */
-	int metric_refs_cnt;
+	/** Optional null terminated array of referenced metrics. */
+	struct metric_ref *metric_refs;
 	/**
 	 * Is there a constraint on the group of events? In which case the
 	 * events won't be grouped.
@@ -202,20 +200,14 @@ static struct metric *metric__new(const struct pmu_event *pe,
 	m->metric_unit = pe->unit;
 	m->pctx->runtime = runtime;
 	m->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
-	INIT_LIST_HEAD(&m->metric_refs);
-	m->metric_refs_cnt = 0;
+	m->metric_refs = NULL;
 
 	return m;
 }
 
 static void metric__free(struct metric *m)
 {
-	struct metric_ref_node *ref, *tmp;
-
-	list_for_each_entry_safe(ref, tmp, &m->metric_refs, list) {
-		list_del(&ref->list);
-		free(ref);
-	}
+	free(m->metric_refs);
 	expr__ctx_free(m->pctx);
 	free(m);
 }
@@ -393,7 +385,6 @@ static int metricgroup__setup_events(struct list_head *groups,
 
 	list_for_each_entry (m, groups, nd) {
 		struct evsel **metric_events;
-		struct metric_ref *metric_refs = NULL;
 		const size_t ids_size = hashmap__size(m->pctx->ids);
 
 		metric_events = calloc(sizeof(void *),
@@ -427,36 +418,8 @@ static int metricgroup__setup_events(struct list_head *groups,
 			break;
 		}
 
-		/*
-		 * Collect and store collected nested expressions
-		 * for metric processing.
-		 */
-		if (m->metric_refs_cnt) {
-			struct metric_ref_node *ref;
-
-			metric_refs = zalloc(sizeof(struct metric_ref) * (m->metric_refs_cnt + 1));
-			if (!metric_refs) {
-				ret = -ENOMEM;
-				free(metric_events);
-				free(expr);
-				break;
-			}
-
-			i = 0;
-			list_for_each_entry(ref, &m->metric_refs, list) {
-				/*
-				 * Intentionally passing just const char pointers,
-				 * originally from 'struct pmu_event' object.
-				 * We don't need to change them, so there's no
-				 * need to create our own copy.
-				 */
-				metric_refs[i].metric_name = ref->metric_name;
-				metric_refs[i].metric_expr = ref->metric_expr;
-				i++;
-			}
-		}
-
-		expr->metric_refs = metric_refs;
+		expr->metric_refs = m->metric_refs;
+		m->metric_refs = NULL;
 		expr->metric_expr = m->metric_expr;
 		expr->metric_name = m->metric_name;
 		expr->metric_unit = m->metric_unit;
@@ -936,7 +899,6 @@ static int __add_metric(struct list_head *metric_list,
 			const struct visited_metric *visited,
 			const struct pmu_events_map *map)
 {
-	struct metric_ref_node *ref;
 	const struct visited_metric *vm;
 	int ret;
 	bool is_root = !root_metric;
@@ -962,19 +924,25 @@ static int __add_metric(struct list_head *metric_list,
 			return -ENOMEM;
 
 	} else {
+		int cnt = 0;
+
 		/*
 		 * This metric was referenced in a metric higher in the
 		 * tree. Check if the same metric is already resolved in the
 		 * metric_refs list.
 		 */
-		list_for_each_entry(ref, &root_metric->metric_refs, list) {
-			if (!strcmp(pe->metric_name, ref->metric_name))
-				return 0;
+		if (root_metric->metric_refs) {
+			for (; root_metric->metric_refs[cnt].metric_name; cnt++) {
+				if (!strcmp(pe->metric_name,
+					    root_metric->metric_refs[cnt].metric_name))
+					return 0;
+			}
 		}
 
-		/* Create reference */
-		ref = malloc(sizeof(*ref));
-		if (!ref)
+		/* Create reference. Need space for the entry and the terminator. */
+		root_metric->metric_refs = realloc(root_metric->metric_refs,
+						(cnt + 2) * sizeof(struct metric_ref));
+		if (!root_metric->metric_refs)
 			return -ENOMEM;
 
 		/*
@@ -983,11 +951,12 @@ static int __add_metric(struct list_head *metric_list,
 		 * need to change them, so there's no need to create
 		 * our own copy.
 		 */
-		ref->metric_name = pe->metric_name;
-		ref->metric_expr = pe->metric_expr;
+		root_metric->metric_refs[cnt].metric_name = pe->metric_name;
+		root_metric->metric_refs[cnt].metric_expr = pe->metric_expr;
 
-		list_add(&ref->list, &root_metric->metric_refs);
-		root_metric->metric_refs_cnt++;
+		/* Null terminate array. */
+		root_metric->metric_refs[cnt+1].metric_name = NULL;
+		root_metric->metric_refs[cnt+1].metric_expr = NULL;
 	}
 
 	/*
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 13/21] perf parse-events: Add const to evsel name

[Ian Rogers]

The evsel name is strdup-ed before assignment and so can be const.
A later change will add another similar string. Using const makes it
clearer that these are not out arguments.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events-hybrid.c | 15 +++++++++------
 tools/perf/util/parse-events-hybrid.h |  6 ++++--
 tools/perf/util/parse-events.c        | 15 ++++++++-------
 tools/perf/util/parse-events.h        |  7 ++++---
 tools/perf/util/pmu.c                 |  2 +-
 tools/perf/util/pmu.h                 |  2 +-
 6 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/parse-events-hybrid.c b/tools/perf/util/parse-events-hybrid.c
index b234d95fb10a..7e44deee1343 100644
--- a/tools/perf/util/parse-events-hybrid.c
+++ b/tools/perf/util/parse-events-hybrid.c
@@ -38,7 +38,7 @@ static void config_hybrid_attr(struct perf_event_attr *attr,
 
 static int create_event_hybrid(__u32 config_type, int *idx,
 			       struct list_head *list,
-			       struct perf_event_attr *attr, char *name,
+			       struct perf_event_attr *attr, const char *name,
 			       struct list_head *config_terms,
 			       struct perf_pmu *pmu)
 {
@@ -70,7 +70,7 @@ static int pmu_cmp(struct parse_events_state *parse_state,
 
 static int add_hw_hybrid(struct parse_events_state *parse_state,
 			 struct list_head *list, struct perf_event_attr *attr,
-			 char *name, struct list_head *config_terms)
+			 const char *name, struct list_head *config_terms)
 {
 	struct perf_pmu *pmu;
 	int ret;
@@ -94,7 +94,8 @@ static int add_hw_hybrid(struct parse_events_state *parse_state,
 }
 
 static int create_raw_event_hybrid(int *idx, struct list_head *list,
-				   struct perf_event_attr *attr, char *name,
+				   struct perf_event_attr *attr,
+				   const char *name,
 				   struct list_head *config_terms,
 				   struct perf_pmu *pmu)
 {
@@ -113,7 +114,7 @@ static int create_raw_event_hybrid(int *idx, struct list_head *list,
 
 static int add_raw_hybrid(struct parse_events_state *parse_state,
 			  struct list_head *list, struct perf_event_attr *attr,
-			  char *name, struct list_head *config_terms)
+			  const char *name, struct list_head *config_terms)
 {
 	struct perf_pmu *pmu;
 	int ret;
@@ -138,7 +139,8 @@ static int add_raw_hybrid(struct parse_events_state *parse_state,
 int parse_events__add_numeric_hybrid(struct parse_events_state *parse_state,
 				     struct list_head *list,
 				     struct perf_event_attr *attr,
-				     char *name, struct list_head *config_terms,
+				     const char *name,
+				     struct list_head *config_terms,
 				     bool *hybrid)
 {
 	*hybrid = false;
@@ -159,7 +161,8 @@ int parse_events__add_numeric_hybrid(struct parse_events_state *parse_state,
 }
 
 int parse_events__add_cache_hybrid(struct list_head *list, int *idx,
-				   struct perf_event_attr *attr, char *name,
+				   struct perf_event_attr *attr,
+				   const char *name,
 				   struct list_head *config_terms,
 				   bool *hybrid,
 				   struct parse_events_state *parse_state)
diff --git a/tools/perf/util/parse-events-hybrid.h b/tools/perf/util/parse-events-hybrid.h
index f33bd67aa851..25a4a4f73f3a 100644
--- a/tools/perf/util/parse-events-hybrid.h
+++ b/tools/perf/util/parse-events-hybrid.h
@@ -11,11 +11,13 @@
 int parse_events__add_numeric_hybrid(struct parse_events_state *parse_state,
 				     struct list_head *list,
 				     struct perf_event_attr *attr,
-				     char *name, struct list_head *config_terms,
+				     const char *name,
+				     struct list_head *config_terms,
 				     bool *hybrid);
 
 int parse_events__add_cache_hybrid(struct list_head *list, int *idx,
-				   struct perf_event_attr *attr, char *name,
+				   struct perf_event_attr *attr,
+				   const char *name,
 				   struct list_head *config_terms,
 				   bool *hybrid,
 				   struct parse_events_state *parse_state);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1acac3e13b32..88f181a985b7 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -337,7 +337,7 @@ static int parse_events__is_name_term(struct parse_events_term *term)
 	return term->type_term == PARSE_EVENTS__TERM_TYPE_NAME;
 }
 
-static char *get_config_name(struct list_head *head_terms)
+static const char *get_config_name(struct list_head *head_terms)
 {
 	struct parse_events_term *term;
 
@@ -355,7 +355,7 @@ static struct evsel *
 __add_event(struct list_head *list, int *idx,
 	    struct perf_event_attr *attr,
 	    bool init_attr,
-	    char *name, struct perf_pmu *pmu,
+	    const char *name, struct perf_pmu *pmu,
 	    struct list_head *config_terms, bool auto_merge_stats,
 	    const char *cpu_list)
 {
@@ -394,14 +394,14 @@ __add_event(struct list_head *list, int *idx,
 }
 
 struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
-					char *name, struct perf_pmu *pmu)
+				      const char *name, struct perf_pmu *pmu)
 {
 	return __add_event(NULL, &idx, attr, false, name, pmu, NULL, false,
 			   NULL);
 }
 
 static int add_event(struct list_head *list, int *idx,
-		     struct perf_event_attr *attr, char *name,
+		     struct perf_event_attr *attr, const char *name,
 		     struct list_head *config_terms)
 {
 	return __add_event(list, idx, attr, true, name, NULL, config_terms,
@@ -464,7 +464,8 @@ int parse_events_add_cache(struct list_head *list, int *idx,
 {
 	struct perf_event_attr attr;
 	LIST_HEAD(config_terms);
-	char name[MAX_NAME_LEN], *config_name;
+	char name[MAX_NAME_LEN];
+	const char *config_name;
 	int cache_type = -1, cache_op = -1, cache_result = -1;
 	char *op_result[2] = { op_result1, op_result2 };
 	int i, n, ret;
@@ -2027,7 +2028,7 @@ int parse_events__modifier_event(struct list_head *list, char *str, bool add)
 	return 0;
 }
 
-int parse_events_name(struct list_head *list, char *name)
+int parse_events_name(struct list_head *list, const char *name)
 {
 	struct evsel *evsel;
 
@@ -3344,7 +3345,7 @@ char *parse_events_formats_error_string(char *additional_terms)
 
 struct evsel *parse_events__add_event_hybrid(struct list_head *list, int *idx,
 					     struct perf_event_attr *attr,
-					     char *name, struct perf_pmu *pmu,
+					     const char *name, struct perf_pmu *pmu,
 					     struct list_head *config_terms)
 {
 	return __add_event(list, idx, attr, true, name, pmu,
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index b32ed3064c49..54d24c24d074 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -162,7 +162,7 @@ void parse_events_terms__purge(struct list_head *terms);
 void parse_events__clear_array(struct parse_events_array *a);
 int parse_events__modifier_event(struct list_head *list, char *str, bool add);
 int parse_events__modifier_group(struct list_head *list, char *event_mod);
-int parse_events_name(struct list_head *list, char *name);
+int parse_events_name(struct list_head *list, const char *name);
 int parse_events_add_tracepoint(struct list_head *list, int *idx,
 				const char *sys, const char *event,
 				struct parse_events_error *error,
@@ -199,7 +199,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 			 bool use_alias);
 
 struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
-					char *name, struct perf_pmu *pmu);
+				      const char *name, struct perf_pmu *pmu);
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			       char *str,
@@ -266,7 +266,8 @@ int perf_pmu__test_parse_init(void);
 
 struct evsel *parse_events__add_event_hybrid(struct list_head *list, int *idx,
 					     struct perf_event_attr *attr,
-					     char *name, struct perf_pmu *pmu,
+					     const char *name,
+					     struct perf_pmu *pmu,
 					     struct list_head *config_terms);
 
 #endif /* __PERF_PARSE_EVENTS_H */
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index cdd6c3f6caf1..9b5039bf909a 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1906,7 +1906,7 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
 }
 
 void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
-				   char *name)
+				   const char *name)
 {
 	struct perf_pmu_format *format;
 	__u64 masks = 0, bits;
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index cc9f9e001347..f9743eab07b6 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -134,7 +134,7 @@ int perf_pmu__convert_scale(const char *scale, char **end, double *sval);
 int perf_pmu__caps_parse(struct perf_pmu *pmu);
 
 void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
-				   char *name);
+				   const char *name);
 
 bool perf_pmu__has_hybrid(void);
 int perf_pmu__match(char *pattern, char *name, char *tok);
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 14/21] perf parse-events: Add new "metric-id" term.

[Ian Rogers]

Add a new "metric-id" term to events so that metric parsing can set an
ID that can be reliably looked up. Metric parsing currently will turn a
metric like "instructions/cycles" into a parse events string of
"{instructions,cycles}:W". However, parse-events may change
"instructions" into "instructions:u" if perf_event_paranoid=2. When this
happens expr__resolve_id currently fails as stat-shadow adds the ID
"instructions:u" to match with the counter value and the metric tries to
look up the ID just "instructions".

A later patch will use the new term.

An example of the current problem:
$ echo -1 > /proc/sys/kernel/perf_event_paranoid
$ perf stat -M IPC /bin/true
 Performance counter stats for '/bin/true':

         1,217,161      inst_retired.any          #     0.97 IPC
         1,250,389      cpu_clk_unhalted.thread

       0.002064773 seconds time elapsed

       0.002378000 seconds user
       0.000000000 seconds sys

$ echo 2 > /proc/sys/kernel/perf_event_paranoid
$ perf stat -M IPC /bin/true
 Performance counter stats for '/bin/true':

           150,298      inst_retired.any:u        #      nan IPC
           187,095      cpu_clk_unhalted.thread:u

       0.002042731 seconds time elapsed

       0.000000000 seconds user
       0.002377000 seconds sys

Note: nan IPC is printed as an effect of "perf metric: Use NAN for
missing event IDs." but earlier versions of perf just fail with a parse
error and display no value.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c               | 17 +++++
 tools/perf/util/evsel.h               |  2 +
 tools/perf/util/parse-events-hybrid.c | 25 ++++---
 tools/perf/util/parse-events-hybrid.h |  4 +-
 tools/perf/util/parse-events.c        | 95 ++++++++++++++++++---------
 tools/perf/util/parse-events.h        |  5 +-
 tools/perf/util/parse-events.l        |  1 +
 tools/perf/util/pfm.c                 |  3 +-
 8 files changed, 107 insertions(+), 45 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index dbfeceb2546c..96ef6a4a7c14 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -410,6 +410,11 @@ struct evsel *evsel__clone(struct evsel *orig)
 		if (evsel->filter == NULL)
 			goto out_err;
 	}
+	if (orig->metric_id) {
+		evsel->metric_id = strdup(orig->metric_id);
+		if (evsel->metric_id == NULL)
+			goto out_err;
+	}
 	evsel->cgrp = cgroup__get(orig->cgrp);
 	evsel->tp_format = orig->tp_format;
 	evsel->handler = orig->handler;
@@ -779,6 +784,17 @@ const char *evsel__name(struct evsel *evsel)
 	return "unknown";
 }
 
+const char *evsel__metric_id(const struct evsel *evsel)
+{
+	if (evsel->metric_id)
+		return evsel->metric_id;
+
+	if (evsel->core.attr.type == PERF_TYPE_SOFTWARE && evsel->tool_event)
+		return "duration_time";
+
+	return "unknown";
+}
+
 const char *evsel__group_name(struct evsel *evsel)
 {
 	return evsel->group_name ?: "anon group";
@@ -1423,6 +1439,7 @@ void evsel__exit(struct evsel *evsel)
 	zfree(&evsel->group_name);
 	zfree(&evsel->name);
 	zfree(&evsel->pmu_name);
+	zfree(&evsel->metric_id);
 	evsel__zero_per_pkg(evsel);
 	hashmap__free(evsel->per_pkg_mask);
 	evsel->per_pkg_mask = NULL;
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 1f7edfa8568a..45476a888942 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -68,6 +68,7 @@ struct evsel {
 		double			scale;
 		const char		*unit;
 		struct cgroup		*cgrp;
+		const char		*metric_id;
 		enum perf_tool_event	tool_event;
 		/* parse modifier helper */
 		int			exclude_GH;
@@ -261,6 +262,7 @@ bool evsel__match_bpf_counter_events(const char *name);
 
 int __evsel__hw_cache_type_op_res_name(u8 type, u8 op, u8 result, char *bf, size_t size);
 const char *evsel__name(struct evsel *evsel);
+const char *evsel__metric_id(const struct evsel *evsel);
 
 const char *evsel__group_name(struct evsel *evsel);
 int evsel__group_desc(struct evsel *evsel, char *buf, size_t size);
diff --git a/tools/perf/util/parse-events-hybrid.c b/tools/perf/util/parse-events-hybrid.c
index 7e44deee1343..9fc86971027b 100644
--- a/tools/perf/util/parse-events-hybrid.c
+++ b/tools/perf/util/parse-events-hybrid.c
@@ -39,6 +39,7 @@ static void config_hybrid_attr(struct perf_event_attr *attr,
 static int create_event_hybrid(__u32 config_type, int *idx,
 			       struct list_head *list,
 			       struct perf_event_attr *attr, const char *name,
+			       const char *metric_id,
 			       struct list_head *config_terms,
 			       struct perf_pmu *pmu)
 {
@@ -47,7 +48,7 @@ static int create_event_hybrid(__u32 config_type, int *idx,
 	__u64 config = attr->config;
 
 	config_hybrid_attr(attr, config_type, pmu->type);
-	evsel = parse_events__add_event_hybrid(list, idx, attr, name,
+	evsel = parse_events__add_event_hybrid(list, idx, attr, name, metric_id,
 					       pmu, config_terms);
 	if (evsel)
 		evsel->pmu_name = strdup(pmu->name);
@@ -70,7 +71,8 @@ static int pmu_cmp(struct parse_events_state *parse_state,
 
 static int add_hw_hybrid(struct parse_events_state *parse_state,
 			 struct list_head *list, struct perf_event_attr *attr,
-			 const char *name, struct list_head *config_terms)
+			 const char *name, const char *metric_id,
+			 struct list_head *config_terms)
 {
 	struct perf_pmu *pmu;
 	int ret;
@@ -84,7 +86,7 @@ static int add_hw_hybrid(struct parse_events_state *parse_state,
 		copy_config_terms(&terms, config_terms);
 		ret = create_event_hybrid(PERF_TYPE_HARDWARE,
 					  &parse_state->idx, list, attr, name,
-					  &terms, pmu);
+					  metric_id, &terms, pmu);
 		free_config_terms(&terms);
 		if (ret)
 			return ret;
@@ -96,13 +98,14 @@ static int add_hw_hybrid(struct parse_events_state *parse_state,
 static int create_raw_event_hybrid(int *idx, struct list_head *list,
 				   struct perf_event_attr *attr,
 				   const char *name,
+				   const char *metric_id,
 				   struct list_head *config_terms,
 				   struct perf_pmu *pmu)
 {
 	struct evsel *evsel;
 
 	attr->type = pmu->type;
-	evsel = parse_events__add_event_hybrid(list, idx, attr, name,
+	evsel = parse_events__add_event_hybrid(list, idx, attr, name, metric_id,
 					       pmu, config_terms);
 	if (evsel)
 		evsel->pmu_name = strdup(pmu->name);
@@ -114,7 +117,8 @@ static int create_raw_event_hybrid(int *idx, struct list_head *list,
 
 static int add_raw_hybrid(struct parse_events_state *parse_state,
 			  struct list_head *list, struct perf_event_attr *attr,
-			  const char *name, struct list_head *config_terms)
+			  const char *name, const char *metric_id,
+			  struct list_head *config_terms)
 {
 	struct perf_pmu *pmu;
 	int ret;
@@ -127,7 +131,7 @@ static int add_raw_hybrid(struct parse_events_state *parse_state,
 
 		copy_config_terms(&terms, config_terms);
 		ret = create_raw_event_hybrid(&parse_state->idx, list, attr,
-					      name, &terms, pmu);
+					      name, metric_id, &terms, pmu);
 		free_config_terms(&terms);
 		if (ret)
 			return ret;
@@ -139,7 +143,7 @@ static int add_raw_hybrid(struct parse_events_state *parse_state,
 int parse_events__add_numeric_hybrid(struct parse_events_state *parse_state,
 				     struct list_head *list,
 				     struct perf_event_attr *attr,
-				     const char *name,
+				     const char *name, const char *metric_id,
 				     struct list_head *config_terms,
 				     bool *hybrid)
 {
@@ -152,17 +156,18 @@ int parse_events__add_numeric_hybrid(struct parse_events_state *parse_state,
 
 	*hybrid = true;
 	if (attr->type != PERF_TYPE_RAW) {
-		return add_hw_hybrid(parse_state, list, attr, name,
+		return add_hw_hybrid(parse_state, list, attr, name, metric_id,
 				     config_terms);
 	}
 
-	return add_raw_hybrid(parse_state, list, attr, name,
+	return add_raw_hybrid(parse_state, list, attr, name, metric_id,
 			      config_terms);
 }
 
 int parse_events__add_cache_hybrid(struct list_head *list, int *idx,
 				   struct perf_event_attr *attr,
 				   const char *name,
+				   const char *metric_id,
 				   struct list_head *config_terms,
 				   bool *hybrid,
 				   struct parse_events_state *parse_state)
@@ -183,7 +188,7 @@ int parse_events__add_cache_hybrid(struct list_head *list, int *idx,
 
 		copy_config_terms(&terms, config_terms);
 		ret = create_event_hybrid(PERF_TYPE_HW_CACHE, idx, list,
-					  attr, name, &terms, pmu);
+					  attr, name, metric_id, &terms, pmu);
 		free_config_terms(&terms);
 		if (ret)
 			return ret;
diff --git a/tools/perf/util/parse-events-hybrid.h b/tools/perf/util/parse-events-hybrid.h
index 25a4a4f73f3a..cbc05fec02a2 100644
--- a/tools/perf/util/parse-events-hybrid.h
+++ b/tools/perf/util/parse-events-hybrid.h
@@ -11,13 +11,13 @@
 int parse_events__add_numeric_hybrid(struct parse_events_state *parse_state,
 				     struct list_head *list,
 				     struct perf_event_attr *attr,
-				     const char *name,
+				     const char *name, const char *metric_id,
 				     struct list_head *config_terms,
 				     bool *hybrid);
 
 int parse_events__add_cache_hybrid(struct list_head *list, int *idx,
 				   struct perf_event_attr *attr,
-				   const char *name,
+				   const char *name, const char *metric_id,
 				   struct list_head *config_terms,
 				   bool *hybrid,
 				   struct parse_events_state *parse_state);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 88f181a985b7..89494b6213a6 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -332,12 +332,7 @@ const char *event_type(int type)
 	return "unknown";
 }
 
-static int parse_events__is_name_term(struct parse_events_term *term)
-{
-	return term->type_term == PARSE_EVENTS__TERM_TYPE_NAME;
-}
-
-static const char *get_config_name(struct list_head *head_terms)
+static char *get_config_str(struct list_head *head_terms, int type_term)
 {
 	struct parse_events_term *term;
 
@@ -345,17 +340,27 @@ static const char *get_config_name(struct list_head *head_terms)
 		return NULL;
 
 	list_for_each_entry(term, head_terms, list)
-		if (parse_events__is_name_term(term))
+		if (term->type_term == type_term)
 			return term->val.str;
 
 	return NULL;
 }
 
+static char *get_config_metric_id(struct list_head *head_terms)
+{
+	return get_config_str(head_terms, PARSE_EVENTS__TERM_TYPE_METRIC_ID);
+}
+
+static char *get_config_name(struct list_head *head_terms)
+{
+	return get_config_str(head_terms, PARSE_EVENTS__TERM_TYPE_NAME);
+}
+
 static struct evsel *
 __add_event(struct list_head *list, int *idx,
 	    struct perf_event_attr *attr,
 	    bool init_attr,
-	    const char *name, struct perf_pmu *pmu,
+	    const char *name, const char *metric_id, struct perf_pmu *pmu,
 	    struct list_head *config_terms, bool auto_merge_stats,
 	    const char *cpu_list)
 {
@@ -384,6 +389,9 @@ __add_event(struct list_head *list, int *idx,
 	if (name)
 		evsel->name = strdup(name);
 
+	if (metric_id)
+		evsel->metric_id = strdup(metric_id);
+
 	if (config_terms)
 		list_splice_init(config_terms, &evsel->config_terms);
 
@@ -394,18 +402,21 @@ __add_event(struct list_head *list, int *idx,
 }
 
 struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
-				      const char *name, struct perf_pmu *pmu)
+				      const char *name, const char *metric_id,
+				      struct perf_pmu *pmu)
 {
-	return __add_event(NULL, &idx, attr, false, name, pmu, NULL, false,
-			   NULL);
+	return __add_event(/*list=*/NULL, &idx, attr, /*init_attr=*/false, name,
+			   metric_id, pmu, /*config_terms=*/NULL,
+			   /*auto_merge_stats=*/false, /*cpu_list=*/NULL);
 }
 
 static int add_event(struct list_head *list, int *idx,
 		     struct perf_event_attr *attr, const char *name,
-		     struct list_head *config_terms)
+		     const char *metric_id, struct list_head *config_terms)
 {
-	return __add_event(list, idx, attr, true, name, NULL, config_terms,
-			   false, NULL) ? 0 : -ENOMEM;
+	return __add_event(list, idx, attr, /*init_attr*/true, name, metric_id,
+			   /*pmu=*/NULL, config_terms,
+			   /*auto_merge_stats=*/false, /*cpu_list=*/NULL) ? 0 : -ENOMEM;
 }
 
 static int add_event_tool(struct list_head *list, int *idx,
@@ -417,8 +428,10 @@ static int add_event_tool(struct list_head *list, int *idx,
 		.config = PERF_COUNT_SW_DUMMY,
 	};
 
-	evsel = __add_event(list, idx, &attr, true, NULL, NULL, NULL, false,
-			    "0");
+	evsel = __add_event(list, idx, &attr, /*init_attr=*/true, /*name=*/NULL,
+			    /*metric_id=*/NULL, /*pmu=*/NULL,
+			    /*config_terms=*/NULL, /*auto_merge_stats=*/false,
+			    /*cpu_list=*/"0");
 	if (!evsel)
 		return -ENOMEM;
 	evsel->tool_event = tool_event;
@@ -465,7 +478,7 @@ int parse_events_add_cache(struct list_head *list, int *idx,
 	struct perf_event_attr attr;
 	LIST_HEAD(config_terms);
 	char name[MAX_NAME_LEN];
-	const char *config_name;
+	const char *config_name, *metric_id;
 	int cache_type = -1, cache_op = -1, cache_result = -1;
 	char *op_result[2] = { op_result1, op_result2 };
 	int i, n, ret;
@@ -530,13 +543,17 @@ int parse_events_add_cache(struct list_head *list, int *idx,
 			return -ENOMEM;
 	}
 
+	metric_id = get_config_metric_id(head_config);
 	ret = parse_events__add_cache_hybrid(list, idx, &attr,
-					     config_name ? : name, &config_terms,
+					     config_name ? : name,
+					     metric_id,
+					     &config_terms,
 					     &hybrid, parse_state);
 	if (hybrid)
 		goto out_free_terms;
 
-	ret = add_event(list, idx, &attr, config_name ? : name, &config_terms);
+	ret = add_event(list, idx, &attr, config_name ? : name, metric_id,
+			&config_terms);
 out_free_terms:
 	free_config_terms(&config_terms);
 	return ret;
@@ -1013,7 +1030,8 @@ int parse_events_add_breakpoint(struct list_head *list, int *idx,
 	attr.type = PERF_TYPE_BREAKPOINT;
 	attr.sample_period = 1;
 
-	return add_event(list, idx, &attr, NULL, NULL);
+	return add_event(list, idx, &attr, /*name=*/NULL, /*mertic_id=*/NULL,
+			 /*config_terms=*/NULL);
 }
 
 static int check_type_val(struct parse_events_term *term,
@@ -1058,6 +1076,7 @@ static const char *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
 	[PARSE_EVENTS__TERM_TYPE_PERCORE]		= "percore",
 	[PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT]		= "aux-output",
 	[PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE]	= "aux-sample-size",
+	[PARSE_EVENTS__TERM_TYPE_METRIC_ID]		= "metric-id",
 };
 
 static bool config_term_shrinked;
@@ -1080,6 +1099,7 @@ config_term_avail(int term_type, struct parse_events_error *err)
 	case PARSE_EVENTS__TERM_TYPE_CONFIG1:
 	case PARSE_EVENTS__TERM_TYPE_CONFIG2:
 	case PARSE_EVENTS__TERM_TYPE_NAME:
+	case PARSE_EVENTS__TERM_TYPE_METRIC_ID:
 	case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
 	case PARSE_EVENTS__TERM_TYPE_PERCORE:
 		return true;
@@ -1170,6 +1190,9 @@ do {									   \
 	case PARSE_EVENTS__TERM_TYPE_NAME:
 		CHECK_TYPE_VAL(STR);
 		break;
+	case PARSE_EVENTS__TERM_TYPE_METRIC_ID:
+		CHECK_TYPE_VAL(STR);
+		break;
 	case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
 		CHECK_TYPE_VAL(NUM);
 		break;
@@ -1439,6 +1462,7 @@ int parse_events_add_numeric(struct parse_events_state *parse_state,
 {
 	struct perf_event_attr attr;
 	LIST_HEAD(config_terms);
+	const char *name, *metric_id;
 	bool hybrid;
 	int ret;
 
@@ -1455,14 +1479,16 @@ int parse_events_add_numeric(struct parse_events_state *parse_state,
 			return -ENOMEM;
 	}
 
+	name = get_config_name(head_config);
+	metric_id = get_config_metric_id(head_config);
 	ret = parse_events__add_numeric_hybrid(parse_state, list, &attr,
-					       get_config_name(head_config),
+					       name, metric_id,
 					       &config_terms, &hybrid);
 	if (hybrid)
 		goto out_free_terms;
 
-	ret = add_event(list, &parse_state->idx, &attr,
-			get_config_name(head_config), &config_terms);
+	ret = add_event(list, &parse_state->idx, &attr, name, metric_id,
+			&config_terms);
 out_free_terms:
 	free_config_terms(&config_terms);
 	return ret;
@@ -1563,8 +1589,11 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 
 	if (!head_config) {
 		attr.type = pmu->type;
-		evsel = __add_event(list, &parse_state->idx, &attr, true, NULL,
-				    pmu, NULL, auto_merge_stats, NULL);
+		evsel = __add_event(list, &parse_state->idx, &attr,
+				    /*init_attr=*/true, /*name=*/NULL,
+				    /*metric_id=*/NULL, pmu,
+				    /*config_terms=*/NULL, auto_merge_stats,
+				    /*cpu_list=*/NULL);
 		if (evsel) {
 			evsel->pmu_name = name ? strdup(name) : NULL;
 			evsel->use_uncore_alias = use_uncore_alias;
@@ -1617,9 +1646,10 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		return -EINVAL;
 	}
 
-	evsel = __add_event(list, &parse_state->idx, &attr, true,
-			    get_config_name(head_config), pmu,
-			    &config_terms, auto_merge_stats, NULL);
+	evsel = __add_event(list, &parse_state->idx, &attr, /*init_attr=*/true,
+			    get_config_name(head_config),
+			    get_config_metric_id(head_config), pmu,
+			    &config_terms, auto_merge_stats, /*cpu_list=*/NULL);
 	if (!evsel)
 		return -ENOMEM;
 
@@ -3345,9 +3375,12 @@ char *parse_events_formats_error_string(char *additional_terms)
 
 struct evsel *parse_events__add_event_hybrid(struct list_head *list, int *idx,
 					     struct perf_event_attr *attr,
-					     const char *name, struct perf_pmu *pmu,
+					     const char *name,
+					     const char *metric_id,
+					     struct perf_pmu *pmu,
 					     struct list_head *config_terms)
 {
-	return __add_event(list, idx, attr, true, name, pmu,
-			   config_terms, false, NULL);
+	return __add_event(list, idx, attr, /*init_attr=*/true, name, metric_id,
+			   pmu, config_terms, /*auto_merge_stats=*/false,
+			   /*cpu_list=*/NULL);
 }
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 54d24c24d074..c6c8343d311b 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -87,6 +87,7 @@ enum {
 	PARSE_EVENTS__TERM_TYPE_PERCORE,
 	PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT,
 	PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE,
+	PARSE_EVENTS__TERM_TYPE_METRIC_ID,
 	__PARSE_EVENTS__TERM_TYPE_NR,
 };
 
@@ -199,7 +200,8 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 			 bool use_alias);
 
 struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
-				      const char *name, struct perf_pmu *pmu);
+				      const char *name, const char *metric_id,
+				      struct perf_pmu *pmu);
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			       char *str,
@@ -267,6 +269,7 @@ int perf_pmu__test_parse_init(void);
 struct evsel *parse_events__add_event_hybrid(struct list_head *list, int *idx,
 					     struct perf_event_attr *attr,
 					     const char *name,
+					     const char *metric_id,
 					     struct perf_pmu *pmu,
 					     struct list_head *config_terms);
 
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index 47da7a0c5df4..b1e29b97d261 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -294,6 +294,7 @@ no-overwrite		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_NOOVERWRITE); }
 percore			{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_PERCORE); }
 aux-output		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT); }
 aux-sample-size		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE); }
+metric-id		{ return term(yyscanner, PARSE_EVENTS__TERM_TYPE_METRIC_ID); }
 r{num_raw_hex}		{ return raw(yyscanner); }
 r0x{num_raw_hex}	{ return raw(yyscanner); }
 ,			{ return ','; }
diff --git a/tools/perf/util/pfm.c b/tools/perf/util/pfm.c
index 756295dedccc..f0bcfcab1a93 100644
--- a/tools/perf/util/pfm.c
+++ b/tools/perf/util/pfm.c
@@ -87,7 +87,8 @@ int parse_libpfm_events_option(const struct option *opt, const char *str,
 
 		pmu = perf_pmu__find_by_type((unsigned int)attr.type);
 		evsel = parse_events__add_event(evlist->core.nr_entries,
-						&attr, q, pmu);
+						&attr, q, /*metric_id=*/NULL,
+						pmu);
 		if (evsel == NULL)
 			goto error;
 
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 15/21] perf parse-events: Allow config on kernel PMU events

[Ian Rogers]

An event like inst_retired.any on an Intel skylake is found in the
pmu-events code created from the pipeline event json. The event is an
alias for cpu/event=0xc0,period=2000003/ and parse-events recognizes the
event with the token PE_KERNEL_PMU_EVENT. The parser doesn't currently
allow extra configuration on such events, except for modifiers, so:

$ perf stat -e inst_retired.any// /bin/true
event syntax error: 'inst_retired.any//'
                     \___ parser error
Run 'perf list' for a list of valid events

 Usage: perf stat [<options>] [<command>]

    -e, --event <event>   event selector. use 'perf list' to list available events

This patch adds configuration to these events which can be useful for
a number of parameters like name and call-graph:

$ sudo perf record -e inst_retired.any/call-graph=lbr/ -a sleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 1.856 MB perf.data (44 samples) ]

It is necessary for the metric code so that we may add metric-id values
to these events before they are parsed.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 66 ++++++++++++++++++----------------
 tools/perf/util/parse-events.h |  1 +
 tools/perf/util/parse-events.y | 17 +++++++--
 3 files changed, 52 insertions(+), 32 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 89494b6213a6..006a7f721549 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1673,44 +1673,50 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 }
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
-			       char *str, struct list_head **listp)
+			       char *str, struct list_head *head,
+			       struct list_head **listp)
 {
 	struct parse_events_term *term;
-	struct list_head *list;
+	struct list_head *list = NULL;
 	struct perf_pmu *pmu = NULL;
 	int ok = 0;
+	char *config;
 
 	*listp = NULL;
+
+	if (!head) {
+		head = malloc(sizeof(struct list_head));
+		if (!head)
+			goto out_err;
+
+		INIT_LIST_HEAD(head);
+	}
+	config = strdup(str);
+	if (!config)
+		goto out_err;
+
+	if (parse_events_term__num(&term,
+				   PARSE_EVENTS__TERM_TYPE_USER,
+				   config, 1, false, &config,
+					NULL) < 0) {
+		free(config);
+		goto out_err;
+	}
+	list_add_tail(&term->list, head);
+
+
 	/* Add it for all PMUs that support the alias */
 	list = malloc(sizeof(struct list_head));
 	if (!list)
-		return -1;
+		goto out_err;
+
 	INIT_LIST_HEAD(list);
+
 	while ((pmu = perf_pmu__scan(pmu)) != NULL) {
 		struct perf_pmu_alias *alias;
 
 		list_for_each_entry(alias, &pmu->aliases, list) {
 			if (!strcasecmp(alias->name, str)) {
-				struct list_head *head;
-				char *config;
-
-				head = malloc(sizeof(struct list_head));
-				if (!head)
-					return -1;
-				INIT_LIST_HEAD(head);
-				config = strdup(str);
-				if (!config)
-					return -1;
-				if (parse_events_term__num(&term,
-						   PARSE_EVENTS__TERM_TYPE_USER,
-						   config, 1, false, &config,
-						   NULL) < 0) {
-					free(list);
-					free(config);
-					return -1;
-				}
-				list_add_tail(&term->list, head);
-
 				if (!parse_events_add_pmu(parse_state, list,
 							  pmu->name, head,
 							  true, true)) {
@@ -1718,17 +1724,17 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 						 pmu->name, alias->str);
 					ok++;
 				}
-
-				parse_events_terms__delete(head);
 			}
 		}
 	}
-	if (!ok) {
+out_err:
+	if (ok)
+		*listp = list;
+	else
 		free(list);
-		return -1;
-	}
-	*listp = list;
-	return 0;
+
+	parse_events_terms__delete(head);
+	return ok ? 0 : -1;
 }
 
 int parse_events__modifier_group(struct list_head *list,
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index c6c8343d311b..07f879e525fe 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -205,6 +205,7 @@ struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			       char *str,
+			       struct list_head *head_config,
 			       struct list_head **listp);
 
 int parse_events_copy_term_list(struct list_head *old,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index d94e48e1ff9b..17c8c66f3f51 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -342,7 +342,20 @@ PE_KERNEL_PMU_EVENT sep_dc
 	struct list_head *list;
 	int err;
 
-	err = parse_events_multi_pmu_add(_parse_state, $1, &list);
+	err = parse_events_multi_pmu_add(_parse_state, $1, NULL, &list);
+	free($1);
+	if (err < 0)
+		YYABORT;
+	$$ = list;
+}
+|
+PE_KERNEL_PMU_EVENT opt_pmu_config
+{
+	struct list_head *list;
+	int err;
+
+	/* frees $2 */
+	err = parse_events_multi_pmu_add(_parse_state, $1, $2, &list);
 	free($1);
 	if (err < 0)
 		YYABORT;
@@ -357,7 +370,7 @@ PE_PMU_EVENT_PRE '-' PE_PMU_EVENT_SUF sep_dc
 	snprintf(pmu_name, sizeof(pmu_name), "%s-%s", $1, $3);
 	free($1);
 	free($3);
-	if (parse_events_multi_pmu_add(_parse_state, pmu_name, &list) < 0)
+	if (parse_events_multi_pmu_add(_parse_state, pmu_name, NULL, &list) < 0)
 		YYABORT;
 	$$ = list;
 }
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 16/21] perf metric: Encode and use metric-id as qualifier

[Ian Rogers]

For a metric like IPC a group of events like {instructions,cycles}:W
would be formed. If the events names were changed in parsing then the
metric expression parser would fail to find them. This change makes the
event encoding be something like {instructions/metric-id=instructions/,
cycles/metric-id=cycles/} and then uses the evsel's stable metric-id
value to locate the events. This fixes the case that an event is
restricted to user because of the paranoia setting:

    $ echo 2 > /proc/sys/kernel/perf_event_paranoid
    $ perf stat -M IPC /bin/true
     Performance counter stats for '/bin/true':

               150,298      inst_retired.any:u        #      0.77 IPC
               187,095      cpu_clk_unhalted.thread:u

           0.002042731 seconds time elapsed

           0.000000000 seconds user
           0.002377000 seconds sys

Adding the metric-id as a qualifier has a complication in that
qualifiers will become embedded in qualifiers. For example, msr/tsc/
could become msr/tsc,metric-id=msr/tsc// which will fail parse-events.
To solve this problem the metric is encoded and decoded for the
metric-id with !<num> standing in for an encoded value. Previously !
wasn't parsed. With this msr/tsc/ becomese msr/tsc,metric-id=msr!3tsc!3/

The metric expression parser is changed so that @ isn't changed to /,
instead this is done when the ID is encoded for parse events.

metricgroup__add_metric_non_group and metricgroup__add_metric_weak_group
need to inject the metric-id qualifier, so to avoid repetition they are
merged into a single metricgroup__build_event_string with error codes
more rigorously checked.

stat-shadow's prepare_metric uses the metric-id to match the metricgroup
code.

As "metric-id=..." is added to all events, it is adding during testing
with the fake PMU. This complicates pmu_str_check code as
PE_PMU_EVENT_FAKE won't match as part of a configuration. The testing
fake PMU case is fixed so that if a known qualifier with an ! is parsed
then it isn't reported as a fake PMU. This is sufficient to pass all
testing but it and the original mechanism are somewhat brittle.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/expr.c        |   4 +-
 tools/perf/tests/pmu-events.c  |  13 +-
 tools/perf/util/expr.l         |   6 +-
 tools/perf/util/metricgroup.c  | 263 ++++++++++++++++++++++++++-------
 tools/perf/util/parse-events.l |  17 ++-
 tools/perf/util/stat-shadow.c  |  20 +--
 6 files changed, 242 insertions(+), 81 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 718c13e5a0f4..077783223ce0 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -129,9 +129,9 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
 			expr__find_ids("EVENT1\\,param\\=?@ + EVENT2\\,param\\=?@",
 					NULL, ctx) == 0);
 	TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 2);
-	TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "EVENT1,param=3/",
+	TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "EVENT1,param=3@",
 						    (void **)&val_ptr));
-	TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "EVENT2,param=3/",
+	TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "EVENT2,param=3@",
 						    (void **)&val_ptr));
 
 	/* Only EVENT1 or EVENT2 need be measured depending on the value of smt_on. */
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index 71b08c296410..50b1299fe643 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -761,6 +761,7 @@ static int check_parse_id(const char *id, struct parse_events_error *error,
 {
 	struct evlist *evlist;
 	int ret;
+	char *dup, *cur;
 
 	/* Numbers are always valid. */
 	if (is_number(id))
@@ -769,7 +770,17 @@ static int check_parse_id(const char *id, struct parse_events_error *error,
 	evlist = evlist__new();
 	if (!evlist)
 		return -ENOMEM;
-	ret = __parse_events(evlist, id, error, fake_pmu);
+
+	dup = strdup(id);
+	if (!dup)
+		return -ENOMEM;
+
+	for (cur = strchr(dup, '@') ; cur; cur = strchr(++cur, '@'))
+		*cur = '/';
+
+	ret = __parse_events(evlist, dup, error, fake_pmu);
+	free(dup);
+
 	evlist__delete(evlist);
 	return ret;
 }
diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
index 702fdf6456ca..bd20f33418ba 100644
--- a/tools/perf/util/expr.l
+++ b/tools/perf/util/expr.l
@@ -41,11 +41,9 @@ static char *normalize(char *str, int runtime)
 	char *dst = str;
 
 	while (*str) {
-		if (*str == '@')
-			*dst++ = '/';
-		else if (*str == '\\')
+		if (*str == '\\')
 			*dst++ = *++str;
-		 else if (*str == '?') {
+		else if (*str == '?') {
 			char *paramval;
 			int i = 0;
 			int size = asprintf(&paramval, "%d", runtime);
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index b48836d7c080..9c16a956fd2c 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -212,13 +212,13 @@ static void metric__free(struct metric *m)
 	free(m);
 }
 
-static bool contains_event(struct evsel **metric_events, int num_events,
-			const char *event_name)
+static bool contains_metric_id(struct evsel **metric_events, int num_events,
+			       const char *metric_id)
 {
 	int i;
 
 	for (i = 0; i < num_events; i++) {
-		if (!strcmp(metric_events[i]->name, event_name))
+		if (!strcmp(evsel__metric_id(metric_events[i]), metric_id))
 			return true;
 	}
 	return false;
@@ -259,6 +259,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 {
 	struct evsel *ev, *current_leader = NULL;
 	struct expr_id_data *val_ptr;
+	const char *metric_id;
 	int i = 0, matched_events = 0, events_to_match;
 	int idnum = (int)hashmap__size(pctx->ids);
 
@@ -300,10 +301,11 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 			 * different sibling groups aren't both added to
 			 * metric_events.
 			 */
-			if (contains_event(metric_events, matched_events, ev->name))
+			metric_id = evsel__metric_id(ev);
+			if (contains_metric_id(metric_events, matched_events, metric_id))
 				continue;
 			/* Does this event belong to the parse context? */
-			if (hashmap__find(pctx->ids, ev->name, (void **)&val_ptr))
+			if (hashmap__find(pctx->ids, metric_id, (void **)&val_ptr))
 				metric_events[matched_events++] = ev;
 
 			if (matched_events == events_to_match)
@@ -347,6 +349,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 		 * for each pmu. Set the metric leader of such events to be the
 		 * event that appears in metric_events.
 		 */
+		metric_id = evsel__metric_id(ev);
 		evlist__for_each_entry_continue(perf_evlist, ev) {
 			/*
 			 * If events are grouped then the search can terminate
@@ -356,7 +359,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 			    ev->core.leader != metric_events[i]->core.leader &&
 			    evsel_same_pmu_or_none(evsel__leader(ev), evsel__leader(metric_events[i])))
 				break;
-			if (!strcmp(metric_events[i]->name, ev->name)) {
+			if (!strcmp(evsel__metric_id(metric_events[i]), metric_id)) {
 				set_bit(ev->core.idx, evlist_used);
 				ev->metric_leader = metric_events[i];
 			}
@@ -724,50 +727,191 @@ void metricgroup__print(bool metrics, bool metricgroups, char *filter,
 	strlist__delete(metriclist);
 }
 
-static void metricgroup__add_metric_weak_group(struct strbuf *events,
-					       struct expr_parse_ctx *ctx)
+static const char *code_characters = ",-=@";
+
+static int encode_metric_id(struct strbuf *sb, const char *x)
+{
+	char *c;
+	int ret = 0;
+
+	for (; *x; x++) {
+		c = strchr(code_characters, *x);
+		if (c) {
+			ret = strbuf_addch(sb, '!');
+			if (ret)
+				break;
+
+			ret = strbuf_addch(sb, '0' + (c - code_characters));
+			if (ret)
+				break;
+		} else {
+			ret = strbuf_addch(sb, *x);
+			if (ret)
+				break;
+		}
+	}
+	return ret;
+}
+
+static int decode_metric_id(struct strbuf *sb, const char *x)
+{
+	const char *orig = x;
+	size_t i;
+	char c;
+	int ret;
+
+	for (; *x; x++) {
+		c = *x;
+		if (*x == '!') {
+			x++;
+			i = *x - '0';
+			if (i > strlen(code_characters)) {
+				pr_err("Bad metric-id encoding in: '%s'", orig);
+				return -1;
+			}
+			c = code_characters[i];
+		}
+		ret = strbuf_addch(sb, c);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static int decode_all_metric_ids(struct evlist *perf_evlist)
+{
+	struct evsel *ev;
+	struct strbuf sb = STRBUF_INIT;
+	char *cur;
+	int ret = 0;
+
+	evlist__for_each_entry(perf_evlist, ev) {
+		if (!ev->metric_id)
+			continue;
+
+		ret = strbuf_setlen(&sb, 0);
+		if (ret)
+			break;
+
+		ret = decode_metric_id(&sb, ev->metric_id);
+		if (ret)
+			break;
+
+		free((char *)ev->metric_id);
+		ev->metric_id = strdup(sb.buf);
+		if (!ev->metric_id) {
+			ret = -ENOMEM;
+			break;
+		}
+		/*
+		 * If the name is just the parsed event, use the metric-id to
+		 * give a more friendly display version.
+		 */
+		if (strstr(ev->name, "metric-id=")) {
+			free(ev->name);
+			for (cur = strchr(sb.buf, '@') ; cur; cur = strchr(++cur, '@'))
+				*cur = '/';
+
+			ev->name = strdup(sb.buf);
+			if (!ev->name) {
+				ret = -ENOMEM;
+				break;
+			}
+		}
+	}
+	strbuf_release(&sb);
+	return ret;
+}
+
+static int metricgroup__build_event_string(struct strbuf *events,
+					   const struct expr_parse_ctx *ctx,
+					   bool has_constraint)
 {
 	struct hashmap_entry *cur;
 	size_t bkt;
 	bool no_group = true, has_duration = false;
+	int ret = 0;
+
+#define RETURN_IF_NON_ZERO(x) do { if (x) return x; } while (0)
 
 	hashmap__for_each_entry(ctx->ids, cur, bkt) {
-		pr_debug("found event %s\n", (const char *)cur->key);
+		const char *sep, *rsep, *id = cur->key;
+
+		pr_debug("found event %s\n", id);
 		/*
 		 * Duration time maps to a software event and can make
 		 * groups not count. Always use it outside a
 		 * group.
 		 */
-		if (!strcmp(cur->key, "duration_time")) {
+		if (!strcmp(id, "duration_time")) {
 			has_duration = true;
 			continue;
 		}
-		strbuf_addf(events, "%s%s",
-			no_group ? "{" : ",",
-			(const char *)cur->key);
-		no_group = false;
-	}
-	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,
-					      struct expr_parse_ctx *ctx)
-{
-	struct hashmap_entry *cur;
-	size_t bkt;
-	bool first = true;
+		/* Separate events with commas and open the group if necessary. */
+		if (no_group) {
+			if (!has_constraint) {
+				ret = strbuf_addch(events, '{');
+				RETURN_IF_NON_ZERO(ret);
+			}
 
-	hashmap__for_each_entry(ctx->ids, cur, bkt) {
-		if (!first)
-			strbuf_addf(events, ",");
-		strbuf_addf(events, "%s", (const char *)cur->key);
-		first = false;
+			no_group = false;
+		} else {
+			ret = strbuf_addch(events, ',');
+			RETURN_IF_NON_ZERO(ret);
+		}
+		/*
+		 * Encode the ID as an event string. Add a qualifier for
+		 * metric_id that is the original name except with characters
+		 * that parse-events can't parse replaced. For example,
+		 * 'msr@tsc@' gets added as msr/tsc,metric-id=msr!3tsc!3/
+		 */
+		sep = strchr(id, '@');
+		if (sep != NULL) {
+			ret = strbuf_add(events, id, sep - id);
+			RETURN_IF_NON_ZERO(ret);
+			ret = strbuf_addch(events, '/');
+			RETURN_IF_NON_ZERO(ret);
+			rsep = strrchr(sep, '@');
+			ret = strbuf_add(events, sep + 1, rsep - sep - 1);
+			RETURN_IF_NON_ZERO(ret);
+			ret = strbuf_addstr(events, ",metric-id=");
+			RETURN_IF_NON_ZERO(ret);
+			sep = rsep;
+		} else {
+			sep = strchr(id, ':');
+			if (sep != NULL) {
+				ret = strbuf_add(events, id, sep - id);
+				RETURN_IF_NON_ZERO(ret);
+			} else {
+				ret = strbuf_addstr(events, id);
+				RETURN_IF_NON_ZERO(ret);
+			}
+			ret = strbuf_addstr(events, "/metric-id=");
+			RETURN_IF_NON_ZERO(ret);
+		}
+		ret = encode_metric_id(events, id);
+		RETURN_IF_NON_ZERO(ret);
+		ret = strbuf_addstr(events, "/");
+		RETURN_IF_NON_ZERO(ret);
+
+		if (sep != NULL) {
+			ret = strbuf_addstr(events, sep + 1);
+			RETURN_IF_NON_ZERO(ret);
+		}
 	}
+	if (has_duration) {
+		if (no_group) {
+			/* Strange case of a metric of just duration_time. */
+			ret = strbuf_addf(events, "duration_time");
+		} else if (!has_constraint)
+			ret = strbuf_addf(events, "}:W,duration_time");
+		else
+			ret = strbuf_addf(events, ",duration_time");
+	} else if (!no_group && !has_constraint)
+		ret = strbuf_addf(events, "}:W");
+
+	return ret;
+#undef RETURN_IF_NON_ZERO
 }
 
 int __weak arch_get_runtimeparam(const struct pmu_event *pe __maybe_unused)
@@ -1134,16 +1278,17 @@ static int metricgroup__add_metric(const char *metric_name, bool metric_no_group
 	list_sort(NULL,  &list, metric_list_cmp);
 
 	list_for_each_entry(m, &list, nd) {
-		if (events->len > 0)
-			strbuf_addf(events, ",");
-
-		if (m->has_constraint) {
-			metricgroup__add_metric_non_group(events,
-							  m->pctx);
-		} else {
-			metricgroup__add_metric_weak_group(events,
-							   m->pctx);
+		if (events->len > 0) {
+			ret = strbuf_addf(events, ",");
+			if (ret)
+				break;
 		}
+
+		ret = metricgroup__build_event_string(events,
+						m->pctx,
+						m->has_constraint);
+		if (ret)
+			break;
 	}
 
 out:
@@ -1180,30 +1325,40 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 					const struct pmu_events_map *map)
 {
 	char *llist, *nlist, *p;
-	int ret = -EINVAL;
+	int ret, count = 0;
 
 	nlist = strdup(list);
 	if (!nlist)
 		return -ENOMEM;
 	llist = nlist;
 
-	strbuf_init(events, 100);
-	strbuf_addf(events, "%s", "");
+	ret = strbuf_init(events, 100);
+	if (ret)
+		return ret;
 
 	while ((p = strsep(&llist, ",")) != NULL) {
 		ret = metricgroup__add_metric(p, metric_no_group, events,
 					      metric_list, map);
-		if (ret == -EINVAL) {
-			fprintf(stderr, "Cannot find metric or group `%s'\n",
-					p);
+		if (ret == -EINVAL)
+			fprintf(stderr, "Cannot find metric or group `%s'\n", p);
+
+		if (ret)
 			break;
-		}
+
+		count++;
 	}
 	free(nlist);
 
-	if (!ret)
+	if (!ret) {
+		/*
+		 * Warn about nmi_watchdog if any parsed metrics had the
+		 * NO_NMI_WATCHDOG constraint.
+		 */
 		metricgroup___watchdog_constraint_hint(NULL, true);
-
+		/* No metrics. */
+		if (count == 0)
+			return -EINVAL;
+	}
 	return ret;
 }
 
@@ -1243,6 +1398,10 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 		parse_events_print_error(&parse_error, extra_events.buf);
 		goto out;
 	}
+	ret = decode_all_metric_ids(perf_evlist);
+	if (ret)
+		goto out;
+
 	ret = metricgroup__setup_events(&metric_list, metric_no_merge,
 					perf_evlist, metric_events);
 out:
diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
index b1e29b97d261..4efe9872c667 100644
--- a/tools/perf/util/parse-events.l
+++ b/tools/perf/util/parse-events.l
@@ -138,18 +138,23 @@ static int pmu_str_check(yyscan_t scanner, struct parse_events_state *parse_stat
 
 	yylval->str = strdup(text);
 
-	if (parse_state->fake_pmu)
-		return PE_PMU_EVENT_FAKE;
-
+	/*
+	 * If we're not testing then parse check determines the PMU event type
+	 * which if it isn't a PMU returns PE_NAME. When testing the result of
+	 * parse check can't be trusted so we return PE_PMU_EVENT_FAKE unless
+	 * an '!' is present in which case the text can't be a PMU name.
+	 */
 	switch (perf_pmu__parse_check(text)) {
 		case PMU_EVENT_SYMBOL_PREFIX:
 			return PE_PMU_EVENT_PRE;
 		case PMU_EVENT_SYMBOL_SUFFIX:
 			return PE_PMU_EVENT_SUF;
 		case PMU_EVENT_SYMBOL:
-			return PE_KERNEL_PMU_EVENT;
+			return parse_state->fake_pmu
+				? PE_PMU_EVENT_FAKE : PE_KERNEL_PMU_EVENT;
 		default:
-			return PE_NAME;
+			return parse_state->fake_pmu && !strchr(text,'!')
+				? PE_PMU_EVENT_FAKE : PE_NAME;
 	}
 }
 
@@ -204,7 +209,7 @@ bpf_source	[^,{}]+\.c[a-zA-Z0-9._]*
 num_dec		[0-9]+
 num_hex		0x[a-fA-F0-9]+
 num_raw_hex	[a-fA-F0-9]+
-name		[a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]]*
+name		[a-zA-Z_*?\[\]][a-zA-Z0-9_*?.\[\]!]*
 name_tag	[\'][a-zA-Z_*?\[\]][a-zA-Z0-9_*?\-,\.\[\]:=]*[\']
 name_minus	[a-zA-Z_*?][a-zA-Z0-9\-_*?.:]*
 drv_cfg_term	[a-zA-Z0-9_\.]+(=[a-zA-Z0-9_*?\.:]+)?
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 20f1b9d0f272..69f3cf3b4a44 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -821,7 +821,7 @@ static int prepare_metric(struct evsel **metric_events,
 			  struct runtime_stat *st)
 {
 	double scale;
-	char *n, *pn;
+	char *n;
 	int i, j, ret;
 
 	for (i = 0; metric_events[i]; i++) {
@@ -844,23 +844,11 @@ static int prepare_metric(struct evsel **metric_events,
 			if (v->metric_other)
 				metric_total = v->metric_total;
 		}
-
-		n = strdup(metric_events[i]->name);
+		n = strdup(evsel__metric_id(metric_events[i]));
 		if (!n)
 			return -ENOMEM;
-		/*
-		 * This display code with --no-merge adds [cpu] postfixes.
-		 * These are not supported by the parser. Remove everything
-		 * after the space.
-		 */
-		pn = strchr(n, ' ');
-		if (pn)
-			*pn = 0;
-
-		if (metric_total)
-			expr__add_id_val(pctx, n, metric_total);
-		else
-			expr__add_id_val(pctx, n, avg_stats(stats)*scale);
+
+		expr__add_id_val(pctx, n, metric_total ? : avg_stats(stats) * scale);
 	}
 
 	for (j = 0; metric_refs && metric_refs[j].metric_name; j++) {
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 17/21] perf expr: Add subset utility.

[Ian Rogers]

Add a helper that returns true if all the IDs in needles are present in
haystack. Later this will be used in sharing events between metrics.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/expr.c | 15 +++++++++++++++
 tools/perf/util/expr.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 5657222aaa25..77c6ad81a923 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -211,6 +211,21 @@ int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
 	return hashmap__find(ctx->ids, id, (void **)data) ? 0 : -1;
 }
 
+bool expr__subset_of_ids(struct expr_parse_ctx *haystack,
+			 struct expr_parse_ctx *needles)
+{
+	struct hashmap_entry *cur;
+	size_t bkt;
+	struct expr_id_data *data;
+
+	hashmap__for_each_entry(needles->ids, cur, bkt) {
+		if (expr__get_id(haystack, cur->key, &data))
+			return false;
+	}
+	return true;
+}
+
+
 int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id,
 		     struct expr_id_data **datap)
 {
diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
index c6e534f633c3..cf81f9166dbb 100644
--- a/tools/perf/util/expr.h
+++ b/tools/perf/util/expr.h
@@ -43,6 +43,8 @@ int expr__add_id_val(struct expr_parse_ctx *ctx, const char *id, double val);
 int expr__add_ref(struct expr_parse_ctx *ctx, struct metric_ref *ref);
 int expr__get_id(struct expr_parse_ctx *ctx, const char *id,
 		 struct expr_id_data **data);
+bool expr__subset_of_ids(struct expr_parse_ctx *haystack,
+			 struct expr_parse_ctx *needles);
 int expr__resolve_id(struct expr_parse_ctx *ctx, const char *id,
 		     struct expr_id_data **datap);
 
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 18/21] perf metrics: Modify setup and deduplication

[Ian Rogers]

Previously find_evsel_group was trying to share events while
mark-sweeping to eliminate unused events, this was complicated and had
issues around uncore events and grouped sharing. This was further
complicated by the event string being created while metrics and metric
groups were being added, with the string affecting the evlist order.
This change moves deduplication before event parsing. Ungrouped events
are placed in a single combined set. Groups are checked to see if an
earlier (larger) group can support their events. As the deduplication
and sharing detection is done on metric IDs before parsing, wildcard
expansion problems with uncore events are avoided. Overall the code is
simpler while working better.

An example of failing to deduplicate can be seen with a list of metrics
like the following, where in the after case multiplexing has been
avoided:

Before:
$ perf stat -M Bad_Speculation,Backend_Bound,Frontend_Bound,Retiring -a sleep 2

 Performance counter stats for 'system wide':

       959,620,872      uops_issued.any           #     0.06 Bad_Speculation          (50.03%)
     2,163,072,261      cycles
                                                  #     0.09 Retiring                 (50.03%)
       735,827,436      uops_retired.retire_slots                                     (50.03%)
        74,676,484      int_misc.recovery_cycles                                      (50.03%)
       987,062,794      uops_issued.any           #     0.50 Backend_Bound            (49.97%)
     2,203,734,187      cycles
                                                  #     0.35 Frontend_Bound           (49.97%)
     3,085,016,091      idq_uops_not_delivered.core                                     (49.97%)
       758,599,232      uops_retired.retire_slots                                     (49.97%)
        75,807,526      int_misc.recovery_cycles                                      (49.97%)

       2.002103760 seconds time elapsed

After:
$ sudo perf stat -M Bad_Speculation,Backend_Bound,Frontend_Bound,Retiring -a sleep 2

 Performance counter stats for 'system wide':

       769,694,676      uops_issued.any           #     0.08 Bad_Speculation
                                                  #     0.41 Backend_Bound
     1,087,548,633      cycles
                                                  #     0.38 Frontend_Bound
                                                  #     0.14 Retiring
     1,642,085,777      idq_uops_not_delivered.core
       603,112,590      uops_retired.retire_slots
        43,787,854      int_misc.recovery_cycles

       2.003844383 seconds time elapsed

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 9c16a956fd2c..ac60c9f1b3b5 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -144,6 +144,12 @@ struct metric {
 	 * events won't be grouped.
 	 */
 	bool has_constraint;
+	/**
+	 * Parsed events for the metric. Optional as events may be taken from a
+	 * different metric whose group contains all the IDs necessary for this
+	 * one.
+	 */
+	struct evlist *evlist;
 };
 
 static void metricgroup___watchdog_constraint_hint(const char *name, bool foot)
@@ -201,6 +207,7 @@ static struct metric *metric__new(const struct pmu_event *pe,
 	m->pctx->runtime = runtime;
 	m->has_constraint = metric_no_group || metricgroup__has_constraint(pe);
 	m->metric_refs = NULL;
+	m->evlist = NULL;
 
 	return m;
 }
@@ -224,222 +231,82 @@ static bool contains_metric_id(struct evsel **metric_events, int num_events,
 	return false;
 }
 
-static bool evsel_same_pmu_or_none(struct evsel *ev1, struct evsel *ev2)
-{
-	if (!ev1->pmu_name || !ev2->pmu_name)
-		return true;
-
-	return !strcmp(ev1->pmu_name, ev2->pmu_name);
-}
-
 /**
- * Find a group of events in perf_evlist that correspond to those from a parsed
- * metric expression. Note, as find_evsel_group is called in the same order as
- * perf_evlist was constructed, metric_no_merge doesn't need to test for
- * underfilling a group.
- * @perf_evlist: a list of events something like: {metric1 leader, metric1
- * sibling, metric1 sibling}:W,duration_time,{metric2 leader, metric2 sibling,
- * metric2 sibling}:W,duration_time
- * @pctx: the parse context for the metric expression.
- * @metric_no_merge: don't attempt to share events for the metric with other
- * metrics.
- * @has_constraint: is there a constraint 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.
+ * setup_metric_events - Find a group of events in metric_evlist that correspond
+ *                       to the IDs from a parsed metric expression.
+ * @ids: the metric IDs to match.
+ * @metric_evlist: the list of perf events.
+ * @out_metric_events: holds the created metric events array.
  */
-static struct evsel *find_evsel_group(struct evlist *perf_evlist,
-				      struct expr_parse_ctx *pctx,
-				      bool metric_no_merge,
-				      bool has_constraint,
-				      struct evsel **metric_events,
-				      unsigned long *evlist_used)
+static int setup_metric_events(struct hashmap *ids,
+			       struct evlist *metric_evlist,
+			       struct evsel ***out_metric_events)
 {
-	struct evsel *ev, *current_leader = NULL;
-	struct expr_id_data *val_ptr;
+	struct evsel **metric_events;
 	const char *metric_id;
-	int i = 0, matched_events = 0, events_to_match;
-	int idnum = (int)hashmap__size(pctx->ids);
+	struct evsel *ev;
+	size_t ids_size, matched_events, i;
 
-	if (idnum != 0) {
-		/*
-		 * duration_time is always grouped separately, when events are
-		 * grouped (ie has_constraint is false) then ignore it in the
-		 * matching loop and add it to metric_events at the end.
-		 */
-		events_to_match = idnum;
-		if (!has_constraint && hashmap__find(pctx->ids, "duration_time", (void **)&val_ptr))
-			events_to_match--;
+	*out_metric_events = NULL;
+	ids_size = hashmap__size(ids);
 
-		evlist__for_each_entry(perf_evlist, ev) {
-			/*
-			 * Events with a constraint aren't grouped and match the
-			 * first events available.
-			 */
-			if (has_constraint && ev->weak_group)
-				continue;
-			/* Ignore event if already used and merging is disabled. */
-			if (metric_no_merge && test_bit(ev->core.idx, evlist_used))
-				continue;
-			if (!has_constraint && !evsel__has_leader(ev, 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 = evsel__leader(ev);
-			}
-			/*
-			 * Check for duplicate events with the same name. For
-			 * example, uncore_imc/cas_count_read/ will turn into 6
-			 * events per socket on skylakex. Only the first such
-			 * event is placed in metric_events. If events aren't
-			 * grouped then this also ensures that the same event in
-			 * different sibling groups aren't both added to
-			 * metric_events.
-			 */
-			metric_id = evsel__metric_id(ev);
-			if (contains_metric_id(metric_events, matched_events, metric_id))
-				continue;
-			/* Does this event belong to the parse context? */
-			if (hashmap__find(pctx->ids, metric_id, (void **)&val_ptr))
-				metric_events[matched_events++] = ev;
+	metric_events = calloc(sizeof(void *), ids_size + 1);
+	if (!metric_events)
+		return -ENOMEM;
+
+	matched_events = 0;
+	evlist__for_each_entry(metric_evlist, ev) {
+		struct expr_id_data *val_ptr;
 
-			if (matched_events == events_to_match)
-				break;
-		}
-	} else {
 		/*
-		 * There are no events to match, but we need to associate the
-		 * metric with an event for printing. A duration_time event was
-		 * parsed for this.
+		 * Check for duplicate events with the same name. For
+		 * example, uncore_imc/cas_count_read/ will turn into 6
+		 * events per socket on skylakex. Only the first such
+		 * event is placed in metric_events.
 		 */
-		idnum = 1;
-		events_to_match = 0;
-	}
-	if (events_to_match != idnum) {
-		/* Add the first duration_time. */
-		ev = evlist__find_evsel_by_str(perf_evlist, "duration_time");
-		if (ev)
+		metric_id = evsel__metric_id(ev);
+		if (contains_metric_id(metric_events, matched_events, metric_id))
+			continue;
+		/*
+		 * Does this event belong to the parse context? For
+		 * combined or shared groups, this metric may not care
+		 * about this event.
+		 */
+		if (hashmap__find(ids, metric_id, (void **)&val_ptr)) {
 			metric_events[matched_events++] = ev;
-	}
 
-	if (matched_events != idnum) {
-		/* Not a whole match */
-		return NULL;
+			if (matched_events >= ids_size)
+				break;
+		}
 	}
-
-	metric_events[idnum] = NULL;
-
-	for (i = 0; i < idnum; i++) {
+	if (matched_events < ids_size) {
+		free(metric_events);
+		return -EINVAL;
+	}
+	for (i = 0; i < ids_size; i++) {
 		ev = metric_events[i];
-		/* Don't free the used events. */
-		set_bit(ev->core.idx, evlist_used);
+		ev->collect_stat = true;
+
 		/*
-		 * The metric leader points to the identically named event in
-		 * metric_events.
+		 * The metric leader points to the identically named
+		 * event in metric_events.
 		 */
 		ev->metric_leader = ev;
 		/*
-		 * Mark two events with identical names in the same group (or
-		 * globally) as being in use as uncore events may be duplicated
-		 * for each pmu. Set the metric leader of such events to be the
-		 * event that appears in metric_events.
+		 * Mark two events with identical names in the same
+		 * group (or globally) as being in use as uncore events
+		 * may be duplicated for each pmu. Set the metric leader
+		 * of such events to be the event that appears in
+		 * metric_events.
 		 */
 		metric_id = evsel__metric_id(ev);
-		evlist__for_each_entry_continue(perf_evlist, ev) {
-			/*
-			 * If events are grouped then the search can terminate
-			 * when then group is left.
-			 */
-			if (!has_constraint &&
-			    ev->core.leader != metric_events[i]->core.leader &&
-			    evsel_same_pmu_or_none(evsel__leader(ev), evsel__leader(metric_events[i])))
-				break;
-			if (!strcmp(evsel__metric_id(metric_events[i]), metric_id)) {
-				set_bit(ev->core.idx, evlist_used);
+		evlist__for_each_entry_continue(metric_evlist, ev) {
+			if (!strcmp(evsel__metric_id(metric_events[i]), metric_id))
 				ev->metric_leader = metric_events[i];
-			}
-		}
-	}
-
-	return metric_events[0];
-}
-
-static int metricgroup__setup_events(struct list_head *groups,
-				     bool metric_no_merge,
-				     struct evlist *perf_evlist,
-				     struct rblist *metric_events_list)
-{
-	struct metric_event *me;
-	struct metric_expr *expr;
-	int i = 0;
-	int ret = 0;
-	struct metric *m;
-	struct evsel *evsel, *tmp;
-	unsigned long *evlist_used;
-
-	evlist_used = bitmap_zalloc(perf_evlist->core.nr_entries);
-	if (!evlist_used)
-		return -ENOMEM;
-
-	list_for_each_entry (m, groups, nd) {
-		struct evsel **metric_events;
-		const size_t ids_size = hashmap__size(m->pctx->ids);
-
-		metric_events = calloc(sizeof(void *),
-				ids_size == 0 ? 2 : ids_size + 1);
-		if (!metric_events) {
-			ret = -ENOMEM;
-			break;
-		}
-		evsel = find_evsel_group(perf_evlist, m->pctx,
-					 metric_no_merge,
-					 m->has_constraint, metric_events,
-					 evlist_used);
-		if (!evsel) {
-			pr_debug("Cannot resolve %s: %s\n",
-					m->metric_name, m->metric_expr);
-			free(metric_events);
-			continue;
 		}
-		for (i = 0; metric_events[i]; i++)
-			metric_events[i]->collect_stat = true;
-		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_refs = m->metric_refs;
-		m->metric_refs = NULL;
-		expr->metric_expr = m->metric_expr;
-		expr->metric_name = m->metric_name;
-		expr->metric_unit = m->metric_unit;
-		expr->metric_events = metric_events;
-		expr->runtime = m->pctx->runtime;
-		list_add(&expr->nd, &me->head);
 	}
-
-	evlist__for_each_entry_safe(perf_evlist, tmp, evsel) {
-		if (!test_bit(evsel->core.idx, evlist_used)) {
-			evlist__remove(perf_evlist, evsel);
-			evsel__delete(evsel);
-		}
-	}
-	bitmap_free(evlist_used);
-
-	return ret;
+	*out_metric_events = metric_events;
+	return 0;
 }
 
 static bool match_metric(const char *n, const char *list)
@@ -1222,20 +1089,15 @@ static int metric_list_cmp(void *priv __maybe_unused, const struct list_head *l,
  * @metric_no_group: Should events written to events be grouped "{}" or
  *                   global. Grouping is the default but due to multiplexing the
  *                   user may override.
- * @events: an out argument string of events that need to be parsed and
- *          associated with the metric. For example, the metric "IPC" would
- *          create an events string like "{instructions,cycles}:W".
  * @metric_list: The list that the metric or metric group are added to.
  * @map: The map that is searched for metrics, most commonly the table for the
  *       architecture perf is running upon.
  */
 static int metricgroup__add_metric(const char *metric_name, bool metric_no_group,
-				   struct strbuf *events,
 				   struct list_head *metric_list,
 				   const struct pmu_events_map *map)
 {
 	const struct pmu_event *pe;
-	struct metric *m;
 	LIST_HEAD(list);
 	int i, ret;
 	bool has_match = false;
@@ -1269,27 +1131,8 @@ static int metricgroup__add_metric(const char *metric_name, bool metric_no_group
 		pmu_for_each_sys_event(metricgroup__sys_event_iter, &data);
 	}
 	/* End of pmu events. */
-	if (!has_match) {
+	if (!has_match)
 		ret = -EINVAL;
-		goto out;
-	}
-
-	/* Sort metrics from largest to smallest. */
-	list_sort(NULL,  &list, metric_list_cmp);
-
-	list_for_each_entry(m, &list, nd) {
-		if (events->len > 0) {
-			ret = strbuf_addf(events, ",");
-			if (ret)
-				break;
-		}
-
-		ret = metricgroup__build_event_string(events,
-						m->pctx,
-						m->has_constraint);
-		if (ret)
-			break;
-	}
 
 out:
 	/*
@@ -1297,9 +1140,6 @@ static int metricgroup__add_metric(const char *metric_name, bool metric_no_group
 	 * even if it's failed
 	 */
 	list_splice(&list, metric_list);
-
-	/* Sort metrics from largest to smallest. */
-	list_sort(NULL, metric_list, metric_list_cmp);
 	return ret;
 }
 
@@ -1312,15 +1152,11 @@ static int metricgroup__add_metric(const char *metric_name, bool metric_no_group
  * @metric_no_group: Should events written to events be grouped "{}" or
  *                   global. Grouping is the default but due to multiplexing the
  *                   user may override.
- * @events: an out argument string of events that need to be parsed and
- *          associated with the metric. For example, the metric "IPC" would
- *          create an events string like "{instructions,cycles}:W".
  * @metric_list: The list that metrics are added to.
  * @map: The map that is searched for metrics, most commonly the table for the
  *       architecture perf is running upon.
  */
 static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
-					struct strbuf *events,
 					struct list_head *metric_list,
 					const struct pmu_events_map *map)
 {
@@ -1332,13 +1168,9 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 		return -ENOMEM;
 	llist = nlist;
 
-	ret = strbuf_init(events, 100);
-	if (ret)
-		return ret;
-
 	while ((p = strsep(&llist, ",")) != NULL) {
-		ret = metricgroup__add_metric(p, metric_no_group, events,
-					      metric_list, map);
+		ret = metricgroup__add_metric(p, metric_no_group, metric_list,
+					      map);
 		if (ret == -EINVAL)
 			fprintf(stderr, "Cannot find metric or group `%s'\n", p);
 
@@ -1372,41 +1204,205 @@ static void metricgroup__free_metrics(struct list_head *metric_list)
 	}
 }
 
+/**
+ * build_combined_expr_ctx - Make an expr_parse_ctx with all has_constraint
+ *                           metric IDs, as the IDs are held in a set,
+ *                           duplicates will be removed.
+ * @metric_list: List to take metrics from.
+ * @combined: Out argument for result.
+ */
+static int build_combined_expr_ctx(const struct list_head *metric_list,
+				   struct expr_parse_ctx **combined)
+{
+	struct hashmap_entry *cur;
+	size_t bkt;
+	struct metric *m;
+	char *dup;
+	int ret;
+
+	*combined = expr__ctx_new();
+	if (!*combined)
+		return -ENOMEM;
+
+	list_for_each_entry(m, metric_list, nd) {
+		if (m->has_constraint) {
+			hashmap__for_each_entry(m->pctx->ids, cur, bkt) {
+				dup = strdup(cur->key);
+				if (!dup) {
+					ret = -ENOMEM;
+					goto err_out;
+				}
+				ret = expr__add_id(*combined, dup);
+				if (ret)
+					goto err_out;
+			}
+		}
+	}
+	return 0;
+err_out:
+	expr__ctx_free(*combined);
+	*combined = NULL;
+	return ret;
+}
+
+/**
+ * parse_ids - Build the event string for the ids and parse them creating an
+ *             evlist. The encoded metric_ids are decoded.
+ * @fake_pmu: used when testing metrics not supported by the current CPU.
+ * @ids: the event identifiers parsed from a metric.
+ * @has_constraint: false if events should be placed in a weak group.
+ * @out_evlist: the created list of events.
+ */
+static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
+		     bool has_constraint, struct evlist **out_evlist)
+{
+	struct parse_events_error parse_error;
+	struct evlist *parsed_evlist;
+	struct strbuf events = STRBUF_INIT;
+	int ret;
+
+	*out_evlist = NULL;
+	ret = metricgroup__build_event_string(&events, ids, has_constraint);
+	if (ret)
+		return ret;
+
+	parsed_evlist = evlist__new();
+	if (!parsed_evlist) {
+		ret = -ENOMEM;
+		goto err_out;
+	}
+	pr_debug("Parsing metric events '%s'\n", events.buf);
+	bzero(&parse_error, sizeof(parse_error));
+	ret = __parse_events(parsed_evlist, events.buf, &parse_error, fake_pmu);
+	if (ret) {
+		parse_events_print_error(&parse_error, events.buf);
+		goto err_out;
+	}
+	ret = decode_all_metric_ids(parsed_evlist);
+	if (ret)
+		goto err_out;
+
+	*out_evlist = parsed_evlist;
+	parsed_evlist = NULL;
+err_out:
+	evlist__delete(parsed_evlist);
+	strbuf_release(&events);
+	return ret;
+}
+
 static int parse_groups(struct evlist *perf_evlist, const char *str,
 			bool metric_no_group,
 			bool metric_no_merge,
 			struct perf_pmu *fake_pmu,
-			struct rblist *metric_events,
+			struct rblist *metric_events_list,
 			const struct pmu_events_map *map)
 {
-	struct parse_events_error parse_error;
-	struct strbuf extra_events;
+	struct evlist *combined_evlist = NULL;
 	LIST_HEAD(metric_list);
+	struct metric *m;
 	int ret;
 
-	if (metric_events->nr_entries == 0)
-		metricgroup__rblist_init(metric_events);
+	if (metric_events_list->nr_entries == 0)
+		metricgroup__rblist_init(metric_events_list);
 	ret = metricgroup__add_metric_list(str, metric_no_group,
-					   &extra_events, &metric_list, map);
+					   &metric_list, map);
 	if (ret)
 		goto out;
-	pr_debug("adding %s\n", extra_events.buf);
-	bzero(&parse_error, sizeof(parse_error));
-	ret = __parse_events(perf_evlist, extra_events.len > 0 ? extra_events.buf : "duration_time",
-			     &parse_error, fake_pmu);
-	if (ret) {
-		parse_events_print_error(&parse_error, extra_events.buf);
-		goto out;
+
+	/* Sort metrics from largest to smallest. */
+	list_sort(NULL, &metric_list, metric_list_cmp);
+
+	if (!metric_no_merge) {
+		struct expr_parse_ctx *combined = NULL;
+
+		ret = build_combined_expr_ctx(&metric_list, &combined);
+
+		if (!ret && combined && hashmap__size(combined->ids)) {
+			ret = parse_ids(fake_pmu, combined, /*has_constraint=*/true,
+					&combined_evlist);
+		}
+		if (combined)
+			expr__ctx_free(combined);
+
+		if (ret)
+			goto out;
+	}
+
+	list_for_each_entry(m, &metric_list, nd) {
+		struct metric_event *me;
+		struct evsel **metric_events;
+		struct evlist *metric_evlist = NULL;
+		struct metric *n;
+		struct metric_expr *expr;
+
+		if (combined_evlist && m->has_constraint) {
+			metric_evlist = combined_evlist;
+		} else if (!metric_no_merge) {
+			/*
+			 * See if the IDs for this metric are a subset of an
+			 * earlier metric.
+			 */
+			list_for_each_entry(n, &metric_list, nd) {
+				if (m == n)
+					break;
+
+				if (n->evlist == NULL)
+					continue;
+
+				if (expr__subset_of_ids(n->pctx, m->pctx)) {
+					pr_debug("Events in '%s' fully contained within '%s'\n",
+						 m->metric_name, n->metric_name);
+					metric_evlist = n->evlist;
+					break;
+				}
+
+			}
+		}
+		if (!metric_evlist) {
+			ret = parse_ids(fake_pmu, m->pctx, m->has_constraint,
+					&m->evlist);
+			if (ret)
+				goto out;
+
+			metric_evlist = m->evlist;
+		}
+		ret = setup_metric_events(m->pctx->ids, metric_evlist, &metric_events);
+		if (ret) {
+			pr_debug("Cannot resolve IDs for %s: %s\n",
+				m->metric_name, m->metric_expr);
+			goto out;
+		}
+
+		me = metricgroup__lookup(metric_events_list, metric_events[0], true);
+
+		expr = malloc(sizeof(struct metric_expr));
+		if (!expr) {
+			ret = -ENOMEM;
+			free(metric_events);
+			goto out;
+		}
+
+		expr->metric_refs = m->metric_refs;
+		m->metric_refs = NULL;
+		expr->metric_expr = m->metric_expr;
+		expr->metric_name = m->metric_name;
+		expr->metric_unit = m->metric_unit;
+		expr->metric_events = metric_events;
+		expr->runtime = m->pctx->runtime;
+		list_add(&expr->nd, &me->head);
+	}
+
+
+	if (combined_evlist)
+		evlist__splice_list_tail(perf_evlist, &combined_evlist->core.entries);
+
+	list_for_each_entry(m, &metric_list, nd) {
+		if (m->evlist)
+			evlist__splice_list_tail(perf_evlist, &m->evlist->core.entries);
 	}
-	ret = decode_all_metric_ids(perf_evlist);
-	if (ret)
-		goto out;
 
-	ret = metricgroup__setup_events(&metric_list, metric_no_merge,
-					perf_evlist, metric_events);
 out:
 	metricgroup__free_metrics(&metric_list);
-	strbuf_release(&extra_events);
 	return ret;
 }
 
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 19/21] perf metric: Switch fprintf to pr_err.

[Ian Rogers]

There's no clear reason for the inconsistency that stems from the
initial commit.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index ac60c9f1b3b5..c588243a2d53 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -1172,7 +1172,7 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 		ret = metricgroup__add_metric(p, metric_no_group, metric_list,
 					      map);
 		if (ret == -EINVAL)
-			fprintf(stderr, "Cannot find metric or group `%s'\n", p);
+			pr_err("Cannot find metric or group `%s'\n", p);
 
 		if (ret)
 			break;
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 20/21] perf parse-events: Identify broken modifiers.

[Ian Rogers]

Previously the broken modifier causes a usage message to printed but
nothing else. After:

$ perf stat -e 'cycles:kk' -a sleep 2
event syntax error: 'cycles:kk'
                            \___ Bad modifier
Run 'perf list' for a list of valid events

 Usage: perf stat [<options>] [<command>]

    -e, --event <event>   event selector. use 'perf list' to list available events

$ perf stat -e '{instructions,cycles}:kk' -a sleep 2
event syntax error: '..ns,cycles}:kk'
                                  \___ Bad modifier
Run 'perf list' for a list of valid events

 Usage: perf stat [<options>] [<command>]

    -e, --event <event>   event selector. use 'perf list' to list available events

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

diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 17c8c66f3f51..2d60f3cbe42b 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -183,6 +183,11 @@ group_def ':' PE_MODIFIER_EVENT
 	err = parse_events__modifier_group(list, $3);
 	free($3);
 	if (err) {
+		struct parse_events_state *parse_state = _parse_state;
+		struct parse_events_error *error = parse_state->error;
+
+		parse_events__handle_error(error, @3.first_column,
+					   strdup("Bad modifier"), NULL);
 		free_list_evsel(list);
 		YYABORT;
 	}
@@ -240,6 +245,11 @@ event_name PE_MODIFIER_EVENT
 	err = parse_events__modifier_event(list, $2, false);
 	free($2);
 	if (err) {
+		struct parse_events_state *parse_state = _parse_state;
+		struct parse_events_error *error = parse_state->error;
+
+		parse_events__handle_error(error, @2.first_column,
+					   strdup("Bad modifier"), NULL);
 		free_list_evsel(list);
 		YYABORT;
 	}
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 21/21] perf metric: Allow modifiers on metrics.

[Ian Rogers]

By allowing modifiers on metrics we can, for example, gather the
same metric for kernel and user mode. On a SkylakeX with
TopDownL1 this gives:

$ perf stat -M TopDownL1:u,TopDownL1:k -a sleep 2

 Performance counter stats for 'system wide':

       849,855,577      uops_issued.any:k         #     0.06 Bad_Speculation:k
                                                  #     0.51 Backend_Bound:k          (16.71%)
     1,995,257,996      cycles:k
                                                  # 7981031984.00 SLOTS:k
                                                  #     0.35 Frontend_Bound:k
                                                  #     0.08 Retiring:k               (16.71%)
     2,791,940,753      idq_uops_not_delivered.core:k                                     (16.71%)
       641,961,928      uops_retired.retire_slots:k                                     (16.71%)
        72,239,337      int_misc.recovery_cycles:k                                     (16.71%)
     2,294,413,647      uops_issued.any:u         #     0.04 Bad_Speculation:u
                                                  #     0.39 Backend_Bound:u          (16.78%)
     1,333,248,940      cycles:u
                                                  # 5332995760.00 SLOTS:u
                                                  #     0.16 Frontend_Bound:u
                                                  #     0.40 Retiring:u               (16.78%)
       858,517,081      idq_uops_not_delivered.core:u                                     (16.78%)
     2,153,789,582      uops_retired.retire_slots:u                                     (16.78%)
        19,373,627      int_misc.recovery_cycles:u                                     (16.78%)
        31,503,661      cpu_clk_unhalted.one_thread_active:k #     0.18 CoreIPC_SMT:k            (16.73%)
       315,454,104      inst_retired.any:k        # 315454104.00 Instructions:k       (16.73%)
        42,533,729      cpu_clk_unhalted.ref_xclk:k                                     (16.73%)
     2,043,119,037      cpu_clk_unhalted.thread:k                                     (16.73%)
        28,843,803      cpu_clk_unhalted.one_thread_active:u #     1.55 CoreIPC_SMT:u            (16.60%)
     2,153,353,869      inst_retired.any:u        # 2153353869.00 Instructions:u      (16.60%)
        28,844,743      cpu_clk_unhalted.ref_xclk:u                                     (16.60%)
     1,387,544,378      cpu_clk_unhalted.thread:u                                     (16.60%)
       308,031,603      inst_retired.any:k        #     0.15 CoreIPC:k                (33.19%)
     2,036,774,753      cycles:k                                                      (33.19%)
     1,994,344,281      inst_retired.any:u        #     1.59 CoreIPC:u                (33.18%)
     1,251,538,227      cycles:u                                                      (33.18%)

       2.000342948 seconds time elapsed

Modifiers are naively copy and pasted on to events, this can yield errors like:

$ perf stat -M Kernel_Utilization:k -a sleep 2
event syntax error: '..d.thread:k/kk,cpu_clk_unhalted.thread/metric-id=cpu_clk_unhalted.thread/k..'
                                  \___ Bad modifier

 Usage: perf stat [<options>] [<command>]

    -M, --metrics <metric/metric group list>
                          monitor specified metrics or metric groups (separated by ,)

When modifiers are present with constraints, from
--metric-no-group or the NMI watchdog, they are no longer placed
in the same set - which may miss deduplicating events.

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

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index c588243a2d53..a28f61d59fe9 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -85,6 +85,7 @@ static void metric_event_delete(struct rblist *rblist __maybe_unused,
 	struct metric_expr *expr, *tmp;
 
 	list_for_each_entry_safe(expr, tmp, &me->head, nd) {
+		free((char *)expr->metric_name);
 		free(expr->metric_refs);
 		free(expr->metric_events);
 		free(expr);
@@ -130,6 +131,8 @@ struct metric {
 	struct expr_parse_ctx *pctx;
 	/** The name of the metric such as "IPC". */
 	const char *metric_name;
+	/** Modifier on the metric such as "u" or NULL for none. */
+	const char *modifier;
 	/** The expression to parse, for example, "instructions/cycles". */
 	const char *metric_expr;
 	/**
@@ -186,6 +189,7 @@ static bool metricgroup__has_constraint(const struct pmu_event *pe)
 }
 
 static struct metric *metric__new(const struct pmu_event *pe,
+				  const char *modifier,
 				  bool metric_no_group,
 				  int runtime)
 {
@@ -202,6 +206,12 @@ static struct metric *metric__new(const struct pmu_event *pe,
 	}
 
 	m->metric_name = pe->metric_name;
+	m->modifier = modifier ? strdup(modifier) : NULL;
+	if (modifier && !m->modifier) {
+		free(m);
+		expr__ctx_free(m->pctx);
+		return NULL;
+	}
 	m->metric_expr = pe->metric_expr;
 	m->metric_unit = pe->unit;
 	m->pctx->runtime = runtime;
@@ -216,6 +226,7 @@ static void metric__free(struct metric *m)
 {
 	free(m->metric_refs);
 	expr__ctx_free(m->pctx);
+	free((char *)m->modifier);
 	free(m);
 }
 
@@ -645,7 +656,7 @@ static int decode_metric_id(struct strbuf *sb, const char *x)
 	return 0;
 }
 
-static int decode_all_metric_ids(struct evlist *perf_evlist)
+static int decode_all_metric_ids(struct evlist *perf_evlist, const char *modifier)
 {
 	struct evsel *ev;
 	struct strbuf sb = STRBUF_INIT;
@@ -675,10 +686,24 @@ static int decode_all_metric_ids(struct evlist *perf_evlist)
 		 * give a more friendly display version.
 		 */
 		if (strstr(ev->name, "metric-id=")) {
+			bool has_slash = false;
+
 			free(ev->name);
-			for (cur = strchr(sb.buf, '@') ; cur; cur = strchr(++cur, '@'))
+			for (cur = strchr(sb.buf, '@') ; cur; cur = strchr(++cur, '@')) {
 				*cur = '/';
+				has_slash = true;
+			}
 
+			if (modifier) {
+				if (!has_slash && !strchr(sb.buf, ':')) {
+					ret = strbuf_addch(&sb, ':');
+					if (ret)
+						break;
+				}
+				ret = strbuf_addstr(&sb, modifier);
+				if (ret)
+					break;
+			}
 			ev->name = strdup(sb.buf);
 			if (!ev->name) {
 				ret = -ENOMEM;
@@ -692,6 +717,7 @@ static int decode_all_metric_ids(struct evlist *perf_evlist)
 
 static int metricgroup__build_event_string(struct strbuf *events,
 					   const struct expr_parse_ctx *ctx,
+					   const char *modifier,
 					   bool has_constraint)
 {
 	struct hashmap_entry *cur;
@@ -765,6 +791,10 @@ static int metricgroup__build_event_string(struct strbuf *events,
 			ret = strbuf_addstr(events, sep + 1);
 			RETURN_IF_NON_ZERO(ret);
 		}
+		if (modifier) {
+			ret = strbuf_addstr(events, modifier);
+			RETURN_IF_NON_ZERO(ret);
+		}
 	}
 	if (has_duration) {
 		if (no_group) {
@@ -798,6 +828,7 @@ struct visited_metric {
 struct metricgroup_add_iter_data {
 	struct list_head *metric_list;
 	const char *metric_name;
+	const char *modifier;
 	int *ret;
 	bool *has_match;
 	bool metric_no_group;
@@ -808,6 +839,7 @@ struct metricgroup_add_iter_data {
 
 static int add_metric(struct list_head *metric_list,
 		      const struct pmu_event *pe,
+		      const char *modifier,
 		      bool metric_no_group,
 		      struct metric *root_metric,
 		      const struct visited_metric *visited,
@@ -817,6 +849,7 @@ static int add_metric(struct list_head *metric_list,
  * resolve_metric - Locate metrics within the root metric and recursively add
  *                    references to them.
  * @metric_list: The list the metric is added to.
+ * @modifier: if non-null event modifiers like "u".
  * @metric_no_group: Should events written to events be grouped "{}" or
  *                   global. Grouping is the default but due to multiplexing the
  *                   user may override.
@@ -829,6 +862,7 @@ static int add_metric(struct list_head *metric_list,
  *       architecture perf is running upon.
  */
 static int resolve_metric(struct list_head *metric_list,
+			  const char *modifier,
 			  bool metric_no_group,
 			  struct metric *root_metric,
 			  const struct visited_metric *visited,
@@ -876,7 +910,7 @@ static int resolve_metric(struct list_head *metric_list,
 	 * context.
 	 */
 	for (i = 0; i < pending_cnt; i++) {
-		ret = add_metric(metric_list, pending[i].pe, metric_no_group,
+		ret = add_metric(metric_list, pending[i].pe, modifier, metric_no_group,
 				root_metric, visited, map);
 		if (ret)
 			break;
@@ -890,6 +924,7 @@ static int resolve_metric(struct list_head *metric_list,
  * __add_metric - Add a metric to metric_list.
  * @metric_list: The list the metric is added to.
  * @pe: The pmu_event containing the metric to be added.
+ * @modifier: if non-null event modifiers like "u".
  * @metric_no_group: Should events written to events be grouped "{}" or
  *                   global. Grouping is the default but due to multiplexing the
  *                   user may override.
@@ -904,6 +939,7 @@ static int resolve_metric(struct list_head *metric_list,
  */
 static int __add_metric(struct list_head *metric_list,
 			const struct pmu_event *pe,
+			const char *modifier,
 			bool metric_no_group,
 			int runtime,
 			struct metric *root_metric,
@@ -930,7 +966,7 @@ static int __add_metric(struct list_head *metric_list,
 		 * This metric is the root of a tree and may reference other
 		 * metrics that are added recursively.
 		 */
-		root_metric = metric__new(pe, metric_no_group, runtime);
+		root_metric = metric__new(pe, modifier, metric_no_group, runtime);
 		if (!root_metric)
 			return -ENOMEM;
 
@@ -979,7 +1015,7 @@ static int __add_metric(struct list_head *metric_list,
 		ret = -EINVAL;
 	} else {
 		/* Resolve referenced metrics. */
-		ret = resolve_metric(metric_list, metric_no_group, root_metric,
+		ret = resolve_metric(metric_list, modifier, metric_no_group, root_metric,
 				     &visited_node, map);
 	}
 
@@ -1021,6 +1057,7 @@ const struct pmu_event *metricgroup__find_metric(const char *metric,
 
 static int add_metric(struct list_head *metric_list,
 		      const struct pmu_event *pe,
+		      const char *modifier,
 		      bool metric_no_group,
 		      struct metric *root_metric,
 		      const struct visited_metric *visited,
@@ -1031,7 +1068,7 @@ static int add_metric(struct list_head *metric_list,
 	pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
 
 	if (!strstr(pe->metric_expr, "?")) {
-		ret = __add_metric(metric_list, pe, metric_no_group, 0,
+		ret = __add_metric(metric_list, pe, modifier, metric_no_group, 0,
 				   root_metric, visited, map);
 	} else {
 		int j, count;
@@ -1044,7 +1081,7 @@ static int add_metric(struct list_head *metric_list,
 		 */
 
 		for (j = 0; j < count && !ret; j++)
-			ret = __add_metric(metric_list, pe, metric_no_group, j,
+			ret = __add_metric(metric_list, pe, modifier, metric_no_group, j,
 					root_metric, visited, map);
 	}
 
@@ -1060,7 +1097,7 @@ static int metricgroup__add_metric_sys_event_iter(const struct pmu_event *pe,
 	if (!match_pe_metric(pe, d->metric_name))
 		return 0;
 
-	ret = add_metric(d->metric_list, pe, d->metric_no_group,
+	ret = add_metric(d->metric_list, pe, d->modifier, d->metric_no_group,
 			 d->root_metric, d->visited, d->map);
 	if (ret)
 		goto out;
@@ -1086,6 +1123,7 @@ static int metric_list_cmp(void *priv __maybe_unused, const struct list_head *l,
  * @metric_name: The name of the metric or metric group. For example, "IPC"
  *               could be the name of a metric and "TopDownL1" the name of a
  *               metric group.
+ * @modifier: if non-null event modifiers like "u".
  * @metric_no_group: Should events written to events be grouped "{}" or
  *                   global. Grouping is the default but due to multiplexing the
  *                   user may override.
@@ -1093,7 +1131,8 @@ static int metric_list_cmp(void *priv __maybe_unused, const struct list_head *l,
  * @map: The map that is searched for metrics, most commonly the table for the
  *       architecture perf is running upon.
  */
-static int metricgroup__add_metric(const char *metric_name, bool metric_no_group,
+static int metricgroup__add_metric(const char *metric_name, const char *modifier,
+				   bool metric_no_group,
 				   struct list_head *metric_list,
 				   const struct pmu_events_map *map)
 {
@@ -1108,7 +1147,7 @@ static int metricgroup__add_metric(const char *metric_name, bool metric_no_group
 	 */
 	map_for_each_metric(pe, i, map, metric_name) {
 		has_match = true;
-		ret = add_metric(&list, pe, metric_no_group,
+		ret = add_metric(&list, pe, modifier, metric_no_group,
 				 /*root_metric=*/NULL,
 				 /*visited_metrics=*/NULL, map);
 		if (ret)
@@ -1121,6 +1160,7 @@ static int metricgroup__add_metric(const char *metric_name, bool metric_no_group
 			.data = (void *) &(struct metricgroup_add_iter_data) {
 				.metric_list = &list,
 				.metric_name = metric_name,
+				.modifier = modifier,
 				.metric_no_group = metric_no_group,
 				.has_match = &has_match,
 				.ret = &ret,
@@ -1160,26 +1200,31 @@ static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
 					struct list_head *metric_list,
 					const struct pmu_events_map *map)
 {
-	char *llist, *nlist, *p;
+	char *list_itr, *list_copy, *metric_name, *modifier;
 	int ret, count = 0;
 
-	nlist = strdup(list);
-	if (!nlist)
+	list_copy = strdup(list);
+	if (!list_copy)
 		return -ENOMEM;
-	llist = nlist;
+	list_itr = list_copy;
+
+	while ((metric_name = strsep(&list_itr, ",")) != NULL) {
+		modifier = strchr(metric_name, ':');
+		if (modifier)
+			*modifier++ = '\0';
 
-	while ((p = strsep(&llist, ",")) != NULL) {
-		ret = metricgroup__add_metric(p, metric_no_group, metric_list,
+		ret = metricgroup__add_metric(metric_name, modifier,
+					      metric_no_group, metric_list,
 					      map);
 		if (ret == -EINVAL)
-			pr_err("Cannot find metric or group `%s'\n", p);
+			pr_err("Cannot find metric or group `%s'\n", metric_name);
 
 		if (ret)
 			break;
 
 		count++;
 	}
-	free(nlist);
+	free(list_copy);
 
 	if (!ret) {
 		/*
@@ -1225,7 +1270,7 @@ static int build_combined_expr_ctx(const struct list_head *metric_list,
 		return -ENOMEM;
 
 	list_for_each_entry(m, metric_list, nd) {
-		if (m->has_constraint) {
+		if (m->has_constraint && !m->modifier) {
 			hashmap__for_each_entry(m->pctx->ids, cur, bkt) {
 				dup = strdup(cur->key);
 				if (!dup) {
@@ -1250,11 +1295,12 @@ static int build_combined_expr_ctx(const struct list_head *metric_list,
  *             evlist. The encoded metric_ids are decoded.
  * @fake_pmu: used when testing metrics not supported by the current CPU.
  * @ids: the event identifiers parsed from a metric.
+ * @modifier: any modifiers added to the events.
  * @has_constraint: false if events should be placed in a weak group.
  * @out_evlist: the created list of events.
  */
 static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
-		     bool has_constraint, struct evlist **out_evlist)
+		     const char *modifier, bool has_constraint, struct evlist **out_evlist)
 {
 	struct parse_events_error parse_error;
 	struct evlist *parsed_evlist;
@@ -1262,7 +1308,8 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
 	int ret;
 
 	*out_evlist = NULL;
-	ret = metricgroup__build_event_string(&events, ids, has_constraint);
+	ret = metricgroup__build_event_string(&events, ids, modifier,
+					      has_constraint);
 	if (ret)
 		return ret;
 
@@ -1278,7 +1325,7 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
 		parse_events_print_error(&parse_error, events.buf);
 		goto err_out;
 	}
-	ret = decode_all_metric_ids(parsed_evlist);
+	ret = decode_all_metric_ids(parsed_evlist, modifier);
 	if (ret)
 		goto err_out;
 
@@ -1318,7 +1365,8 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 		ret = build_combined_expr_ctx(&metric_list, &combined);
 
 		if (!ret && combined && hashmap__size(combined->ids)) {
-			ret = parse_ids(fake_pmu, combined, /*has_constraint=*/true,
+			ret = parse_ids(fake_pmu, combined, /*modifier=*/NULL,
+					/*has_constraint=*/true,
 					&combined_evlist);
 		}
 		if (combined)
@@ -1349,6 +1397,12 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 				if (n->evlist == NULL)
 					continue;
 
+				if ((!m->modifier && n->modifier) ||
+				    (m->modifier && !n->modifier) ||
+				    (m->modifier && n->modifier &&
+					    strcmp(m->modifier, n->modifier)))
+					continue;
+
 				if (expr__subset_of_ids(n->pctx, m->pctx)) {
 					pr_debug("Events in '%s' fully contained within '%s'\n",
 						 m->metric_name, n->metric_name);
@@ -1359,8 +1413,8 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 			}
 		}
 		if (!metric_evlist) {
-			ret = parse_ids(fake_pmu, m->pctx, m->has_constraint,
-					&m->evlist);
+			ret = parse_ids(fake_pmu, m->pctx, m->modifier,
+					m->has_constraint, &m->evlist);
 			if (ret)
 				goto out;
 
@@ -1385,7 +1439,21 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
 		expr->metric_refs = m->metric_refs;
 		m->metric_refs = NULL;
 		expr->metric_expr = m->metric_expr;
-		expr->metric_name = m->metric_name;
+		if (m->modifier) {
+			char *tmp;
+
+			if (asprintf(&tmp, "%s:%s", m->metric_name, m->modifier) < 0)
+				expr->metric_name = NULL;
+			else
+				expr->metric_name = tmp;
+		} else
+			expr->metric_name = strdup(m->metric_name);
+
+		if (!expr->metric_name) {
+			ret = -ENOMEM;
+			free(metric_events);
+			goto out;
+		}
 		expr->metric_unit = m->metric_unit;
 		expr->metric_events = metric_events;
 		expr->runtime = m->pctx->runtime;
-- 
2.33.0.882.g93a45727a2-goog

Re: [PATCH 00/21] perf metric: Fixes and allow modifiers

[Andi Kleen]

On 10/7/2021 9:56 AM, Ian Rogers wrote:
> There are 4 main changes in this patch set:
>   - perf metric: Modify resolution and recursion check.
>   - perf parse-events: Add new "metric-id" term.
>   - perf metrics: Modify setup and deduplication
>   - perf metric: Allow modifiers on metrics.


Patches look all good to me from a quick read


Acked-by: Andi Kleen <ak@linux.intel.com>